Allow read/write flags on xml elements

This changes aapt2 so that it now allows xml elements with the
android:featureFlag attribute even when that attribute is read/write (or
not found altogether). This also makes it so that attribute is removed
when the flag is read only.

Bug: 377974898
Test: Automated
Flag: EXEMPT Aconfig not supported on host tools
Change-Id: I330eefe897ab6dd1301b073d7f69443e3428b5b6

6 files changed, 48 insertions, 20 deletions

diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index eb71189ffc46..4718fbf085f8 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -674,7 +674,8 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
             }
 
             FeatureFlagsFilterOptions flags_filter_options;
-            flags_filter_options.flags_must_be_readonly = true;
+            flags_filter_options.fail_on_unrecognized_flags = false;
+            flags_filter_options.flags_must_have_value = false;
             FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
             if (!flags_filter.Consume(context_, doc.get())) {
               return 1;
diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp
index 6cc42f17c0a1..a2dc8f8ce0fd 100644
--- a/tools/aapt2/cmd/Link_test.cpp
+++ b/tools/aapt2/cmd/Link_test.cpp
@@ -1026,7 +1026,7 @@ TEST_F(LinkTest, FeatureFlagDisabled_SdkAtMostUDC) {
                            .SetManifestFile(app_manifest)
                            .AddParameter("-I", android_apk)
                            .AddParameter("--java", app_java)
-                           .AddParameter("--feature-flags", "flag=false");
+                           .AddParameter("--feature-flags", "flag:ro=false");
 
   const std::string app_apk = GetTestPath("app.apk");
   BuildApk({}, app_apk, std::move(app_link_args), this, &diag);
diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/layout/layout1.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/layout/layout1.xml
index 8b9ce134a9de..c595cdcff482 100644
--- a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/layout/layout1.xml
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/layout/layout1.xml
@@ -6,12 +6,14 @@
 
     <TextView android:id="@+id/text1"
         android:layout_width="wrap_content"
-        android:layout_height="wrap_content"/>
+        android:layout_height="wrap_content"
+        android:featureFlag="test.package.readWriteFlag"/>
     <TextView android:id="@+id/disabled_text"
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
         android:featureFlag="test.package.falseFlag" />
     <TextView android:id="@+id/text2"
+        android:text="FIND_ME"
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
         android:featureFlag="test.package.trueFlag" />
diff --git a/tools/aapt2/link/FeatureFlagsFilter.cpp b/tools/aapt2/link/FeatureFlagsFilter.cpp
index 4e7c1b4d8e54..23f78388b930 100644
--- a/tools/aapt2/link/FeatureFlagsFilter.cpp
+++ b/tools/aapt2/link/FeatureFlagsFilter.cpp
@@ -50,7 +50,7 @@ class FlagsVisitor : public xml::Visitor {
 
  private:
   bool ShouldRemove(std::unique_ptr<xml::Node>& node) {
-    if (const auto* el = NodeCast<Element>(node.get())) {
+    if (auto* el = NodeCast<Element>(node.get())) {
       auto* attr = el->FindAttribute(xml::kSchemaAndroid, "featureFlag");
       if (attr == nullptr) {
         return false;
@@ -72,9 +72,13 @@ class FlagsVisitor : public xml::Visitor {
             has_error_ = true;
             return false;
           }
-          if (options_.remove_disabled_elements) {
+          if (options_.remove_disabled_elements && it->second.read_only) {
             // Remove if flag==true && attr=="!flag" (negated) OR flag==false && attr=="flag"
-            return *it->second.enabled == negated;
+            bool remove = *it->second.enabled == negated;
+            if (!remove) {
+              el->RemoveAttribute(xml::kSchemaAndroid, "featureFlag");
+            }
+            return remove;
           }
         } else if (options_.flags_must_have_value) {
           diagnostics_->Error(android::DiagMessage(node->line_number)
diff --git a/tools/aapt2/link/FeatureFlagsFilter_test.cpp b/tools/aapt2/link/FeatureFlagsFilter_test.cpp
index 2db2899e716c..744045588506 100644
--- a/tools/aapt2/link/FeatureFlagsFilter_test.cpp
+++ b/tools/aapt2/link/FeatureFlagsFilter_test.cpp
@@ -48,7 +48,7 @@ TEST(FeatureFlagsFilterTest, NoFeatureFlagAttributes) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, false}}});
+                    {{"flag", FeatureFlagProperties{true, false}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -60,7 +60,7 @@ TEST(FeatureFlagsFilterTest, RemoveElementWithDisabledFlag) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="flag" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, false}}});
+                    {{"flag", FeatureFlagProperties{true, false}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -73,7 +73,7 @@ TEST(FeatureFlagsFilterTest, RemoveElementWithNegatedEnabledFlag) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="!flag" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -86,7 +86,7 @@ TEST(FeatureFlagsFilterTest, KeepElementWithEnabledFlag) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="flag" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -102,7 +102,7 @@ TEST(FeatureFlagsFilterTest, SideBySideEnabledAndDisabled) {
       <permission android:name="FOO" android:featureFlag="flag"
                   android:protectionLevel="dangerous" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -123,7 +123,7 @@ TEST(FeatureFlagsFilterTest, RemoveDeeplyNestedElement) {
         </activity>
       </application>
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -145,7 +145,7 @@ TEST(FeatureFlagsFilterTest, KeepDeeplyNestedElement) {
         </activity>
       </application>
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
   ASSERT_THAT(root, NotNull());
@@ -162,7 +162,7 @@ TEST(FeatureFlagsFilterTest, FailOnEmptyFeatureFlagAttribute) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag=" " />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, false}}});
+                    {{"flag", FeatureFlagProperties{true, false}}});
   ASSERT_THAT(doc, IsNull());
 }
 
@@ -171,7 +171,7 @@ TEST(FeatureFlagsFilterTest, FailOnFlagWithNoGivenValue) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="flag" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, std::nullopt}}});
+                    {{"flag", FeatureFlagProperties{true, std::nullopt}}});
   ASSERT_THAT(doc, IsNull());
 }
 
