Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7143e061 authored by Jeremy Meyer's avatar Jeremy Meyer
Browse files

Don't let aapt2 dedupe configs with different sizes 

With the previous logic in aapt2's resource deduper a configuration that
specified a screen size could dominate another with a different screen
size. For example, w600dp-h800dp could dominate w800dp-h1000dp because
everything that would match the latter would also match the former.

If the values were the same the more specific one would get removed
because that code assumed anything that would have gotten the more
specific one will just the more general one instead. But this isn't
necessarily true because there could be a third value that neither
dominates nor is dominated by either of them, say w500dp-h1200dp.

This makes it so we don't dedupe if there is both a width and height and
they are different than the parent node.

Fixes: 414775283
Test: ResourceDeduper_test
Flag: EXEMPT bugfix
Change-Id: Ide687ba0c976316ec9ba0c2b5f91068456bdb7b4
parent a2d6105f

tools/aapt2/optimize/ResourceDeduper.cpp

+15 −0
Original line number Diff line number Diff line
@@ -60,6 +60,21 @@ class DominatedKeyValueRemover : public DominatorTree::BottomUpVisitor { 
      return;

    }



    // It is not safe to remove a value when both screenWidthDp and screenHeightDp are defined on

    // the node unless they are the same width and height. This is because there could

    // be a third value C not in this hierarchy and there are values that should match the current

    // node but if we remove the value they will instead match C. This is because domination is

    // determined first by width and then height but runtime matching is both at the same time

    // (b/414775283). For example, say we had A with config w600dp-h800dp and value foo, B with

    // config w800dp-h1000dp and value foo, and C with config w500dp-h1200dp and value bar. A would

    // dominate B but C would neither dominate nor me dominated by either A or B. If we removed

    // the B value there would be some screen sizes like w800dp-h1200dp that should have gotten foo

    // but now get bar.

    if (node_value->config.screenWidthDp && node_value->config.screenHeightDp &&

        node_value->config.screenSizeDp != parent_value->config.screenSizeDp) {

      return;

    }



    // Compare compatible configs for this entry and ensure the values are

    // equivalent.

    const ConfigDescription& node_configuration = node_value->config;

tools/aapt2/optimize/ResourceDeduper_test.cpp

+106 −0
Original line number Diff line number Diff line
@@ -172,5 +172,111 @@ TEST(ResourceDeduperTest, MccMncValuesAreKept) { 
  EXPECT_THAT(table, HasValue("android:string/keep", mnc_config));

}



