[webkit-reviews] review granted: [Bug 177714] XMLHttpRequest's responseXML should be annotated with [Exposed=Window] : [Attachment 322321] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 177714: XMLHttpRequest's responseXML should be annotated with
[Exposed=Window]
https://bugs.webkit.org/show_bug.cgi?id=177714

Attachment 322321: Patch

https://bugs.webkit.org/attachment.cgi?id=322321&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 322321
  --> https://bugs.webkit.org/attachment.cgi?id=322321
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:175
> +    ASSERT(scriptExecutionContext()->isDocument());

I would write:

    ASSERT(is<Document>(scriptExecutionContext()));

But I see the file already has three places that do it the other way, calling
isDocument().

> Source/WebCore/xml/XMLHttpRequest.cpp:257
> +    if (!scriptExecutionContext()->isDocument() && type ==
ResponseType::Document)
> +	   return { };

As I said above, I like the is<Document> style better. If you wrote it
is<Document>(scriptExecutionContext() then you would include an unnecessary
null check.

But it’s annoying that the assumption that script execution context is non-null
is implicit; when called from script I guess that is sort of obvious, but on
the other hand it’s annoying that it’s called “script execution context” yet we
stash it when created and don’t get a fresh one when being called from script
again.

I kind of wish there was a version that returned a reference and asserted.