Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement XHR progress event changes #15455

Merged
merged 2 commits into from Mar 3, 2017

Conversation

@charlesvdv
Copy link
Contributor

commented Feb 8, 2017

This PR implements change in the specs in XHR.

Unfortunatelly, @jdm the test are not passing. It looks like the fetch module send more bytes that we should according the Content-Length. This lines make the test fail.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13276 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 8, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/xmlhttprequest.rs
  • @KiChjang: components/script/dom/xmlhttprequest.rs
@highfive

This comment has been minimized.

Copy link

commented Feb 8, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@@ -1009,14 +1013,14 @@ impl XMLHttpRequest {
let upload_complete = &self.upload_complete;
if !upload_complete.get() {
upload_complete.set(true);
self.dispatch_upload_progress_event(atom!("progress"), None);
self.dispatch_upload_progress_event(atom!("event"), None);

This comment has been minimized.

Copy link
@nox

nox Feb 8, 2017

Member

Why rename the event?

This comment has been minimized.

Copy link
@charlesvdv

charlesvdv Feb 8, 2017

Author Contributor

This changes removes the progress event named progress.

This comment has been minimized.

Copy link
@nox

nox Feb 8, 2017

Member

Ugh disregard me, didn't read the diff correctly...

This comment has been minimized.

Copy link
@jdm

jdm Feb 13, 2017

Member

So it turns out that the spec does not actually want an event name "event". https://xhr.spec.whatwg.org/#request-error-steps shows that it's a variable representing names passed in by code that invokes that algorithm, so this code will require further changes.

return_if_fetch_was_terminated!();
self.dispatch_upload_progress_event(Atom::from(errormsg), None);
return_if_fetch_was_terminated!();
self.dispatch_upload_progress_event(atom!("loadend"), None);
return_if_fetch_was_terminated!();
}
self.dispatch_response_progress_event(atom!("progress"));
self.dispatch_response_progress_event(atom!("event"));

This comment has been minimized.

Copy link
@nox

nox Feb 8, 2017

Member

Why rename the event?

@charlesvdv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2017

@jdm ping.

@jdm jdm assigned jdm and unassigned wafflespeanut Feb 11, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

With respect to the failing test, I believe the problem is https://dxr.mozilla.org/servo/rev/e394334739635e58bc4d160e9d27bf7217945746/components/script/dom/xmlhttprequest.rs#1058 when the response is gzip'd. I suspect that other browsers do not provide a total size (or perhaps just use a false lengthComputable value) for content for which the Content-Length value cannot be used to infer anything.

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

We could try making lengthComputable false if a Content-Encoding header is present in the response.

@@ -1009,14 +1013,14 @@ impl XMLHttpRequest {
let upload_complete = &self.upload_complete;
if !upload_complete.get() {
upload_complete.set(true);
self.dispatch_upload_progress_event(atom!("progress"), None);
self.dispatch_upload_progress_event(atom!("event"), None);

This comment has been minimized.

Copy link
@jdm

jdm Feb 13, 2017

Member

So it turns out that the spec does not actually want an event name "event". https://xhr.spec.whatwg.org/#request-error-steps shows that it's a variable representing names passed in by code that invokes that algorithm, so this code will require further changes.

return_if_fetch_was_terminated!();
// Subsubsteps 11-12
// self.dispatch_response_progress_event(atom!("progress"));
// return_if_fetch_was_terminated!();

This comment has been minimized.

Copy link
@jdm

jdm Feb 13, 2017

Member

We shouldn't leave commented out code here.

@charlesvdv charlesvdv force-pushed the charlesvdv:xhr-process-event branch from 08bc99e to f0211c0 Feb 16, 2017

self.send_flag.set(false);

self.change_ready_state(XMLHttpRequestState::Done);
return_if_fetch_was_terminated!();
// Subsubsteps 10-12
self.dispatch_response_progress_event(atom!("progress"));

This comment has been minimized.

Copy link
@charlesvdv

charlesvdv Feb 16, 2017

Author Contributor

This removes is not actually following the spec but this is necessary for this test behaviour. If I leave this event, the test fails.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 17, 2017

Contributor

Our goal is generally to match the spec; if there are tests that do not match the spec, we should fix them.

This comment has been minimized.

Copy link
@charlesvdv

charlesvdv Feb 17, 2017

Author Contributor

Looking a bit more into details, I think this change is conforming to the spec. The XHRProgress::Done is only passed when we detect that the transmittion is done (so a progress event has been emitted with the total bits transmitted). According to this note and this isssue whatwg/xhr#72, we can't emit a new process event if we don't have new bits which we don't have because the transmission is already over.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 18, 2017

Member

The note that you linked applies to the previous subsubstep (process request body), and does not apply to process request end-of-body.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 18, 2017

Member

In addition, subsubstep 5 is missing, which does fire a progress event named "progress".

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 24, 2017

Member

Yes. You still need to check the ContentLength here to decide whether or not to fire a progress event.

This comment has been minimized.

Copy link
@charlesvdv

charlesvdv Feb 28, 2017

Author Contributor

I'm really sorry @KiChjang but I don't understand why we need to check the ContentLength? As I've explained above, we are sure that the progress event is dispatched by this line. So when we are asynchronous, we don't need to fire an event at the Done step because each time we add data, a progress event is already fired.

On the other hand, i think that checking if we are synchronous to fire a progress event could be nice. Like this:

if (self.sync.get()) {
    self.dispatch_response_progress_event(atom!("progress"));
}

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 28, 2017

Member

Did you not mention that the ContentLength may be zero? In which case, the Loading case would not have been reached.

This comment has been minimized.

Copy link
@charlesvdv

charlesvdv Feb 28, 2017

Author Contributor

I don't really see why the Loading case is not reached when the ContentLength is zero ?
When I was testing out this PR, the XHRProgress::Loading event was fired even if the ContentLength is zero.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 28, 2017

Member

Oh, I misread. I thought you meant that the ContentLength 0 which makes it impossible to have entered the Loading case.

@charlesvdv charlesvdv force-pushed the charlesvdv:xhr-process-event branch from f0211c0 to c9e0b2f Feb 16, 2017

@KiChjang

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

@highfive highfive assigned KiChjang and unassigned jdm Feb 17, 2017

let mut total_length = total.unwrap_or(0);
let mut length_computable = total.is_some();

if self.response_headers.borrow_mut().has::<ContentEncoding>() {

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 18, 2017

Member
let (total_length, length_computable) = if self.response_headers.borrow().has::<ContentEncoding>() {
    (0, false)
} else {
    (total.unwrap_or(0), total.is_some())
};

@charlesvdv charlesvdv force-pushed the charlesvdv:xhr-process-event branch from c9e0b2f to f938c95 Feb 18, 2017

self.send_flag.set(false);

self.change_ready_state(XMLHttpRequestState::Done);
return_if_fetch_was_terminated!();
// Subsubsteps 10-12
self.dispatch_response_progress_event(atom!("progress"));

This comment has been minimized.

Copy link
@KiChjang

KiChjang Feb 18, 2017

Member

I've added whatwg/xhr#72 (comment) to clarify what the intention of the spec is here. Assuming that not firing progress event when e.loaded == e.total is the way forward even in the case of processing a response, the easiest way to fix the issue you raised is to check whether the ContentLength is 0 before firing a progress event.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

📌 Commit f938c95 has been approved by KiChjang

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

⌛️ Testing commit f938c95 with merge 082671c...

bors-servo added a commit that referenced this pull request Feb 28, 2017
Auto merge of #15455 - charlesvdv:xhr-process-event, r=KiChjang
Implement XHR progress event changes

<!-- Please describe your changes on the following line: -->

This PR implements change in the specs in XHR.

Unfortunatelly, @jdm the test are not passing. It looks like the fetch module send more bytes that we should according the ```Content-Length```. This [lines](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/XMLHttpRequest/progress-events-response-data-gzip.htm#L70) make the test fail.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13276  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15455)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

💔 Test failed - arm32

@KiChjang

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

@bors-servo retry

  • infra
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

⌛️ Testing commit f938c95 with merge 1c10736...

bors-servo added a commit that referenced this pull request Mar 3, 2017
Auto merge of #15455 - charlesvdv:xhr-process-event, r=KiChjang
Implement XHR progress event changes

<!-- Please describe your changes on the following line: -->

This PR implements change in the specs in XHR.

Unfortunatelly, @jdm the test are not passing. It looks like the fetch module send more bytes that we should according the ```Content-Length```. This [lines](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/XMLHttpRequest/progress-events-response-data-gzip.htm#L70) make the test fail.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13276  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15455)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

💔 Test failed - linux-rel-wpt

@KiChjang

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

  ▶ Unexpected subtest result in /XMLHttpRequest/security-consideration.sub.html:
  └ PASS [expected FAIL] ProgressEvent: security consideration

  ▶ Unexpected subtest result in /XMLHttpRequest/send-response-event-order.htm:
  └ PASS [expected FAIL] XMLHttpRequest: The send() method: event order when synchronous flag is unset

  ▶ Unexpected subtest result in /XMLHttpRequest/send-sync-response-event-order.htm:
  └ PASS [expected FAIL] XMLHttpRequest: The send() method: event order when synchronous flag is set

Great work! Update the rest of the test expectations, then you can r=me again.

@bors-servo delegate+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

✌️ @charlesvdv can now approve this pull request

@charlesvdv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2017

@bors-servo r=KiChjang

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

📌 Commit e215cdb has been approved by KiChjang

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

⌛️ Testing commit e215cdb with merge 0294a47...

bors-servo added a commit that referenced this pull request Mar 3, 2017
Auto merge of #15455 - charlesvdv:xhr-process-event, r=KiChjang
Implement XHR progress event changes

<!-- Please describe your changes on the following line: -->

This PR implements change in the specs in XHR.

Unfortunatelly, @jdm the test are not passing. It looks like the fetch module send more bytes that we should according the ```Content-Length```. This [lines](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/XMLHttpRequest/progress-events-response-data-gzip.htm#L70) make the test fail.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13276  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15455)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: KiChjang
Pushing 0294a47 to master...

@bors-servo bors-servo merged commit e215cdb into servo:master Mar 3, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.