Implemented XMLHttpRequest.send(Document) #14648

Merged
merged 1 commit into from Jan 8, 2017

Projects

None yet

6 participants

@cynicaldevil
Contributor
cynicaldevil commented Dec 20, 2016 edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #9490 .

r? @KiChjang

XMLHttpRequest/send-entity-body-document.htm is not working atm, I get this output:

$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.

This change is Reviewable

components/script/dom/xmlhttprequest.rs
+ Some(DocumentOrBodyInit::Blob(ref b)) => { Some(b.extract()) },
+ Some(DocumentOrBodyInit::FormData(ref formdata)) => { Some(formdata.extract()) },
+ Some(DocumentOrBodyInit::String(ref str)) => { Some(str.extract()) },
+ Some(DocumentOrBodyInit::URLSearchParams(ref urlsp)) => { Some(urlsp.extract()) },
@KiChjang
KiChjang Dec 20, 2016 Member

nit: Braces are not required when you only have 1 expression inside of it.

components/script/dom/xmlhttprequest.rs
+ }
+}
+
+pub fn serialize_document(doc: &Document) -> Fallible<DOMString> {
@KiChjang
KiChjang Dec 20, 2016 edited Member

nit: Doesn't really need to be pub.

@KiChjang
Member

Great work! This is very easy to review for the eyes. For the ignored test case, check to make sure that the tests under that directory is enabled (look for tests/wpt/include.ini and tests/wpt/metadata/XMLHttpRequest/send-entity-body-document.htm.ini and inspect their contents to see if they've marked the test as disabled).

@KiChjang KiChjang self-assigned this Dec 20, 2016
@cynicaldevil
Contributor
cynicaldevil commented Dec 20, 2016 edited

It is. Are there any other tests related to this? Like send-entity-body-document-bogus.htm?

@KiChjang
Member

Ah ok, you need to remove tests/wpt/metadata/XMLHttpRequest/send-entity-body-document.htm.ini.

@cynicaldevil
Contributor

I tried it, I am getting something like this (image stolen from prev. PR, its identical) :
img

components/script/dom/xmlhttprequest.rs
- let extracted = data.as_ref().map(|d| d.extract());
+ let extracted_or_serialized = match data {
+ Some(DocumentOrBodyInit::Document(ref doc)) => {
+ let data = Vec::from(serialize_document(&doc).unwrap().as_ref());
@KiChjang
KiChjang Dec 20, 2016 Member

You should really check whether serialize_document returns an Err or not instead of unwrapping. If it does, return Err(Error::InvalidState).

@cynicaldevil
cynicaldevil Dec 20, 2016 Contributor

got it.

@KiChjang
Member
@bors-servo
Contributor

⌛️ Trying commit f62ae44 with merge 54dd4ab...

@bors-servo bors-servo added a commit that referenced this pull request Dec 20, 2016
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=<try>
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
54dd4ab
@bors-servo
Contributor

💔 Test failed - mac-rel-wpt1

@KiChjang
Member

Not a lot of tests are passing; the implementation looks like it needs improvement.

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:
  └ PASS [expected FAIL] iframe-noload-noallowfullscreen

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:
  └ PASS [expected FAIL] iframe-noload-allowfullscreen

  ▶ Unexpected subtest result in /XMLHttpRequest/setrequestheader-content-type.htm:
  └ PASS [expected FAIL] XML Document request has correct default Content-Type of "application/xml;charset=UTF-8"

  ▶ Unexpected subtest result in /XMLHttpRequest/setrequestheader-content-type.htm:
  └ PASS [expected FAIL] HTML Document request has correct default Content-Type of "text/html;charset=UTF-8"

  ▶ Unexpected subtest result in /XMLHttpRequest/setrequestheader-content-type.htm:
  │ FAIL [expected PASS] HTML Document request keeps setRequestHeader() Content-Type, with charset adjusted to UTF-8
  │   → assert_equals: expected "content-type: text/html+junk;charset=utf-8" but got "content-type: text/html+junk;charset=ascii"
  │ 
  │ request/<@http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:26:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ request@http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:3:9
  └ @http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:96:7

  ▶ Unexpected subtest result in /XMLHttpRequest/setrequestheader-content-type.htm:
  │ FAIL [expected PASS] XML Document request keeps setRequestHeader() Content-Type, with charset adjusted to UTF-8
  │   → assert_equals: expected "content-type: application/xhtml+xml;charset=utf-8" but got "content-type: application/xhtml+xml;charset=ascii"
  │ 
  │ request/<@http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:26:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ request@http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:3:9
  └ @http://web-platform.test:8000/XMLHttpRequest/setrequestheader-content-type.htm:78:7
@cynicaldevil
Contributor

hmm...two failing tests, plus send-entity-body-document.htm and send-entity-body-document-bogus.htm...I'll look into these tomorrow.

@Ms2ger
Contributor
Ms2ger commented Dec 21, 2016

iframe-allowfullscreen.html is #14607; you can ignore that. As for the failure in setrequestheader-content-type.htm, you may be missing the requirement

Otherwise, if the header named Content-Type in author request headers has a value that is a valid MIME type, which has a charset parameter whose value is not a case-insensitive match for encoding, and encoding is not null, then set all the charset parameters whose value is not a case-insensitive match for encoding of that Content-Type header's value to encoding.

@cynicaldevil
Contributor
cynicaldevil commented Dec 23, 2016 edited

@Ms2ger fixed it, also updated the manifests for passing tests.

For send-entity-body-document-bogus.htm.ini the error's defintely occuring in the function call serialize in xmlhttprequest.rs (line 1394), because the test passes when I explicitly return Err(Error::InvalidState) from serialize_document.

@KiChjang
Member
@bors-servo
Contributor

⌛️ Trying commit 4133207 with merge 7fa803b...

@bors-servo bors-servo added a commit that referenced this pull request Dec 23, 2016
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=<try>
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
7fa803b
@bors-servo
Contributor

💔 Test failed - arm32

@bors-servo
Contributor

⌛️ Trying commit 4133207 with merge 37177b1...

@bors-servo bors-servo added a commit that referenced this pull request Dec 24, 2016
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=<try>
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
37177b1
@cynicaldevil
Contributor

Ok I might have found out why send-entity-body-document-bogus.htm isn't working. The node.serialize function (specifically this part here) returns Ok() regardless of whether the document is valid or not. It returns an error only if there is one while serializing the children, but in this case, the document being tested doesn't have any.

Also, the spec for serializing a Document node mentions this:

If the require well-formed flag is set (its value is true), and this node has no documentElement (the documentElement attribute's value is null), then throw an exception; the serialization of this node would not be a well-formed document.

I searched for the flag mentioned above, but the only flags I could find for Node are these, and I don't think these contain the flag specified above.

@Ms2ger
Contributor
Ms2ger commented Dec 25, 2016

You can ignore that test, I think. We handle XML wellformedness errors differently.

@cynicaldevil
Contributor

@Ms2ger: The only relevant test left then is probably XMLHttpRequest/send-entity-body-document.htm, which keeps timing out, but I can't find out why.

@cynicaldevil
Contributor

I dug into the timeouts further, and found that it was happening due to two failing tests (still don't know why it should timeout only because the tests are failing).

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-document.htm:
  │ FAIL [expected PASS] XML document, windows-1252
  │   → assert_true: "<ÿ/>" not in "<html><body><p>Unknown content type (application/xml).</p></body></html>" expected true got false
  │ 

and:

  ▶ Unexpected subtest result in /XMLHttpRequest/send-entity-body-document.htm:
  │ FAIL [expected PASS] HTML document, shift-jis
  │   → assert_true: "<body>テスト</body>" not in "<html><head></head><body>�e�X�g</body></html>" expected true got false

I can't figure out why the first test's failing, but the second test seems to be failing because of some encoding error.

@cynicaldevil
Contributor

The first document (with the "ÿ" character) is being serialized here, and cdata.data() returns Unknown content type (application/xml), which appears in the test output as well. I can't figure out why this happening, and need some help to move forward.

@jdm
Member
jdm commented Jan 3, 2017

That's a result of this code not dealing with the application/xml mime type. We should probably handle it the same as text/xml.

@cynicaldevil
Contributor

Doesn't work, it prints some random characters.

@cynicaldevil
Contributor

@jdm: Updated PR as per conversation on IRC.

components/script/dom/xmlhttprequest.rs
- }
+ BodyInit::String(ref s) => { s.extract() }
+ BodyInit::URLSearchParams(ref usp) => { usp.extract() }
+ BodyInit::Blob(ref b) => { b.extract()}
@KiChjang
KiChjang Jan 5, 2017 Member

nit: missing space after ()

@cynicaldevil
Contributor

@KiChjang : Fixed.

components/script/dom/xmlhttprequest.rs
+
+impl Extractable for DOMString {
+ fn extract(&self) -> (Vec<u8>, Option<DOMString>) {
+ let encoding = UTF_8 as EncodingRef;
@KiChjang
KiChjang Jan 7, 2017 Member

Is this cast necessary?

@cynicaldevil
cynicaldevil Jan 7, 2017 Contributor

hmm....I don't remember. Let me try to compile it without the cast.

@KiChjang
Member
KiChjang commented Jan 7, 2017

r=me once you address the comment I left.

@bors-servo delegate+

@bors-servo
Contributor

✌️ @cynicaldevil can now approve this pull request

@KiChjang
Member
KiChjang commented Jan 7, 2017

Also remember to file a followup issue per discussion in http://logs.glob.uno/?c=mozilla%23servo&s=4+Jan+2017&e=4+Jan+2017&h=cynicaldevil#c586898.

@cynicaldevil
Contributor
@bors-servo
Contributor

📌 Commit 3dab561 has been approved by cynicaldevil

@cynicaldevil cynicaldevil Implemented XMLHttpRequest.send(Document)
401836e
@cynicaldevil
Contributor
cynicaldevil commented Jan 7, 2017 edited
@cynicaldevil
Contributor
cynicaldevil commented Jan 7, 2017 edited

@bors-servo r+
@KiChjang A debug file had snuck into the original commit due to my negligence. Sorry for that.

@bors-servo
Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #14893
@bors-servo
Contributor

⌛️ Testing commit 401836e with merge 063fc17...

@bors-servo bors-servo added a commit that referenced this pull request Jan 7, 2017
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=cynicaldevil
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
063fc17
@bors-servo
Contributor

💔 Test failed - linux-dev

@KiChjang
Member
KiChjang commented Jan 8, 2017

@bors-servo r- r+ retry

@bors-servo
Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #14905
@bors-servo
Contributor

📌 Commit 401836e has been approved by KiChjang

@bors-servo
Contributor

⌛️ Testing commit 401836e with merge 95dec99...

@bors-servo bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=KiChjang
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
95dec99
@bors-servo
Contributor

💔 Test failed - linux-dev

@KiChjang
Member
KiChjang commented Jan 8, 2017

@bors-servo retry

@bors-servo
Contributor

⌛️ Testing commit 401836e with merge 87cce81...

@bors-servo bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo bors-servo Auto merge of #14648 - cynicaldevil:xmlhttpreq-send, r=KiChjang
Implemented XMLHttpRequest.send(Document)

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

---
<!-- 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 #9490 .

<!-- Either: -->
r? @KiChjang

`XMLHttpRequest/send-entity-body-document.htm` is not working atm, I get this output:
```
$ ./mach test-wpt tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm
Running 1 tests in web-platform-tests

Ran 1 tests finished in 0.0 seconds.
  • 1 ran as expected. 1 tests skipped.
```
<!-- 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/14648)
<!-- Reviewable:end -->
87cce81
@bors-servo
Contributor

💔 Test failed - linux-dev

@KiChjang
Member
KiChjang commented Jan 8, 2017

@bors-servo retry

@bors-servo
Contributor

⚡️ Previous build results for android, arm64, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only arm32, linux-dev...

@bors-servo bors-servo merged commit 401836e into servo:master Jan 8, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment