DOMImplementation::createDocument should set content type based on namespace #10830

cjkenn · 2016-04-24T22:11:36Z

Set document content type based on the namespace.
Standard: https://dom.spec.whatwg.org/#dom-domimplementation-createdocument
Fixes #10743.

This change is

highfive · 2016-04-24T22:11:40Z

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.

highfive · 2016-04-24T22:11:41Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/domimplementation.rs

Ms2ger · 2016-04-25T09:01:47Z

Thanks for your PR.

You'll need to update ./tests/wpt/metadata/dom/nodes/DOMImplementation-createDocument.html.ini (probably by removing it entirely).

-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/domimplementation.rs, line 76 [r2] (raw file):
Maybe move the DOMString::from call into the XMLDocument::new call, so this match just has the static strings.

Comments from Reviewable

cjkenn · 2016-04-25T14:20:57Z

Ahhh I didn't realize there was a config file for expected test failures. Since all the tests pass, I will remove it.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Ms2ger · 2016-04-26T08:23:03Z

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/domimplementation.rs, line 84 [r3] (raw file):
I'm sorry, I didn't explain that very well. I'd like you to write Some(DOMString::from(content_type)) here, and not change the XMLDocument::new API itself.

Comments from Reviewable

cjkenn · 2016-04-27T01:31:48Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/domimplementation.rs, line 84 [r3] (raw file):
I really should have clarified that before I made the change! I did feel awkward doing it that way >_<

Comments from Reviewable

Ms2ger · 2016-04-27T06:55:30Z

Thanks! This is good to go once you squash the commits into one; let me know if you need help with that.

-S-awaiting-review +S-needs-squash

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

cjkenn · 2016-04-28T04:17:01Z

Squashed and ready to go!

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

KiChjang · 2016-04-28T05:17:03Z

@bors-servo r=Ms2ger

bors-servo · 2016-04-28T05:17:04Z

📌 Commit fb1ac9c has been approved by Ms2ger

bors-servo · 2016-04-28T05:17:06Z

⌛ Testing commit fb1ac9c with merge e28cab5...


        Auto merge of #10830 - cjkenn:10743, r=Ms2ger

DOMImplementation::createDocument should set content type based on namespace Set document content type based on the namespace. Standard: https://dom.spec.whatwg.org/#dom-domimplementation-createdocument Fixes #10743.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10830)

bors-servo · 2016-04-28T06:05:51Z

💔 Test failed - mac-rel-wpt

highfive · 2016-04-28T06:05:54Z

  ▶ Unexpected subtest result in /dom/nodes/Document-contentType/contentType/createDocument.html:
  │ FAIL [expected PASS] document.implementation.createDocument: document.contentType === &#39;application/xml&#39;
  │   → assert_equals: expected &#34;application/xml&#34; but got &#34;application/xhtml+xml&#34;
  │ 
  │ @http://web-platform.test:8000/dom/nodes/Document-contentType/contentType/createDocument.html:3:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  └ @http://web-platform.test:8000/dom/nodes/Document-contentType/contentType/createDocument.html:1:1

Ms2ger · 2016-04-28T09:12:11Z

You'll have to update tests/wpt/web-platform-tests/dom/nodes/Document-contentType/contentType/createDocument.html too.


        [10743] Add content type to XmlDocument constructor based on namespace

[10743] Fix namespace in createDocument test [10743] Remove test ini file, match returns static strings instead of DOMString. [10743] Fix arguments to XMLDocument::new Update failing test [10743] Add content type to XmlDocument constructor based on namespace [10743] Fix namespace in createDocument test [10743] Remove test ini file, match returns static strings instead of DOMString. [10743] Fix arguments to XMLDocument::new Update failing test

cjkenn · 2016-04-29T02:29:11Z

Updated that test file and squashed again. I didn't run the full wpt-test suite, as I don't have enough time to wait for it (that or I cannot figure out how to run it faster). I checked a few tests in areas where they might fail because of this and they were passing, so hopefully there is no more failures...

KiChjang · 2016-04-29T03:25:53Z

