Final nits; fix custom elements rare data usage; s/owner_s_r/containing_s_r

Clarify special case for containing_shadow_root and add it to layout
accessor

components/layout_thread/dom_wrapper.rs

@@ -711,7 +711,7 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         unsafe {
             self.element
                 .upcast()
-                .owner_shadow_root_for_layout()
+                .containing_shadow_root_for_layout()
                 .map(ServoShadowRoot::from_layout_js)
         }
     }

components/script/dom/element.rs

@@ -347,10 +347,7 @@ impl Element {
     }
 
     pub fn get_custom_element_definition(&self) -> Option<Rc<CustomElementDefinition>> {
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            return rare_data.custom_element_definition.clone();
-        }
-        None
+        self.rare_data().as_ref()?.custom_element_definition.clone()
     }
 
     pub fn push_callback_reaction(&self, function: Rc<Function>, args: Box<[Heap<JSVal>]>) {
@@ -366,28 +363,32 @@ impl Element {
     }
 
     pub fn clear_reaction_queue(&self) {
-        self.ensure_rare_data()
-            .custom_element_reaction_queue
-            .clear();
+        if let Some(ref mut rare_data) = *self.rare_data_mut() {
+            rare_data.custom_element_reaction_queue.clear();
+        }
     }
 
     pub fn invoke_reactions(&self) {
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            // TODO: This is not spec compliant, as this will allow some reactions to be processed
-            // after clear_reaction_queue has been called.
+        loop {
             rooted_vec!(let mut reactions);
-            while !rare_data.custom_element_reaction_queue.is_empty() {
-                mem::swap(
-                    &mut *reactions,
-                    &mut self.ensure_rare_data().custom_element_reaction_queue,
-                );
+            match *self.rare_data_mut() {
+                Some(ref mut data) => {
+                    mem::swap(&mut *reactions, &mut data.custom_element_reaction_queue)
+                },
+                None => break,
+            };
+
+            if reactions.is_empty() {
+                break;
+            }
+
             for reaction in reactions.iter() {
                 reaction.invoke(self);
             }
+
             reactions.clear();
         }
     }
-    }
 
     /// style will be `None` for elements in a `display: none` subtree. otherwise, the element has a
     /// layout box iff it doesn't have `display: none`.
@@ -442,11 +443,16 @@ impl Element {
         })
     }
 
-    pub fn is_shadow_host(&self) -> bool {
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            return rare_data.shadow_root.is_some();
+    fn shadow_root(&self) -> Option<DomRoot<ShadowRoot>> {
+        self.rare_data()
+            .as_ref()?
+            .shadow_root
+            .as_ref()
+            .map(|sr| DomRoot::from_ref(&**sr))
     }
-        false
+
+    pub fn is_shadow_host(&self) -> bool {
+        self.shadow_root().is_some()
     }
 
     /// https://dom.spec.whatwg.org/#dom-element-attachshadow
@@ -1065,10 +1071,12 @@ impl LayoutElementHelpers for LayoutDom<Element> {
     #[inline]
     #[allow(unsafe_code)]
     unsafe fn get_shadow_root_for_layout(&self) -> Option<LayoutDom<ShadowRoot>> {
-        if let Some(rare_data) = (*self.unsafe_get()).rare_data_for_layout().as_ref() {
-            return rare_data.shadow_root.as_ref().map(|sr| sr.to_layout());
-        }
-        None
+        (*self.unsafe_get())
+            .rare_data_for_layout()
+            .as_ref()?
+            .shadow_root
+            .as_ref()
+            .map(|sr| sr.to_layout())
     }
 }
 
@@ -2742,21 +2750,21 @@ impl VirtualMethods for Element {
                         None
                     }
                 });
