M1504: Implement support for missing XMLHttpRequest APIs

[jitendra29]

We have completed the initial steps for "Implement support for missing XMLHttpRequest APIs"

• Implemented overrideMimeType according to XHR specifications
• Updated the test expectations

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

[eefriedman]

It would probably make sense to update the text_response function in this pull request to actually use the computed override charset and override MIME type. If you don't, at least leave a FIXME there.

---

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

^{components/script/dom/xmlhttprequest.rs, line 661 [r1] (raw file):}
let parsed_mime = try!(mime.parse::<Mime>.map_err(|_| Error::Syntax))

---

^{components/script/dom/xmlhttprequest.rs, line 668 [r1] (raw file):}
let Mime(ref toplevel, ref sublevel, ref params) = override_mime;

---

^{components/script/dom/xmlhttprequest.rs, line 671 [r1] (raw file):}
I don't think there's any reason to convert the MIME type back to a string; the resulting string isn't exposed anywhere.

---

^{components/script/dom/xmlhttprequest.rs, line 675 [r1] (raw file):}
A literal reading of the standard might lead to this... but if you take a moment to think about it, it doesn't make sense for multiple calls to overrideMimeType to partially preserve the state. This should just be *self.override_charset.borrow_mut() = encoding.

---

Comments from the review on Reviewable.io

[eefriedman]

Also, ./mach test-tidy results:

./components/script/dom/xmlhttprequest.rs:657: tab on line
./components/script/dom/xmlhttprequest.rs:659: tab on line
./components/script/dom/xmlhttprequest.rs:660: tab on line
./components/script/dom/xmlhttprequest.rs:661: tab on line
./components/script/dom/xmlhttprequest.rs:668: trailing whitespace
./components/script/dom/xmlhttprequest.rs:671: Line is longer than 120 characters
./components/script/dom/xmlhttprequest.rs:674: Line is longer than 120 characters
./components/script/dom/xmlhttprequest.rs:675: Line is longer than 120 characters
./components/script/dom/xmlhttprequest.rs:675: trailing whitespace
./components/script/dom/xmlhttprequest.rs:676: Line is longer than 120 characters

[jitendra29]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

^{components/script/dom/xmlhttprequest.rs, line 671 [r1] (raw file):}
I am assuming that the override_mime_type field to be added to the XMLHttpRequest structure is a string. Also the specification mentions that "If mime is successfully parsed, set override MIME type to its MIME type, excluding any parameters, and converted to ASCII lowercase."

Could you provide some clarification on this especially since we need to assign the override_mime_type to final MIME type in the further steps which again I am assuming is a string from the xhr specifications?

---

Comments from the review on Reviewable.io

[eefriedman]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

^{components/script/dom/xmlhttprequest.rs, line 671 [r1] (raw file):}
The override MIME type and final MIME type are strings according to the specification... but there isn't any reason to actually store them that way; we just have to behave as if they were represented that way. Storing a mime::Mime makes it clear that you're actually storing a MIME type, not just an arbitrary string.

---

Comments from the review on Reviewable.io

[jitendra29]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

^{components/script/dom/xmlhttprequest.rs, line 671 [r1] (raw file):}
That means we just assign the parsed mime (mime::Mime) to the override_mime_type field and not worry about excluding any parameters, and converting to ASCII lowercase as mentioned in the spec right ?

---

Comments from the review on Reviewable.io

[eefriedman]

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

^{components/script/dom/xmlhttprequest.rs, line 671 [r1] (raw file):}
Yes.

---

Comments from the review on Reviewable.io

[jitendra29]

We have made the changes suggested by @eefriedman and are waiting for someone to review the changes.

[eefriedman]

Reviewed 5 of 6 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{components/script/dom/xmlhttprequest.rs, line 123 [r2] (raw file):}
Unnecessary parentheses.

---

^{components/script/dom/xmlhttprequest.rs, line 666 [r2] (raw file):}
Just encoding_from_whatwg_label(&value) should work, I think? Also, whitespace.

---

^{components/script/dom/xmlhttprequest.rs, line 667 [r2] (raw file):}
Missing semicolon.

Hmm, maybe my previous suggestion for override charset wasn't precisely correct; what I wanted to say was that self.override_charset should be set unconditionally based on the input MIME type; Some(encoding) if there's a charset parameter, None otherwise. So the setting of override_charset should be moved outside of the loop.

---

Comments from the review on Reviewable.io

[eefriedman]

Oh, also, it probably makes sense to use http://doc.servo.org/mime/struct.Mime.html#method.get_param instead of a for loop.

[jitendra29]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/xmlhttprequest.rs, line 667 [r2] (raw file):}
encoding itself is an Option so shouldn't need to do Some(encoding). The default value for self.override_charset is None and it will get assigned to encoding only if the charset parameter is present( in which case the if condition in the loop succeeds)

Could you shed some more light on this ?

---

Comments from the review on Reviewable.io

[eefriedman]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/xmlhttprequest.rs, line 667 [r2] (raw file):}
Specifically, this code should be written so that if someone calls overrideMimeType("text/plain;charset=utf-8"), then overrideMimeType("text/plain"), override_charset is set to None. You could accomplish this by, for example, putting*self.override_charset.borrow_mut() = None; before the loop (but that's not the best way to write it).

---

Comments from the review on Reviewable.io

[jitendra29]

Review status: 5 of 7 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/xmlhttprequest.rs, line 666 [r2] (raw file):}
Done.

