From 1d358ff5bb59f56ab19aa31d6afcf82c46b7c7bc Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Wed, 6 Mar 2019 15:06:49 -0800 Subject: [PATCH] Fix aapt2 whitespace diffs from aapt(1) CDATA blocks were being processed differently in aapt2 so this change fixes aapt2 to not treat cdata blocks differently and still trime whitespace. Also, aapt did not process escapes when compiling xml files. This change removes over-processing of xml text nodes. All test strings are what aapt(1) would output. Test: aapt2_tests Bug: 124470332 Change-Id: I90ee0c1e5e9208f8a5c60cee93e3ba02712c9b2c --- tools/aapt2/ResourceParser.cpp | 27 ++----------------- tools/aapt2/ResourceParser_test.cpp | 25 +++++++++-------- tools/aapt2/ResourceUtils.cpp | 12 +++------ tools/aapt2/ResourceUtils.h | 6 ++--- tools/aapt2/ResourceUtils_test.cpp | 23 ---------------- tools/aapt2/format/binary/XmlFlattener.cpp | 3 --- .../aapt2/format/binary/XmlFlattener_test.cpp | 11 ++++++-- 7 files changed, 31 insertions(+), 76 deletions(-) diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp index d0237f80a8f0e..5e8d870488c40 100644 --- a/tools/aapt2/ResourceParser.cpp +++ b/tools/aapt2/ResourceParser.cpp @@ -206,15 +206,6 @@ class SegmentNode : public Node { } }; -// A chunk of text in the XML string within a CDATA tags. -class CdataSegmentNode : public SegmentNode { - public: - - void Build(StringBuilder* builder) const override { - builder->AppendText(data, /* preserve_spaces */ true); - } -}; - // A tag that will be encoded into the final flattened string. Tags like or . class SpanNode : public Node { public: @@ -251,7 +242,6 @@ bool ResourceParser::FlattenXmlSubtree( std::vector node_stack; node_stack.push_back(&root); - bool cdata_block = false; bool saw_span_node = false; SegmentNode* first_segment = nullptr; SegmentNode* last_segment = nullptr; @@ -262,12 +252,9 @@ bool ResourceParser::FlattenXmlSubtree( // First take care of any SegmentNodes that should be created. if (event == xml::XmlPullParser::Event::kStartElement - || event == xml::XmlPullParser::Event::kEndElement - || event == xml::XmlPullParser::Event::kCdataStart - || event == xml::XmlPullParser::Event::kCdataEnd) { + || event == xml::XmlPullParser::Event::kEndElement) { if (!current_text.empty()) { - std::unique_ptr segment_node = (cdata_block) - ? util::make_unique() : util::make_unique(); + auto segment_node = util::make_unique(); segment_node->data = std::move(current_text); last_segment = node_stack.back()->AddChild(std::move(segment_node)); @@ -345,16 +332,6 @@ bool ResourceParser::FlattenXmlSubtree( } } break; - case xml::XmlPullParser::Event::kCdataStart: { - cdata_block = true; - break; - } - - case xml::XmlPullParser::Event::kCdataEnd: { - cdata_block = false; - break; - } - default: // ignore. break; diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp index 25b76b0c1fa0f..251ca0c9256e7 100644 --- a/tools/aapt2/ResourceParser_test.cpp +++ b/tools/aapt2/ResourceParser_test.cpp @@ -1195,27 +1195,26 @@ TEST_F(ResourceParserTest, ParseIdItem) { } TEST_F(ResourceParserTest, ParseCData) { - std::string input = R"( - )"; - + // Double quotes should still change the state of whitespace processing + std::string input = R"(Hello' ]]> World)"; ASSERT_TRUE(TestParse(input)); - String* output = test::GetValue(&table_, "string/foo"); + auto output = test::GetValue(&table_, "string/foo"); ASSERT_THAT(output, NotNull()); - EXPECT_THAT(*output, StrValueEq("some text and ' apostrophe")); + EXPECT_THAT(*output, StrValueEq(std::string("Hello ' World").data())); - // Double quotes should not change the state of whitespace processing - input = R"(Hello' ]]> World)"; + input = R"()"; ASSERT_TRUE(TestParse(input)); output = test::GetValue(&table_, "string/foo2"); ASSERT_THAT(output, NotNull()); - EXPECT_THAT(*output, StrValueEq(std::string("Hello \"' World").data())); + EXPECT_THAT(*output, StrValueEq(std::string("Hello World").data())); - // Cdata blocks should not have their whitespace trimmed + // Cdata blocks should have their whitespace trimmed input = R"( )"; ASSERT_TRUE(TestParse(input)); output = test::GetValue(&table_, "string/foo3"); ASSERT_THAT(output, NotNull()); - EXPECT_THAT(*output, StrValueEq(std::string(" text ").data())); + EXPECT_THAT(*output, StrValueEq(std::string("text").data())); input = R"( )"; ASSERT_TRUE(TestParse(input)); @@ -1227,7 +1226,11 @@ TEST_F(ResourceParserTest, ParseCData) { ASSERT_TRUE(TestParse(input)); output = test::GetValue(&table_, "string/foo5"); ASSERT_THAT(output, NotNull()); - EXPECT_THAT(*output, StrValueEq(std::string(" ").data())); + EXPECT_THAT(*output, StrValueEq(std::string("").data())); + + // Single quotes must still be escaped + input = R"()"; + ASSERT_FALSE(TestParse(input)); } } // namespace aapt diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp index ffc1a92a88b07..f0e4d9e7ce8c9 100644 --- a/tools/aapt2/ResourceUtils.cpp +++ b/tools/aapt2/ResourceUtils.cpp @@ -845,20 +845,16 @@ StringBuilder::StringBuilder(bool preserve_spaces) : preserve_spaces_(preserve_spaces), quote_(preserve_spaces) { } -StringBuilder& StringBuilder::AppendText(const std::string& text, bool preserve_spaces) { +StringBuilder& StringBuilder::AppendText(const std::string& text) { if (!error_.empty()) { return *this; } - // Enable preserving spaces if it is enabled for this append or the StringBuilder was constructed - // to preserve spaces - preserve_spaces = (preserve_spaces) ? preserve_spaces : preserve_spaces_; - const size_t previous_len = xml_string_.text.size(); Utf8Iterator iter(text); while (iter.HasNext()) { char32_t codepoint = iter.Next(); - if (!preserve_spaces && !quote_ && codepoint != kNonBreakingSpace && iswspace(codepoint)) { + if (!preserve_spaces_ && !quote_ && codepoint != kNonBreakingSpace && iswspace(codepoint)) { if (!last_codepoint_was_space_) { // Emit a space if it's the first. xml_string_.text += ' '; @@ -906,11 +902,11 @@ StringBuilder& StringBuilder::AppendText(const std::string& text, bool preserve_ break; } } - } else if (!preserve_spaces && codepoint == U'"') { + } else if (!preserve_spaces_ && codepoint == U'"') { // Only toggle the quote state when we are not preserving spaces. quote_ = !quote_; - } else if (!preserve_spaces && !quote_ && codepoint == U'\'') { + } else if (!preserve_spaces_ && !quote_ && codepoint == U'\'') { // This should be escaped when we are not preserving spaces error_ = StringPrintf("unescaped apostrophe in string\n\"%s\"", text.c_str()); return *this; diff --git a/tools/aapt2/ResourceUtils.h b/tools/aapt2/ResourceUtils.h index a8a312005e4ed..f77766ee90612 100644 --- a/tools/aapt2/ResourceUtils.h +++ b/tools/aapt2/ResourceUtils.h @@ -276,10 +276,8 @@ class StringBuilder { // single quotations can be used without escaping them. explicit StringBuilder(bool preserve_spaces = false); - // Appends a chunk of text. If preserve_spaces is true, whitespace removal is not performed, and - // single quotations can be used without escaping them for this append. Otherwise, the - // StringBuilder will behave as it was constructed. - StringBuilder& AppendText(const std::string& text, bool preserve_spaces = false); + // Appends a chunk of text. + StringBuilder& AppendText(const std::string& text); // Starts a Span (tag) with the given name. The name is expected to be of the form: // "tag_name;attr1=value;attr2=value;" diff --git a/tools/aapt2/ResourceUtils_test.cpp b/tools/aapt2/ResourceUtils_test.cpp index 9018b0fc372a2..3b77135a09ebf 100644 --- a/tools/aapt2/ResourceUtils_test.cpp +++ b/tools/aapt2/ResourceUtils_test.cpp @@ -266,29 +266,6 @@ TEST(ResourceUtilsTest, StringBuilderUnicodeCodes) { TEST(ResourceUtilsTest, StringBuilderPreserveSpaces) { EXPECT_THAT(ResourceUtils::StringBuilder(true /*preserve_spaces*/).AppendText("\"").to_string(), Eq("\"")); - - // Single quotes should be able to be used without escaping them when preserving spaces and the - // spaces should not be trimmed - EXPECT_THAT(ResourceUtils::StringBuilder() - .AppendText(" hey guys ") - .AppendText(" 'this is so cool' ", /* preserve_spaces */ true) - .AppendText(" wow ") - .to_string(), - Eq(" hey guys 'this is so cool' wow ")); - - // Reading a double quote while preserving spaces should not change the quote state - EXPECT_THAT(ResourceUtils::StringBuilder() - .AppendText(" hey guys ") - .AppendText(" \"this is so cool' ", /* preserve_spaces */ true) - .AppendText(" wow ") - .to_string(), - Eq(" hey guys \"this is so cool' wow ")); - EXPECT_THAT(ResourceUtils::StringBuilder() - .AppendText(" hey guys\" ") - .AppendText(" \"this is so cool' ", /* preserve_spaces */ true) - .AppendText(" wow \" ") - .to_string(), - Eq(" hey guys \"this is so cool' wow ")); } } // namespace aapt diff --git a/tools/aapt2/format/binary/XmlFlattener.cpp b/tools/aapt2/format/binary/XmlFlattener.cpp index 2fe24245f3d98..afbaae4ee2a8a 100644 --- a/tools/aapt2/format/binary/XmlFlattener.cpp +++ b/tools/aapt2/format/binary/XmlFlattener.cpp @@ -99,9 +99,6 @@ class XmlFlattenerVisitor : public xml::ConstVisitor { flat_node->lineNumber = util::HostToDevice32(node->line_number); flat_node->comment.index = util::HostToDevice32(-1); - // Process plain strings to make sure they get properly escaped. - text = StringBuilder(true /*preserve_spaces*/).AppendText(text).to_string(); - ResXMLTree_cdataExt* flat_text = writer.NextBlock(); AddString(text, kLowPriority, &flat_text->data); writer.Finish(); diff --git a/tools/aapt2/format/binary/XmlFlattener_test.cpp b/tools/aapt2/format/binary/XmlFlattener_test.cpp index 25786b1659e7d..1dac493359e45 100644 --- a/tools/aapt2/format/binary/XmlFlattener_test.cpp +++ b/tools/aapt2/format/binary/XmlFlattener_test.cpp @@ -118,7 +118,7 @@ TEST_F(XmlFlattenerTest, FlattenXmlWithNoCompiledAttributes) { ASSERT_THAT(tree.getAttributeCount(), Eq(0u)); ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::TEXT)); - EXPECT_THAT(tree.getText(&len), StrEq(u"Some text\\")); + EXPECT_THAT(tree.getText(&len), StrEq(u"Some text\\\\")); ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::END_TAG)); EXPECT_THAT(tree.getElementNamespace(&len), IsNull()); @@ -283,7 +283,7 @@ TEST_F(XmlFlattenerTest, ProcessEscapedStrings) { EXPECT_THAT(tree.getAttributeStringValue(idx, &len), StrEq(u"\"")); ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::TEXT)); - EXPECT_THAT(tree.getText(&len), StrEq(u"\\d{5}")); + EXPECT_THAT(tree.getText(&len), StrEq(u"\\\\d{5}")); } TEST_F(XmlFlattenerTest, ProcessQuotes) { @@ -360,6 +360,7 @@ I J + \t K \n )"); @@ -437,6 +438,12 @@ I EXPECT_THAT(tree.getText(&len), StrEq(u" J ")); ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::END_TAG)); + ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::START_TAG)); + EXPECT_THAT(tree.getElementName(&len), StrEq(u"item")); + ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::TEXT)); + EXPECT_THAT(tree.getText(&len), StrEq(u"\\t K \\n ")); + ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::END_TAG)); + ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::START_TAG)); EXPECT_THAT(tree.getElementName(&len), StrEq(u"item")); ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::END_TAG));