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

Update concept of node filter algorithm #18255

Merged
merged 2 commits into from Oct 21, 2017

Conversation

CYBAI
Copy link
Member

@CYBAI CYBAI commented Aug 26, 2017

Implement new filter algorithm from specification



This change is Reviewable

@highfive
Copy link

highfive commented Aug 26, 2017

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

@highfive
Copy link

highfive commented Aug 26, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/nodeiterator.rs, components/script/dom/treewalker.rs
  • @KiChjang: components/script/dom/nodeiterator.rs, components/script/dom/treewalker.rs

@highfive
Copy link

highfive commented Aug 26, 2017

warning Warning warning

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

// "acceptNode", and a list of arguments consisting of node. If that throws an exception,
// then unset the active flag and rethrow the exception."
// "7. Unset the active flag"
// "8. Return result."
Copy link
Member

@KiChjang KiChjang Aug 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why these step annotations are there, we usually just do step n. @nox?

Copy link
Member Author

@CYBAI CYBAI Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KiChjang Due to the existing step annotation, I just updated them to the new one. If they're unnecessary, please let me remove them.

Copy link
Member

@KiChjang KiChjang Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem to be left by a contributor back in #3253, when we did not have guidelines as to how we're supposed to leave step annotations.
So yes, please change them back into step n instead.

@CYBAI CYBAI force-pushed the prevent-reentrancy-pr18218 branch from 049c2c1 to 777d098 Compare Aug 27, 2017
@jdm
Copy link
Member

jdm commented Aug 27, 2017

@bors-servo: try

@bors-servo
Copy link
Contributor

bors-servo commented Aug 27, 2017

Trying commit 777d098 with merge 80a839b...

bors-servo added a commit that referenced this pull request Aug 27, 2017
Update concept of node filter algorithm

Implement new [filter](https://dom.spec.whatwg.org/#concept-node-filter) algorithm from specification

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18218 (github issue number if applicable).
- [ ]  There are tests for these changes OR
- [X] These changes do not require tests because changes are minimal and the error was triggered by a test

<!-- 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/18255)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 27, 2017

💔 Test failed - mac-rel-wpt2

@CYBAI
Copy link
Member Author

CYBAI commented Aug 27, 2017

@jdm @KiChjang @shinglyu
Is this build error from @bors-servo related to #18228 ?

@KiChjang
Copy link
Member

KiChjang commented Aug 27, 2017

Yeah, looks like it.

@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2017

Trying commit 777d098 with merge 57365363463332696f457fac71184afa83fadafe...

@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2017

Trying commit 777d098 with merge be11311...

bors-servo added a commit that referenced this pull request Aug 29, 2017
Update concept of node filter algorithm

Implement new [filter](https://dom.spec.whatwg.org/#concept-node-filter) algorithm from specification

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18218 (github issue number if applicable).
- [X] These changes do not require tests because changes are minimal and the error was triggered by a test

<!-- 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/18255)
<!-- Reviewable:end -->
@shinglyu
Copy link
Member

shinglyu commented Aug 29, 2017

Sorry for triggering it two times. I though I had the wrong syntax so asked twice.

@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2017

@jdm
Copy link
Member

jdm commented Aug 29, 2017

I bet there are tests upstream that would be affected by this, but we're currently stuck on an old version of web-platform-tests :(

@CYBAI
Copy link
Member Author

CYBAI commented Aug 29, 2017

Hi @jdm , is there anything I can help or update for it?

@shinglyu
Copy link
Member

shinglyu commented Aug 31, 2017

#17416 <= this?

@CYBAI CYBAI force-pushed the prevent-reentrancy-pr18218 branch from 777d098 to 5339123 Compare Sep 17, 2017
@CYBAI
Copy link
Member Author

CYBAI commented Sep 23, 2017

ping @shinglyu @jdm @KiChjang
Is there anything I can help or update for this PR?

@bors-servo
Copy link
Contributor

bors-servo commented Sep 26, 2017

The latest upstream changes (presumably #18635) made this pull request unmergeable. Please resolve the merge conflicts.

@CYBAI CYBAI force-pushed the prevent-reentrancy-pr18218 branch 2 times, most recently from f325650 to f846d87 Compare Sep 30, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

The latest upstream changes (presumably #18921) made this pull request unmergeable. Please resolve the merge conflicts.

@CYBAI
Copy link
Member Author

CYBAI commented Oct 21, 2017

@bors-servo: try

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

@CYBAI: 🔑 Insufficient privileges: and not in try users

@wafflespeanut
Copy link
Member

wafflespeanut commented Oct 21, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

Trying commit 8245aad with merge 6be2082...

bors-servo added a commit that referenced this pull request Oct 21, 2017
Update concept of node filter algorithm

Implement new [filter](https://dom.spec.whatwg.org/#concept-node-filter) algorithm from specification

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18218 (github issue number if applicable).
- [X] These changes do not require tests because changes are minimal and the error was triggered by a test

<!-- 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/18255)
<!-- Reviewable:end -->
@CYBAI
Copy link
Member Author

CYBAI commented Oct 21, 2017

@wafflespeanut Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

@jdm
Copy link
Member

jdm commented Oct 21, 2017

@bors-servo: r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

📌 Commit 8245aad has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

Testing commit 8245aad with merge 1667fcc...

bors-servo added a commit that referenced this pull request Oct 21, 2017
Update concept of node filter algorithm

Implement new [filter](https://dom.spec.whatwg.org/#concept-node-filter) algorithm from specification

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #18218 (github issue number if applicable).
- [X] These changes do not require tests because changes are minimal and the error was triggered by a test

<!-- 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/18255)
<!-- Reviewable:end -->
@KiChjang
Copy link
Member

KiChjang commented Oct 21, 2017

@CYBAI Your 2nd commit message is incorrect, you're not removing legacy tests - you're updating test expectations.

But instead of renaming it, can I ask you to squash your commits into one, please?

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 1667fcc to master...

@bors-servo bors-servo merged commit 8245aad into servo:master Oct 21, 2017
@CYBAI
Copy link
Member Author

CYBAI commented Oct 22, 2017

@KiChjang Oh... sorry for late reply. This PR has been merged. How can I update them or any way I can help? Thank.

@CYBAI CYBAI deleted the prevent-reentrancy-pr18218 branch Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent reentrancy in TreeWalker::accept_node and NodeIterator::accept_node
9 participants