Bug 126575 – Dispatch a progress event before dispatching abort, error or timeout event

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[youenn fablet]

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.

[youenn fablet]

Created attachment 220529 [details]
fix over 120828 patch

fix will require updating some existing http tests

[Csaba Osztrogonác]

Please add a changelog entry and set r? flag to ask review.

[youenn fablet]

Created attachment 221034 [details]
first patch

[youenn fablet]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

(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.

[Alexey Proskuryakov]

> I found the test easier to read if its purpose is "check all the ProgressEvent after abort() is called.".

OK.

[WebKit Commit Bot]

Comment on attachment 221034 [details]
first patch

Clearing flags on attachment: 221034

Committed r161891: <http://trac.webkit.org/changeset/161891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Build fix in <http://trac.webkit.org/r161894>. How did this even build in release mode?

[Alexey Proskuryakov]

And <http://trac.webkit.org/r161896>.

[youenn fablet]

Twice the same thing... Thanks for fixing it