Replacing Node.rootNode for Node.getRootNode. #13703

Rafagd · 2016-10-11T20:24:08Z

Changed the specification for the Node element to reflect the new standard.
Changed the implementation of the NodeMethod.

I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13699 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-11T20:24:14Z

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

highfive · 2016-10-11T20:24:15Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/node.rs, components/script/dom/webidls/Node.webidl

highfive · 2016-10-11T20:24:16Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

Ms2ger · 2016-10-13T10:36:32Z

components/script/dom/webidls/Node.webidl

  [Pure]
-  readonly attribute Node rootNode;
+  Node getRootNode(optional GetRootNodeOptions options);


I would prefer if you declared this as Node getRootNode();, without the argument. That makes it more obvious that the argument is not implemented yet.

I've removed the optional argument.

Ms2ger · 2016-10-13T15:21:32Z

Code looks good, but I expect we'll need to update some test expectations.

@bors-servo try

bors-servo · 2016-10-13T15:21:35Z

⌛ Trying commit 7b68746 with merge 3da821c...


        Auto merge of #13703 - Rafagd:my_changes, r=<try>

Replacing Node.rootNode for Node.getRootNode. - Changed the specification for the Node element to reflect the new standard. - Changed the implementation of the NodeMethod. I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13699 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13703)

bors-servo · 2016-10-13T16:00:15Z

💔 Test failed - mac-rel-wpt1

Rafagd · 2016-10-13T16:38:34Z

@Ms2ger Is there a way for me to only run the failed tests?

KiChjang · 2016-10-13T17:04:34Z

@Rafagd You need to update the test expectations located in tests/wpt/metadata/dom/interfaces.html.ini and tests/wpt/metadata/dom/nodes/rootNode.html and tests/wpt/metadata/dom/historical.html.

Rafagd · 2016-10-13T17:17:07Z

Oh, now I get it. Working on it.

KiChjang

By default, all tests are expected to PASS. You can simply remove the lines of code where it's expected to FAIL instead of changing them to PASS.

KiChjang · 2016-10-13T18:48:07Z

@bors-servo try

bors-servo · 2016-10-13T18:48:11Z

⌛ Trying commit 00dca6c with merge 9d408f4...


        Auto merge of #13703 - Rafagd:my_changes, r=<try>

Replacing Node.rootNode for Node.getRootNode. - Changed the specification for the Node element to reflect the new standard. - Changed the implementation of the NodeMethod. I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13699 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13703)

Rafagd · 2016-10-13T19:00:09Z

@KiChjang I'm removing the PASS tests now

Rafagd · 2016-10-13T19:07:27Z

@KiChjang I suppose I can remove those 2 files that only have the lines below, right?

[rootNode.html] type: testharness

KiChjang · 2016-10-13T19:08:10Z

Yes.

KiChjang · 2016-10-13T19:22:36Z

@bors-servo try


        Auto merge of #13703 - Rafagd:my_changes, r=<try>

Replacing Node.rootNode for Node.getRootNode. - Changed the specification for the Node element to reflect the new standard. - Changed the implementation of the NodeMethod. I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13699 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13703)

bors-servo · 2016-10-13T19:22:40Z

⌛ Trying commit 53a6182 with merge 1ba7e59...

bors-servo · 2016-10-13T21:01:23Z

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

KiChjang · 2016-10-13T21:16:07Z

@Rafagd You'll now need to squash all your commits into one, and we're ready to land this!


        Replacing Node.prototype.rootNode for Node.prototype.getRootNode.

Rafagd · 2016-10-14T09:49:05Z

Everything is squashed into a single commit now.

Ms2ger · 2016-10-14T15:59:55Z

Thanks!

@bors-servo r+

bors-servo · 2016-10-14T15:59:56Z

📌 Commit 0074d2a has been approved by Ms2ger

bors-servo · 2016-10-14T16:35:28Z

⌛ Testing commit 0074d2a with merge fbe96d1...


        Auto merge of #13703 - Rafagd:my_changes, r=Ms2ger

Replacing Node.rootNode for Node.getRootNode. - Changed the specification for the Node element to reflect the new standard. - Changed the implementation of the NodeMethod. I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13699 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13703)

bors-servo · 2016-10-14T17:10:04Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-14T17:10:07Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html aed6845267bba6c52d3aa69c4889449b454d8117
/_mozilla/css/inline_block_opacity_change_ref.html febec1b4730afab195f280694dad47200b09ea3c
Testing aed6845267bba6c52d3aa69c4889449b454d8117 == febec1b4730afab195f280694dad47200b09ea3c

