Bug 83432 – XMLSerializer().serializeToString() doesn't generate the XML declaration markup like Opera and Firefox

• Component: XML
• Status: RESOLVED FIXED

[Rob Buis]

Moving bug 25026 to this new bug because EWS has problems.

[Rob Buis]

Created attachment 136147 [details]
Patch

[Rob Buis]

Finally some linux chromium EWS results, no regressions :)

[Nikolas Zimmermann]

Comment on attachment 136147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136147&action=review

You're close :-) Great to see no regressions, still some comments:

> Source/WebCore/dom/Document.cpp:1208
> +    if (!implementation()->hasFeature("XML", String()))

Shouldn't "XML" be a static String/AtomicString/.. ?
s/String()/emptyString()/

It's completely unclear to me, why you only respect 'hasXmlDeclaration' if impl()->hasFeature() returns true.
This needs to be documented.

> Source/WebCore/dom/Document.h:400
> +    bool xmlStandalone() const { return m_xmlStandalone == Standalone; }
> +    StandaloneStatus xmlStandaloneStatus() const { return m_xmlStandalone; }

Why expose both? Are there lots of call sites using xmlStandalone()? If not I'd just remove this.

> Source/WebCore/dom/Document.h:1386
> +    StandaloneStatus m_xmlStandalone : 2;
> +    bool m_hasXmlDeclaration : 1;

Again, why both?

> Source/WebCore/editing/MarkupAccumulator.cpp:305
> +    String encoding = document->xmlEncoding();

const String&

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1015
> +    if (standalone == -1) {

You should document libxmls meaning of -1 here.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1022
>      if (version)
>          document()->setXMLVersion(toString(version), ec);

Do you know about the new ASSERT_NO_EXCEPTION sheme?
Instead of using document()->blub(.., ec), you can save the local variable and just use document()->blub(.., ASSERT_NO_EXCEPTION)

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-get-expected.txt:13
> -<!DOCTYPE doc><doc>
> +<?xml version="1.0"?><!DOCTYPE doc><doc>

I'd like this to be okayed by XMLHttpRequest folks - I think Alexey is a good contact for this.

[Alexey Proskuryakov]

Comment on attachment 136147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136147&action=review

> I'd like this to be okayed by XMLHttpRequest folks - I think Alexey is a good contact for this.

This changes "serialized" part of result, i.e. it's not actually testing anything XMLHttpRequest related.

It's a shame that these tests are image ones.

> Source/WebCore/dom/Document.cpp:1206
> +void Document::setHasXmlDeclaration(bool hasXmlDeclaration)

Why non-standard capitalization of XML?

[Rob Buis]

(In reply to comment #4)
> > Source/WebCore/dom/Document.cpp:1206
> > +void Document::setHasXmlDeclaration(bool hasXmlDeclaration)
> 
> Why non-standard capitalization of XML?

Good point, will fix.

[Rob Buis]

Hi Niko,

(In reply to comment #3)
> (From update of attachment 136147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136147&action=review
> 
> You're close :-) Great to see no regressions, still some comments:

Ok.

> > Source/WebCore/dom/Document.cpp:1208
> > +    if (!implementation()->hasFeature("XML", String()))
> 
> Shouldn't "XML" be a static String/AtomicString/.. ?
> s/String()/emptyString()/
> 
> It's completely unclear to me, why you only respect 'hasXmlDeclaration' if impl()->hasFeature() returns true.
> This needs to be documented.

Yes this was to mimic setXMLVersion etc., but those are dictated by a spec and sethasXMLDeclaration is internal only. So I removed that now.

> > Source/WebCore/dom/Document.h:400
> > +    bool xmlStandalone() const { return m_xmlStandalone == Standalone; }
> > +    StandaloneStatus xmlStandaloneStatus() const { return m_xmlStandalone; }
> 
> Why expose both? Are there lots of call sites using xmlStandalone()? If not I'd just remove this.

See below.

> > Source/WebCore/dom/Document.h:1386
> > +    StandaloneStatus m_xmlStandalone : 2;
> > +    bool m_hasXmlDeclaration : 1;
> 
> Again, why both?

Unfortunately, a standalone not being specified and not having seen a xml declaration at all are two different things. We have to track both for the serializing to be accurate. Again, the standalone could be unspecified but the xml declaration still given. Only different way of storing the same info would be to extend the enum, basically using the same approach as libxml2 does for int standalone (see below), but I think that is abusing the enum.

> > Source/WebCore/editing/MarkupAccumulator.cpp:305
> > +    String encoding = document->xmlEncoding();
> 
> const String&

Fixed.

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1015
> > +    if (standalone == -1) {
> 
> You should document libxmls meaning of -1 here.

Fixed.

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1022
> >      if (version)
> >          document()->setXMLVersion(toString(version), ec);
> 
> Do you know about the new ASSERT_NO_EXCEPTION sheme?
> Instead of using document()->blub(.., ec), you can save the local variable and just use document()->blub(.., ASSERT_NO_EXCEPTION)

Did not know this, thanks! Fixed.

> > LayoutTests/fast/xmlhttprequest/xmlhttprequest-get-expected.txt:13
> > -<!DOCTYPE doc><doc>
> > +<?xml version="1.0"?><!DOCTYPE doc><doc>
> 
> I'd like this to be okayed by XMLHttpRequest folks - I think Alexey is a good contact for this.

He seemed ok with it.
Will upload new patch.
Cheers,

Rob.

[Rob Buis]

Created attachment 136378 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 136378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136378&action=review

It seems like most of these tests don't require pixel results. Can we first convert them to dumpAsText or dump-as-markup tests? (There are quite few examples of dump-as-markup tests in editing).

> Source/WebCore/editing/MarkupAccumulator.cpp:304
> +    result.append(String("<?xml version=\"") + document->xmlVersion());

Could you always use append instad of mixing append and String::operator+?
Once you do that, then you don't need the explicit String::String call.

> Source/WebCore/editing/MarkupAccumulator.cpp:309
> +        result.append(String("\" encoding=\"") + encoding);
> +    if (document->xmlStandaloneStatus() != Document::StandaloneUnspecified)
> +        result.append(String("\" standalone=\"") + String(document->xmlStandalone() ? "yes" : "no"));

Ditto.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1018
> +    // standalone = 1  --> standalone="yes"
> +    // standalone = 0  --> standalone="no"
> +    // standalone =-1  --> no XML declaration
> +    // standalone =-2  --> XML declaration, but no standalone attribute

Instead of adding comments like this, can we add an enum for it?

[Rob Buis]

Hi Ryosuke,

(In reply to comment #8)
> (From update of attachment 136378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136378&action=review
> 
> It seems like most of these tests don't require pixel results. Can we first convert them to dumpAsText or dump-as-markup tests? (There are quite few examples of dump-as-markup tests in editing).

Sounds like a good cleanup, but I don't think this is the bug for that.

> > Source/WebCore/editing/MarkupAccumulator.cpp:304
> > +    result.append(String("<?xml version=\"") + document->xmlVersion());
> 
> Could you always use append instad of mixing append and String::operator+?
> Once you do that, then you don't need the explicit String::String call.

I tried to do this in my new patch. Note that Nikolas suggested the operator+ for efficiency, though I could have been doing it wrong.

> > Source/WebCore/editing/MarkupAccumulator.cpp:309
> > +        result.append(String("\" encoding=\"") + encoding);
> > +    if (document->xmlStandaloneStatus() != Document::StandaloneUnspecified)
> > +        result.append(String("\" standalone=\"") + String(document->xmlStandalone() ? "yes" : "no"));
> 
> Ditto.

Fixed as well.

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:1018
> > +    // standalone = 1  --> standalone="yes"
> > +    // standalone = 0  --> standalone="no"
> > +    // standalone =-1  --> no XML declaration
> > +    // standalone =-2  --> XML declaration, but no standalone attribute
> 
> Instead of adding comments like this, can we add an enum for it?

A very good suggestion! I added an enum as suggested.

New patch coming up.
Cheers,

Rob.

[Rob Buis]

Created attachment 136578 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #9)
> Hi Ryosuke,
> 
> (In reply to comment #8)
> > (From update of attachment 136378 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136378&action=review
> > 
> > It seems like most of these tests don't require pixel results. Can we first convert them to dumpAsText or dump-as-markup tests? (There are quite few examples of dump-as-markup tests in editing).
> 
> Sounds like a good cleanup, but I don't think this is the bug for that.

Could you do that first? If you don't want to do it, then let me know. I'll do it myself.

[Rob Buis]

> Could you do that first? If you don't want to do it, then let me know. I'll do it myself.

It is probably easier for you since you dealt with dump-as-markup before, please go ahead. I guess the new bug can block this one and you may want to clear the r? on this one. This is not a high prio bug for me but I'd like to see it go in before the committer meeting, I can work on another xml serialization bug and stuff in the meantime. Thanks!
Cheers,

Rob.

[Rob Buis]

Thanks to Ryosuke to get the testcase conversion out of the way! Now the patch is up for review again :)
Cheers,

Rob.

[Ryosuke Niwa]

Comment on attachment 136578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136578&action=review

> Source/WebCore/dom/Document.h:1380
> +    StandaloneStatus m_xmlStandalone : 2;

You can't use enum in bitfields since MSVC uses signed integer for enums.
Also, bitfields won't work when different types are mixed.
You need to use unsigned for both of these fields.
r- because of this.

[Rob Buis]

Created attachment 136888 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136888 [details]
Patch

Attachment 136888 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12391439

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[WebKit Review Bot]

Created attachment 136900 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

(In reply to comment #17)
> Created an attachment (id=136900) [details]
> Archive of layout-test-results from ec2-cr-linux-04
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

I don't understand this one, the expected result should be updated, retrying.
Cheers,

Rob.

[Rob Buis]

Created attachment 136909 [details]
Retry for EWS

[WebKit Review Bot]

Comment on attachment 136909 [details]
Retry for EWS

Attachment 136909 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12395321

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[WebKit Review Bot]

Created attachment 136921 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 136930 [details]
Patch

[Rob Buis]

(In reply to comment #9)
> > > Source/WebCore/editing/MarkupAccumulator.cpp:304
> > > +    result.append(String("<?xml version=\"") + document->xmlVersion());
> > 
> > Could you always use append instad of mixing append and String::operator+?
> > Once you do that, then you don't need the explicit String::String call.
> 
> I tried to do this in my new patch. Note that Nikolas suggested the operator+ for efficiency, though I could have been doing it wrong.

Not sure about this now. I saw a speedup patch by haroken which did the same append approach, so I guess it is the way to go.

[Rob Buis]

Landed in 114059,, marking as fixed.

[Csaba Osztrogonác]

It made 4 tests fail on the Qt bots:

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-display-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-display-actual.txt 
@@ -19,7 +19,7 @@
 
 document object serialized
 
-<?xml version="1.0"?><?xml-stylesheet href="display.css" type="text/css"?><!DOCTYPE doc><doc>
+<?xml version="1.0" standalone="no"?><?xml-stylesheet href="display.css" type="text/css"?><!DOCTYPE doc><doc>
   <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
   <d id="id3">Three</d>
 </doc>


--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-xmldecl-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-xmldecl-actual.txt 
@@ -3,12 +3,12 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' encoding='UTF-8'?><root><test/></root>"
+FAIL new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") should be <?xml version='1.0' encoding='UTF-8'?><root><test/></root>. Was <?xml version='1.0' encoding='UTF-8' standalone='no'?><root><test/></root>.
 PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' encoding='UTF-8' standalone='yes'?><root><test/></root>"
 PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' encoding='UTF-8' standalone='no'?><root><test/></root>"
 PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' standalone='yes'?><root><test/></root>"
 PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0' standalone='no'?><root><test/></root>"
-PASS new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") is "<?xml version='1.0'?><root><test/></root>"
+FAIL new XMLSerializer().serializeToString(xmldoc).replace(/"/g,"'") should be <?xml version='1.0'?><root><test/></root>. Was <?xml version='1.0' standalone='no'?><root><test/></root>.
 PASS successfullyParsed is true
 
 TEST COMPLETE


--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/dom/dom-parse-serialize-actual.txt 
@@ -18,7 +18,7 @@
 
 document object serialized
 
-<?xml version="1.0"?><!DOCTYPE doc><doc>
+<?xml version="1.0" standalone="no"?><!DOCTYPE doc><doc>
   <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
   <d id="id3">Three</d>
 <f id="&amp;&lt;&gt;">Four&amp;&lt;&gt;</f><empty/><empty/></doc>


--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-get-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-get-actual.txt 
@@ -10,7 +10,7 @@
   <d id="id3">Three</d>
 </doc>
 responseXML serialized
-<?xml version="1.0"?><!DOCTYPE doc><doc>
+<?xml version="1.0" standalone="no"?><!DOCTYPE doc><doc>
   <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
   <d id="id3">Three</d>
 </doc>

Have you got any idea why?

[Rob Buis]

Hi Ossy,

(In reply to comment #25)
> It made 4 tests fail on the Qt bots:
> --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-get-expected.txt 
> +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-get-actual.txt 
> @@ -10,7 +10,7 @@
>    <d id="id3">Three</d>
>  </doc>
>  responseXML serialized
> -<?xml version="1.0"?><!DOCTYPE doc><doc>
> +<?xml version="1.0" standalone="no"?><!DOCTYPE doc><doc>
>    <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
>    <d id="id3">Three</d>
>  </doc>
> 
> Have you got any idea why?

Yes, it seems like Qt xml parser can't distinguish between xmlStandalone unspecified, yes and no, libxml2 can. We may have to always write out standalone to fix this. Let me check what FF does...

[Rob Buis]

Ok, FF behaviour on dom-parse-serialize-xmldecl.html is slightly different. It always writes out encoding and version, standalone only if specified. Opera matches our behaviour and passes this test.

Note that adding standalone="no" is not wrong, since no is the default, it is just superfluous.
Based on the above, I propose to generate Qt specific results for these 4 tests.

[Ryosuke Niwa]

(In reply to comment #27)
> Ok, FF behaviour on dom-parse-serialize-xmldecl.html is slightly different. It always writes out encoding and version, standalone only if specified. Opera matches our behaviour and passes this test.
> 
> Note that adding standalone="no" is not wrong, since no is the default, it is just superfluous.
> Based on the above, I propose to generate Qt specific results for these 4 tests.

It might be that you need to modify Source/WebCore/xml/parser/XMLDocumentParserQt.cpp more. Qt doesn't use Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp.

[Rob Buis]

(In reply to comment #28)
> (In reply to comment #27)
> > Ok, FF behaviour on dom-parse-serialize-xmldecl.html is slightly different. It always writes out encoding and version, standalone only if specified. Opera matches our behaviour and passes this test.
> > 
> > Note that adding standalone="no" is not wrong, since no is the default, it is just superfluous.
> > Based on the above, I propose to generate Qt specific results for these 4 tests.
> 
> It might be that you need to modify Source/WebCore/xml/parser/XMLDocumentParserQt.cpp more. Qt doesn't use Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp.

I am aware of this :) I looked at the QXmlStreamReader sources but the fact whether standalone was parsed is not exposed. Also see the docs:

http://qt-project.org/doc/qt-5.0/qxmlstreamreader.html#isStandaloneDocument

"Returns true if this document has been declared standalone in the XML declaration; otherwise returns false.

If no XML declaration has been parsed, this function returns false."

So there is unfortunately no way to detect whether standalone was parsed or not. One fix for this problem is making the libxml2 backend dumber and always write out standalone for xml declaration serializing, or rebaseline these 4 tests, I like the former more.
Cheers,

Rob.

[Rob Buis]

*** Bug 25206 has been marked as a duplicate of this bug. ***

[Rob Buis]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Ok, FF behaviour on dom-parse-serialize-xmldecl.html is slightly different. It always writes out encoding and version, standalone only if specified. Opera matches our behaviour and passes this test.
> > > 
> > > Note that adding standalone="no" is not wrong, since no is the default, it is just superfluous.
> > > Based on the above, I propose to generate Qt specific results for these 4 tests.
> > 
> > It might be that you need to modify Source/WebCore/xml/parser/XMLDocumentParserQt.cpp more. Qt doesn't use Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp.
> 
> I am aware of this :) I looked at the QXmlStreamReader sources but the fact whether standalone was parsed is not exposed. Also see the docs:
> 
> http://qt-project.org/doc/qt-5.0/qxmlstreamreader.html#isStandaloneDocument
> 
> "Returns true if this document has been declared standalone in the XML declaration; otherwise returns false.
> 
> If no XML declaration has been parsed, this function returns false."
> 
> So there is unfortunately no way to detect whether standalone was parsed or not. One fix for this problem is making the libxml2 backend dumber and always write out standalone for xml declaration serializing, or rebaseline these 4 tests, I like the former more.

Anyone has an opinion on this? I'd like to close this bug soon as it is basically done...
Cheers,

Rob.

[Alexey Proskuryakov]

Ideally, these tests would not bark on semantically equivalent XML.

Also, I wonder if any of these documents should actually be standalone.

[Ryosuke Niwa]

The patch has been landed for this bug as far as I know.