Switched Element::Get_attributes to use a RootedVec #5692

pgonda · 2015-04-14T20:44:44Z

Changes Element::get_attributes to use a &mut Rooted<JS<Attr>> param instead of returning a Vec<Temporary<Attr>>, fixes #5684.

pgonda · 2015-04-15T15:58:48Z

@jdm Could this get a code review? Thanks!

jdm · 2015-04-15T16:31:50Z

Reviewed files:

components/script/dom/element.rs @ r1
components/script/dom/node.rs @ r1

^{components/script/dom/element.rs, line 703 [r1] (raw file):}
Is the explicit type here necessary?

^{components/script/dom/element.rs, line 956 [r1] (raw file):}
This doesn't look necessary.

^{components/script/dom/node.rs, line 2426 [r1] (raw file):}
Is the explicit type here necessary?

Comments from the review on Reviewable.io

nox · 2015-04-17T08:55:59Z

components/script/dom/element.rs

-        self.get_attributes(local_name).into_iter().map(|attr| attr.root())
+        let mut attributes = RootedVec::new();
+        self.get_attributes(local_name, &mut attributes);
+        attributes.iter().map(|attr| attr.root())


Now that Temporary::root() does not consume the Temporary value, that could just be:

attributes.iter().find(|attr| attr.root().r().namespace() == namespace)

I'm not even sure the best way to write get_attribute() is by calling get_attributes(), maybe it should just work on self.attrs directly?

bors-servo · 2015-04-21T17:59:40Z

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

jdm · 2015-04-21T18:12:03Z

Reviewed files:

components/script/dom/element.rs @ r2

Comments from the review on Reviewable.io

jdm · 2015-04-21T18:12:32Z

Sorry, I was thrown off by the change from awaiting-review to needs-code-changes.

pgonda · 2015-04-21T18:25:34Z

^{components/script/dom/element.rs, line 705 [r2] (raw file):}
Without calling get_attributes() wouldn't we just have all that code duplicated?

Comments from the review on Reviewable.io

jdm · 2015-04-21T18:55:22Z

Let's leave it using get_attributes; we can file an issue separately if we want to change that.


        fixing review notes, fixes servo#5692

pgonda · 2015-04-22T14:55:58Z

How can I resolve these conflicts?

jdm · 2015-04-22T15:26:50Z

@pgonda You will need to rebase your changes on a recent version of the master branch. You can follow the steps at https://github.com/servo/servo/wiki/Github-&-Critic-PR-handling-101 - the step starting at "When you know there is a substantive change" is the relevant one here. You can ignore the step about "Use the Rebase Branch button".

jdm · 2015-04-23T18:45:58Z

@bors-servo: r+

bors-servo · 2015-04-23T18:45:59Z

📌 Commit dbfbbc5 has been approved by jdm

bors-servo · 2015-04-23T19:14:42Z

⌛ Testing commit dbfbbc5 with merge e40253c...


        Auto merge of #5692 - pgonda:get_attributes-memory-safety, r=jdm

Changes Element::get_attributes to use a `&mut Rooted<JS<Attr>>` param instead of returning a `Vec<Temporary<Attr>>`, fixes #5684.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5692)

bors-servo · 2015-04-23T19:21:16Z

💔 Test failed - mac2

jdm · 2015-04-27T13:18:34Z

@pgonda Could you squash these again?


        Switched Element::Get_attributes to use a RootedVec instead of return…

…ing a Vec<Temporary>, fixes #5684

pgonda · 2015-04-27T20:24:58Z

squashed!

jdm · 2015-04-27T20:25:27Z

@bors-servo: r+

bors-servo · 2015-04-27T20:25:28Z

📌 Commit a270f3e has been approved by jdm

bors-servo · 2015-04-27T20:25:30Z

⌛ Testing commit a270f3e with merge 8ecb9d6...


        Auto merge of #5692 - pgonda:get_attributes-memory-safety, r=jdm

Changes Element::get_attributes to use a `&mut Rooted<JS<Attr>>` param instead of returning a `Vec<Temporary<Attr>>`, fixes #5684.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5692)

bors-servo · 2015-04-27T21:03:48Z

💔 Test failed - linux2

jdm · 2015-04-27T21:32:13Z

@bors-servo: retry

#5798

bors-servo · 2015-04-27T21:42:20Z

⚡ Previous build results for android, gonk, linux1, mac1, mac2 are reusable. Rebuilding only linux2...

bors-servo · 2015-04-27T22:27:33Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

highfive added the S-awaiting-review label Apr 14, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 15, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 15, 2015

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Apr 16, 2015

nox reviewed Apr 17, 2015
View changes

metajack assigned jdm Apr 17, 2015

pgonda added a commit to pgonda/servo that referenced this pull request Apr 21, 2015

fixing review notes, fixes servo#5692

29a6789

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 21, 2015

pgonda force-pushed the pgonda:get_attributes-memory-safety branch from 29a6789 to dbfbbc5 Apr 22, 2015

jdm added S-awaiting-merge and removed S-awaiting-review labels Apr 23, 2015

highfive removed the S-awaiting-merge label Apr 26, 2015

highfive added the S-awaiting-review label Apr 26, 2015

jdm added S-needs-squash and removed S-awaiting-review labels Apr 27, 2015

Switched Element::Get_attributes to use a RootedVec instead of return…

Loading status checks…

a270f3e

…ing a Vec<Temporary>, fixes #5684

pgonda force-pushed the pgonda:get_attributes-memory-safety branch from 0783e30 to a270f3e Apr 27, 2015

highfive added the S-awaiting-review label Apr 27, 2015

jdm added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Apr 27, 2015

bors-servo merged commit a270f3e into servo:master Apr 27, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Switched Element::Get_attributes to use a RootedVec #5692

Switched Element::Get_attributes to use a RootedVec #5692

pgonda commented Apr 14, 2015

pgonda commented Apr 15, 2015

jdm commented Apr 15, 2015

This comment has been minimized.

This comment has been minimized.

bors-servo commented Apr 21, 2015

jdm commented Apr 21, 2015

jdm commented Apr 21, 2015

pgonda commented Apr 21, 2015

jdm commented Apr 21, 2015

pgonda commented Apr 22, 2015

jdm commented Apr 22, 2015

jdm commented Apr 23, 2015

bors-servo commented Apr 23, 2015

bors-servo commented Apr 23, 2015

bors-servo commented Apr 23, 2015

jdm commented Apr 27, 2015

pgonda commented Apr 27, 2015

jdm commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015

jdm commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015

servo / servo

Join GitHub today

Switched Element::Get_attributes to use a RootedVec #5692

Switched Element::Get_attributes to use a RootedVec #5692

Conversation

pgonda commented Apr 14, 2015

pgonda commented Apr 15, 2015

jdm commented Apr 15, 2015

This comment has been minimized.

nox Apr 17, 2015

This comment has been minimized.

nox Apr 17, 2015

bors-servo commented Apr 21, 2015

jdm commented Apr 21, 2015

jdm commented Apr 21, 2015

pgonda commented Apr 21, 2015

jdm commented Apr 21, 2015

pgonda commented Apr 22, 2015

jdm commented Apr 22, 2015

jdm commented Apr 23, 2015

bors-servo commented Apr 23, 2015

bors-servo commented Apr 23, 2015

bors-servo commented Apr 23, 2015

jdm commented Apr 27, 2015

pgonda commented Apr 27, 2015

jdm commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015

jdm commented Apr 27, 2015

bors-servo commented Apr 27, 2015

bors-servo commented Apr 27, 2015