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

am: 242c22c900 Change-Id: I8fa65b89f1f10fe8920aa45d14c7b917b6bc74d8
2017-08-07 17:43:05 +00:00
parent 13fac42125 242c22c900
commit dc901a6318
4 changed files with 118 additions and 62 deletions
--- a/tools/aapt2/ConfigDescription.cpp
+++ b/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) &&
--- a/tools/aapt2/DominatorTree_test.cpp
+++ b/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 =
-      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));
 }

--- a/tools/aapt2/optimize/ResourceDeduper_test.cpp
+++ b/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,
-                     "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
--- a/tools/aapt2/test/Common.h
+++ b/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