Thomas Wisniewski Assignee
	Description • 8 years ago

We fail the referenced web platform test only because we're not waiting until after readyState=4 to set mLoadTotal=mLoadTransferred. This is a trivial change, but there were three minor complications to address:

1) one web platform test was failing because we were reading responseBody's length instead of responseText's length, and thus getting the wrong value after this change.

2) two mochitests were relying on our incorrect behavior of firing the last progress event before readyState=4, where the spec says to fire the readystatechange first. I changed one of them to simply follow the spec's behavior. The other one was however related to using the non-standard -moz-chunked-arraybuffer response type, and rather than play with fire there I opted to retain the old behavior for those response types (while they last).

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4794defe5d82

	Andrea Marchesini [:baku]
	Comment 1 • 8 years ago

Comment on attachment 8790708 [details] [diff] [review]
wait-until-after-xhr-readystate=4-to-set-total-to-loaded.diff

Review of attachment 8790708 [details] [diff] [review]:
-----------------------------------------------------------------

This change is OK, but I want to see the feedback of smaug about XHRMainThread::Moz_chunked_text.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 8 years ago

Comment on attachment 8790708 [details] [diff] [review]
wait-until-after-xhr-readystate=4-to-set-total-to-loaded.diff

>   if (NS_SUCCEEDED(status) &&
>       (mResponseType == XMLHttpRequestResponseType::_empty ||
>        mResponseType == XMLHttpRequestResponseType::Text)) {
>-    mLoadTotal = mResponseBody.Length();
>+    mLoadTotal = mResponseText.Length();
I don't understand this change. Why do we want to report the decoded length?
Can you point what in the spec says that should happen?

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

Good point. The spec doesn't seem to overtly specify which to use, but then it doesn't even mention decoding except when the time comes to get the response as text or JSON. That would seem to imply that the spec only deals in non-decoded lengths for ProgressEvents, so I'll revert that change (but there are some test failures in that case which I'll have to investigate).

Thanks!

	Thomas Wisniewski Assignee
	Comment 4 • 8 years ago

Anne, could you help clarify something for me?

There does not seem to be a guarantee that we must always fire an event named "progress" on the XHR *before* readyState=4 (disregarding any that are fired on the upload object). The only times we fire them on the XHR are:

1) after 50ms have passed, but the download is not completed yet.
2) after readyState=4 in response-end-of-body.

And yet, the web platform test XMLHttpRequest/send-response-event-order.htm expects there to always be an event named "progress" before readyState 4, even if 50ms have not passed. Is this a spec issue, or a bug with the test (as in, it should not *require* the event, but rather just accept it if it happens to occur)?

If we must ensure that a progress event is always sent between readyState=3 and readyState=4, it can throw off the test XMLHttpRequest/progress-events-response-data-gzip.htm (since it can happen that loaded=total in an event before readyState=4), which adds to my belief that send-response-event-order.htm is incorrect.

	Anne (:annevk)
	Comment 5 • 8 years ago

Thomas, the issue is tracked in https://github.com/whatwg/xhr/issues/72. If you have recommendations for how to change the specification please add them there.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years ago

Alright, done. Thanks Anne!

	Thomas Wisniewski Assignee
	Comment 7 • 8 years ago

For the record, the spec has resolved to match current browser behavior and fire all progress events before readyState=4, making this bug obsolete (the test will begin passing once it and the spec are appropriately updated).