Auto merge of #18268 - emilio:dirty-viewport-followup, r=SimonSapin

style: Recascade the document instead of using the dirty_on_viewport_size_change bit

This allows us to simplify a lot of code.

On top of #18267.

<!-- 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/18268)
<!-- Reviewable:end -->

components/layout_thread/dom_wrapper.rs

@@ -38,7 +38,7 @@ use layout::data::StyleAndLayoutData;
 use layout::wrapper::GetRawData;
 use msg::constellation_msg::{BrowsingContextId, PipelineId};
 use range::Range;
-use script::layout_exports::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
+use script::layout_exports::{CAN_BE_FRAGMENTED, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
 use script::layout_exports::{CharacterDataTypeId, ElementTypeId, HTMLElementTypeId, NodeTypeId};
 use script::layout_exports::{Document, Element, Node, Text};
 use script::layout_exports::{HANDLED_SNAPSHOT, HAS_SNAPSHOT};
@@ -205,14 +205,6 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         as_element(self.node)
     }
 
-    fn needs_dirty_on_viewport_size_changed(&self) -> bool {
-        unsafe { self.node.get_flag(DIRTY_ON_VIEWPORT_SIZE_CHANGE) }
-    }
-
-    unsafe fn set_dirty_on_viewport_size_changed(&self) {
-        self.node.set_flag(DIRTY_ON_VIEWPORT_SIZE_CHANGE, true);
-    }
-
     fn can_be_fragmented(&self) -> bool {
         unsafe { self.node.get_flag(CAN_BE_FRAGMENTED) }
     }

components/layout_thread/lib.rs

@@ -1167,27 +1167,8 @@ impl LayoutThread {
                        .unwrap();
             }
             if had_used_viewport_units {
-                let mut iter = element.as_node().traverse_preorder();
+                if let Some(mut data) = element.mutate_data() {
-
+                    data.restyle.hint.insert(RestyleHint::recascade_subtree());
-                let mut next = iter.next();
-                while let Some(node) = next {
-                    if node.needs_dirty_on_viewport_size_changed() {
-                        let el = node.as_element().unwrap();
-                        if let Some(mut d) = element.mutate_data() {
-                            if d.has_styles() {
-                                // FIXME(emilio): This only needs to recascade,
-                                // afaict.
-                                d.restyle.hint.insert(RestyleHint::restyle_subtree());
-                            }
-                        }
-                        if let Some(p) = el.parent_element() {
-                            unsafe { p.note_dirty_descendant() };
-                        }
-
-                        next = iter.next_skipping_children();
-                    } else {
-                        next = iter.next();
-                    }
                 }
             }
         }

components/script/dom/node.rs

