[webkit-reviews] review granted: [Bug 74626] Bring XHR.responseXML behavior up to the latest W3C spec (error handling, HTML doc support, responseType compliance) : [Attachment 119618] Patch

Fri Dec 16 11:52:47 PST 2011

Alexey Proskuryakov <ap at webkit.org> has granted Jarred Nicholls
<jarred at webkit.org>'s request for review:
Bug 74626: Bring XHR.responseXML behavior up to the latest W3C spec (error
handling, HTML doc support, responseType compliance)
https://bugs.webkit.org/show_bug.cgi?id=74626

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119618&action=review

Thank you! Separating the tests made them much easier to follow indeed.

Please do follow up with a bug on XML MIME type case sensitivity. Even for
HTML, we still have a case sensitive comparison in
XMLHttpRequest::didReceiveData!

I have s few comments, some of which need to be addressed. It doesn't appear
necessary to have another review round, but you are of course welcome to post
the patch for review again if you see a need for it.

> Source/WebCore/ChangeLog:3
> +	   Bring XHR.responseXML behavior up to the latest W3C spec (error
handling, HTML doc support, responseType compliance)

I still think that the correct title would be "Support HTML documents in
XHR.responseXML". It does make sense to structure your effort by API, but for
people following WebKit development, HTML support is an order of magnitude more
interesting than the other changes. In fact, if the other changes were anything
major, I would have suggested moving those into separate patches.

> Source/WebCore/xml/XMLHttpRequest.cpp:227
> +	   bool isHTML = equalIgnoringCase(responseMIMEType(), "text/html");

It would seem more robust to perform lowercasing earlier (either store a
normalized content type, or have responseMIMEType() perform it). Not something
that would be appropriate to do in this patch.

> LayoutTests/fast/frames/iframe-reparenting.html:67
>      xhr.open("GET","data:text/html,<html
xmlns='http://www.w3.org/1999/xhtml'><body id='body'>Hello</body></html>",
false);
> +    xhr.responseType = 'document';

I would have changed data URL MIME type instead, so that the test would take
the same code path as before. It used to perform XML parsing.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML-after-error.html:6

> +	   description('This tests the XMLHttpRequest responseXML attribute
accessibility after a network error.');

Maybe s/accessibility/availability/ ?

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML-after-error.html:18
> +	       shouldBeNull('xhr.responseXML');

How did this test fail before your changes? There is no data, so it seems that
parsing would have failed anyway, producing a null document.

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML-html-no-responsetype
.html:41
> +    <!-- Leave these break tags malformed/open to test HTML parsing.
> +	    They should be siblings to div#description, and not nested within
one another.
> +	    The XMLDocument parser would treat the second <br> as a child to
the first. -->
> +    <br><br>
> +
> +    <div id="console"></div>
> +
> +    <script>
> +	   // This code will manipulate the first BR node by adding an "id" to
it.
> +	   // This same BR will be inspected after XHR loads the document to
see
> +	   // if this script executed or not.  If it didn't execute, the first
BR
> +	   // will not have an "id" specified and the test passes.
> +	   var br = document.querySelector('div#description + br');
> +	   br.id = 'break-tag';
> +    </script>

Please remove this part, it's not used in this test.

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML-xml-no-responsetype.
html:6
> +	   description('This tests the XMLHttpRequest responseXML loading an
XML document with no specified responseType.');

Is this test needed? We have lots of tests for responseXML that were written
before responseType existed.

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML-xml-text-responsetyp
e.html:13
> +	       shouldThrow('xhr.responseXML', '"Error: INVALID_STATE_ERR: DOM
Exception 11"');

Whenever possible, I prefer tests that can pass in other browsers. Hardcoding
error string will unnecessarily limit PASS to WebKit, or maybe even to WebKit
with JSC (or V8) bindings.