[webkit-reviews] review granted: [Bug 177714] XMLHttpRequest's responseXML should be annotated with [Exposed=Window] : [Attachment 322321] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 1 13:45:10 PDT 2017
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.
More information about the webkit-reviews
mailing list