[webkit-reviews] review denied: [Bug 25206] XMLSerializer().serializeToString() doesn't generate the XML declaration markup like Opera and Firefox : [Attachment 133669] Patch

Sun Mar 25 04:58:11 PDT 2012

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 25206: XMLSerializer().serializeToString() doesn't generate the XML
declaration markup like Opera and Firefox
https://bugs.webkit.org/show_bug.cgi?id=25206

Attachment 133669: Patch
https://bugs.webkit.org/attachment.cgi?id=133669&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133669&action=review

Good catch! I have some issues with appendXMLDecl that I'd like to see resolved
first:

> Source/WebCore/ChangeLog:8
> +	   Serialize the xml declaraion if present in the document.

typo: declaration.

> Source/WebCore/editing/MarkupAccumulator.cpp:299
> +void MarkupAccumulator::appendXMLDecl(StringBuilder& result, const Document*
doc)

s/doc/document/.  Also the function should be renamed to something like
appendXMLDeclaration, to avoid abbreviations.

> Source/WebCore/editing/MarkupAccumulator.cpp:315
> +    static const char xmlDeclString[] = "<?xml ";
> +    result.append(xmlDeclString, sizeof(xmlDeclString) - 1);
> +    result.append("version=");
> +    result.append('"');
> +    result.append(doc->xmlVersion());
> +    result.append('"');
> +    result.append(" encoding=");
> +    result.append('"');
> +    result.append(encoding);
> +    result.append('"');
> +    result.append("?>");

Using StringBuilder this way is inefficient, if you know off-hand how long the
final string is.
I'd advice to switch to use String::operator+, which is most-efficient for this
kind of string concatenation.
result.append(String("<?xml " + xmlDeclString + "version="......)
While this may look inefficient on first sight, it only causes one allocation
of the final string size, because of the magic behind the scenes when using
String::operator+.

If you repeatedly call result.append(), you're growing a Vector of Strings
inside the StringBuilder, which is needless.

> Source/WebCore/editing/MarkupAccumulator.cpp:446
> +	   appendXMLDecl(result, static_cast<const Document*>(node));

I'd break; here for consistency after calling appendXMLDecl.

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:15
> +	   document.body.innerHTML = smarkup.indexOf(xmlpi) === 0 ? "PASS" :
"FAIL";

This is fine, but what if there's a typo in the serialization code, we
wouldn't' notice this with such a test.
I'd suggest to add an "expectedString" result, and compare that. If it differs,
print FAIL, and include the differences.
Easiest is to include fast/js/resources/js-test-pre.js and use the
shouldBeEqualToString() functionality for that.

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:19
> +<body onload="runTest();"/>

s/;//