Bug 75190 – Allow XMLHttpRequest responseType to be set at any state up to and including HEADERS_RECEIVED

• Component: New Bugs
• Status: RESOLVED FIXED

[Jarred Nicholls]

Allow XMLHttpRequest responseType to be set prior to a call to open()

[Jarred Nicholls]

Created attachment 120493 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120493 [details]
Patch

There doesn't seem to be a test for changing responseType in HEADERS_RECEIVED state. Didn't onreadystatechange idea work?

[Alexey Proskuryakov]

Comment on attachment 120493 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:295
> -    if (m_state != OPENED || m_loader) {
> +    if (m_state >= LOADING || m_loader) {

Actually, I think that the m_loader check here is now incorrect.

[Jarred Nicholls]

(In reply to comment #3)
> (From update of attachment 120493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120493&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:295
> > -    if (m_state != OPENED || m_loader) {
> > +    if (m_state >= LOADING || m_loader) {
> 
> Actually, I think that the m_loader check here is now incorrect.

Yep you're right, the spec doesn't say the send flag needs to be checked (I associate having a loader instantiated and running as being the "send flag") which makes sense if the value can be set when headers are received but before loading takes place.  So I'll remove that old m_loader check.

I will add the headers received test. But, I think there's a high probability I can create a test that breaks text decoding or the "arraybuffer" response type :)  I can test those scenarios and if it's indeed an issue, I'll can create a new bug for that.  The test I'll add to this patch will simply test that the responseType can in fact be set when the state is HEADERS_RECEIVED.

Thanks.

[Jarred Nicholls]

Created attachment 120506 [details]
Patch

[WebKit Review Bot]

Attachment 120506 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarred Nicholls]

Created attachment 120507 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120507 [details]
Patch

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

Looks great.

> Source/WebCore/ChangeLog:3
> +        Allow XMLHttpRequest responseType to be set prior to a call to open()

The bug title is now slightly misleading, as we ended up allowing more than that.

> Source/WebCore/ChangeLog:10
> +        See http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html

Generally, giving URLs to common specs is not very helpful. It's easy to find these, URLs change with time, and length of explanatory text increases cognitive load.

It's not immediately clear how this fairly long description relates to the change. I'd say something like "The most practically interesting aspect of this change is that responseType attribute can be set before a call to open(). Therefore, <...>".

[Jarred Nicholls]

(In reply to comment #8)
> (From update of attachment 120507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120507&action=review
> 
> Looks great.
> 
> > Source/WebCore/ChangeLog:3
> > +        Allow XMLHttpRequest responseType to be set prior to a call to open()
> 
> The bug title is now slightly misleading, as we ended up allowing more than that.

Thanks, changing before landing.

> 
> > Source/WebCore/ChangeLog:10
> > +        See http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html
> 
> Generally, giving URLs to common specs is not very helpful. It's easy to find these, URLs change with time, and length of explanatory text increases cognitive load.

Yeah I know this, and yet I continue to do it ;)  I'll break that habit.  It's useful today, but 2 months from now it could be completely misinformative.

> 
> It's not immediately clear how this fairly long description relates to the change. I'd say something like "The most practically interesting aspect of this change is that responseType attribute can be set before a call to open(). Therefore, <...>".

Will edit before landing.

Thanks!

[Jarred Nicholls]

Committed r103675: <http://trac.webkit.org/changeset/103675>