Bug 126575 - Dispatch a progress event before dispatching abort, error or timeout event
Summary: Dispatch a progress event before dispatching abort, error or timeout event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 120828
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-07 05:31 PST by youenn fablet
Modified: 2014-01-13 12:06 PST (History)
3 users (show)

See Also:


Attachments
fix over 120828 patch (2.92 KB, patch)
2014-01-07 07:57 PST, youenn fablet
no flags Details | Formatted Diff | Diff
first patch (14.23 KB, patch)
2014-01-13 06:26 PST, youenn fablet
no flags Details | Formatted Diff | Diff

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