From f80d0ae89b118414e4c2f994fd684f4a47323b1a Mon Sep 17 00:00:00 2001
From: Donald Chai <dchai@google.com>
Date: Mon, 6 Jan 2020 13:52:41 -0800
Subject: [PATCH] Add option to avoid clobbering visibility of
 <declare-styleable>

"aapt2 compile" marks styleables as public instead of preserving
information from <public/> or --visibility options.  This behavior can
now be disabled via --preserve-visibility-of-styleables.

Bug: 146649511
Change-Id: Ifb8ab396573d1393df737a59625e79e9cf2494a7
Tested: aapt2_tests
---
 tools/aapt2/ResourceParser.cpp      | 10 ++++++++--
 tools/aapt2/ResourceParser.h        |  6 ++++++
 tools/aapt2/ResourceParser_test.cpp | 26 ++++++++++++++++++++++++++
 tools/aapt2/cmd/Compile.cpp         |  1 +
 tools/aapt2/cmd/Compile.h           |  7 +++++++
 5 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp
index 45cea81908448..40eae545c06b9 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -1643,8 +1643,14 @@ bool ResourceParser::ParseDeclareStyleable(xml::XmlPullParser* parser,
                                            ParsedResource* out_resource) {
   out_resource->name.type = ResourceType::kStyleable;
 
-  // Declare-styleable is kPrivate by default, because it technically only exists in R.java.
-  out_resource->visibility_level = Visibility::Level::kPublic;
+  if (!options_.preserve_visibility_of_styleables) {
+    // This was added in change Idd21b5de4d20be06c6f8c8eb5a22ccd68afc4927 to mimic aapt1, but no one
+    // knows exactly what for.
+    //
+    // FWIW, styleables only appear in generated R classes.  For custom views these should always be
+    // package-private (to be used only by the view class); themes are a different story.
+    out_resource->visibility_level = Visibility::Level::kPublic;
+  }
 
   // Declare-styleable only ends up in default config;
   if (out_resource->config != ConfigDescription::DefaultConfig()) {
diff --git a/tools/aapt2/ResourceParser.h b/tools/aapt2/ResourceParser.h
index 06bb0c9cf264a..9d3ecc866c5df 100644
--- a/tools/aapt2/ResourceParser.h
+++ b/tools/aapt2/ResourceParser.h
@@ -46,6 +46,12 @@ struct ResourceParserOptions {
    */
   bool error_on_positional_arguments = true;
 
+  /**
+   * If true, apply the same visibility rules for styleables as are used for
+   * all other resources.  Otherwise, all styleables will be made public.
+   */
+  bool preserve_visibility_of_styleables = false;
+
   // If visibility was forced, we need to use it when creating a new resource and also error if we
   // try to parse the <public>, <public-group>, <java-symbol> or <symbol> tags.
   Maybe<Visibility::Level> visibility;
diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp
index 464225fefb857..46ad7cbe49e92 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -611,6 +611,32 @@ TEST_F(ResourceParserTest, ParseAttributesDeclareStyleable) {
   EXPECT_THAT(styleable->entries[2].name, Eq(make_value(test::ParseNameOrDie("attr/baz"))));
 }
 
+TEST_F(ResourceParserTest, ParseDeclareStyleablePreservingVisibility) {
+  StringInputStream input(R"(
+      <resources>
+        <declare-styleable name="foo">
+          <attr name="myattr" />
+        </declare-styleable>
+        <declare-styleable name="bar">
+          <attr name="myattr" />
+        </declare-styleable>
+        <public type="styleable" name="bar" />
+      </resources>)");
+  ResourceParser parser(context_->GetDiagnostics(), &table_, Source{"test"},
+                        ConfigDescription::DefaultConfig(),
+                        ResourceParserOptions{.preserve_visibility_of_styleables = true});
+
+  xml::XmlPullParser xml_parser(&input);
+  ASSERT_TRUE(parser.Parse(&xml_parser));
+
+  EXPECT_EQ(
+      table_.FindResource(test::ParseNameOrDie("styleable/foo")).value().entry->visibility.level,
+      Visibility::Level::kUndefined);
+  EXPECT_EQ(
+      table_.FindResource(test::ParseNameOrDie("styleable/bar")).value().entry->visibility.level,
+      Visibility::Level::kPublic);
+}
+
 TEST_F(ResourceParserTest, ParsePrivateAttributesDeclareStyleable) {
   std::string input = R"(
       <declare-styleable xmlns:privAndroid="http://schemas.android.com/apk/prv/res/android"
diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp
index 9b81369fa9f04..21719705838dd 100644
--- a/tools/aapt2/cmd/Compile.cpp
+++ b/tools/aapt2/cmd/Compile.cpp
@@ -159,6 +159,7 @@ static bool CompileTable(IAaptContext* context, const CompileOptions& options,
 
     ResourceParserOptions parser_options;
     parser_options.error_on_positional_arguments = !options.legacy_mode;
+    parser_options.preserve_visibility_of_styleables = options.preserve_visibility_of_styleables;
     parser_options.translatable = translatable_file;
 
     // If visibility was forced, we need to use it when creating a new resource and also error if
diff --git a/tools/aapt2/cmd/Compile.h b/tools/aapt2/cmd/Compile.h
index d3456b25da9a0..1752a1adac246 100644
--- a/tools/aapt2/cmd/Compile.h
+++ b/tools/aapt2/cmd/Compile.h
@@ -35,6 +35,8 @@ struct CompileOptions {
   bool pseudolocalize = false;
   bool no_png_crunch = false;
   bool legacy_mode = false;
+  // See comments on aapt::ResourceParserOptions.
+  bool preserve_visibility_of_styleables = false;
   bool verbose = false;
 };
 
@@ -56,6 +58,11 @@ class CompileCommand : public Command {
     AddOptionalSwitch("--no-crunch", "Disables PNG processing", &options_.no_png_crunch);
     AddOptionalSwitch("--legacy", "Treat errors that used to be valid in AAPT as warnings",
         &options_.legacy_mode);
+    AddOptionalSwitch("--preserve-visibility-of-styleables",
+                      "If specified, apply the same visibility rules for\n"
+                      "styleables as are used for all other resources.\n"
+                      "Otherwise, all stylesables will be made public.",
+                      &options_.preserve_visibility_of_styleables);
     AddOptionalFlag("--visibility",
         "Sets the visibility of the compiled resources to the specified\n"
             "level. Accepted levels: public, private, default", &visibility_);