Bug 126575

Summary: Dispatch a progress event before dispatching abort, error or timeout event
Product: WebKit Reporter: youenn fablet <youennf>
Component: XMLAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 120828    
Bug Blocks:    
Attachments:
Description Flags
fix over 120828 patch
none
first patch none

Description youenn fablet 2014-01-07 05:31:16 PST
According http://www.w3.org/TR/XMLHttpRequest/#request-error, a progress event should be fired before dispatching abort, error or tiemout events.
Corresponding WPT test case is XMLHttpRequest/send-response-event-order.htm
A progress event may also be needed between readystatechange and load event in the case of successful XHR exchange, although WPT test case does not seem to expect this behavior.
Comment 1 youenn fablet 2014-01-07 07:57:20 PST
Created attachment 220529 [details]
fix over 120828 patch

fix will require updating some existing http tests
Comment 2 Csaba Osztrogonác 2014-01-13 05:44:19 PST
Please add a changelog entry and set r? flag to ask review.
Comment 3 youenn fablet 2014-01-13 06:26:04 PST
Created attachment 221034 [details]
first patch
Comment 4 youenn fablet 2014-01-13 06:36:03 PST
Patch only takes care of abort, error and timeout events.
This behavior aligns with the XHR spec and with Blink and Firefox.

It would be nice to also have a progress ProgressEvent in the normal load case (for consistency with the error cases plus it aligns with the spec).
But Firefox, Blink and WPT tests consistently disagree with that behavior.
Comment 5 Alexey Proskuryakov 2014-01-13 09:18:30 PST
Comment on attachment 221034 [details]
first patch

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

> LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html:-45
> -        logProgressEvent(e);

This doesn't look right - why don't we log this event any more?

It doesn't materially affect what this test is testing for, but it could be confusing in the future.
Comment 6 youenn fablet 2014-01-13 09:54:53 PST
(In reply to comment #5)
> (From update of attachment 221034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221034&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html:-45
> > -        logProgressEvent(e);
> 
> This doesn't look right - why don't we log this event any more?
> 
> It doesn't materially affect what this test is testing for, but it could be confusing in the future.

I found the test easier to read if its purpose is "check all the ProgressEvent after abort() is called.". The not-logged-anymore event is sent before abort() is called.
I can revert the change if it adds more confusion than clarity.
Comment 7 Alexey Proskuryakov 2014-01-13 10:00:02 PST
> I found the test easier to read if its purpose is "check all the ProgressEvent after abort() is called.".

OK.
Comment 8 WebKit Commit Bot 2014-01-13 10:37:11 PST
Comment on attachment 221034 [details]
first patch

Clearing flags on attachment: 221034

Committed r161891: <http://trac.webkit.org/changeset/161891>
Comment 9 WebKit Commit Bot 2014-01-13 10:37:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2014-01-13 11:07:39 PST
Build fix in <http://trac.webkit.org/r161894>. How did this even build in release mode?
Comment 11 Alexey Proskuryakov 2014-01-13 11:13:51 PST
And <http://trac.webkit.org/r161896>.
Comment 12 youenn fablet 2014-01-13 12:06:28 PST
Twice the same thing... Thanks for fixing it