TEST(ResourceDeduperTest, WidthDpHeightDpValuesAreKept) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();

  const ConfigDescription default_config = {};

  const ConfigDescription w600dp_config = test::ParseConfigOrDie("w600dp-h900dp");

  const ConfigDescription w840dp_config = test::ParseConfigOrDie("w840dp-h900dp");

  const ConfigDescription h480dp_config = test::ParseConfigOrDie("w840dp-h480dp");

  const ConfigDescription h900dp_config = test::ParseConfigOrDie("w840dp-h900dp");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

          .AddString("android:string/keep1", ResourceId{}, default_config, "keep")

          .AddString("android:string/keep1", ResourceId{}, w600dp_config, "keep")

          .AddString("android:string/keep1", ResourceId{}, w840dp_config, "keep")

          .AddString("android:string/keep2", ResourceId{}, default_config, "keep")

          .AddString("android:string/keep2", ResourceId{}, h480dp_config, "keep")

          .AddString("android:string/keep2", ResourceId{}, h900dp_config, "keep")

          .Build();



  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));

  EXPECT_THAT(table, HasValue("android:string/keep1", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep1", w600dp_config));

  EXPECT_THAT(table, HasValue("android:string/keep1", w840dp_config));

  EXPECT_THAT(table, HasValue("android:string/keep2", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep2", h480dp_config));

  EXPECT_THAT(table, HasValue("android:string/keep2", h900dp_config));

}



TEST(ResourceDeduperTest, WidthDpHeightDpValuesSameAreDeduped) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();

  const ConfigDescription default_config = {};

  const ConfigDescription wh_config = test::ParseConfigOrDie("w600dp-h900dp");

  const ConfigDescription wh_port_config = test::ParseConfigOrDie("w600dp-h900dp-port");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

          .AddString("android:string/keep", ResourceId{}, default_config, "dedupe")

          .AddString("android:string/keep", ResourceId{}, wh_config, "dedupe")

          .AddString("android:string/keep", ResourceId{}, wh_port_config, "dedupe")

          .Build();



  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));

  EXPECT_THAT(table, HasValue("android:string/keep", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep", wh_config));

  EXPECT_THAT(table, Not(HasValue("android:string/keep", wh_port_config)));

}



TEST(ResourceDeduperTest, WidthDpXorHeightDpValuesAreDeduped) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();

  const ConfigDescription default_config = {};

  const ConfigDescription w1_config = test::ParseConfigOrDie("w600dp");

  const ConfigDescription w2_config = test::ParseConfigOrDie("w800dp");

  const ConfigDescription h1_config = test::ParseConfigOrDie("h600dp");

  const ConfigDescription h2_config = test::ParseConfigOrDie("h800dp");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

          .AddString("android:string/keep1", ResourceId{}, default_config, "keep")

          .AddString("android:string/keep1", ResourceId{}, w1_config, "dedupe")

          .AddString("android:string/keep1", ResourceId{}, w2_config, "dedupe")

          .AddString("android:string/keep2", ResourceId{}, default_config, "keep")

          .AddString("android:string/keep2", ResourceId{}, h1_config, "dedupe")

          .AddString("android:string/keep2", ResourceId{}, h2_config, "dedupe")

          .Build();



  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));

  EXPECT_THAT(table, HasValue("android:string/keep1", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep1", w1_config));

  EXPECT_THAT(table, Not(HasValue("android:string/keep1", w2_config)));

  EXPECT_THAT(table, HasValue("android:string/keep2", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep2", h1_config));

  EXPECT_THAT(table, Not(HasValue("android:string/keep2", h2_config)));

}



TEST(ResourceDeduperTest, SizeDpComplex) {

  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();

  const ConfigDescription default_config = {};

  const ConfigDescription config1 = test::ParseConfigOrDie("w600dp");

  const ConfigDescription config2 = test::ParseConfigOrDie("w800dp");

  const ConfigDescription config3 = test::ParseConfigOrDie("w600dp-h600dp");

  const ConfigDescription config4 = test::ParseConfigOrDie("w800dp-h600dp");

  const ConfigDescription config5 = test::ParseConfigOrDie("w800dp-h800dp-port");

  const ConfigDescription config6 = test::ParseConfigOrDie("w600dp-port");

  const ConfigDescription config7 = test::ParseConfigOrDie("w800dp-port");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

          .AddString("android:string/keep", ResourceId{}, default_config, "keep")

          .AddString("android:string/keep", ResourceId{}, config1, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config2, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config3, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config4, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config5, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config6, "dedupe")

          .AddString("android:string/keep", ResourceId{}, config7, "dedupe")

          .Build();



  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));

  EXPECT_THAT(table, HasValue("android:string/keep", default_config));

  EXPECT_THAT(table, HasValue("android:string/keep", config1));

  EXPECT_THAT(table, Not(HasValue("android:string/keep", config2)));

  EXPECT_THAT(table, HasValue("android:string/keep", config3));

  EXPECT_THAT(table, HasValue("android:string/keep", config4));

  EXPECT_THAT(table, HasValue("android:string/keep", config5));

  EXPECT_THAT(table, Not(HasValue("android:string/keep", config6)));

  EXPECT_THAT(table, Not(HasValue("android:string/keep", config7)));

}



}  // namespace aapt