---

^{components/script/dom/xmlhttprequest.rs, line 667 [r2] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[eefriedman]

Review status: 5 of 7 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/xmlhttprequest.rs, line 664 [r3] (raw file):}

*self.override_charset.borrow_mut() = value.and_then(|value| {
    encoding_from_whatwg_label(value)
});

---

Comments from the review on Reviewable.io

[jitendra29]

Review status: 5 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

^{components/script/dom/xmlhttprequest.rs, line 664 [r3] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[eefriedman]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[eefriedman]

@bors-servo r+

[jitendra29]

Hi @jdm,

I am assuming that after running the commands you specified, the test expectations shall be adjusted to reflect the current state of the code and thus resulting in no unexpected test results if I run the tests thereafter.

However after running the commands you specified (./mach test-wpt XMLHttpRequest/ --log-raw /tmp/servo.log, followed by ./mach update-wpt /tmp/servo.log), I tried to run ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/ and see that there are still 2 unexpected results. Following is the output of running the above command.

jitendra@jitendra-ubuntu:~/Mozilla/servo$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/
Running 186 tests in web-platform-tests

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (101)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (102)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  │ TIMEOUT [expected PASS] XMLHttpRequest.send(URLSearchParams) (103)
  └   → Test timed out

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  │ TIMEOUT [expected PASS] XMLHttpRequest.send(URLSearchParams) (104)
  └   → Test timed out

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.worker:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (79)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.worker:
  │ TIMEOUT [expected PASS] XMLHttpRequest.send(URLSearchParams) (83)
  └   → Test timed out

Ran 186 tests finished in 327.0 seconds.
  • 184 ran as expected. 5 tests skipped.
  • 2 tests had unexpected subtest results

Any insights on why this is happening as I expect 0 unexpected results after the expectations have been adjusted? Is this the expected behavior? This is the reason I've not updated the pull request still.

[jdm]

I agree, that result is surprising. @jitendra29 Could you try running ./mach build --release and seeing if ./mach test-wpt XMLHttpRequest/ --release shows any different result? I would push your changes to this branch, regardless.

[jitendra29]

I ran the command but I got some new unexpected results this time. The results are as follows:

 ./mach test-wpt XMLHttpRequest/ --release
Running 186 tests in web-platform-tests

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-document.htm:
  │ FAIL [expected PASS] HTML document, shift-jis
  │   → assert_equals: document should be serialized and sent as text/html;charset=UTF-8 (TEST#2) expected "text/html;charset=UTF-8" but got "text/plain;charset=UTF-8"
  │ 
  │ request/<@http://web-platform.test:8000/XMLHttpRequest/send-entity-body-document.htm:18:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1381:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ request@http://web-platform.test:8000/XMLHttpRequest/send-entity-body-document.htm:13:1
  │ init@http://web-platform.test:8000/XMLHttpRequest/send-entity-body-document.htm:29:42
  └ load@http://web-platform.test:8000/XMLHttpRequest/send-entity-body-document.htm:0:1

  ▶ OK [expected TIMEOUT] /XMLHttpRequest/send-usp.html

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (101)

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.html:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (102)

  ▶ OK [expected TIMEOUT] /XMLHttpRequest/send-usp.worker

  ▶ Unexpected subtest result in /XMLHttpRequest/send-usp.worker:
  └ PASS [expected TIMEOUT] XMLHttpRequest.send(URLSearchParams) (79)

Ran 186 tests finished in 198.0 seconds.
  • 183 ran as expected. 5 tests skipped.
  • 2 tests unexpectedly okay
  • 3 tests had unexpected subtest results

Any tips on how should I proceed now ? Should I go ahead and update the pull request anyway ?

[eefriedman]

The send-entity-body-document.htm failure is spurious (#8157).

Regenerate the log with a release build (./mach test-wpt XMLHttpRequest/ --log-raw /tmp/servo.log --release), update-wpt, and upload whatever the result is.

[jitendra29]

The previous update to the pull request just rebases the pull request with servo/master and resolves the merge conflicts.

@jdm @eefriedman Thanks a lot. This time the tests ran without any unexpected results. Following is the output of running the tests:

jitendra@jitendra-ubuntu:~/Mozilla/servo$ ./mach test-wpt XMLHttpRequest/ --release
Running 186 tests in web-platform-tests

Ran 186 tests finished in 193.0 seconds.
  • 186 ran as expected. 5 tests skipped.

I can now go ahead and update the pull request with the new commit updating the test expectations right ?

[eefriedman]

Yes, please update the pull request.

[jitendra29]

@eefriedman done. Do I need to squash these 2 commits into a single commit ?

[eefriedman]

Please squash.

[jitendra29]

done

[eefriedman]

@bors-servo r+

[bors-servo]

📌 Commit ed809a6 has been approved by eefriedman

[bors-servo]

⌛ Testing commit ed809a6 with merge 9fea6d2...

[bors-servo]

💔 Test failed - linux-rel

[jdm]

@bors-servo: retry

• odd git failure

[bors-servo]

⚡ Previous build results for gonk are reusable. Rebuilding only android, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt