Bug 177714 – XMLHttpRequest's responseXML should be annotated with [Exposed=Window]

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

XMLHttpRequest's responseXML should be annotated with [Exposed=Window]

[Sam Weinig]

Created attachment 322309 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 322309 [details]
Patch

Is this not testable?

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 322309 [details]
> Patch
> 
> Is this not testable?

Yes. I was hoping there was already a test and it would fail (hence the no r?) but it looks like I’ll have to write one.

[Sam Weinig]

Created attachment 322317 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #3)
> (In reply to Alexey Proskuryakov from comment #2)
> > Comment on attachment 322309 [details]
> > Patch
> > 
> > Is this not testable?
> 
> Yes.

By which of course I mean, “yes, it is testable” (double negative caught me). 

Last WIP adds the test. I’m surprised that not having a result didn’t cause the EWS to fail.

[Sam Weinig]

(In reply to Sam Weinig from comment #5)
> (In reply to Sam Weinig from comment #3)
> > (In reply to Alexey Proskuryakov from comment #2)
> > > Comment on attachment 322309 [details]
> > > Patch
> > > 
> > > Is this not testable?
> > 
> > Yes.
> 
> By which of course I mean, “yes, it is testable” (double negative caught
> me). 

Ahhhh. I meant negative not double negative!

[Sam Weinig]

Created attachment 322319 [details]
Patch

[Sam Weinig]

Filed https://github.com/w3c/web-platform-tests/pull/7530 to add the new tests to WPT.

[Build Bot]

Comment on attachment 322319 [details]
Patch

Attachment 322319 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4715745

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responseXML-in-worker.html

[Build Bot]

Created attachment 322320 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 322321 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #11)
> Created attachment 322321 [details]
> Patch

New patch includes the now landed WPTs, imported back into WebKit.

[Alexey Proskuryakov]

> I’m surprised that not having a result didn’t cause the EWS to fail.

Please feel encouraged to file a bug! I think that's a pretty bad issue.

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #13)
> > I’m surprised that not having a result didn’t cause the EWS to fail.
> 
> Please feel encouraged to file a bug! I think that's a pretty bad issue.

Ok. https://bugs.webkit.org/show_bug.cgi?id=177723.

[Darin Adler]

Comment on attachment 322321 [details]
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.

[WebKit Commit Bot]

Comment on attachment 322321 [details]
Patch

Clearing flags on attachment: 322321

Committed r222690: <http://trac.webkit.org/changeset/222690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34761304>

[Sam Weinig]

Add missing test results in r222693.