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

Proof of concept: parameterized node traversal methods. #14451

Closed
wants to merge 3 commits into from

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Dec 3, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Dec 3, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmltitleelement.rs, components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/documentfragment.rs, components/script/dom/htmlfieldsetelement.rs, components/script/dom/characterdata.rs, components/script/dom/servoparser/html.rs, components/script/dom/htmloptgroupelement.rs, components/script/dom/servoparser/mod.rs, components/script/dom/htmltableelement.rs, components/script/dom/element.rs, components/script/dom/htmltablecellelement.rs, components/script/dom/htmloptionelement.rs, components/script/devtools.rs
  • @KiChjang: components/script/dom/htmltitleelement.rs, components/script/dom/document.rs, components/script/dom/node.rs, components/script/dom/documentfragment.rs, components/script/dom/htmlfieldsetelement.rs, components/script/dom/characterdata.rs, components/script/dom/servoparser/html.rs, components/script/dom/htmloptgroupelement.rs, components/script/dom/servoparser/mod.rs, components/script/dom/htmltableelement.rs, components/script/dom/element.rs, components/script/dom/htmltablecellelement.rs, components/script/dom/htmloptionelement.rs, components/script/devtools.rs
@highfive
Copy link

highfive commented Dec 3, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 3, 2016

I've had this sitting around in a branch for a while, thought I might as well open a PR. There's still a little more work to do here, but I didn't want to spend any more time on it if the idea is not well received.

@frewsxcv frewsxcv changed the title Proof of concept idea: parameterized node traversal methods. Proof of concep: parameterized node traversal methods. Dec 3, 2016
@jdm
Copy link
Member

jdm commented Dec 3, 2016

I'm in favour!

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 3, 2016

Can we use default type parameters yet?

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 3, 2016

Can we use default type parameters yet?

I thought about making it ....<T=Node>(..), but then I ran into rust-lang/rust#36887

frewsxcv added 2 commits Dec 6, 2016
tmp
@@ -2,6 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#![allow(unrooted_must_root)]

This comment has been minimized.

Copy link
@jdm

jdm Dec 6, 2016

Member

Please remove this. We should only scope it for the code that requires it.

This comment has been minimized.

Copy link
@frewsxcv

frewsxcv Dec 6, 2016

Author Member

What? This isn't a wanted improvement? :)

But actually, this is just temporary until I can figure out why the lint is complaining about the iterator structure being unrooted

@frewsxcv frewsxcv changed the title Proof of concep: parameterized node traversal methods. Proof of concept: parameterized node traversal methods. Dec 8, 2016
@jdm jdm assigned jdm and unassigned glennw Dec 15, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Dec 16, 2016

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

@wafflespeanut
Copy link
Member

wafflespeanut commented Jan 12, 2017

Ping?

@frewsxcv
Copy link
Member Author

frewsxcv commented Jan 12, 2017

I don't have much time right now to finish this up. If someone else wants to, go for it :)

@jdm
Copy link
Member

jdm commented Jan 27, 2017

Tracking this work in #15270.

@jdm jdm closed this Jan 27, 2017
@DominoTree DominoTree mentioned this pull request Feb 16, 2017
2 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.