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

Commit e3856748 authored by Adam Lesinski's avatar Adam Lesinski
Browse files

AAPT2: Disable locale domination for deduping 

Locale deduping isn't straightforward, as parenting rules
change between platform versions and the selection
preference of a specific locale variant over the default
configuration lead to incorrect results at runtime.

Bug: 62409213
Test: make aapt2_tests
Change-Id: Iec8f1cfba7ae43c847d163529891fdc15f3db826
parent 52feccbf

tools/aapt2/ConfigDescription.cpp

+10 −1
Original line number Diff line number Diff line
@@ -877,7 +877,16 @@ ConfigDescription ConfigDescription::CopyWithoutSdkVersion() const { 
}



bool ConfigDescription::Dominates(const ConfigDescription& o) const {

  if (*this == DefaultConfig() || *this == o) {

  if (*this == o) {

    return true;

  }



  // Locale de-duping is not-trivial, disable for now (b/62409213).

  if (diff(o) & CONFIG_LOCALE) {

    return false;

  }



  if (*this == DefaultConfig()) {

    return true;

  }

  return MatchWithDensity(o) && !o.MatchWithDensity(*this) &&

tools/aapt2/DominatorTree_test.cpp

+36 −25
Original line number Diff line number Diff line
@@ -69,14 +69,12 @@ class PrettyPrinter : public DominatorTree::Visitor { 
TEST(DominatorTreeTest, DefaultDominatesEverything) {

  const ConfigDescription default_config = {};

  const ConfigDescription land_config = test::ParseConfigOrDie("land");

  const ConfigDescription sw600dp_land_config =

      test::ParseConfigOrDie("sw600dp-land-v13");

  const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");



  std::vector<std::unique_ptr<ResourceConfigValue>> configs;

  configs.push_back(util::make_unique<ResourceConfigValue>(default_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(land_config, ""));

  configs.push_back(

      util::make_unique<ResourceConfigValue>(sw600dp_land_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_land_config, ""));



  DominatorTree tree(configs);

  PrettyPrinter printer;
@@ -91,16 +89,13 @@ TEST(DominatorTreeTest, DefaultDominatesEverything) { 
TEST(DominatorTreeTest, ProductsAreDominatedSeparately) {

  const ConfigDescription default_config = {};

  const ConfigDescription land_config = test::ParseConfigOrDie("land");

  const ConfigDescription sw600dp_land_config =

      test::ParseConfigOrDie("sw600dp-land-v13");

  const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");



  std::vector<std::unique_ptr<ResourceConfigValue>> configs;

  configs.push_back(util::make_unique<ResourceConfigValue>(default_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(land_config, ""));

  configs.push_back(

      util::make_unique<ResourceConfigValue>(default_config, "phablet"));

  configs.push_back(

      util::make_unique<ResourceConfigValue>(sw600dp_land_config, "phablet"));

  configs.push_back(util::make_unique<ResourceConfigValue>(default_config, "phablet"));

  configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_land_config, "phablet"));



  DominatorTree tree(configs);

  PrettyPrinter printer;
@@ -118,16 +113,11 @@ TEST(DominatorTreeTest, MoreSpecificConfigurationsAreDominated) { 
  const ConfigDescription en_config = test::ParseConfigOrDie("en");

  const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");

  const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl-v4");

  const ConfigDescription ldrtl_xhdpi_config =

      test::ParseConfigOrDie("ldrtl-xhdpi-v4");

  const ConfigDescription sw300dp_config =

      test::ParseConfigOrDie("sw300dp-v13");

  const ConfigDescription sw540dp_config =

      test::ParseConfigOrDie("sw540dp-v14");

  const ConfigDescription sw600dp_config =

      test::ParseConfigOrDie("sw600dp-v14");

  const ConfigDescription sw720dp_config =

      test::ParseConfigOrDie("sw720dp-v13");

  const ConfigDescription ldrtl_xhdpi_config = test::ParseConfigOrDie("ldrtl-xhdpi-v4");

  const ConfigDescription sw300dp_config = test::ParseConfigOrDie("sw300dp-v13");

  const ConfigDescription sw540dp_config = test::ParseConfigOrDie("sw540dp-v14");

  const ConfigDescription sw600dp_config = test::ParseConfigOrDie("sw600dp-v14");

  const ConfigDescription sw720dp_config = test::ParseConfigOrDie("sw720dp-v13");

  const ConfigDescription v20_config = test::ParseConfigOrDie("v20");



  std::vector<std::unique_ptr<ResourceConfigValue>> configs;
@@ -135,8 +125,7 @@ TEST(DominatorTreeTest, MoreSpecificConfigurationsAreDominated) { 
  configs.push_back(util::make_unique<ResourceConfigValue>(en_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(en_v21_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(ldrtl_config, ""));

  configs.push_back(

      util::make_unique<ResourceConfigValue>(ldrtl_xhdpi_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(ldrtl_xhdpi_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(sw300dp_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(sw540dp_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_config, ""));
@@ -148,15 +137,37 @@ TEST(DominatorTreeTest, MoreSpecificConfigurationsAreDominated) { 


  std::string expected =

      "<default>\n"

      "  en\n"

      "    en-v21\n"

      "  ldrtl-v4\n"

      "    ldrtl-xhdpi-v4\n"

      "  sw300dp-v13\n"

      "    sw540dp-v14\n"

      "      sw600dp-v14\n"

      "    sw720dp-v13\n"

      "  v20\n";

      "  v20\n"

      "en\n"

      "  en-v21\n";

  EXPECT_EQ(expected, printer.ToString(&tree));

}



TEST(DominatorTreeTest, LocalesAreNeverDominated) {

  const ConfigDescription fr_config = test::ParseConfigOrDie("fr");

  const ConfigDescription fr_rCA_config = test::ParseConfigOrDie("fr-rCA");

  const ConfigDescription fr_rFR_config = test::ParseConfigOrDie("fr-rFR");



  std::vector<std::unique_ptr<ResourceConfigValue>> configs;

  configs.push_back(util::make_unique<ResourceConfigValue>(ConfigDescription::DefaultConfig(), ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(fr_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(fr_rCA_config, ""));

  configs.push_back(util::make_unique<ResourceConfigValue>(fr_rFR_config, ""));



  DominatorTree tree(configs);

  PrettyPrinter printer;



  std::string expected =

      "<default>\n"

      "fr\n"

      "fr-rCA\n"

      "fr-rFR\n";

  EXPECT_EQ(expected, printer.ToString(&tree));

}

tools/aapt2/optimize/ResourceDeduper_test.cpp

+55 −36
Original line number Diff line number Diff line
@@ -19,69 +19,88 @@ 
#include "ResourceTable.h"

#include "test/Test.h"



using ::aapt::test::HasValue;

using ::testing::Not;



namespace aapt {



TEST(ResourceDeduperTest, SameValuesAreDeduped) {

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

  const ConfigDescription default_config = {};

  const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");

  const ConfigDescription ldrtl_v21_config = test::ParseConfigOrDie("ldrtl-v21");

  const ConfigDescription en_config = test::ParseConfigOrDie("en");

  const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");

  // Chosen because this configuration is compatible with en.

  // Chosen because this configuration is compatible with ldrtl/en.

  const ConfigDescription land_config = test::ParseConfigOrDie("land");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

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

                     "dedupe")

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

          .AddString("android:string/dedupe", ResourceId{}, land_config,

                     "dedupe")

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

                     "dedupe")

          .AddString("android:string/dedupe2", ResourceId{}, en_config,

                     "dedupe")

          .AddString("android:string/dedupe2", ResourceId{}, en_v21_config,

                     "keep")

          .AddString("android:string/dedupe2", ResourceId{}, land_config,

                     "dedupe")

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

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

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



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

          .AddString("android:string/dedupe2", ResourceId{}, ldrtl_config, "dedupe")

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

          .AddString("android:string/dedupe2", ResourceId{}, land_config, "dedupe")



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

          .AddString("android:string/dedupe3", ResourceId{}, en_config, "dedupe")

          .AddString("android:string/dedupe3", ResourceId{}, en_v21_config, "dedupe")

          .Build();



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

  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/dedupe", en_config));

  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/dedupe", land_config));

  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/dedupe2", en_config));

  EXPECT_NE(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/dedupe2", en_v21_config));

  EXPECT_THAT(table, Not(HasValue("android:string/dedupe", ldrtl_config)));

  EXPECT_THAT(table, Not(HasValue("android:string/dedupe", land_config)));



  EXPECT_THAT(table, HasValue("android:string/dedupe2", ldrtl_v21_config));

  EXPECT_THAT(table, Not(HasValue("android:string/dedupe2", ldrtl_config)));



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

  EXPECT_THAT(table, HasValue("android:string/dedupe3", en_config));

  EXPECT_THAT(table, Not(HasValue("android:string/dedupe3", en_v21_config)));

}



TEST(ResourceDeduperTest, DifferentValuesAreKept) {

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

  const ConfigDescription default_config = {};

  const ConfigDescription en_config = test::ParseConfigOrDie("en");

  const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");

  // Chosen because this configuration is compatible with en.

  const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");

  const ConfigDescription ldrtl_v21_config = test::ParseConfigOrDie("ldrtl-v21");

  // Chosen because this configuration is compatible with ldrtl.

  const ConfigDescription land_config = test::ParseConfigOrDie("land");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

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

                     "keep")

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

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

                     "keep2")

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

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

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

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

          .Build();



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

  EXPECT_NE(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/keep", en_config));

  EXPECT_NE(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/keep", en_v21_config));

  EXPECT_NE(nullptr, test::GetValueForConfig<String>(

                         table.get(), "android:string/keep", land_config));

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

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

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

}



TEST(ResourceDeduperTest, LocalesValuesAreKept) {

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

  const ConfigDescription default_config = {};

  const ConfigDescription fr_config = test::ParseConfigOrDie("fr");

  const ConfigDescription fr_rCA_config = test::ParseConfigOrDie("fr-rCA");



  std::unique_ptr<ResourceTable> table =

      test::ResourceTableBuilder()

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

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

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

          .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", fr_config));

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

}



}  // namespace aapt

tools/aapt2/test/Common.h

+14 −1
Original line number Diff line number Diff line
@@ -141,10 +141,23 @@ MATCHER_P(ValueEq, a, 
  return arg.Equals(&a);

}



MATCHER_P(StrValueEq, a, std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {

MATCHER_P(StrValueEq, a,

          std::string(negation ? "isn't" : "is") + " equal to " + ::testing::PrintToString(a)) {

  return *(arg.value) == a;

}



MATCHER_P(HasValue, name,

          std::string(negation ? "does not have" : "has") + " value " +

              ::testing::PrintToString(name)) {

  return GetValueForConfig<Value>(&(*arg), name, {}) != nullptr;

}



MATCHER_P2(HasValue, name, config,

           std::string(negation ? "does not have" : "has") + " value " +

               ::testing::PrintToString(name) + " for config " + ::testing::PrintToString(config)) {

  return GetValueForConfig<Value>(&(*arg), name, config) != nullptr;

}



}  // namespace test

}  // namespace aapt