Auto merge of #13841 - bholley:has_changed, r=emilio

Simplify TNode a bit, removing has_changed from style

A couple of changes here:
* Remove the option to unset with the dirty bit settes.
* Add an explicit API for setting text node style.
* Hoist has_changed handling into the restyle damage setter and text node style setter.
* Make set_style take a non-Option.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13841)
<!-- Reviewable:end -->

components/layout/traversal.rs

@@ -118,12 +118,7 @@ fn construct_flows_at<'a, N: LayoutNode>(context: &'a LayoutContext<'a>, root: O
         tnode.set_restyle_damage(RestyleDamage::empty());
     }
 
-    unsafe {
+    unsafe { node.clear_dirty_bits(); }
-        node.set_changed(false);
-        node.set_dirty(false);
-        node.set_dirty_descendants(false);
-    }
-
     remove_from_bloom_filter(context, root, node);
 }
 

components/layout_thread/lib.rs

@@ -1135,12 +1135,12 @@ impl LayoutThread {
                 while let Some(node) = next {
                     if node.needs_dirty_on_viewport_size_changed() {
                         // NB: The dirty bit is propagated down the tree.
-                        unsafe { node.set_dirty(true); }
+                        unsafe { node.set_dirty(); }
 
                         let mut current = node.parent_node();
                         while let Some(node) = current {
                             if node.has_dirty_descendants() { break; }
-                            unsafe { node.set_dirty_descendants(true); }
+                            unsafe { node.set_dirty_descendants(); }
                             current = node.parent_node();
                         }
 
@@ -1161,7 +1161,7 @@ impl LayoutThread {
             if needs_dirtying {
                 // NB: The dirty flag is propagated down during the restyle
                 // process.
-                node.set_dirty(true);
+                node.set_dirty();
             }
         }
         if needs_reflow {

components/script/layout_wrapper.rs

@@ -62,7 +62,7 @@ use style::computed_values::display;
 use style::context::SharedStyleContext;
 use style::data::{PersistentStyleData, PseudoStyles};
 use style::dom::{LayoutIterator, NodeInfo, OpaqueNode, PresentationalHintsSynthetizer, TDocument, TElement, TNode};
-use style::dom::UnsafeNode;
+use style::dom::{TRestyleDamage, UnsafeNode};
 use style::element_state::*;
 use style::properties::{ComputedValues, PropertyDeclarationBlock};
 use style::selector_impl::{ElementSnapshot, NonTSPseudoClass, PseudoElement, ServoSelectorImpl};
@@ -190,28 +190,20 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         self.node.downcast().map(ServoLayoutDocument::from_layout_js)
     }
 
-    fn has_changed(&self) -> bool {
-        unsafe { self.node.get_flag(HAS_CHANGED) }
-    }
-
-    unsafe fn set_changed(&self, value: bool) {
-        self.node.set_flag(HAS_CHANGED, value)
-    }
-
     fn is_dirty(&self) -> bool {
         unsafe { self.node.get_flag(IS_DIRTY) }
     }
 
-    unsafe fn set_dirty(&self, value: bool) {
+    unsafe fn set_dirty(&self) {
-        self.node.set_flag(IS_DIRTY, value)
+        self.node.set_flag(IS_DIRTY, true)
     }
 
     fn has_dirty_descendants(&self) -> bool {
         unsafe { self.node.get_flag(HAS_DIRTY_DESCENDANTS) }
     }
 
-    unsafe fn set_dirty_descendants(&self, value: bool) {
+    unsafe fn set_dirty_descendants(&self) {
-        self.node.set_flag(HAS_DIRTY_DESCENDANTS, value)
+        self.node.set_flag(HAS_DIRTY_DESCENDANTS, true)
     }
 
     fn needs_dirty_on_viewport_size_changed(&self) -> bool {
@@ -246,8 +238,8 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         self.borrow_data().and_then(|x| x.style.clone())
     }
 
-    fn set_style(&self, style: Option<Arc<ComputedValues>>) {
+    fn set_style(&self, style: Arc<ComputedValues>) {
-        self.mutate_data().unwrap().style = style;
+        self.mutate_data().unwrap().style = Some(style);
     }
 
     fn take_pseudo_styles(&self) -> PseudoStyles {
@@ -259,11 +251,24 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         self.mutate_data().unwrap().per_pseudo = styles;
     }
 
+    fn style_text_node(&self, style: Arc<ComputedValues>) {
+        debug_assert!(self.is_text_node());
+        let mut data = self.get_partial_layout_data().unwrap().borrow_mut();
+        data.style_data.style = Some(style);
+        if self.has_changed() {
+            data.restyle_damage = RestyleDamage::rebuild_and_reflow();
+        }
+    }
+
     fn restyle_damage(self) -> RestyleDamage {
         self.get_partial_layout_data().unwrap().borrow().restyle_damage
     }
 
     fn set_restyle_damage(self, damage: RestyleDamage) {
+        let mut damage = damage;
+        if self.has_changed() {
+            damage = RestyleDamage::rebuild_and_reflow();
+        }
         self.get_partial_layout_data().unwrap().borrow_mut().restyle_damage = damage;
     }
 
@@ -330,6 +335,16 @@ impl<'ln> LayoutNode for ServoLayoutNode<'ln> {
         self.script_type_id().into()
     }
 
+    fn has_changed(&self) -> bool {
+        unsafe { self.node.get_flag(HAS_CHANGED) }
+    }
+
+    unsafe fn clear_dirty_bits(&self) {
+        self.node.set_flag(HAS_CHANGED, false);
+        self.node.set_flag(IS_DIRTY, false);
+        self.node.set_flag(HAS_DIRTY_DESCENDANTS, false);
+    }
+
     fn get_style_data(&self) -> Option<&AtomicRefCell<PersistentStyleData>> {
         unsafe {
             self.get_jsmanaged().get_style_and_layout_data().map(|d| {

components/script_layout_interface/wrapper_traits.rs

@@ -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(unsafe_code)]
+
 use HTMLCanvasData;
 use LayoutNodeType;
 use OpaqueStyleAndLayoutData;
@@ -77,6 +79,10 @@ pub trait LayoutNode: TNode {
     /// Returns the type ID of this node.
     fn type_id(&self) -> LayoutNodeType;
 
+    fn has_changed(&self) -> bool;
+
+    unsafe fn clear_dirty_bits(&self);
+
     fn get_style_data(&self) -> Option<&AtomicRefCell<PersistentStyleData>>;
 
     fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData);

components/style/dom.rs

@@ -116,17 +116,13 @@ pub trait TNode : Sized + Copy + Clone + NodeInfo {
 
     fn as_document(&self) -> Option<Self::ConcreteDocument>;
 
-    fn has_changed(&self) -> bool;
-
-    unsafe fn set_changed(&self, value: bool);
-
     fn is_dirty(&self) -> bool;
 
-    unsafe fn set_dirty(&self, value: bool);
+    unsafe fn set_dirty(&self);
 
     fn has_dirty_descendants(&self) -> bool;
 
-    unsafe fn set_dirty_descendants(&self, value: bool);
+    unsafe fn set_dirty_descendants(&self);
 
     fn needs_dirty_on_viewport_size_changed(&self) -> bool;
 
@@ -155,7 +151,7 @@ pub trait TNode : Sized + Copy + Clone + NodeInfo {
     fn get_existing_style(&self) -> Option<Arc<ComputedValues>>;
 
     /// Sets the computed style for this node.
-    fn set_style(&self, style: Option<Arc<ComputedValues>>);
+    fn set_style(&self, style: Arc<ComputedValues>);
 
     /// Transfers ownership of the existing pseudo styles, if any, to the
     /// caller. The stored pseudo styles are replaced with an empty map.
@@ -164,6 +160,9 @@ pub trait TNode : Sized + Copy + Clone + NodeInfo {
     /// Sets the pseudo styles on the element, replacing any existing styles.
     fn set_pseudo_styles(&self, styles: PseudoStyles);
 
+    /// Set the style for a text node.
+    fn style_text_node(&self, style: Arc<ComputedValues>);
+
     /// Get the description of how to account for recent style changes.
     fn restyle_damage(self) -> Self::ConcreteRestyleDamage;
 
@@ -235,19 +234,19 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
         let mut curr = node;
         while let Some(parent) = curr.parent_node() {
             if parent.has_dirty_descendants() { break }
-            unsafe { parent.set_dirty_descendants(true); }
+            unsafe { parent.set_dirty_descendants(); }
             curr = parent;
         }
 
         // Process hints.
         if hint.contains(RESTYLE_SELF) {
-            unsafe { node.set_dirty(true); }
+            unsafe { node.set_dirty(); }
         // XXX(emilio): For now, dirty implies dirty descendants if found.
         } else if hint.contains(RESTYLE_DESCENDANTS) {
-            unsafe { node.set_dirty_descendants(true); }
+            unsafe { node.set_dirty_descendants(); }
             let mut current = node.first_child();
             while let Some(node) = current {
-                unsafe { node.set_dirty(true); }
+                unsafe { node.set_dirty(); }
                 current = node.next_sibling();
             }
         }
@@ -256,7 +255,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
             let mut next = ::selectors::Element::next_sibling_element(self);
             while let Some(sib) = next {
                 let sib_node = sib.as_node();
-                unsafe { sib_node.set_dirty(true) };
+                unsafe { sib_node.set_dirty() };
                 next = ::selectors::Element::next_sibling_element(&sib);
             }
         }

components/style/gecko/wrapper.rs

@@ -22,9 +22,9 @@ use gecko_bindings::bindings::{Gecko_GetLastChild, Gecko_GetNextStyleChild};
 use gecko_bindings::bindings::{Gecko_GetServoDeclarationBlock, Gecko_IsHTMLElementInHTMLDocument};
 use gecko_bindings::bindings::{Gecko_IsLink, Gecko_IsRootElement};
 use gecko_bindings::bindings::{Gecko_IsUnvisitedLink, Gecko_IsVisitedLink, Gecko_Namespace};
-use gecko_bindings::bindings::{Gecko_SetNodeFlags, Gecko_UnsetNodeFlags};
 use gecko_bindings::bindings::Gecko_ClassOrClassList;
 use gecko_bindings::bindings::Gecko_GetStyleContext;
+use gecko_bindings::bindings::Gecko_SetNodeFlags;
 use gecko_bindings::structs;
 use gecko_bindings::structs::{NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO, NODE_IS_DIRTY_FOR_SERVO};
 use gecko_bindings::structs::{RawGeckoDocument, RawGeckoElement, RawGeckoNode};
@@ -94,10 +94,6 @@ impl<'ln> GeckoNode<'ln> {
         unsafe { Gecko_SetNodeFlags(self.0, flags) }
     }
 
-    fn unset_flags(&self, flags: u32) {
-        unsafe { Gecko_UnsetNodeFlags(self.0, flags) }
-    }
-
     fn get_node_data(&self) -> Option<&NonOpaqueStyleData> {
         unsafe {
             from_opaque_style_data(self.0.mServoData.get()).as_ref()
@@ -241,14 +237,6 @@ impl<'ln> TNode for GeckoNode<'ln> {
         unimplemented!()
     }
 
-    // NOTE: This is not relevant for Gecko, since we get explicit restyle hints
-    // when a content has changed.
-    fn has_changed(&self) -> bool { false }
-
-    unsafe fn set_changed(&self, _value: bool) {
-        unimplemented!()
-    }
-
     fn is_dirty(&self) -> bool {
         // Return true unconditionally if we're not yet styled. This is a hack
         // and should go away soon.
@@ -259,12 +247,8 @@ impl<'ln> TNode for GeckoNode<'ln> {
         self.flags() & (NODE_IS_DIRTY_FOR_SERVO as u32) != 0
     }
 
-    unsafe fn set_dirty(&self, value: bool) {
+    unsafe fn set_dirty(&self) {
-        if value {
         self.set_flags(NODE_IS_DIRTY_FOR_SERVO as u32)
-        } else {
-            self.unset_flags(NODE_IS_DIRTY_FOR_SERVO as u32)
-        }
     }
 
     fn has_dirty_descendants(&self) -> bool {
@@ -276,12 +260,8 @@ impl<'ln> TNode for GeckoNode<'ln> {
         self.flags() & (NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) != 0
     }
 
-    unsafe fn set_dirty_descendants(&self, value: bool) {
+    unsafe fn set_dirty_descendants(&self) {
-        if value {
         self.set_flags(NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32)
-        } else {
-            self.unset_flags(NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32)
-        }
     }
 
     fn can_be_fragmented(&self) -> bool {
@@ -307,8 +287,8 @@ impl<'ln> TNode for GeckoNode<'ln> {
         self.borrow_data().and_then(|x| x.style.clone())
     }
 
-    fn set_style(&self, style: Option<Arc<ComputedValues>>) {
+    fn set_style(&self, style: Arc<ComputedValues>) {
-        self.mutate_data().unwrap().style = style;
+        self.mutate_data().unwrap().style = Some(style);
     }
 
     fn take_pseudo_styles(&self) -> PseudoStyles {
@@ -323,6 +303,11 @@ impl<'ln> TNode for GeckoNode<'ln> {
         self.mutate_data().unwrap().per_pseudo = styles;
     }
 
+    fn style_text_node(&self, style: Arc<ComputedValues>) {
+        debug_assert!(self.is_text_node());
+        self.mutate_data().unwrap().style = Some(style);
+    }
+
     fn restyle_damage(self) -> Self::ConcreteRestyleDamage {
         // Not called from style, only for layout.
         unimplemented!();

components/style/matching.rs

@@ -745,7 +745,7 @@ pub trait ElementMatchMethods : TElement {
                         RestyleResult::Continue
                     };
 
-                    node.set_style(Some(shared_style));
+                    node.set_style(shared_style);
 
                     return StyleSharingResult::StyleWasShared(i, damage, restyle_result)
                 }
@@ -890,9 +890,7 @@ pub trait MatchMethods : TNode {
         // In Servo, this is also true, since text nodes generate UnscannedText
         // fragments, which aren't repairable by incremental layout.
         if self.is_text_node() {
-            let cloned_parent_style = ComputedValues::style_for_child_text_node(parent_style.as_ref().unwrap());
+            self.style_text_node(ComputedValues::style_for_child_text_node(parent_style.as_ref().unwrap()));
-
-            self.set_style(Some(cloned_parent_style));
 
             return RestyleResult::Continue;
         }
@@ -931,7 +929,7 @@ pub trait MatchMethods : TNode {
                                                         context, applicable_declarations,
                                                         &mut applicable_declarations_cache);
 
-            self.set_style(Some(new_style));
+            self.set_style(new_style);
 
             self.set_can_be_fragmented(parent.map_or(false, |p| {
                 p.can_be_fragmented() ||

components/style/traversal.rs

@@ -6,7 +6,7 @@
 
 use animation;
 use context::{LocalStyleContext, SharedStyleContext, StyleContext};
-use dom::{OpaqueNode, TNode, TRestyleDamage, UnsafeNode};
+use dom::{OpaqueNode, TNode, UnsafeNode};
 use matching::{ApplicableDeclarations, ElementMatchMethods, MatchMethods, StyleSharingResult};
 use selectors::bloom::BloomFilter;
 use selectors::matching::StyleRelations;
@@ -191,8 +191,8 @@ pub trait DomTraversalContext<N: TNode> {
         // the child, since we have exclusive access to both of them.
         if parent.is_dirty() {
             unsafe {
-                kid.set_dirty(true);
+                kid.set_dirty();
-                parent.set_dirty_descendants(true);
+                parent.set_dirty_descendants();
             }
         }
     }
@@ -298,12 +298,6 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
     let nonincremental_layout = opts::get().nonincremental_layout;
     let mut restyle_result = RestyleResult::Continue;
     if nonincremental_layout || node.is_dirty() {
-        // Remove existing CSS styles from nodes whose content has changed (e.g. text changed),
-        // to force non-incremental reflow.
-        if node.has_changed() {
-            node.set_style(None);
-        }
-
         // Check to see whether we can share a style with someone.
         let style_sharing_candidate_cache =
             &mut context.local_context().style_sharing_candidate_cache.borrow_mut();
@@ -348,9 +342,6 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
                     },
                     None => {
                         relations = StyleRelations::empty();
-                        if node.has_changed() {
-                            node.set_restyle_damage(N::ConcreteRestyleDamage::rebuild_and_reflow())
-                        }
                         None
                     },
                 };
@@ -385,7 +376,7 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
             context.shared_context()
         );
         if had_animations_to_expire {
-            node.set_style(Some(existing_style));
+            node.set_style(existing_style);
         }
     }