Bug 75190

Summary: Allow XMLHttpRequest responseType to be set at any state up to and including HEADERS_RECEIVED
Product: WebKit Reporter: Jarred Nicholls <jarred>
Component: New BugsAssignee: Jarred Nicholls <jarred>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, jchaffraix, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jarred Nicholls 2011-12-24 08:50:18 PST
Allow XMLHttpRequest responseType to be set prior to a call to open()
Comment 1 Jarred Nicholls 2011-12-24 09:01:04 PST
Created attachment 120493 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-12-24 10:40:27 PST
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?
Comment 3 Alexey Proskuryakov 2011-12-24 10:41:41 PST
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.
Comment 4 Jarred Nicholls 2011-12-24 13:43:09 PST
(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.
Comment 5 Jarred Nicholls 2011-12-24 18:32:55 PST
Created attachment 120506 [details]
Patch
Comment 6 WebKit Review Bot 2011-12-24 18:35:29 PST
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.
Comment 7 Jarred Nicholls 2011-12-24 18:40:37 PST
Created attachment 120507 [details]
Patch
Comment 8 Alexey Proskuryakov 2011-12-25 11:50:26 PST
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, <...>".
Comment 9 Jarred Nicholls 2011-12-25 19:57:00 PST
(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!
Comment 10 Jarred Nicholls 2011-12-25 20:02:39 PST
Committed r103675: <http://trac.webkit.org/changeset/103675>