@@ -180,7 +180,7 @@ TEST(FeatureFlagsFilterTest, FailOnUnrecognizedFlag) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="unrecognized" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, true}}});
+                    {{"flag", FeatureFlagProperties{true, true}}});
   ASSERT_THAT(doc, IsNull());
 }
 
@@ -190,7 +190,7 @@ TEST(FeatureFlagsFilterTest, FailOnMultipleValidationErrors) {
       <permission android:name="FOO" android:featureFlag="bar" />
       <permission android:name="FOO" android:featureFlag="unrecognized" />
     </manifest>)EOF",
-                    {{"flag", FeatureFlagProperties{false, std::nullopt}}});
+                    {{"flag", FeatureFlagProperties{true, std::nullopt}}});
   ASSERT_THAT(doc, IsNull());
 }
 
@@ -199,7 +199,7 @@ TEST(FeatureFlagsFilterTest, OptionRemoveDisabledElementsIsFalse) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="flag" />
     </manifest>)EOF",
-                               {{"flag", FeatureFlagProperties{false, false}}},
+                               {{"flag", FeatureFlagProperties{true, false}}},
                                {.remove_disabled_elements = false});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
@@ -213,7 +213,7 @@ TEST(FeatureFlagsFilterTest, OptionFlagsMustHaveValueIsFalse) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="flag" />
     </manifest>)EOF",
-                               {{"flag", FeatureFlagProperties{false, std::nullopt}}},
+                               {{"flag", FeatureFlagProperties{true, std::nullopt}}},
                                {.flags_must_have_value = false});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
@@ -227,7 +227,7 @@ TEST(FeatureFlagsFilterTest, OptionFailOnUnrecognizedFlagsIsFalse) {
     <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="android">
       <permission android:name="FOO" android:featureFlag="unrecognized" />
     </manifest>)EOF",
-                               {{"flag", FeatureFlagProperties{false, true}}},
+                               {{"flag", FeatureFlagProperties{true, true}}},
                                {.fail_on_unrecognized_flags = false});
   ASSERT_THAT(doc, NotNull());
   auto root = doc->root.get();
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
index 629300838bbe..adf711ecfcbb 100644
--- a/tools/aapt2/link/FlaggedResources_test.cpp
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -59,6 +59,16 @@ void DumpChunksToString(LoadedApk* loaded_apk, std::string* output) {
   output_stream.Flush();
 }
 
+void DumpXmlTreeToString(LoadedApk* loaded_apk, std::string file, std::string* output) {
+  StringOutputStream output_stream(output);
+  Printer printer(&output_stream);
+
+  auto xml = loaded_apk->LoadXml(file, &noop_diag);
+  ASSERT_NE(xml, nullptr);
+  Debug::DumpXml(*xml, &printer);
+  output_stream.Flush();
+}
+
 TEST_F(FlaggedResourcesTest, DisabledStringRemovedFromPool) {
   auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
   auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
@@ -148,4 +158,15 @@ TEST_F(FlaggedResourcesTest, TwoValuesSameDisabledFlagDifferentFiles) {
   ASSERT_TRUE(diag.GetLog().contains("duplicate value for resource 'bool1'"));
 }
 
+TEST_F(FlaggedResourcesTest, EnabledXmlELementAttributeRemoved) {
+  auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
+  auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
+
+  std::string output;
+  DumpXmlTreeToString(loaded_apk.get(), "res/layout-v22/layout1.xml", &output);
+  ASSERT_FALSE(output.contains("test.package.trueFlag"));
+  ASSERT_TRUE(output.contains("FIND_ME"));
+  ASSERT_TRUE(output.contains("test.package.readWriteFlag"));
+}
+
 }  // namespace aapt