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
diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp
index d0237f8..5e8d870 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -206,15 +206,6 @@
}
};
-// 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 <b> or <i>.
class SpanNode : public Node {
public:
@@ -251,7 +242,6 @@
std::vector<Node*> 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 @@
// 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<SegmentNode> segment_node = (cdata_block)
- ? util::make_unique<CdataSegmentNode>() : util::make_unique<SegmentNode>();
+ auto segment_node = util::make_unique<SegmentNode>();
segment_node->data = std::move(current_text);
last_segment = node_stack.back()->AddChild(std::move(segment_node));
@@ -345,16 +332,6 @@
}
} 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 25b76b0..251ca0c 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -1195,27 +1195,26 @@
}
TEST_F(ResourceParserTest, ParseCData) {
- std::string input = R"(
- <string name="foo"><![CDATA[some text and ' apostrophe]]></string>)";
-
+ // Double quotes should still change the state of whitespace processing
+ std::string input = R"(<string name="foo">Hello<![CDATA[ "</string>' ]]> World</string>)";
ASSERT_TRUE(TestParse(input));
- String* output = test::GetValue<String>(&table_, "string/foo");
+ auto output = test::GetValue<String>(&table_, "string/foo");
ASSERT_THAT(output, NotNull());
- EXPECT_THAT(*output, StrValueEq("some text and ' apostrophe"));
+ EXPECT_THAT(*output, StrValueEq(std::string("Hello </string>' World").data()));
- // Double quotes should not change the state of whitespace processing
- input = R"(<string name="foo2">Hello<![CDATA[ "</string>' ]]> World</string>)";
+ input = R"(<string name="foo2"><![CDATA[Hello
+ World]]></string>)";
ASSERT_TRUE(TestParse(input));
output = test::GetValue<String>(&table_, "string/foo2");
ASSERT_THAT(output, NotNull());
- EXPECT_THAT(*output, StrValueEq(std::string("Hello \"</string>' 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"(<string name="foo3"> <![CDATA[ text ]]> </string>)";
ASSERT_TRUE(TestParse(input));
output = test::GetValue<String>(&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"(<string name="foo4"> <![CDATA[]]> </string>)";
ASSERT_TRUE(TestParse(input));
@@ -1227,7 +1226,11 @@
ASSERT_TRUE(TestParse(input));
output = test::GetValue<String>(&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"(<string name="foo6"><![CDATA[some text and ' apostrophe]]></string>)";
+ ASSERT_FALSE(TestParse(input));
}
} // namespace aapt
diff --git a/tools/aapt2/ResourceUtils.cpp b/tools/aapt2/ResourceUtils.cpp
index ffc1a92..f0e4d9e 100644
--- a/tools/aapt2/ResourceUtils.cpp
+++ b/tools/aapt2/ResourceUtils.cpp
@@ -845,20 +845,16 @@
: 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 @@
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 a8a3120..f77766e 100644
--- a/tools/aapt2/ResourceUtils.h
+++ b/tools/aapt2/ResourceUtils.h
@@ -276,10 +276,8 @@
// 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 9018b0f..3b77135 100644
--- a/tools/aapt2/ResourceUtils_test.cpp
+++ b/tools/aapt2/ResourceUtils_test.cpp
@@ -266,29 +266,6 @@
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 2fe2424..afbaae4 100644
--- a/tools/aapt2/format/binary/XmlFlattener.cpp
+++ b/tools/aapt2/format/binary/XmlFlattener.cpp
@@ -99,9 +99,6 @@
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<ResXMLTree_cdataExt>();
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 25786b1..1dac493 100644
--- a/tools/aapt2/format/binary/XmlFlattener_test.cpp
+++ b/tools/aapt2/format/binary/XmlFlattener_test.cpp
@@ -118,7 +118,7 @@
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 @@
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 @@
J
</item>
+ <item>\t K \n </item>
<item>
</item>
</root>)");
@@ -439,6 +440,12 @@
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));
ASSERT_THAT(tree.next(), Eq(android::ResXMLTree::END_TAG));