@@ -169,8 +169,7 @@ bitflags! {
         /// Whether any ancestor is a fragmentation container
         const CAN_BE_FRAGMENTED = 1 << 4,
 
-        #[doc = "Specifies whether this node needs to be dirted when viewport size changed."]
+        // There's a free bit here.
-        const DIRTY_ON_VIEWPORT_SIZE_CHANGE = 1 << 5,
 
         #[doc = "Specifies whether the parser has set an associated form owner for \
                  this element. Only applicable for form-associatable elements."]

components/script/lib.rs

@@ -142,7 +142,7 @@ pub mod layout_exports {
     pub use dom::characterdata::LayoutCharacterDataHelpers;
     pub use dom::document::{Document, LayoutDocumentHelpers, PendingRestyle};
     pub use dom::element::{Element, LayoutElementHelpers, RawLayoutElementHelpers};
-    pub use dom::node::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
+    pub use dom::node::{CAN_BE_FRAGMENTED, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
     pub use dom::node::{HANDLED_SNAPSHOT, HAS_SNAPSHOT};
     pub use dom::node::{LayoutNodeHelpers, Node};
     pub use dom::text::Text;

components/style/animation.rs

@@ -502,7 +502,6 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
                                                Some(previous_style),
                                                Some(previous_style),
                                                Some(previous_style),
-                                               /* cascade_info = */ None,
                                                /* visited_style = */ None,
                                                font_metrics_provider,
                                                CascadeFlags::empty(),

components/style/cascade_info.rs

@@ -1,95 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * 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/. */
-
-//! A structure to collect information about the cascade.
-
-#![deny(missing_docs)]
-
-use dom::TNode;
-use properties::{DeclaredValue, PropertyDeclaration};
-use style_traits::HasViewportPercentage;
-
-/// A structure to collect information about the cascade.
-///
-/// This is useful to gather information about what an element is affected by,
-/// and can be used in the future to track optimisations like when a
-/// non-inherited property is explicitly inherited, in order to cut-off the
-/// traversal.
-pub struct CascadeInfo {
-    /// Whether we've seen viewport units so far.
-    pub saw_viewport_units: bool,
-    /// Whether the cascade has been marked as finished. This is a debug-only
-    /// flag to ensure `finish` is called, given it's optional to not pass a
-    /// `CascadeInfo`.
-    #[cfg(debug_assertions)]
-    finished: bool,
-}
-
-impl CascadeInfo {
-    /// Construct a new `CascadeInfo`.
-    #[cfg(debug_assertions)]
-    pub fn new() -> Self {
-        CascadeInfo {
-            saw_viewport_units: false,
-            finished: false,
-        }
-    }
-
-    /// Construct a new `CascadeInfo`.
-    #[cfg(not(debug_assertions))]
-    pub fn new() -> Self {
-        CascadeInfo {
-            saw_viewport_units: false,
-        }
-    }
-
-    /// Called when a property is cascaded.
-    ///
-    /// NOTE: We can add a vast amount of information here.
-    #[inline]
-    pub fn on_cascade_property<T>(&mut self,
-                                  _property_declaration: &PropertyDeclaration,
-                                  value: &DeclaredValue<T>)
-        where T: HasViewportPercentage,
-    {
-        // TODO: we can be smarter and keep a property bitfield to keep track of
-        // the last applying rule.
-        if value.has_viewport_percentage() {
-            self.saw_viewport_units = true;
-        }
-    }
-
-    #[cfg(debug_assertions)]
-    fn mark_as_finished_if_appropriate(&mut self) {
-        self.finished = true;
-    }
-
-    #[cfg(not(debug_assertions))]
-    fn mark_as_finished_if_appropriate(&mut self) {}
-
-    /// Called when the cascade is finished, in order to use the information
-    /// we've collected.
-    ///
-    /// Currently used for styling to mark a node as needing restyling when the
-    /// viewport size changes.
-    #[allow(unsafe_code)]
-    pub fn finish<N: TNode>(mut self, node: &N) {
-        self.mark_as_finished_if_appropriate();
-
-        if self.saw_viewport_units {
-            unsafe {
-                node.set_dirty_on_viewport_size_changed();
-            }
-        }
-    }
-}
-
-#[cfg(debug_assertions)]
-impl Drop for CascadeInfo {
-    fn drop(&mut self) {
-        debug_assert!(self.finished,
-                      "Didn't use the result of CascadeInfo, if you don't need \
-                      it, consider passing None");
-    }
-}

components/style/dom.rs

@@ -138,12 +138,6 @@ pub trait TNode : Sized + Copy + Clone + Debug + NodeInfo {
     /// Get this node as an element, if it's one.
     fn as_element(&self) -> Option<Self::ConcreteElement>;
 
-    /// Whether this node needs to be laid out on viewport size change.
-    fn needs_dirty_on_viewport_size_changed(&self) -> bool;
-
-    /// Mark this node as needing layout on viewport size change.
-    unsafe fn set_dirty_on_viewport_size_changed(&self);
-
     /// Whether this node can be fragmented. This is used for multicol, and only
     /// for Servo.
     fn can_be_fragmented(&self) -> bool;

components/style/gecko/wrapper.rs

@@ -318,17 +318,6 @@ impl<'ln> TNode for GeckoNode<'ln> {
     fn is_in_doc(&self) -> bool {
         unsafe { bindings::Gecko_IsInDocument(self.0) }
     }
-
-    fn needs_dirty_on_viewport_size_changed(&self) -> bool {
-        // Gecko's node doesn't have the DIRTY_ON_VIEWPORT_SIZE_CHANGE flag,
-        // so we force them to be dirtied on viewport size change, regardless if
-        // they use viewport percentage size or not.
-        // TODO(shinglyu): implement this in Gecko: https://github.com/servo/servo/pull/11890
-        true
-    }
-
-    // TODO(shinglyu): implement this in Gecko: https://github.com/servo/servo/pull/11890
-    unsafe fn set_dirty_on_viewport_size_changed(&self) {}
 }
 
 /// A wrapper on top of two kind of iterators, depending on the parent being

components/style/lib.rs

@@ -99,7 +99,6 @@ pub mod applicable_declarations;
 pub mod bezier;
 pub mod bloom;
 pub mod cache;
-pub mod cascade_info;
 pub mod context;
 pub mod counter_style;
 pub mod custom_properties;

components/style/properties/helpers.mako.rs

@@ -275,8 +275,6 @@
         #[allow(unused_imports)]
         use values::{Auto, Either, None_, Normal};
         #[allow(unused_imports)]
-        use cascade_info::CascadeInfo;
-        #[allow(unused_imports)]
         use error_reporting::ParseErrorReporter;
         #[allow(unused_imports)]
         use properties::longhands;
@@ -303,7 +301,6 @@
         pub fn cascade_property(
             declaration: &PropertyDeclaration,
             context: &mut computed::Context,
-            cascade_info: &mut Option<<&mut CascadeInfo>,
         ) {
             let value = match *declaration {
                 PropertyDeclaration::${property.camel_case}(ref value) => {
@@ -320,9 +317,6 @@
             };
 
             % if not property.derived_from:
-                if let Some(ref mut cascade_info) = *cascade_info {
-                    cascade_info.on_cascade_property(&declaration, &value);
-                }
                 match value {
                     DeclaredValue::Value(ref specified_value) => {
                         % if property.ident in SYSTEM_FONT_LONGHANDS and product == "gecko":

components/style/properties/properties.mako.rs

@@ -43,7 +43,6 @@ use stylesheets::{CssRuleType, MallocSizeOf, MallocSizeOfFn, Origin, UrlExtraDat
 use values::generics::text::LineHeight;
 use values::computed;
 use values::computed::NonNegativeAu;
-use cascade_info::CascadeInfo;
 use rule_tree::{CascadeLevel, StrongRuleNode};
 use self::computed_value_flags::ComputedValueFlags;
 use style_adjuster::StyleAdjuster;
@@ -2953,9 +2952,10 @@ mod lazy_static_module {
 
 /// A per-longhand function that performs the CSS cascade for that longhand.
 pub type CascadePropertyFn =
-    extern "Rust" fn(declaration: &PropertyDeclaration,
+    extern "Rust" fn(
-                     context: &mut computed::Context,
+        declaration: &PropertyDeclaration,
-                     cascade_info: &mut Option<<&mut CascadeInfo>);
+        context: &mut computed::Context,
+    );
 
 /// A per-longhand array of functions to perform the CSS cascade on each of
 /// them, effectively doing virtual dispatch.
@@ -3030,7 +3030,6 @@ pub fn cascade(
     parent_style_ignoring_first_line: Option<<&ComputedValues>,
     layout_parent_style: Option<<&ComputedValues>,
     visited_style: Option<Arc<ComputedValues>>,
-    cascade_info: Option<<&mut CascadeInfo>,
     font_metrics_provider: &FontMetricsProvider,
     flags: CascadeFlags,
     quirks_mode: QuirksMode
@@ -3089,7 +3088,6 @@ pub fn cascade(
         parent_style_ignoring_first_line,
         layout_parent_style,
         visited_style,
-        cascade_info,
         font_metrics_provider,
         flags,
         quirks_mode,
@@ -3108,7 +3106,6 @@ pub fn apply_declarations<'a, F, I>(
     parent_style_ignoring_first_line: Option<<&ComputedValues>,
     layout_parent_style: Option<<&ComputedValues>,
     visited_style: Option<Arc<ComputedValues>>,
-    mut cascade_info: Option<<&mut CascadeInfo>,
     font_metrics_provider: &FontMetricsProvider,
     flags: CascadeFlags,
     quirks_mode: QuirksMode,
@@ -3280,9 +3277,7 @@ where
             % endif
 
             let discriminant = longhand_id as usize;
-            (CASCADE_PROPERTY[discriminant])(&*declaration,
+            (CASCADE_PROPERTY[discriminant])(&*declaration, &mut context);
-                                             &mut context,
-                                             &mut cascade_info);
         }
         % if category_to_cascade_now == "early":
             let writing_mode = get_writing_mode(context.builder.get_inheritedbox());
@@ -3363,9 +3358,7 @@ where
                 if let Some(ref declaration) = font_family {
 
                     let discriminant = LonghandId::FontFamily as usize;
-                    (CASCADE_PROPERTY[discriminant])(declaration,
+                    (CASCADE_PROPERTY[discriminant])(declaration, &mut context);
-                                                     &mut context,
-                                                     &mut cascade_info);
                     % if product == "gecko":
                         let device = context.builder.device;
                         if let PropertyDeclaration::FontFamily(ref val) = **declaration {
@@ -3383,9 +3376,7 @@ where
 
             if let Some(ref declaration) = font_size {
                 let discriminant = LonghandId::FontSize as usize;
-                (CASCADE_PROPERTY[discriminant])(declaration,
+                (CASCADE_PROPERTY[discriminant])(declaration, &mut context);
-                                                 &mut context,
-                                                 &mut cascade_info);
             % if product == "gecko":
             // Font size must be explicitly inherited to handle lang changes and
             // scriptlevel changes.
@@ -3397,9 +3388,7 @@ where
                 let size = PropertyDeclaration::CSSWideKeyword(
                     LonghandId::FontSize, CSSWideKeyword::Inherit);
 
-                (CASCADE_PROPERTY[discriminant])(&size,
+                (CASCADE_PROPERTY[discriminant])(&size, &mut context);
-                                                 &mut context,
-                                                 &mut cascade_info);
             % endif
             }
         % endif

components/style/style_resolver.rs

@@ -5,7 +5,6 @@
 //! Style resolution for a given element or pseudo-element.
 
 use applicable_declarations::ApplicableDeclarationList;
-use cascade_info::CascadeInfo;
 use context::{CascadeInputs, ElementCascadeInputs, StyleContext};
 use data::{ElementStyles, EagerPseudoStyles};
 use dom::TElement;
@@ -486,7 +485,6 @@ where
         cascade_visited: CascadeVisitedMode,
         pseudo: Option<&PseudoElement>,
     ) -> Arc<ComputedValues> {
-        let mut cascade_info = CascadeInfo::new();
         let mut cascade_flags = CascadeFlags::empty();
 
         if self.element.skip_root_and_item_based_display_fixup() ||
@@ -530,14 +528,11 @@ where
                 parent_style,
                 layout_parent_style,
                 style_if_visited,
-                Some(&mut cascade_info),
                 &self.context.thread_local.font_metrics_provider,
                 cascade_flags,
                 self.context.shared.quirks_mode(),
             );
 
-        cascade_info.finish(&self.element.as_node());
-
         values
     }
 }

components/style/stylist.rs

@@ -721,7 +721,6 @@ impl Stylist {
             parent,
             parent,
             None,
-            None,
             font_metrics,
             cascade_flags,
             self.quirks_mode,
@@ -901,7 +900,6 @@ impl Stylist {
                 Some(inherited_style_ignoring_first_line),
                 Some(layout_parent_style_for_visited),
                 None,
-                None,
                 font_metrics,
                 cascade_flags,
                 self.quirks_mode,
@@ -927,7 +925,6 @@ impl Stylist {
             Some(parent_style_ignoring_first_line),
             Some(layout_parent_style),
             visited_values,
-            None,
             font_metrics,
             cascade_flags,
             self.quirks_mode,
@@ -1543,7 +1540,6 @@ impl Stylist {
             Some(parent_style),
             Some(parent_style),
             None,
-            None,
             &metrics,
             CascadeFlags::empty(),
             self.quirks_mode,