-                let owner_shadow_root = self.upcast::<Node>().owner_shadow_root();
+                let containing_shadow_root = self.upcast::<Node>().containing_shadow_root();
                 if node.is_connected() {
                     let value = attr.value().as_atom().clone();
                     match mutation {
                         AttributeMutation::Set(old_value) => {
                             if let Some(old_value) = old_value {
                                 let old_value = old_value.as_atom().clone();
-                                if let Some(ref shadow_root) = owner_shadow_root {
+                                if let Some(ref shadow_root) = containing_shadow_root {
                                     shadow_root.unregister_named_element(self, old_value);
                                 } else {
                                     doc.unregister_named_element(self, old_value);
                                 }
                             }
                             if value != atom!("") {
-                                if let Some(ref shadow_root) = owner_shadow_root {
+                                if let Some(ref shadow_root) = containing_shadow_root {
                                     shadow_root.register_named_element(self, value);
                                 } else {
                                     doc.register_named_element(self, value);
@@ -2765,7 +2773,7 @@ impl VirtualMethods for Element {
                         },
                         AttributeMutation::Removed => {
                             if value != atom!("") {
-                                if let Some(ref shadow_root) = owner_shadow_root {
+                                if let Some(ref shadow_root) = containing_shadow_root {
                                     shadow_root.unregister_named_element(self, value);
                                 } else {
                                     doc.unregister_named_element(self, value);
@@ -2812,8 +2820,7 @@ impl VirtualMethods for Element {
 
         let doc = document_from_node(self);
 
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            if let Some(ref shadow_root) = rare_data.shadow_root {
+        if let Some(ref shadow_root) = self.shadow_root() {
             doc.register_shadow_root(&shadow_root);
             let shadow_root = shadow_root.upcast::<Node>();
             shadow_root.set_flag(NodeFlags::IS_CONNECTED, context.tree_connected);
@@ -2822,14 +2829,13 @@ impl VirtualMethods for Element {
                 node.bind_to_tree(context);
             }
         }
-        }
 
         if !context.tree_connected {
             return;
         }
 
         if let Some(ref value) = *self.id_attribute.borrow() {
-            if let Some(shadow_root) = self.upcast::<Node>().owner_shadow_root() {
+            if let Some(shadow_root) = self.upcast::<Node>().containing_shadow_root() {
                 shadow_root.register_named_element(self, value.clone());
             } else {
                 doc.register_named_element(self, value.clone());
@@ -2852,8 +2858,7 @@ impl VirtualMethods for Element {
 
         let doc = document_from_node(self);
 
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            if let Some(ref shadow_root) = rare_data.shadow_root {
+        if let Some(ref shadow_root) = self.shadow_root() {
             doc.unregister_shadow_root(&shadow_root);
             let shadow_root = shadow_root.upcast::<Node>();
             shadow_root.set_flag(NodeFlags::IS_CONNECTED, false);
@@ -2862,7 +2867,6 @@ impl VirtualMethods for Element {
                 node.unbind_from_tree(context);
             }
         }
-        }
 
         let fullscreen = doc.GetFullscreenElement();
         if fullscreen.deref() == Some(self) {
@@ -2931,7 +2935,7 @@ impl<'a> SelectorsElement for DomRoot<Element> {
     }
 
     fn containing_shadow_host(&self) -> Option<Self> {
-        if let Some(shadow_root) = self.upcast::<Node>().owner_shadow_root() {
+        if let Some(shadow_root) = self.upcast::<Node>().containing_shadow_root() {
             Some(shadow_root.Host())
         } else {
             None

components/script/dom/htmlbaseelement.rs

@@ -64,7 +64,7 @@ impl HTMLBaseElement {
     /// Update the cached base element in response to binding or unbinding from
     /// a tree.
     pub fn bind_unbind(&self, tree_in_doc: bool) {
-        if !tree_in_doc || self.upcast::<Node>().owner_shadow_root().is_some() {
+        if !tree_in_doc || self.upcast::<Node>().containing_shadow_root().is_some() {
             return;
         }
 

components/script/dom/macros.rs

@@ -639,6 +639,11 @@ macro_rules! impl_rare_data (
             self.rare_data.borrow()
         }
 
+        #[allow(dead_code)]
+        fn rare_data_mut(&self) -> RefMut<Option<Box<$type>>> {
+            self.rare_data.borrow_mut()
+        }
+
         fn ensure_rare_data(&self) -> RefMut<Box<$type>> {
             let mut rare_data = self.rare_data.borrow_mut();
             if rare_data.is_none() {

components/script/dom/node.rs

@@ -282,10 +282,10 @@ impl Node {
 
         for node in new_child.traverse_preorder(ShadowIncluding::No) {
             if parent_in_shadow_tree {
-                if let Some(shadow_root) = self.owner_shadow_root() {
-                    node.set_owner_shadow_root(&*shadow_root);
+                if let Some(shadow_root) = self.containing_shadow_root() {
+                    node.set_containing_shadow_root(&*shadow_root);
                 }
-                debug_assert!(node.owner_shadow_root().is_some());
+                debug_assert!(node.containing_shadow_root().is_some());
             }
             node.set_flag(NodeFlags::IS_IN_DOC, parent_in_doc);
             node.set_flag(NodeFlags::IS_IN_SHADOW_TREE, parent_in_shadow_tree);
@@ -946,21 +946,21 @@ impl Node {
         self.owner_doc.set(Some(document));
     }
 
-    pub fn owner_shadow_root(&self) -> Option<DomRoot<ShadowRoot>> {
+    pub fn containing_shadow_root(&self) -> Option<DomRoot<ShadowRoot>> {
+        // NodeRareData contains the shadow root the node belongs to,
+        // but this node may be a shadow root itself.
         if let Some(ref shadow_root) = self.downcast::<ShadowRoot>() {
             return Some(DomRoot::from_ref(shadow_root));
         }
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            return rare_data
-                .owner_shadow_root
+        self.rare_data()
+            .as_ref()?
+            .containing_shadow_root
             .as_ref()
-                .map(|sr| DomRoot::from_ref(&**sr));
-        }
-        None
+            .map(|sr| DomRoot::from_ref(&**sr))
     }
 
-    pub fn set_owner_shadow_root(&self, shadow_root: &ShadowRoot) {
-        self.ensure_rare_data().owner_shadow_root = Some(Dom::from_ref(shadow_root));
+    pub fn set_containing_shadow_root(&self, shadow_root: &ShadowRoot) {
+        self.ensure_rare_data().containing_shadow_root = Some(Dom::from_ref(shadow_root));
     }
 
     pub fn is_in_html_doc(&self) -> bool {
@@ -1195,7 +1195,7 @@ pub trait LayoutNodeHelpers {
     unsafe fn next_sibling_ref(&self) -> Option<LayoutDom<Node>>;
 
     unsafe fn owner_doc_for_layout(&self) -> LayoutDom<Document>;
-    unsafe fn owner_shadow_root_for_layout(&self) -> Option<LayoutDom<ShadowRoot>>;
+    unsafe fn containing_shadow_root_for_layout(&self) -> Option<LayoutDom<ShadowRoot>>;
 
     unsafe fn is_element_for_layout(&self) -> bool;
     unsafe fn get_flag(&self, flag: NodeFlags) -> bool;
@@ -1280,14 +1280,16 @@ impl LayoutNodeHelpers for LayoutDom<Node> {
 
     #[inline]
     #[allow(unsafe_code)]
-    unsafe fn owner_shadow_root_for_layout(&self) -> Option<LayoutDom<ShadowRoot>> {
-        if let Some(rare_data) = (*self.unsafe_get()).rare_data_for_layout().as_ref() {
-            return rare_data
-                .owner_shadow_root
-                .as_ref()
-                .map(|sr| sr.to_layout());
+    unsafe fn containing_shadow_root_for_layout(&self) -> Option<LayoutDom<ShadowRoot>> {
+        if let Some(ref shadow_root) = self.downcast::<ShadowRoot>() {
+            return Some(*shadow_root);
         }
-        None
+        (*self.unsafe_get())
+            .rare_data_for_layout()
+            .as_ref()?
+            .containing_shadow_root
+            .as_ref()
+            .map(|sr| sr.to_layout())
     }
 
     #[inline]
@@ -2292,19 +2294,13 @@ impl NodeMethods for Node {
 
     // https://dom.spec.whatwg.org/#dom-node-getrootnode
     fn GetRootNode(&self, options: &GetRootNodeOptions) -> DomRoot<Node> {
-        if options.composed {
-            if let Some(rare_data) = self.rare_data().as_ref() {
-                if let Some(ref shadow_root) = rare_data.owner_shadow_root {
+        if let Some(shadow_root) = self.containing_shadow_root() {
+            return if options.composed {
                 // shadow-including root.
-                    return shadow_root.Host().upcast::<Node>().GetRootNode(options);
-                }
-            }
-        }
-
-        if let Some(rare_data) = self.rare_data().as_ref() {
-            if let Some(ref shadow_root) = rare_data.owner_shadow_root {
-                return DomRoot::from_ref(shadow_root.upcast::<Node>());
-            }
+                shadow_root.Host().upcast::<Node>().GetRootNode(options)
+            } else {
+                DomRoot::from_ref(shadow_root.upcast::<Node>())
+            };
         }
 
         if self.is_in_doc() {
@@ -2837,7 +2833,7 @@ pub fn document_from_node<T: DerivedFrom<Node> + DomObject>(derived: &T) -> DomR
 pub fn containing_shadow_root<T: DerivedFrom<Node> + DomObject>(
     derived: &T,
 ) -> Option<DomRoot<ShadowRoot>> {
-    derived.upcast().owner_shadow_root()
+    derived.upcast().containing_shadow_root()
 }
 
 #[allow(unrooted_must_root)]

components/script/dom/raredata.rs

@@ -19,7 +19,7 @@ pub struct NodeRareData {
     /// The shadow root the node belongs to.
     /// This is None if the node is not in a shadow tree or
     /// if it is a ShadowRoot.
-    pub owner_shadow_root: Option<Dom<ShadowRoot>>,
+    pub containing_shadow_root: Option<Dom<ShadowRoot>>,
     /// Registered observers for this node.
     pub mutation_observers: Vec<RegisteredObserver>,
 }
@@ -28,6 +28,7 @@ pub struct NodeRareData {
 #[must_root]
 pub struct ElementRareData {
     /// https://dom.spec.whatwg.org/#dom-element-shadowroot
+    /// The ShadowRoot this element is host of.
     /// XXX This is currently not exposed to web content. Only for
     ///     internal use.
     pub shadow_root: Option<Dom<ShadowRoot>>,