Bug 75190 - Allow XMLHttpRequest responseType to be set at any state up to and including HEADERS_RECEIVED
Summary: Allow XMLHttpRequest responseType to be set at any state up to and including ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jarred Nicholls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-24 08:50 PST by Jarred Nicholls
Modified: 2011-12-25 20:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.11 KB, patch)
2011-12-24 09:01 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Patch (13.43 KB, patch)
2011-12-24 18:32 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2011-12-24 18:40 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>