Merge "AAPT2: Disable locale domination for deduping" into oc-dr1-dev

tools/aapt2/ConfigDescription.cpp

@@ -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

@@ -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 =
+  const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");
-      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(
+  configs.push_back(util::make_unique<ResourceConfigValue>(sw600dp_land_config, ""));
-      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 =
+  const ConfigDescription sw600dp_land_config = test::ParseConfigOrDie("sw600dp-land-v13");
-      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(
+  configs.push_back(util::make_unique<ResourceConfigValue>(default_config, "phablet"));
-      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>(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 =
+  const ConfigDescription ldrtl_xhdpi_config = test::ParseConfigOrDie("ldrtl-xhdpi-v4");
-      test::ParseConfigOrDie("ldrtl-xhdpi-v4");
+  const ConfigDescription sw300dp_config = test::ParseConfigOrDie("sw300dp-v13");
-  const ConfigDescription sw300dp_config =
+  const ConfigDescription sw540dp_config = test::ParseConfigOrDie("sw540dp-v14");
-      test::ParseConfigOrDie("sw300dp-v13");
+  const ConfigDescription sw600dp_config = test::ParseConfigOrDie("sw600dp-v14");
-  const ConfigDescription sw540dp_config =
+  const ConfigDescription sw720dp_config = test::ParseConfigOrDie("sw720dp-v13");
-      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(
+  configs.push_back(util::make_unique<ResourceConfigValue>(ldrtl_xhdpi_config, ""));
-      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

@@ -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,
+          .AddString("android:string/dedupe", ResourceId{}, default_config, "dedupe")
-                     "dedupe")
+          .AddString("android:string/dedupe", ResourceId{}, ldrtl_config, "dedupe")
-          .AddString("android:string/dedupe", ResourceId{}, en_config, "dedupe")
+          .AddString("android:string/dedupe", ResourceId{}, land_config, "dedupe")
-          .AddString("android:string/dedupe", ResourceId{}, land_config,
+
-                     "dedupe")
+          .AddString("android:string/dedupe2", ResourceId{}, default_config, "dedupe")
-          .AddString("android:string/dedupe2", ResourceId{}, default_config,
+          .AddString("android:string/dedupe2", ResourceId{}, ldrtl_config, "dedupe")
-                     "dedupe")
+          .AddString("android:string/dedupe2", ResourceId{}, ldrtl_v21_config, "keep")
-          .AddString("android:string/dedupe2", ResourceId{}, en_config,
+          .AddString("android:string/dedupe2", ResourceId{}, land_config, "dedupe")
-                     "dedupe")
+
-          .AddString("android:string/dedupe2", ResourceId{}, en_v21_config,
+          .AddString("android:string/dedupe3", ResourceId{}, default_config, "dedupe")
-                     "keep")
+          .AddString("android:string/dedupe3", ResourceId{}, en_config, "dedupe")
-          .AddString("android:string/dedupe2", ResourceId{}, land_config,
+          .AddString("android:string/dedupe3", ResourceId{}, en_v21_config, "dedupe")
-                     "dedupe")
           .Build();
 
   ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
-  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
+  EXPECT_THAT(table, Not(HasValue("android:string/dedupe", ldrtl_config)));
-                         table.get(), "android:string/dedupe", en_config));
+  EXPECT_THAT(table, Not(HasValue("android:string/dedupe", land_config)));
-  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
+
-                         table.get(), "android:string/dedupe", land_config));
+  EXPECT_THAT(table, HasValue("android:string/dedupe2", ldrtl_v21_config));
-  EXPECT_EQ(nullptr, test::GetValueForConfig<String>(
+  EXPECT_THAT(table, Not(HasValue("android:string/dedupe2", ldrtl_config)));
-                         table.get(), "android:string/dedupe2", en_config));
+
-  EXPECT_NE(nullptr, test::GetValueForConfig<String>(
+  EXPECT_THAT(table, HasValue("android:string/dedupe3", default_config));
-                         table.get(), "android:string/dedupe2", en_v21_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 ldrtl_config = test::ParseConfigOrDie("ldrtl");
-  const ConfigDescription en_v21_config = test::ParseConfigOrDie("en-v21");
+  const ConfigDescription ldrtl_v21_config = test::ParseConfigOrDie("ldrtl-v21");
-  // Chosen because this configuration is compatible with en.
+  // 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,
+          .AddString("android:string/keep", ResourceId{}, default_config, "keep")
-                     "keep")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_config, "keep")
-          .AddString("android:string/keep", ResourceId{}, en_config, "keep")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_v21_config, "keep2")
-          .AddString("android:string/keep", ResourceId{}, en_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>(
+  EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_config));
-                         table.get(), "android:string/keep", en_config));
+  EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_v21_config));
-  EXPECT_NE(nullptr, test::GetValueForConfig<String>(
+  EXPECT_THAT(table, HasValue("android:string/keep", land_config));
-                         table.get(), "android:string/keep", en_v21_config));
+}
-  EXPECT_NE(nullptr, test::GetValueForConfig<String>(
+
-                         table.get(), "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

@@ -156,6 +156,23 @@ MATCHER_P(ValueEq, a,
   return arg.Equals(&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