@bors-servo r=Ms2ger

bors-servo · 2016-04-29T03:25:56Z

📌 Commit d9128fb has been approved by Ms2ger

bors-servo · 2016-04-29T04:31:31Z

⌛ Testing commit d9128fb with merge 408f9ab...


        Auto merge of #10830 - cjkenn:10743, r=Ms2ger

DOMImplementation::createDocument should set content type based on namespace Set document content type based on the namespace. Standard: https://dom.spec.whatwg.org/#dom-domimplementation-createdocument Fixes #10743.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10830)

bors-servo · 2016-04-29T05:37:38Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

Ms2ger · 2016-04-29T13:18:42Z

@cjkenn thanks! We don't expect you to run all of wpt locally; that's what our bits are for :)

highfive assigned metajack Apr 24, 2016

highfive added the S-awaiting-review label Apr 24, 2016

cjkenn changed the title ~~DOMImplementation::createDocument should set content type based on namespace #10743~~ DOMImplementation::createDocument should set content type based on namespace Apr 24, 2016

cjkenn mentioned this pull request Apr 24, 2016

DOMImplementation::createDocument should set content type based on namespace #10743

Closed

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Apr 25, 2016

Ms2ger assigned Ms2ger and unassigned metajack Apr 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 25, 2016

Ms2ger added S-needs-squash and removed S-awaiting-review labels Apr 27, 2016

cjkenn force-pushed the cjkenn:10743 branch from 3cfa7fe to fb1ac9c Apr 28, 2016

highfive added the S-awaiting-review label Apr 28, 2016

highfive removed S-awaiting-review S-needs-squash labels Apr 28, 2016

highfive added the S-awaiting-merge label Apr 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 28, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Apr 29, 2016

cjkenn force-pushed the cjkenn:10743 branch from 5293892 to d9128fb Apr 29, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 29, 2016

bors-servo merged commit d9128fb into servo:master Apr 29, 2016
3 checks passed

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

highfive removed the S-awaiting-merge label Apr 29, 2016

bors-servo mentioned this pull request Apr 29, 2016

Use SIMD in `gfx` when possible #10900

Closed

servo / servo

DOMImplementation::createDocument should set content type based on namespace #10830

DOMImplementation::createDocument should set content type based on namespace #10830

cjkenn commented Apr 24, 2016 •

edited by Ms2ger

highfive commented Apr 24, 2016

highfive commented Apr 24, 2016

Ms2ger commented Apr 25, 2016

cjkenn commented Apr 25, 2016

Ms2ger commented Apr 26, 2016

cjkenn commented Apr 27, 2016

Ms2ger commented Apr 27, 2016

cjkenn commented Apr 28, 2016

KiChjang commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

highfive commented Apr 28, 2016

Ms2ger commented Apr 28, 2016

cjkenn commented Apr 29, 2016

KiChjang commented Apr 29, 2016

bors-servo commented Apr 29, 2016

bors-servo commented Apr 29, 2016

bors-servo commented Apr 29, 2016

Ms2ger commented Apr 29, 2016

servo / servo

Join GitHub today

DOMImplementation::createDocument should set content type based on namespace #10830

DOMImplementation::createDocument should set content type based on namespace #10830

Conversation

cjkenn commented Apr 24, 2016 • edited by Ms2ger

highfive commented Apr 24, 2016

highfive commented Apr 24, 2016

Ms2ger commented Apr 25, 2016

cjkenn commented Apr 25, 2016

Ms2ger commented Apr 26, 2016

cjkenn commented Apr 27, 2016

Ms2ger commented Apr 27, 2016

cjkenn commented Apr 28, 2016

KiChjang commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

highfive commented Apr 28, 2016

Ms2ger commented Apr 28, 2016

cjkenn commented Apr 29, 2016

KiChjang commented Apr 29, 2016

bors-servo commented Apr 29, 2016

bors-servo commented Apr 29, 2016

bors-servo commented Apr 29, 2016

Ms2ger commented Apr 29, 2016

cjkenn commented Apr 24, 2016 •

edited by Ms2ger