Bug 25206

Summary: XMLSerializer().serializeToString() doesn't generate the XML declaration markup like Opera and Firefox
Product: WebKit Reporter: Michael A. Puls II <shadow2531>
Component: XMLAssignee: Rob Buis <rwlbuis>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: ap, chuck, code.vineet, dglazkov, ossy, rwlbuis, szyk100, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 83432    
Bug Blocks:    
Attachments:
Description Flags
TC. Passes in Firefox and Opera. Fails in Safari.
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch
none
Patch
none
Patch none

Description Michael A. Puls II 2009-04-15 02:05:27 PDT
WebKit r42344

When you use serializeToString() on an xml document that was generated by parsing xml markup that contained an xml declaration, the xml declaration markup is missing from the returned string.

For example:

var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
var markup = xmlpi + '<root><test/></root>';
var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
var smarkup = new XMLSerializer().serializeToString(xmldoc);

In Opera and Firefox, smarkup will begin with '<?xml version="1.0" encoding="UTF-8"?>'. In Safari, it's missing.

Difference between Opera and Firefox though:

1. If the @encoding value of the xml declaration in the parsed markup is lowercase, Firefox will upppercase it for serializeToString(). For example, encoding="utf-8" -> encoding="UTF-8". Opera does not do that and leaves it as-is.

2. If the parsed markup doesn't contain an xml declaration, Firefox won't generate xml declaration markup for serializeToString. Opera will generate '<?xml version="1.0"?>' it seems.

It seems it would be good to at least follow FF here.
Comment 1 Michael A. Puls II 2009-04-15 02:06:40 PDT
Created attachment 29496 [details]
TC. Passes in Firefox and Opera. Fails in Safari.
Comment 2 Alexey Proskuryakov 2009-04-15 02:22:21 PDT
I think that this was discussed in bugzilla before, but I couldn't find any bug.
Comment 3 Alexey Proskuryakov 2011-01-02 12:42:50 PST
*** Bug 51801 has been marked as a duplicate of this bug. ***
Comment 4 Charles Pritchard 2011-10-28 02:01:47 PDT
This bug has been sitting for several years. What's the verdict?
Comment 5 szyk 2011-10-28 03:15:33 PDT
(In reply to comment #4)
> This bug has been sitting for several years. What's the verdict?

It's obvious!!!! This is not bug! This is feature!!!!
Comment 6 Rob Buis 2012-03-24 20:05:37 PDT
Created attachment 133669 [details]
Patch
Comment 7 Nikolas Zimmermann 2012-03-25 04:58:12 PDT
Comment on attachment 133669 [details]
Patch

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/;//
Comment 8 Rob Buis 2012-03-25 07:58:54 PDT
Hi Niko,

(In reply to comment #7)
> (From update of attachment 133669 [details])
> 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/;//

Thanks for the detailed review, I agree on all fronts. Will put up a new patch soon.
Cheers,

Rob.
Comment 9 Rob Buis 2012-03-25 08:23:46 PDT
Created attachment 133680 [details]
Patch
Comment 10 Nikolas Zimmermann 2012-03-25 08:51:45 PDT
Comment on attachment 133680 [details]
Patch

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

Still an issue with the test:

> Source/WebCore/editing/MarkupAccumulator.cpp:303
> +    String encoding = document->xmlEncoding();
> +    if (encoding.isEmpty())
> +        return;

Ah, I forgot to ask: why do you only care for encoding here? Can't xmlVersion() return an empty string?
This needs to be commented, otherwise anyone reading this function will have those questions :-)

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> +PASS smarkup is markup

This is not useful...

> LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> +        var markup = xmlpi + '<root><test/></root>';
> +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> +        shouldBe("smarkup", "markup");

shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
is more useful, as it includes the actual expected output in the test.
Comment 11 Rob Buis 2012-03-25 09:38:21 PDT
Hi Niko,

(In reply to comment #10)
> (From update of attachment 133680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133680&action=review
> 
> Still an issue with the test:
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:303
> > +    String encoding = document->xmlEncoding();
> > +    if (encoding.isEmpty())
> > +        return;
> 
> Ah, I forgot to ask: why do you only care for encoding here? Can't xmlVersion() return an empty string?
> This needs to be commented, otherwise anyone reading this function will have those questions :-)

I use it to check whether it is an xml doc basically. Version is required, encoding is optional. I guess my check is not quite right, maybe I should stick to !HTMLDocument? and then later on test for encoding.

> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl-expected.txt:6
> > +PASS smarkup is markup
> 
> This is not useful...
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> > +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> > +        var markup = xmlpi + '<root><test/></root>';
> > +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> > +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> > +        shouldBe("smarkup", "markup");
> 
> shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
> is more useful, as it includes the actual expected output in the test.

Strangely when I try that, no matter how I escape, the comparison does not work :(
Cheers,

Rob.
Comment 12 WebKit Review Bot 2012-03-25 13:55:40 PDT
Comment on attachment 133680 [details]
Patch

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

New failing tests:
fast/xsl/xslt-processor.html
http/tests/xmlhttprequest/xml-encoding.html
Comment 13 WebKit Review Bot 2012-03-25 13:55:47 PDT
Created attachment 133692 [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
Comment 14 Rob Buis 2012-03-25 19:07:58 PDT
Created attachment 133711 [details]
Patch
Comment 15 WebKit Review Bot 2012-03-25 19:39:21 PDT
Comment on attachment 133711 [details]
Patch

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

New failing tests:
fast/dom/allowed-children.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/Node/normalize-with-cdata.html
http/tests/xmlhttprequest/xml-encoding.html
fast/xsl/xslt-processor.html
fast/html/xhtml-serialize.html
http/tests/xmlhttprequest/serialize-document.html
fast/dom/dom-parse-serialize.html
fast/dom/serialize-nodes.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/Document/replace-child.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/XMLSerializer.html
Comment 16 WebKit Review Bot 2012-03-25 19:39:29 PDT
Created attachment 133722 [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
Comment 17 Rob Buis 2012-03-26 06:01:32 PDT
Created attachment 133787 [details]
Patch
Comment 18 WebKit Review Bot 2012-03-26 06:40:58 PDT
Comment on attachment 133787 [details]
Patch

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

New failing tests:
fast/xsl/xslt-processor.html
http/tests/xmlhttprequest/xml-encoding.html
Comment 19 WebKit Review Bot 2012-03-26 06:41:08 PDT
Created attachment 133793 [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
Comment 20 Rob Buis 2012-03-26 06:58:17 PDT
Hi Niko,

(In reply to comment #10)
> (From update of attachment 133680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133680&action=review
> 
> > LayoutTests/fast/dom/dom-parse-serialize-xmldecl.html:10
> > +        var xmlpi = '<?xml version="1.0" encoding="UTF-8"?>';
> > +        var markup = xmlpi + '<root><test/></root>';
> > +        var xmldoc = new DOMParser().parseFromString(markup, "text/xml");
> > +        var smarkup = new XMLSerializer().serializeToString(xmldoc);
> > +        shouldBe("smarkup", "markup");
> 
> shouldBeEqualToString("new XMLSerializer().serializeToString(xmldoc)", "<?xml....")
> is more useful, as it includes the actual expected output in the test.

Ok, I have this covered in the new test now, it needed some replace() stuff.
Cheers,

Rob.
Comment 21 Nikolas Zimmermann 2012-03-26 07:11:56 PDT
Comment on attachment 133787 [details]
Patch

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.
Comment 22 Rob Buis 2012-03-26 07:18:42 PDT
Hi Niko,

(In reply to comment #21)
> (From update of attachment 133787 [details])
> 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?

Yeah these results should simply be updated, I think it is a slight improvement anyway.

> > 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?

Have to think about it... To be honest the xmlVersion code etc. looks a bit wierd. For instance HTMLDocument has to call clearXMLVersion in the ctor, that does not look optimal to me.

> > 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.

Good :) May do a new patch soon depending on workload...
Cheers,

Rob.
Comment 23 Rob Buis 2012-03-27 06:06:15 PDT
Created attachment 134041 [details]
Patch
Comment 24 Eric Seidel (no email) 2012-03-27 12:37:18 PDT
Comment on attachment 134041 [details]
Patch

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

This seems like a lot of fixes at once, no?  I worry about your test coverage, but otherwise I like the change.

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

Is this tested?

> Source/WebCore/dom/Document.h:1139
> +    Document(Frame*, const KURL&, bool isXHTML, bool isHTML, const String& xmlVersion = String("1.0"));

When does this get initialized?  I worry this is a static initializer?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:993
> +    if (standalone >= 0)

Is this tested?
Comment 25 Rob Buis 2012-03-27 13:02:23 PDT
Hi Eric,

(In reply to comment #24)
> (From update of attachment 134041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134041&action=review
> 
> This seems like a lot of fixes at once, no?  I worry about your test coverage, but otherwise I like the change.

Yes, some fixes. Actually the xmlVersion changes are not essential to the fix, it just seems suboptimal to me to set it to 1.0 and then to change your mind and clear it in HTMLDocument ctor :) The rest is needed to have good serialization.

> > Source/WebCore/dom/Document.cpp:1172
> > +    if (!implementation()->hasFeature("XML", String()))
> > +        return;
> 
> Is this tested?

No. But there is no point changing m_hasXmlDeclaration for non-XML. This is internal API, so not quite sure how you want to test.

> > Source/WebCore/dom/Document.h:1139
> > +    Document(Frame*, const KURL&, bool isXHTML, bool isHTML, const String& xmlVersion = String("1.0"));
> 
> When does this get initialized?  I worry this is a static initializer?

Not completely sure what you mean here. HTMLDocument uses emptyString() but otherwise we use the default param. Another way is to do it in the Document ctor based on isHTML:

m_xmlVersion(isHTML ? emptyString() : "1.0"),

Would that be better/more efficient? Or keep the clearXMLVersion? I dont like that function for some reason.

> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:993
> > +    if (standalone >= 0)
> 
> Is this tested?

No, I'll add more variants of the xml declaration to test, i.e. standalone and encoding being on or off, version is required.
Cheers,

Rob.
Comment 26 Rob Buis 2012-03-30 05:39:23 PDT
Created attachment 134791 [details]
Patch
Comment 27 WebKit Review Bot 2012-03-30 07:32:16 PDT
Comment on attachment 134791 [details]
Patch

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

New failing tests:
fast/dom/Node/normalize-with-cdata.html
fast/html/xhtml-serialize.html
fast/dom/Document/replace-child.html
fast/dom/dom-parse-serialize.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/dom-parse-serialize-display.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/XMLSerializer.html
Comment 28 WebKit Review Bot 2012-03-30 07:32:24 PDT
Created attachment 134811 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 29 WebKit Review Bot 2012-03-30 08:24:21 PDT
Comment on attachment 134791 [details]
Patch

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

New failing tests:
fast/dom/Node/normalize-with-cdata.html
fast/html/xhtml-serialize.html
fast/dom/Document/replace-child.html
fast/dom/dom-parse-serialize.html
svg/custom/dynamic-svg-document-creation.svg
fast/dom/dom-parse-serialize-display.html
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/XMLSerializer.html
Comment 30 WebKit Review Bot 2012-03-30 08:24:28 PDT
Created attachment 134819 [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
Comment 31 Rob Buis 2012-03-31 18:30:04 PDT
Created attachment 134983 [details]
Patch
Comment 32 WebKit Review Bot 2012-03-31 19:46:27 PDT
Comment on attachment 134983 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/dom-parse-serialize.html
Comment 33 WebKit Review Bot 2012-03-31 19:46:35 PDT
Created attachment 134984 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 34 WebKit Review Bot 2012-03-31 20:37:17 PDT
Comment on attachment 134983 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
fast/dom/dom-parse-serialize-display.html
fast/dom/dom-parse-serialize.html
Comment 35 WebKit Review Bot 2012-03-31 20:37:24 PDT
Created attachment 134987 [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
Comment 36 Rob Buis 2012-04-01 08:49:41 PDT
Created attachment 135001 [details]
Patch
Comment 37 WebKit Review Bot 2012-04-01 10:05:00 PDT
Comment on attachment 135001 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
Comment 38 WebKit Review Bot 2012-04-01 10:05:07 PDT
Created attachment 135004 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 39 Rob Buis 2012-04-01 10:28:18 PDT
Created attachment 135006 [details]
Patch
Comment 40 Rob Buis 2012-04-01 10:50:28 PDT
(In reply to comment #39)
> Created an attachment (id=135006) [details]
> Patch

I am hoping this latest patch will fix all chromium regressions. Note that for some other platforms there still will need to be rebaselines.
Comment 41 Csaba Osztrogonác 2012-04-03 08:23:38 PDT
Comment on attachment 135006 [details]
Patch

Remove r? temporarily to try yo save Qt EWS bots. :)
Comment 42 Csaba Osztrogonác 2012-04-03 08:39:05 PDT
(In reply to comment #41)
> (From update of attachment 135006 [details])
> Remove r? temporarily to try yo save Qt EWS bots. :)

I'll put r? back after EWSs dropped it from their queue.
Comment 43 Csaba Osztrogonác 2012-04-03 09:17:39 PDT
Comment on attachment 135006 [details]
Patch

Put r? back, because Qt EWS bots dropped this patch from their queues.
Comment 44 Rob Buis 2012-04-06 18:32:09 PDT
Created attachment 136116 [details]
Patch
Comment 45 Csaba Osztrogonác 2012-04-07 05:52:08 PDT
Comment on attachment 136116 [details]
Patch

Remove r?, because it broke the Qt EWS bots again. :( I'll set r? back after Qt EWS dropped this patch from their queues.
Comment 46 Rob Buis 2012-04-07 06:24:17 PDT
Hi Ossy,

(In reply to comment #45)
> (From update of attachment 136116 [details])
> Remove r?, because it broke the Qt EWS bots again. :( I'll set r? back after Qt EWS dropped this patch from their queues.

Is the problem still that the bug is too big because of the attachments? I am willing to obsolete the attachments, but I did not do it before since then maybe there is no "testcase" to investigate and fix the problem.
Cheers,

Rob.
Comment 47 Csaba Osztrogonác 2012-04-07 12:24:50 PDT
The problem is caused by the size of this bug ( >100Mb with attachments). Unfortunately EWS isn't to optimal and it tries to download the whole bug 3 times and then EWS dies with OOM. See https://bugs.webkit.org/show_bug.cgi?id=60223 and https://bugs.webkit.org/show_bug.cgi?id=74929 for details.
Comment 48 Rob Buis 2012-04-08 05:49:58 PDT
Created attachment 136146 [details]
Patch
Comment 49 Rob Buis 2012-04-24 20:50:59 PDT

*** This bug has been marked as a duplicate of bug 83432 ***