KiChjang · 2016-10-14T17:11:48Z

@bors-servo retry

#13360

bors-servo · 2016-10-14T19:07:13Z

⌛ Testing commit 0074d2a with merge 697e197...


        Auto merge of #13703 - Rafagd:my_changes, r=Ms2ger

Replacing Node.rootNode for Node.getRootNode. - Changed the specification for the Node element to reflect the new standard. - Changed the implementation of the NodeMethod. I'm still waiting for some answers for the questions I've posted on #13699, but I'm already doing the pull request so I can get some feedback as well. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13699 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13703)

bors-servo · 2016-10-14T19:46:19Z

💔 Test failed - linux-rel-css

KiChjang · 2016-10-14T19:48:26Z

@bors-servo retry

#13775

bors-servo · 2016-10-14T19:48:28Z

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

bors-servo · 2016-10-14T20:46:03Z

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

highfive assigned mbrubeck Oct 11, 2016

highfive added the S-awaiting-review label Oct 11, 2016

Rafagd mentioned this pull request Oct 12, 2016

Implement Node.getRootNode #13699

Closed

Ms2ger requested changes Oct 13, 2016

View changes

highfive added the S-tests-failed label Oct 13, 2016

highfive removed the S-tests-failed label Oct 13, 2016

KiChjang requested changes Oct 13, 2016

View changes

KiChjang added S-needs-squash and removed S-awaiting-review labels Oct 13, 2016

Replacing Node.prototype.rootNode for Node.prototype.getRootNode.

Loading status checks…

0074d2a

highfive added the S-awaiting-review label Oct 13, 2016

highfive assigned Ms2ger and unassigned mbrubeck Oct 14, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Oct 14, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 14, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 14, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 14, 2016

bors-servo merged commit 0074d2a into servo:master Oct 14, 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

Rafagd deleted the Rafagd:my_changes branch Oct 14, 2016

servo / servo

Replacing Node.rootNode for Node.getRootNode. #13703

Replacing Node.rootNode for Node.getRootNode. #13703

Rafagd commented Oct 11, 2016 •

edited by larsbergstrom

highfive commented Oct 11, 2016

highfive commented Oct 11, 2016

highfive commented Oct 11, 2016

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Oct 13, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 13, 2016

Rafagd commented Oct 13, 2016

KiChjang commented Oct 13, 2016 •

edited

Rafagd commented Oct 13, 2016

KiChjang left a comment

KiChjang commented Oct 13, 2016

bors-servo commented Oct 13, 2016

Rafagd commented Oct 13, 2016

Rafagd commented Oct 13, 2016

KiChjang commented Oct 13, 2016

KiChjang commented Oct 13, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 13, 2016

KiChjang commented Oct 13, 2016

Rafagd commented Oct 14, 2016

Ms2ger commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

highfive commented Oct 14, 2016

KiChjang commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

KiChjang commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

servo / servo

Join GitHub today

Replacing Node.rootNode for Node.getRootNode. #13703

Replacing Node.rootNode for Node.getRootNode. #13703

Conversation

Rafagd commented Oct 11, 2016 • edited by larsbergstrom

highfive commented Oct 11, 2016

highfive commented Oct 11, 2016

highfive commented Oct 11, 2016

This comment has been minimized.

Ms2ger Oct 13, 2016

This comment has been minimized.

Rafagd Oct 13, 2016

Ms2ger commented Oct 13, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 13, 2016

Rafagd commented Oct 13, 2016

KiChjang commented Oct 13, 2016 • edited

Rafagd commented Oct 13, 2016

KiChjang left a comment

KiChjang commented Oct 13, 2016

bors-servo commented Oct 13, 2016

Rafagd commented Oct 13, 2016

Rafagd commented Oct 13, 2016

KiChjang commented Oct 13, 2016

KiChjang commented Oct 13, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 13, 2016

KiChjang commented Oct 13, 2016

Rafagd commented Oct 14, 2016

Ms2ger commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

highfive commented Oct 14, 2016

KiChjang commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

KiChjang commented Oct 14, 2016

bors-servo commented Oct 14, 2016

bors-servo commented Oct 14, 2016

Rafagd commented Oct 11, 2016 •

edited by larsbergstrom

KiChjang commented Oct 13, 2016 •

edited