From 24b8ff0faf7c59323d0171cdd825ca09e712aa1e Mon Sep 17 00:00:00 2001
From: Adam Lesinski <adamlesinski@google.com>
Date: Wed, 16 Dec 2015 14:01:57 -0800
Subject: [PATCH] AAPT2: Fix references to private parent

Change-Id: Id4697551b6c8cb6167f562de593006ae3c6158c0
---
 tools/aapt2/Debug.cpp                         | 12 ++++++---
 tools/aapt2/ResourceParser_test.cpp           | 26 ++++++++++++------
 tools/aapt2/ResourceUtils.cpp                 | 17 ++++++------
 tools/aapt2/ResourceUtils_test.cpp            |  5 ++++
 tools/aapt2/ResourceValues.cpp                |  3 +++
 tools/aapt2/flatten/TableFlattener.cpp        | 27 +++++++++----------
 tools/aapt2/java/ProguardRules.cpp            |  4 +--
 .../aapt2/unflatten/BinaryResourceParser.cpp  |  7 +++++
 8 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/tools/aapt2/Debug.cpp b/tools/aapt2/Debug.cpp
index d864f664f9db3..5fce2c16f6306 100644
--- a/tools/aapt2/Debug.cpp
+++ b/tools/aapt2/Debug.cpp
@@ -52,13 +52,17 @@ struct PrintVisitor : public ValueVisitor {
     void visit(Style* style) override {
         std::cout << "(style)";
         if (style->parent) {
+            const Reference& parentRef = style->parent.value();
             std::cout << " parent=";
-            if (style->parent.value().name) {
-                std::cout << style->parent.value().name.value() << " ";
+            if (parentRef.name) {
+                if (parentRef.privateReference) {
+                    std::cout << "*";
+                }
+                std::cout << parentRef.name.value() << " ";
             }
 
-            if (style->parent.value().id) {
-                std::cout << style->parent.value().id.value();
+            if (parentRef.id) {
+                std::cout << parentRef.id.value();
             }
         }
 
diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp
index 2cc94d4e97d57..ab44a064a9533 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -215,7 +215,7 @@ TEST_F(ResourceParserTest, ParseFlagAttr) {
     ASSERT_TRUE(testParse(input));
 
     Attribute* flagAttr = test::getValue<Attribute>(&mTable, u"@attr/foo");
-    ASSERT_NE(flagAttr, nullptr);
+    ASSERT_NE(nullptr, flagAttr);
     EXPECT_EQ(flagAttr->typeMask, android::ResTable_map::TYPE_FLAGS);
     ASSERT_EQ(flagAttr->symbols.size(), 3u);
 
@@ -233,7 +233,7 @@ TEST_F(ResourceParserTest, ParseFlagAttr) {
 
     std::unique_ptr<BinaryPrimitive> flagValue = ResourceUtils::tryParseFlagSymbol(flagAttr,
                                                                                    u"baz|bat");
-    ASSERT_NE(flagValue, nullptr);
+    ASSERT_NE(nullptr, flagValue);
     EXPECT_EQ(flagValue->value.data, 1u | 2u);
 }
 
@@ -255,7 +255,7 @@ TEST_F(ResourceParserTest, ParseStyle) {
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     AAPT_ASSERT_TRUE(style->parent);
     AAPT_ASSERT_TRUE(style->parent.value().name);
     EXPECT_EQ(test::parseNameOrDie(u"@style/fu"), style->parent.value().name.value());
@@ -276,7 +276,7 @@ TEST_F(ResourceParserTest, ParseStyleWithShorthandParent) {
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     AAPT_ASSERT_TRUE(style->parent);
     AAPT_ASSERT_TRUE(style->parent.value().name);
     EXPECT_EQ(test::parseNameOrDie(u"@com.app:style/Theme"), style->parent.value().name.value());
@@ -288,7 +288,7 @@ TEST_F(ResourceParserTest, ParseStyleWithPackageAliasedParent) {
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     AAPT_ASSERT_TRUE(style->parent);
     AAPT_ASSERT_TRUE(style->parent.value().name);
     EXPECT_EQ(test::parseNameOrDie(u"@android:style/Theme"), style->parent.value().name.value());
@@ -302,7 +302,7 @@ TEST_F(ResourceParserTest, ParseStyleWithPackageAliasedItems) {
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     ASSERT_EQ(1u, style->entries.size());
     EXPECT_EQ(test::parseNameOrDie(u"@android:attr/bar"), style->entries[0].key.name.value());
 }
@@ -312,7 +312,7 @@ TEST_F(ResourceParserTest, ParseStyleWithInferredParent) {
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo.bar");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     AAPT_ASSERT_TRUE(style->parent);
     AAPT_ASSERT_TRUE(style->parent.value().name);
     EXPECT_EQ(style->parent.value().name.value(), test::parseNameOrDie(u"@style/foo"));
@@ -324,11 +324,21 @@ TEST_F(ResourceParserTest, ParseStyleWithInferredParentOverridenByEmptyParentAtt
     ASSERT_TRUE(testParse(input));
 
     Style* style = test::getValue<Style>(&mTable, u"@style/foo.bar");
-    ASSERT_NE(style, nullptr);
+    ASSERT_NE(nullptr, style);
     AAPT_EXPECT_FALSE(style->parent);
     EXPECT_FALSE(style->parentInferred);
 }
 
+TEST_F(ResourceParserTest, ParseStyleWithPrivateParentReference) {
+    std::string input = R"EOF(<style name="foo" parent="*android:style/bar" />)EOF";
+    ASSERT_TRUE(testParse(input));
+
+    Style* style = test::getValue<Style>(&mTable, u"@style/foo");
+    ASSERT_NE(nullptr, style);
+    AAPT_ASSERT_TRUE(style->parent);
+    EXPECT_TRUE(style->parent.value().privateReference);
+}
+
 TEST_F(ResourceParserTest, ParseAutoGeneratedIdReference) {
     std::string input = "<string name=\"foo\">@+id/bar</string>";
     ASSERT_TRUE(testParse(input));
diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp
index 36c3e702574ed..1dc123e45949e 100644
--- a/tools/aapt2/ResourceUtils.cpp
+++ b/tools/aapt2/ResourceUtils.cpp
@@ -176,10 +176,10 @@ bool isAttributeReference(const StringPiece16& str) {
 /*
  * Style parent's are a bit different. We accept the following formats:
  *
- * @[package:]style/<entry>
- * ?[package:]style/<entry>
- * <package>:[style/]<entry>
- * [package:style/]<entry>
+ * @[[*]package:]style/<entry>
+ * ?[[*]package:]style/<entry>
+ * <[*]package>:[style/]<entry>
+ * [[*]package:style/]<entry>
  */
 Maybe<Reference> parseStyleParentReference(const StringPiece16& str, std::string* outError) {
     if (str.empty()) {
@@ -195,10 +195,11 @@ Maybe<Reference> parseStyleParentReference(const StringPiece16& str, std::string
     if (name.data()[0] == u'@' || name.data()[0] == u'?') {
         hasLeadingIdentifiers = true;
         name = name.substr(1, name.size() - 1);
-        if (name.data()[0] == u'*') {
-            privateRef = true;
-            name = name.substr(1, name.size() - 1);
-        }
+    }
+
+    if (name.data()[0] == u'*') {
+        privateRef = true;
+        name = name.substr(1, name.size() - 1);
     }
 
     ResourceNameRef ref;
diff --git a/tools/aapt2/ResourceUtils_test.cpp b/tools/aapt2/ResourceUtils_test.cpp
index 4bbfc32b9b371..88efa6779021b 100644
--- a/tools/aapt2/ResourceUtils_test.cpp
+++ b/tools/aapt2/ResourceUtils_test.cpp
@@ -157,6 +157,11 @@ TEST(ResourceUtilsTest, ParseStyleParentReference) {
     ref = ResourceUtils::parseStyleParentReference(u"foo", &errStr);
     AAPT_ASSERT_TRUE(ref);
     EXPECT_EQ(ref.value().name.value(), kStyleFooName);
+
+    ref = ResourceUtils::parseStyleParentReference(u"*android:style/foo", &errStr);
+    AAPT_ASSERT_TRUE(ref);
+    EXPECT_EQ(ref.value().name.value(), kAndroidStyleFooName);
+    EXPECT_TRUE(ref.value().privateReference);
 }
 
 } // namespace aapt
diff --git a/tools/aapt2/ResourceValues.cpp b/tools/aapt2/ResourceValues.cpp
index 04c375f5f9740..be963ffde2b3f 100644
--- a/tools/aapt2/ResourceValues.cpp
+++ b/tools/aapt2/ResourceValues.cpp
@@ -457,6 +457,9 @@ Style* Style::clone(StringPool* newPool) const {
 void Style::print(std::ostream* out) const {
     *out << "(style) ";
     if (parent && parent.value().name) {
+        if (parent.value().privateReference) {
+            *out << "*";
+        }
         *out << parent.value().name.value();
     }
     *out << " ["
diff --git a/tools/aapt2/flatten/TableFlattener.cpp b/tools/aapt2/flatten/TableFlattener.cpp
index a2f53e15df69b..26d7c2ca055c9 100644
--- a/tools/aapt2/flatten/TableFlattener.cpp
+++ b/tools/aapt2/flatten/TableFlattener.cpp
@@ -113,8 +113,7 @@ struct MapFlattenVisitor : public RawValueVisitor {
     bool mUseExtendedChunks;
 
     size_t mEntryCount = 0;
-    Maybe<uint32_t> mParentIdent;
-    Maybe<ResourceNameRef> mParentName;
+    const Reference* mParent = nullptr;
 
     MapFlattenVisitor(SymbolWriter* symbols, FlatEntry* entry, BigBuffer* buffer,
                       StringPool* sourcePool, StringPool* commentPool,
@@ -227,13 +226,8 @@ struct MapFlattenVisitor : public RawValueVisitor {
 
     void visit(Style* style) override {
         if (style->parent) {
-            bool privateRef = style->parent.value().privateReference && mUseExtendedChunks;
-            if (!style->parent.value().id || privateRef) {
-                assert(style->parent.value().name && "reference must have a name");
-                mParentName = style->parent.value().name;
-            } else {
-                mParentIdent = style->parent.value().id.value().id;
-            }
+            // Parents are treated a bit differently, so record the existence and move on.
+            mParent = &style->parent.value();
         }
 
         // Sort the style.
@@ -427,11 +421,16 @@ private:
                                       mOptions.useExtendedChunks);
             entry->value->accept(&visitor);
             outEntry->count = util::hostToDevice32(visitor.mEntryCount);
-            if (visitor.mParentName) {
-                mSymbols->addSymbol(visitor.mParentName.value(),
-                                    beforeEntry + offsetof(ResTable_entry_ext, parent));
-            } else if (visitor.mParentIdent) {
-                outEntry->parent.ident = util::hostToDevice32(visitor.mParentIdent.value());
+            if (visitor.mParent) {
+                const bool forceSymbol = visitor.mParent->privateReference &&
+                        mOptions.useExtendedChunks;
+                if (!visitor.mParent->id || forceSymbol) {
+                    assert(visitor.mParent->name && "reference must have a name");
+                    mSymbols->addSymbol(*visitor.mParent,
+                                        beforeEntry + offsetof(ResTable_entry_ext, parent));
+                } else {
+                    outEntry->parent.ident = util::hostToDevice32(visitor.mParent->id.value().id);
+                }
             }
         }
         return true;
diff --git a/tools/aapt2/java/ProguardRules.cpp b/tools/aapt2/java/ProguardRules.cpp
index c0968545ad817..c610bb0f2ff25 100644
--- a/tools/aapt2/java/ProguardRules.cpp
+++ b/tools/aapt2/java/ProguardRules.cpp
@@ -227,14 +227,14 @@ bool collectProguardRules(const Source& source, xml::XmlResource* res, KeepSet*
 bool writeKeepSet(std::ostream* out, const KeepSet& keepSet) {
     for (const auto& entry : keepSet.mKeepSet) {
         for (const Source& source : entry.second) {
-            *out << "// Referenced at " << source << "\n";
+            *out << "# Referenced at " << source << "\n";
         }
         *out << "-keep class " << entry.first << " { <init>(...); }\n" << std::endl;
     }
 
     for (const auto& entry : keepSet.mKeepMethodSet) {
         for (const Source& source : entry.second) {
-            *out << "// Referenced at " << source << "\n";
+            *out << "# Referenced at " << source << "\n";
         }
         *out << "-keepclassmembers class * { *** " << entry.first << "(...); }\n" << std::endl;
     }
diff --git a/tools/aapt2/unflatten/BinaryResourceParser.cpp b/tools/aapt2/unflatten/BinaryResourceParser.cpp
index 21e476fc9c291..6b7a63cf7bf2d 100644
--- a/tools/aapt2/unflatten/BinaryResourceParser.cpp
+++ b/tools/aapt2/unflatten/BinaryResourceParser.cpp
@@ -585,6 +585,13 @@ bool BinaryResourceParser::parseType(const ResourceTablePackage* package,
                 source.path = path.toString();
             }
             source.line = util::deviceToHost32(sourceBlock->line);
+
+            if (Style* style = valueCast<Style>(resourceValue.get())) {
+                // The parent's source is the same as the resource itself, set it here.
+                if (style->parent) {
+                    style->parent.value().setSource(source);
+                }
+            }
         }
 
         StringPiece16 comment = util::getString(mSourcePool,