style: Make the servo and gecko implementations of Element::note_descendants equivalent.

components/script/layout_wrapper.rs

@@ -59,13 +59,13 @@ use std::marker::PhantomData;
 use std::mem::transmute;
 use std::sync::Arc;
 use std::sync::atomic::Ordering;
+use style;
 use style::attr::AttrValue;
 use style::computed_values::display;
 use style::context::{QuirksMode, SharedStyleContext};
 use style::data::ElementData;
-use style::dom::{DirtyDescendants, LayoutIterator, NodeInfo, OpaqueNode, PresentationalHintsSynthetizer};
-use style::dom::{TElement, TNode};
-use style::dom::UnsafeNode;
+use style::dom::{DescendantsBit, DirtyDescendants, LayoutIterator, NodeInfo, OpaqueNode};
+use style::dom::{PresentationalHintsSynthetizer, TElement, TNode, UnsafeNode};
 use style::element_state::*;
 use style::properties::{ComputedValues, PropertyDeclarationBlock};
 use style::selector_parser::{NonTSPseudoClass, PseudoElement, SelectorImpl};
@@ -409,6 +409,11 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         unsafe { self.as_node().node.get_flag(HAS_DIRTY_DESCENDANTS) }
     }
 
+    unsafe fn note_descendants<B: DescendantsBit<Self>>(&self) {
+        debug_assert!(self.get_data().is_some());
+        style::dom::raw_note_descendants::<Self, B>(*self);
+    }
+
     unsafe fn set_dirty_descendants(&self) {
         debug_assert!(self.as_node().node.get_flag(IS_IN_DOC));
         self.as_node().node.set_flag(HAS_DIRTY_DESCENDANTS, true)
@@ -488,19 +493,22 @@ impl<'le> ServoLayoutElement<'le> {
         }
     }
 
-    // FIXME(bholley): This should be merged with TElement::note_dirty_descendants,
+    // FIXME(bholley): This should be merged with TElement::note_descendants,
     // but that requires re-testing and possibly fixing the broken callers given
     // the FIXME below, which I don't have time to do right now.
+    //
+    // FIXME(emilio): We'd also need to relax the invariant in note_descendants
+    // re. the data already available I think.
     pub unsafe fn note_dirty_descendant(&self) {
         use ::selectors::Element;
 
         let mut current = Some(*self);
         while let Some(el) = current {
             // FIXME(bholley): Ideally we'd have the invariant that any element
-            // with has_dirty_descendants also has the bit set on all its ancestors.
-            // However, there are currently some corner-cases where we get that wrong.
-            // I have in-flight patches to fix all this stuff up, so we just always
-            // propagate this bit for now.
+            // with has_dirty_descendants also has the bit set on all its
+            // ancestors.  However, there are currently some corner-cases where
+            // we get that wrong.  I have in-flight patches to fix all this
+            // stuff up, so we just always propagate this bit for now.
             el.set_dirty_descendants();
             current = el.parent_element();
         }

components/style/dom.rs

@@ -222,6 +222,44 @@ fn fmt_subtree<F, N: TNode>(f: &mut fmt::Formatter, stringify: &F, n: N, indent:
     Ok(())
 }
 
+/// Flag that this element has a descendant for style processing, propagating
+/// the bit up to the root as needed.
+///
+/// This is _not_ safe to call during the parallel traversal.
+///
+/// This is intended as a helper so Servo and Gecko can override it with custom
+/// stuff if needed.
+///
+/// Returns whether no parent had already noted it, that is, whether we reached
+/// the root during the walk up.
+pub unsafe fn raw_note_descendants<E, B>(element: E) -> bool
+    where E: TElement,
+          B: DescendantsBit<E>,
+{
+    debug_assert!(!thread_state::get().is_worker());
+    // TODO(emilio, bholley): Documenting the flags setup a bit better wouldn't
+    // really hurt I guess.
+    debug_assert!(element.get_data().is_some(),
+                  "You should ensure you only flag styled elements");
+
+    let mut curr = Some(element);
+    while let Some(el) = curr {
+        if B::has(el) {
+            break;
+        }
+        B::set(el);
+        curr = el.parent_element();
+    }
+
+    // Note: We disable this assertion on servo because of bugs. See the
+    // comment around note_dirty_descendant in layout/wrapper.rs.
+    if cfg!(feature = "gecko") {
+        debug_assert!(element.descendants_bit_is_propagated::<B>());
+    }
+
+    curr.is_none()
+}
+
 /// A trait used to synthesize presentational hints for HTML element attributes.
 pub trait PresentationalHintsSynthetizer {
     /// Generate the proper applicable declarations due to presentational hints,
@@ -301,30 +339,19 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     /// the actual restyle traversal.
     fn has_dirty_descendants(&self) -> bool;
 
+    /// Flags an element and its ancestors with a given `DescendantsBit`.
+    ///
+    /// TODO(emilio): We call this conservatively from restyle_element_internal
+    /// because we never flag unstyled stuff. A different setup for this may be
+    /// a bit cleaner, but it's probably not worth to invest on it right now
+    /// unless necessary.
+    unsafe fn note_descendants<B: DescendantsBit<Self>>(&self);
+
     /// Flag that this element has a descendant for style processing.
     ///
     /// Only safe to call with exclusive access to the element.
     unsafe fn set_dirty_descendants(&self);
 
-    /// Flag that this element has a descendant for style processing, propagating
-    /// the bit up to the root as needed.
-    ///
-    /// This is _not_ safe to call during the parallel traversal.
-    unsafe fn note_descendants<B: DescendantsBit<Self>>(&self) {
-        debug_assert!(!thread_state::get().is_worker());
-        let mut curr = Some(*self);
-        while curr.is_some() && !B::has(curr.unwrap()) {
-            B::set(curr.unwrap());
-            curr = curr.unwrap().parent_element();
-        }
-
-        // Note: We disable this assertion on servo because of bugs. See the
-        // comment around note_dirty_descendant in layout/wrapper.rs.
-        if cfg!(feature = "gecko") {
-            debug_assert!(self.descendants_bit_is_propagated::<B>());
-        }
-    }
-
     /// Debug helper to be sure the bit is propagated.
     fn descendants_bit_is_propagated<B: DescendantsBit<Self>>(&self) -> bool {
         let mut current = Some(*self);

components/style/gecko/wrapper.rs

@@ -17,7 +17,7 @@
 use atomic_refcell::AtomicRefCell;
 use context::UpdateAnimationsTasks;
 use data::ElementData;
-use dom::{AnimationRules, LayoutIterator, NodeInfo, TElement, TNode, UnsafeNode};
+use dom::{self, AnimationRules, DescendantsBit, LayoutIterator, NodeInfo, TElement, TNode, UnsafeNode};
 use dom::{OpaqueNode, PresentationalHintsSynthetizer};
 use element_state::ElementState;
 use error_reporting::StdoutErrorReporter;
@@ -504,10 +504,18 @@ impl<'le> TElement for GeckoElement<'le> {
     }
 
     unsafe fn set_dirty_descendants(&self) {
+        debug_assert!(self.get_data().is_some());
         debug!("Setting dirty descendants: {:?}", self);
         self.set_flags(NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32)
     }
 
+    unsafe fn note_descendants<B: DescendantsBit<Self>>(&self) {
+        debug_assert!(self.get_data().is_some());
+        if dom::raw_note_descendants::<Self, B>(*self) {
+            bindings::Gecko_SetOwnerDocumentNeedsStyleFlush(self.0);
+        }
+    }
+
     unsafe fn unset_dirty_descendants(&self) {
         self.unset_flags(NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32)
     }