From 121c6e8aa037efcbd047bdd5f53e6686a07fa002 Mon Sep 17 00:00:00 2001
From: Donald Chai <dchai@google.com>
Date: Wed, 12 Jun 2019 12:51:57 -0700
Subject: [PATCH] [aapt2] Add "link" option to override styles instead of
 overlaying.

For normal app development, the desired linking semantics are:
* styleables - take union of all definitions
* all other resources - take last non-weak definition

This differs from the semantics needed in other scenarios, where
merging/overlaying styles is desired.

Bug: 134525082
Change-Id: Iac0c43ca2ecf1f3fddc9c3367f8914c12c9258e1
Tested: aapt2_tests
---
 tools/aapt2/cmd/Link.cpp              |  2 +
 tools/aapt2/cmd/Link.h                |  5 ++
 tools/aapt2/cmd/Link_test.cpp         | 84 ++++++++++++++++++++++++++-
 tools/aapt2/link/TableMerger.cpp      | 27 +++++----
 tools/aapt2/link/TableMerger.h        |  2 +
 tools/aapt2/link/TableMerger_test.cpp | 47 +++++++++++++++
 6 files changed, 155 insertions(+), 12 deletions(-)

diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 4b977225fd015..04d12f829e3cc 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -1691,6 +1691,8 @@ class Linker {
 
     TableMergerOptions table_merger_options;
     table_merger_options.auto_add_overlay = options_.auto_add_overlay;
+    table_merger_options.override_styles_instead_of_overlaying =
+        options_.override_styles_instead_of_overlaying;
     table_merger_options.strict_visibility = options_.strict_visibility;
     table_merger_ = util::make_unique<TableMerger>(context_, &final_table_, table_merger_options);
 
diff --git a/tools/aapt2/cmd/Link.h b/tools/aapt2/cmd/Link.h
index 5b0653ed53bd7..37765f6b5d088 100644
--- a/tools/aapt2/cmd/Link.h
+++ b/tools/aapt2/cmd/Link.h
@@ -42,6 +42,7 @@ struct LinkOptions {
   std::vector<std::string> assets_dirs;
   bool output_to_directory = false;
   bool auto_add_overlay = false;
+  bool override_styles_instead_of_overlaying = false;
   OutputFormat output_format = OutputFormat::kApk;
 
   // Java/Proguard options.
@@ -242,6 +243,10 @@ class LinkCommand : public Command {
         "Allows the addition of new resources in overlays without\n"
             "<add-resource> tags.",
         &options_.auto_add_overlay);
+    AddOptionalSwitch("--override-styles-instead-of-overlaying",
+                      "Causes styles defined in -R resources to replace previous definitions\n"
+                      "instead of merging into them\n",
+                      &options_.override_styles_instead_of_overlaying);
     AddOptionalFlag("--rename-manifest-package", "Renames the package in AndroidManifest.xml.",
         &options_.manifest_fixer_options.rename_manifest_package);
     AddOptionalFlag("--rename-instrumentation-target-package",
diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp
index 32ed1dd81b3ff..bf8f043f6b68b 100644
--- a/tools/aapt2/cmd/Link_test.cpp
+++ b/tools/aapt2/cmd/Link_test.cpp
@@ -171,4 +171,86 @@ TEST_F(LinkTest, NoCompressResources) {
   EXPECT_FALSE(file->WasCompressed());
 }
 
-}  // namespace aapt
\ No newline at end of file
+TEST_F(LinkTest, OverlayStyles) {
+  StdErrDiagnostics diag;
+  const std::string compiled_files_dir = GetTestPath("compiled");
+  const std::string override_files_dir = GetTestPath("compiled-override");
+  ASSERT_TRUE(CompileFile(GetTestPath("res/values/values.xml"),
+                          R"(<resources>
+                               <style name="MyStyle">
+                                 <item name="android:textColor">#123</item>
+                               </style>
+                             </resources>)",
+                          compiled_files_dir, &diag));
+  ASSERT_TRUE(CompileFile(GetTestPath("res/values/values-override.xml"),
+                          R"(<resources>
+                               <style name="MyStyle">
+                                 <item name="android:background">#456</item>
+                               </style>
+                             </resources>)",
+                          override_files_dir, &diag));
+
+
+  const std::string out_apk = GetTestPath("out.apk");
+  std::vector<std::string> link_args = {
+      "--manifest", GetDefaultManifest(kDefaultPackageName),
+      "-o", out_apk,
+  };
+  const auto override_files = file::FindFiles(override_files_dir, &diag);
+  for (const auto &override_file : override_files.value()) {
+      link_args.push_back("-R");
+      link_args.push_back(file::BuildPath({override_files_dir, override_file}));
+  }
+  ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag));
+
+  std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  const Style* actual_style = test::GetValue<Style>(
+      apk->GetResourceTable(), std::string(kDefaultPackageName) + ":style/MyStyle");
+  ASSERT_NE(actual_style, nullptr);
+  ASSERT_EQ(actual_style->entries.size(), 2);
+  EXPECT_EQ(actual_style->entries[0].key.id, 0x01010098);  // android:textColor
+  EXPECT_EQ(actual_style->entries[1].key.id, 0x010100d4);  // android:background
+}
+
+TEST_F(LinkTest, OverrideStylesInsteadOfOverlaying) {
+  StdErrDiagnostics diag;
+  const std::string compiled_files_dir = GetTestPath("compiled");
+  const std::string override_files_dir = GetTestPath("compiled-override");
+  ASSERT_TRUE(CompileFile(GetTestPath("res/values/values.xml"),
+                          R"(<resources>
+                               <style name="MyStyle">
+                                 <item name="android:textColor">#123</item>
+                               </style>
+                             </resources>)",
+                          compiled_files_dir, &diag));
+  ASSERT_TRUE(CompileFile(GetTestPath("res/values/values-override.xml"),
+                          R"(<resources>
+                               <style name="MyStyle">
+                                 <item name="android:background">#456</item>
+                               </style>
+                             </resources>)",
+                          override_files_dir, &diag));
+
+
+  const std::string out_apk = GetTestPath("out.apk");
+  std::vector<std::string> link_args = {
+      "--manifest", GetDefaultManifest(kDefaultPackageName),
+      "--override-styles-instead-of-overlaying",
+      "-o", out_apk,
+  };
+  const auto override_files = file::FindFiles(override_files_dir, &diag);
+  for (const auto &override_file : override_files.value()) {
+      link_args.push_back("-R");
+      link_args.push_back(file::BuildPath({override_files_dir, override_file}));
+  }
+  ASSERT_TRUE(Link(link_args, compiled_files_dir, &diag));
+
+  std::unique_ptr<LoadedApk> apk = LoadedApk::LoadApkFromPath(out_apk, &diag);
+  const Style* actual_style = test::GetValue<Style>(
+      apk->GetResourceTable(), std::string(kDefaultPackageName) + ":style/MyStyle");
+  ASSERT_NE(actual_style, nullptr);
+  ASSERT_EQ(actual_style->entries.size(), 1);
+  EXPECT_EQ(actual_style->entries[0].key.id, 0x010100d4);  // android:background
+}
+
+}  // namespace aapt
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index c0802e60103a6..c25e4503a2087 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -172,19 +172,22 @@ static bool MergeEntry(IAaptContext* context, const Source& src,
 //
 // Styleables and Styles don't simply overlay each other, their definitions merge and accumulate.
 // If both values are Styleables/Styles, we just merge them into the existing value.
-static ResourceTable::CollisionResult ResolveMergeCollision(Value* existing, Value* incoming,
-                                                            StringPool* pool) {
+static ResourceTable::CollisionResult ResolveMergeCollision(
+    bool override_styles_instead_of_overlaying, Value* existing, Value* incoming,
+    StringPool* pool) {
   if (Styleable* existing_styleable = ValueCast<Styleable>(existing)) {
     if (Styleable* incoming_styleable = ValueCast<Styleable>(incoming)) {
       // Styleables get merged.
       existing_styleable->MergeWith(incoming_styleable);
       return ResourceTable::CollisionResult::kKeepOriginal;
     }
-  } else if (Style* existing_style = ValueCast<Style>(existing)) {
-    if (Style* incoming_style = ValueCast<Style>(incoming)) {
-      // Styles get merged.
-      existing_style->MergeWith(incoming_style, pool);
-      return ResourceTable::CollisionResult::kKeepOriginal;
+  } else if (!override_styles_instead_of_overlaying) {
+    if (Style* existing_style = ValueCast<Style>(existing)) {
+      if (Style* incoming_style = ValueCast<Style>(incoming)) {
+        // Styles get merged.
+        existing_style->MergeWith(incoming_style, pool);
+        return ResourceTable::CollisionResult::kKeepOriginal;
+      }
     }
   }
   // Delegate to the default handler.
@@ -194,6 +197,7 @@ static ResourceTable::CollisionResult ResolveMergeCollision(Value* existing, Val
 static ResourceTable::CollisionResult MergeConfigValue(IAaptContext* context,
                                                        const ResourceNameRef& res_name,
                                                        bool overlay,
+                                                       bool override_styles_instead_of_overlaying,
                                                        ResourceConfigValue* dst_config_value,
                                                        ResourceConfigValue* src_config_value,
                                                        StringPool* pool) {
@@ -204,7 +208,8 @@ static ResourceTable::CollisionResult MergeConfigValue(IAaptContext* context,
 
   CollisionResult collision_result;
   if (overlay) {
-    collision_result = ResolveMergeCollision(dst_value, src_value, pool);
+    collision_result =
+        ResolveMergeCollision(override_styles_instead_of_overlaying, dst_value, src_value, pool);
   } else {
     collision_result = ResourceTable::ResolveValueCollision(dst_value, src_value);
   }
@@ -272,9 +277,9 @@ bool TableMerger::DoMerge(const Source& src, ResourceTablePackage* src_package,
         ResourceConfigValue* dst_config_value = dst_entry->FindValue(
             src_config_value->config, src_config_value->product);
         if (dst_config_value) {
-          CollisionResult collision_result =
-              MergeConfigValue(context_, res_name, overlay, dst_config_value,
-                               src_config_value.get(), &master_table_->string_pool);
+          CollisionResult collision_result = MergeConfigValue(
+              context_, res_name, overlay, options_.override_styles_instead_of_overlaying,
+              dst_config_value, src_config_value.get(), &master_table_->string_pool);
           if (collision_result == CollisionResult::kConflict) {
             error = true;
             continue;
diff --git a/tools/aapt2/link/TableMerger.h b/tools/aapt2/link/TableMerger.h
index 51305cfcdd255..a35a134a887d2 100644
--- a/tools/aapt2/link/TableMerger.h
+++ b/tools/aapt2/link/TableMerger.h
@@ -37,6 +37,8 @@ struct TableMergerOptions {
   bool auto_add_overlay = false;
   // If true, resource overlays with conflicting visibility are not allowed.
   bool strict_visibility = false;
+  // If true, styles specified via "aapt2 link -R" completely replace any previously-seen resources.
+  bool override_styles_instead_of_overlaying = false;
 };
 
 // TableMerger takes resource tables and merges all packages within the tables that have the same
diff --git a/tools/aapt2/link/TableMerger_test.cpp b/tools/aapt2/link/TableMerger_test.cpp
index 9dd31e6829378..0be4ccf9ae859 100644
--- a/tools/aapt2/link/TableMerger_test.cpp
+++ b/tools/aapt2/link/TableMerger_test.cpp
@@ -436,6 +436,53 @@ TEST_F(TableMergerTest, OverlaidStyleablesAndStylesShouldBeMerged) {
               Eq(make_value(Reference(test::ParseNameOrDie("com.app.a:style/OverlayParent")))));
 }
 
+TEST_F(TableMergerTest, OverrideStyleInsteadOfOverlaying) {
+  std::unique_ptr<ResourceTable> table_a =
+      test::ResourceTableBuilder()
+          .SetPackageId("com.app.a", 0x7f)
+          .AddValue(
+              "com.app.a:styleable/MyWidget",
+              test::StyleableBuilder().AddItem("com.app.a:attr/foo", ResourceId(0x1234)).Build())
+          .AddValue("com.app.a:style/Theme",
+                    test::StyleBuilder()
+                        .AddItem("com.app.a:attr/foo", ResourceUtils::MakeBool(false))
+                        .Build())
+          .Build();
+  std::unique_ptr<ResourceTable> table_b =
+      test::ResourceTableBuilder()
+          .SetPackageId("com.app.a", 0x7f)
+          .AddValue(
+              "com.app.a:styleable/MyWidget",
+              test::StyleableBuilder().AddItem("com.app.a:attr/bar", ResourceId(0x5678)).Build())
+          .AddValue(
+              "com.app.a:style/Theme",
+              test::StyleBuilder().AddItem("com.app.a:attr/bat", util::make_unique<Id>()).Build())
+          .Build();
+
+  ResourceTable final_table;
+  TableMergerOptions options;
+  options.auto_add_overlay = true;
+  options.override_styles_instead_of_overlaying = true;
+  TableMerger merger(context_.get(), &final_table, options);
+  ASSERT_TRUE(merger.Merge({}, table_a.get(), false /*overlay*/));
+  ASSERT_TRUE(merger.Merge({}, table_b.get(), true /*overlay*/));
+
+  // Styleables are always overlaid
+  std::unique_ptr<Styleable> expected_styleable = test::StyleableBuilder()
+      // The merged Styleable has its entries ordered by name.
+      .AddItem("com.app.a:attr/bar", ResourceId(0x5678))
+      .AddItem("com.app.a:attr/foo", ResourceId(0x1234))
+      .Build();
+  const Styleable* actual_styleable =
+      test::GetValue<Styleable>(&final_table, "com.app.a:styleable/MyWidget");
+  ASSERT_NE(actual_styleable, nullptr);
+  EXPECT_TRUE(actual_styleable->Equals(expected_styleable.get()));
+  // Style should be overridden
+  const Style* actual_style = test::GetValue<Style>(&final_table, "com.app.a:style/Theme");
+  ASSERT_NE(actual_style, nullptr);
+  EXPECT_TRUE(actual_style->Equals(test::GetValue<Style>(table_b.get(), "com.app.a:style/Theme")));
+}
+
 TEST_F(TableMergerTest, SetOverlayable) {
   auto overlayable = std::make_shared<Overlayable>("CustomizableResources",
                                                   "overlay://customization");