Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMake attributes lose their owner when removed #5507
Conversation
highfive
commented
Apr 3, 2015
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. |
hoppipolla-critic-bot
commented
Apr 3, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/4501 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
| use dom::bindings::js::{OptionalRootedRootable, RootedReference}; | ||
| use dom::bindings::js::{JSRef, MutNullableJS, Temporary}; | ||
| use dom::bindings::js::{OptionalRootable, OptionalRootedRootable, | ||
| RootedReference}; |
This comment has been minimized.
This comment has been minimized.
Manishearth
Apr 3, 2015
Member
nit: For multiline imports, we usually just import twice, eg
use dom::bindings::js::{OptionalRootable, OptionalRootedRootable};
use dom::bindings::js::RootedReference;
| fn set_owner(self, owner: Option<JSRef<Element>>) { | ||
| let ns = self.namespace.clone(); | ||
| match (self.owner.get().root().r(), owner) { | ||
| (None, Some(new)) => |
This comment has been minimized.
This comment has been minimized.
| match (self.owner.get().root().r(), owner) { | ||
| (None, Some(new)) => | ||
| // Already in the list of attributes of new owner. | ||
| assert!(new.get_attribute(ns, &self.local_name).root().r() == Some(self)), |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Manishearth
Apr 3, 2015
Member
okay, leave it then. We probably should move some asserts to debug asserts later, but for now I guess we can stick to asserts everywhere
| @@ -244,6 +247,24 @@ impl<'a> AttrHelpers<'a> for JSRef<'a, Attr> { | |||
| &self.extended_deref().local_name | |||
| } | |||
|
|
|||
| fn set_owner(self, owner: Option<JSRef<Element>>) { | |||
This comment has been minimized.
This comment has been minimized.
Manishearth
Apr 3, 2015
Member
I suggest you leave a doc comment on this stating that it should be used after having moved the attr
|
Mostly looks good, save a few nits. r=me when they're fixed. I assume there are no tests for this save for the one you upstreamed in wpt (which doesn't yet exist in our copy) |
|
Nits fixed. |
|
Rolled up into #5519 because of CI brokenness |
|
Rebased as #5524 |
nox commentedApr 3, 2015
Test: web-platform-tests/wpt#1718