[webkit-reviews] review granted: [Bug 74626] Bring XHR.responseXML behavior up to the latest W3C spec : [Attachment 119467] Patch

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=74626

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

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


Looks great. I think that tests can be made much clearer, so you may want to
post an updated patch for another quick review round despite already having an
r+.

Please add a test verifying that scripts don't run in HTML documents loaded
with XHR. We have one for XHTML, and we should have one for HTML.

Please also add a test checking for strict/quirks modes - it's not immediately
obvious whether setContent gets it right.

> Source/WebCore/ChangeLog:8
> +	   Latest XHR spec from the W3C:
> +	  
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-responsexml-attribute
> +	  
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#document-response-entity-b
ody

Please mention what changed. A link to the spec provides extremely little
information, especially since editor's draft text will change in the future.

Added support for HTML is something that should be in bug title, in fact. This
is the big change.

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

This appears to match existing code for XML types, but I don't see where the
code that changes MIME type to lower case is. MIME types are not case
sensitive.

Would you be willing to investigate this (not in this bug)?

> Source/WebCore/xml/XMLHttpRequest.cpp:237
> +		   m_responseXML = static_pointer_cast<Document,
HTMLDocument>(HTMLDocument::create(0, m_url));

Why is this cast needed? PassRefPtr<U> can be assigned to RefPtr<T> when U
inherits from T.

Can we remove m_responseXML to m_responseDocument now?

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responseXML.html:60
> +	   var tests = [{

These tests are not checking variations of the same behavior, they are
completely unrelated.

I strongly encourage you to split these into separate tests with their own
expectations. This test is largely incomprehensible due to lots of things
combined together, and failures in it will be very difficult to investigate
when they happen.