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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 26 07:11:56 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 133787: Patch
https://bugs.webkit.org/attachment.cgi?id=133787&action=review

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


I'd r+ it if cr-linux was green, but it isn't and clearly indicates a problem.
The cr-linux bot attached the layout test results archive, can you inspect the
two failing tests?

> Source/WebCore/editing/MarkupAccumulator.cpp:302
> +    // USe encoding as a way to detect that the xml declaration has been
explicitly set.
> +    String encoding = document->xmlEncoding();

To assure we never have to touch this code again in MarkupAccumulator, I'd
really add bool Document::hasXMLDeclaration() const, and move the xmlEncoding()
check there. What do you think?

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> +PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is
"<?xml version='1.0' encoding='UTF-8'?><root><test/></root>"

Looks great and descriptive.


More information about the webkit-reviews mailing list