AAPT2: Erase version qualifiers for resources <= minSdk

When resources share the same configuration, they are much more
clustered in the resulting resources.arsc, which makes for smaller
APKs. Strip version qualifiers for resources <= minSdk so that
they get clustered.

Bug:30050641
Change-Id: I80371b179761501fb7cc41f5f5ac67ffde2fc677
diff --git a/tools/aapt2/link/VersionCollapser.cpp b/tools/aapt2/link/VersionCollapser.cpp
index c0e96be..949d656 100644
--- a/tools/aapt2/link/VersionCollapser.cpp
+++ b/tools/aapt2/link/VersionCollapser.cpp
@@ -109,6 +109,32 @@
                    [](const std::unique_ptr<ResourceConfigValue>& val) -> bool {
         return val == nullptr;
     }), entry->values.end());
+
+    // Strip the version qualifiers for every resource with version <= minSdk. This will ensure
+    // that the resource entries are all packed together in the same ResTable_type struct
+    // and take up less space in the resources.arsc table.
+    bool modified = false;
+    for (std::unique_ptr<ResourceConfigValue>& configValue : entry->values) {
+        if (configValue->config.sdkVersion != 0 && configValue->config.sdkVersion <= minSdk) {
+            // Override the resource with a Configuration without an SDK.
+            std::unique_ptr<ResourceConfigValue> newValue = util::make_unique<ResourceConfigValue>(
+                    configValue->config.copyWithoutSdkVersion(), configValue->product);
+            newValue->value = std::move(configValue->value);
+            configValue = std::move(newValue);
+
+            modified = true;
+        }
+    }
+
+    if (modified) {
+        // We've modified the keys (ConfigDescription) by changing the sdkVersion to 0.
+        // We MUST re-sort to ensure ordering guarantees hold.
+        std::sort(entry->values.begin(), entry->values.end(),
+                  [](const std::unique_ptr<ResourceConfigValue>& a,
+                     const std::unique_ptr<ResourceConfigValue>& b) -> bool {
+            return a->config.compare(b->config) < 0;
+        });
+    }
 }
 
 bool VersionCollapser::consume(IAaptContext* context, ResourceTable* table) {
diff --git a/tools/aapt2/link/VersionCollapser_test.cpp b/tools/aapt2/link/VersionCollapser_test.cpp
index 4113450..dd5f1d1 100644
--- a/tools/aapt2/link/VersionCollapser_test.cpp
+++ b/tools/aapt2/link/VersionCollapser_test.cpp
@@ -49,12 +49,17 @@
               test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v4")));
     EXPECT_EQ(nullptr,
               test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v5")));
+    // This one should be removed because it was renamed to 'land', with the version dropped.
+    EXPECT_EQ(nullptr,
+              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v6")));
 
     // These should remain.
     EXPECT_NE(nullptr,
               test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("sw600dp")));
+
+    // 'land' should be present because it was renamed from 'land-v6'.
     EXPECT_NE(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v6")));
+              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land")));
     EXPECT_NE(nullptr,
               test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v14")));
     EXPECT_NE(nullptr,
@@ -62,32 +67,37 @@
 }
 
 TEST(VersionCollapserTest, CollapseVersionsWhenMinSdkIsHighest) {
-    uptr<IAaptContext> context = test::ContextBuilder().setMinSdkVersion(26).build();
+    uptr<IAaptContext> context = test::ContextBuilder().setMinSdkVersion(21).build();
 
     const StringPiece resName = "@android:string/foo";
 
     uptr<ResourceTable> table =
                 buildTableWithConfigs(resName,
                                       { "land-v4", "land-v5", "sw600dp", "land-v6",
-                                              "land-v14", "land-v21" });
+                                              "land-v14", "land-v21", "land-v22" });
     VersionCollapser collapser;
     ASSERT_TRUE(collapser.consume(context.get(), table.get()));
 
     // These should all be removed.
-    EXPECT_EQ(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v4")));
-    EXPECT_EQ(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v5")));
-    EXPECT_EQ(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v6")));
-    EXPECT_EQ(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v14")));
+    EXPECT_EQ(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land-v4")));
+    EXPECT_EQ(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land-v5")));
+    EXPECT_EQ(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land-v6")));
+    EXPECT_EQ(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land-v14")));
 
     // These should remain.
-    EXPECT_NE(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("sw600dp")));
-    EXPECT_NE(nullptr,
-              test::getValueForConfig<Id>(table.get(), resName, test::parseConfigOrDie("land-v21")));
+    EXPECT_NE(nullptr, test::getValueForConfig<Id>(
+            table.get(), resName, test::parseConfigOrDie("sw600dp").copyWithoutSdkVersion()));
+
+    // land-v21 should have been converted to land.
+    EXPECT_NE(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land")));
+    // land-v22 should remain as-is.
+    EXPECT_NE(nullptr, test::getValueForConfig<Id>(table.get(), resName,
+                                                   test::parseConfigOrDie("land-v22")));
 }
 
 } // namespace aapt