style: Cleanup StyleBuilder.

This is in preparation of a cascade optimization for custom properties.

This fixes various fishiness around our StyleBuilder stuff. In particular,
StyleBuilder::for_derived_style (renamed to for_animation) is only used to
compute specified values, and thus doesn't need to know about rules, visited
style, or other things like that.

The flag propagation that was done in StyleAdjuster is now done in StyleBuilder,
since we know beforehand which ones are always inherited, and it simplified the
callers and the StyleAdjuster code. It also fixed some fishiness wrt which flags
were propagated to anon boxes and text.

The text-decoration-lines bit is interesting, because the way it was implemented
in #17722 meant that display: contents elements did get HAS_DECORATION_LINES
flags only if its parent also had it, so in practice the Contents check
preserves behavior, but it's only an optimization looking at Gecko's call-sites,
so we can remove it too.

MozReview-Commit-ID: 6BHCyEO2U8c

components/style/matching.rs

@@ -336,7 +336,7 @@ trait PrivateMatchMethods: TElement {
 
         // We need to cascade the children in order to ensure the correct
         // propagation of inherited computed value flags.
-        if old_values.flags.inherited() != new_values.flags.inherited() {
+        if old_values.flags.maybe_inherited() != new_values.flags.maybe_inherited() {
             debug!(" > flags changed: {:?} != {:?}", old_values.flags, new_values.flags);
             return ChildCascadeRequirement::MustCascadeChildren;
         }

components/style/properties/computed_value_flags.rs

@@ -71,11 +71,35 @@ bitflags! {
 }
 
 impl ComputedValueFlags {
-    /// Returns the flags that are inherited.
+    /// Flags that are unconditionally propagated to descendants.
+    #[inline]
+    fn inherited_flags() -> Self {
+        ComputedValueFlags::IS_STYLE_IF_VISITED |
+        ComputedValueFlags::IS_RELEVANT_LINK_VISITED |
+        ComputedValueFlags::CAN_BE_FRAGMENTED |
+        ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE |
+        ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE |
+        ComputedValueFlags::HAS_TEXT_DECORATION_LINES
+    }
+
+    /// Flags that may be propagated to descendants.
+    #[inline]
+    fn maybe_inherited_flags() -> Self {
+        Self::inherited_flags() | ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK
+    }
+
+    /// Returns the flags that are always propagated to descendants.
+    ///
+    /// See StyleAdjuster::set_bits and StyleBuilder.
     #[inline]
     pub fn inherited(self) -> Self {
-        self & !(ComputedValueFlags::INHERITS_DISPLAY |
+        self & Self::inherited_flags()
-                 ComputedValueFlags::INHERITS_CONTENT |
+    }
-                 ComputedValueFlags::INHERITS_RESET_STYLE)
+
+    /// Flags that are conditionally propagated to descendants, just to handle
+    /// properly style invalidation.
+    #[inline]
+    pub fn maybe_inherited(self) -> Self {
+        self & Self::maybe_inherited_flags()
     }
 }

components/style/properties/gecko.mako.rs

@@ -252,17 +252,6 @@ impl ops::DerefMut for ComputedValues {
 }
 
 impl ComputedValuesInner {
-    /// Clone the visited style.  Used for inheriting parent styles in
-    /// StyleBuilder::for_derived_style.
-    pub fn clone_visited_style(&self) -> Option<Arc<ComputedValues>> {
-        self.visited_style.as_ref().map(|x| x.clone_arc())
-    }
-
-    #[inline]
-    pub fn is_display_contents(&self) -> bool {
-        self.get_box().clone_display() == longhands::display::computed_value::T::Contents
-    }
-
     /// Returns true if the value of the `content` property would make a
     /// pseudo-element not rendered.
     #[inline]

components/style/properties/properties.mako.rs

@@ -2216,6 +2216,11 @@ pub struct ComputedValues {
 }
 
 impl ComputedValues {
+    /// Returns whether this style's display value is equal to contents.
+    pub fn is_display_contents(&self) -> bool {
+        self.get_box().clone_display().is_contents()
+    }
+
     /// Whether we're a visited style.
     pub fn is_style_if_visited(&self) -> bool {
         self.flags.contains(ComputedValueFlags::IS_STYLE_IF_VISITED)
@@ -2335,17 +2340,6 @@ impl ComputedValuesInner {
     /// Servo for obvious reasons.
     pub fn has_moz_binding(&self) -> bool { false }
 
-    /// Clone the visited style.  Used for inheriting parent styles in
-    /// StyleBuilder::for_derived_style.
-    pub fn clone_visited_style(&self) -> Option<Arc<ComputedValues>> {
-        self.visited_style.clone()
-    }
-
-    /// Returns whether this style's display value is equal to contents.
-    ///
-    /// Since this isn't supported in Servo, this is always false for Servo.
-    pub fn is_display_contents(&self) -> bool { false }
-
     #[inline]
     /// Returns whether the "content" property for the given style is completely
     /// ineffective, and would yield an empty `::before` or `::after`
@@ -2728,8 +2722,6 @@ impl<'a> StyleBuilder<'a> {
         cascade_flags: CascadeFlags,
         rules: Option<StrongRuleNode>,
         custom_properties: Option<Arc<::custom_properties::CustomPropertiesMap>>,
-        writing_mode: WritingMode,
-        mut flags: ComputedValueFlags,
         visited_style: Option<Arc<ComputedValues>>,
     ) -> Self {
         debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
@@ -2751,6 +2743,7 @@ impl<'a> StyleBuilder<'a> {
             reset_style
         };
 
+        let mut flags = inherited_style.flags.inherited();
         if cascade_flags.contains(CascadeFlags::VISITED_DEPENDENT_ONLY) {
             flags.insert(ComputedValueFlags::IS_STYLE_IF_VISITED);
         }
@@ -2765,7 +2758,7 @@ impl<'a> StyleBuilder<'a> {
             rules,
             modified_reset: false,
             custom_properties,
-            writing_mode,
+            writing_mode: inherited_style.writing_mode,
             flags,
             visited_style,
             % for style_struct in data.active_style_structs():
@@ -2789,11 +2782,10 @@ impl<'a> StyleBuilder<'a> {
     ///
     /// Do _not_ actually call this to construct a style, this should mostly be
     /// used for animations.
-    pub fn for_derived_style(
+    pub fn for_animation(
         device: &'a Device,
         style_to_derive_from: &'a ComputedValues,
         parent_style: Option<<&'a ComputedValues>,
-        pseudo: Option<<&'a PseudoElement>,
     ) -> Self {
         let reset_style = device.default_computed_values();
         let inherited_style = parent_style.unwrap_or(reset_style);
@@ -2807,7 +2799,7 @@ impl<'a> StyleBuilder<'a> {
             // None of our callers pass in ::first-line parent styles.
             inherited_style_ignoring_first_line: inherited_style,
             reset_style,
-            pseudo,
+            pseudo: None,
             modified_reset: false,
             rules: None,
             custom_properties: style_to_derive_from.custom_properties().cloned(),
@@ -2916,7 +2908,7 @@ impl<'a> StyleBuilder<'a> {
     /// computed values that need to be provided as well.
     pub fn for_inheritance(
         device: &'a Device,
-        parent: &'a ComputedValues,
+        parent: Option<<&'a ComputedValues>,
         pseudo: Option<<&'a PseudoElement>,
     ) -> Self {
         // Rebuild the visited style from the parent, ensuring that it will also
@@ -2924,26 +2916,23 @@ impl<'a> StyleBuilder<'a> {
         // produced by this builder.  This assumes that the caller doesn't need
         // to adjust or process visited style, so we can just build visited
         // style here for simplicity.
-        let visited_style = parent.visited_style().map(|style| {
+        let visited_style = parent.and_then(|parent| {
-            Self::for_inheritance(
+            parent.visited_style().map(|style| {
-                device,
+                Self::for_inheritance(
-                style,
+                    device,
-                pseudo,
+                    Some(style),
-            ).build()
+                    pseudo,
+                ).build()
+            })
         });
-        // FIXME(emilio): This Some(parent) here is inconsistent with what we
-        // usually do if `parent` is the default computed values, but that's
-        // fine, and we want to eventually get rid of it.
         Self::new(
             device,
-            Some(parent),
+            parent,
-            Some(parent),
+            parent,
             pseudo,
             CascadeFlags::empty(),
             /* rules = */ None,
-            parent.custom_properties().cloned(),
+            parent.and_then(|p| p.custom_properties().cloned()),
-            parent.writing_mode,
-            parent.flags.inherited(),
             visited_style,
         )
     }
@@ -3076,9 +3065,10 @@ impl<'a> StyleBuilder<'a> {
         &self.inherited_style.writing_mode
     }
 
-    /// Inherited style flags.
+    /// The computed value flags of our parent.
-    pub fn inherited_flags(&self) -> &ComputedValueFlags {
+    #[inline]
-        &self.inherited_style.flags
+    pub fn get_parent_flags(&self) -> ComputedValueFlags {
+        self.inherited_style.flags
     }
 
     /// And access to inherited style structs.
@@ -3330,8 +3320,6 @@ where
             flags,
             Some(rules.clone()),
             custom_properties,
-            WritingMode::empty(),
-            ComputedValueFlags::empty(),
             visited_style,
         ),
         cached_system_font: None,

components/style/style_adjuster.rs

@@ -8,6 +8,7 @@
 use app_units::Au;
 use dom::TElement;
 use properties::{self, CascadeFlags, ComputedValues, StyleBuilder};
+use properties::computed_value_flags::ComputedValueFlags;
 use properties::longhands::display::computed_value::T as Display;
 use properties::longhands::float::computed_value::T as Float;
 use properties::longhands::overflow_x::computed_value::T as Overflow;
@@ -111,23 +112,23 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
 
     /// Compute a few common flags for both text and element's style.
     pub fn set_bits(&mut self) {
-        use properties::computed_value_flags::ComputedValueFlags;
+        let display = self.style.get_box().clone_display();
 
-        if self.style.inherited_flags().contains(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE) ||
+        if !display.is_contents() && !self.style.get_text().clone_text_decoration_line().is_empty() {
-            self.style.get_box().clone_display() == Display::None {
+            self.style.flags.insert(ComputedValueFlags::HAS_TEXT_DECORATION_LINES);
+        }
+
+        if display == Display::None {
             self.style.flags.insert(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE);
         }
 
-        if self.style.inherited_flags().contains(ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE) ||
+        if self.style.is_pseudo_element() {
-            self.style.is_pseudo_element() {
             self.style.flags.insert(ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE);
         }
 
         #[cfg(feature = "servo")]
         {
-            if self.style.inherited_flags().contains(ComputedValueFlags::CAN_BE_FRAGMENTED) ||
+            if self.style.get_parent_column().is_multicol() {
-                self.style.get_parent_column().is_multicol()
-            {
                 self.style.flags.insert(ComputedValueFlags::CAN_BE_FRAGMENTED);
             }
         }
@@ -160,7 +161,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     fn adjust_for_text_combine_upright(&mut self) {
         use computed_values::text_combine_upright::T as TextCombineUpright;
         use computed_values::writing_mode::T as WritingMode;
-        use properties::computed_value_flags::ComputedValueFlags;
 
         let writing_mode =
             self.style.get_inheritedbox().clone_writing_mode();
@@ -174,15 +174,18 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         }
     }
 
-    /// Applies the line break suppression flag to text if it is in any ruby
+    /// Unconditionally propagates the line break suppression flag to text, and
-    /// box. This is necessary because its parent may not itself have the flag
+    /// additionally it applies it if it is in any ruby box.
-    /// set (e.g. ruby or ruby containers), thus we may not inherit the flag
+    ///
-    /// from them.
+    /// This is necessary because its parent may not itself have the flag set
+    /// (e.g. ruby or ruby containers), thus we may not inherit the flag from
+    /// them.
     #[cfg(feature = "gecko")]
     fn adjust_for_text_in_ruby(&mut self) {
-        use properties::computed_value_flags::ComputedValueFlags;
         let parent_display = self.style.get_parent_box().clone_display();
-        if parent_display.is_ruby_type() {
+        if parent_display.is_ruby_type() ||
+           self.style.get_parent_flags().contains(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK)
+        {
             self.style.flags.insert(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK);
         }
     }
@@ -426,18 +429,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         self.style.mutate_inheritedtext().set_text_align(TextAlign::Start)
     }
 
-    /// Set the HAS_TEXT_DECORATION_LINES flag based on parent style.
-    fn adjust_for_text_decoration_lines(
-        &mut self,
-        layout_parent_style: &ComputedValues,
-    ) {
-        use properties::computed_value_flags::ComputedValueFlags;
-        if layout_parent_style.flags.contains(ComputedValueFlags::HAS_TEXT_DECORATION_LINES) ||
-           !self.style.get_text().clone_text_decoration_line().is_empty() {
-            self.style.flags.insert(ComputedValueFlags::HAS_TEXT_DECORATION_LINES);
-        }
-    }
-
     /// Computes the used text decoration for Servo.
     ///
     /// FIXME(emilio): This is a layout tree concept, should move away from
@@ -458,7 +449,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         &self,
         layout_parent_style: &ComputedValues,
     ) -> bool {
-        use properties::computed_value_flags::ComputedValueFlags;
         // Line break suppression should only be propagated to in-flow children.
         if self.style.floated() || self.style.out_of_flow_positioned() {
             return false;
@@ -503,7 +493,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     where
         E: TElement,
     {
-        use properties::computed_value_flags::ComputedValueFlags;
         use properties::longhands::unicode_bidi::computed_value::T as UnicodeBidi;
 
         let self_display = self.style.get_box().clone_display();
@@ -555,8 +544,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     where
         E: TElement,
     {
-        use properties::computed_value_flags::ComputedValueFlags;
-
         if !self.style.has_visited_style() {
             return;
         }
@@ -565,13 +552,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
             self.style.pseudo.is_none() &&
             element.map_or(false, |e| e.is_link());
 
-        let relevant_link_visited = if is_link_element {
+        if is_link_element && element.unwrap().is_visited_link() {
-            element.unwrap().is_visited_link()
-        } else {
-            self.style.inherited_flags().contains(ComputedValueFlags::IS_RELEVANT_LINK_VISITED)
-        };
-
-        if relevant_link_visited {
             self.style.flags.insert(ComputedValueFlags::IS_RELEVANT_LINK_VISITED);
         }
     }
@@ -674,7 +655,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         self.adjust_for_border_width();
         self.adjust_for_outline();
         self.adjust_for_writing_mode(layout_parent_style);
-        self.adjust_for_text_decoration_lines(layout_parent_style);
         #[cfg(feature = "gecko")]
         {
             self.adjust_for_ruby(layout_parent_style, element);

components/style/stylesheets/viewport_rule.rs

@@ -708,19 +708,16 @@ impl MaybeNew for ViewportConstraints {
         }
 
         // DEVICE-ADAPT § 6.2.3 Resolve non-auto lengths to pixel lengths
-        //
-        // Note: DEVICE-ADAPT § 5. states that relative length values are
-        // resolved against initial values
         let initial_viewport = device.au_viewport_size();
 
         let provider = get_metrics_provider_for_product();
 
-        let default_values = device.default_computed_values();
-
         let mut conditions = RuleCacheConditions::default();
         let context = Context {
             is_root_element: false,
-            builder: StyleBuilder::for_inheritance(device, default_values, None),
+            // Note: DEVICE-ADAPT § 5. states that relative length values are
+            // resolved against initial values
+            builder: StyleBuilder::for_inheritance(device, None, None),
             font_metrics_provider: &provider,
             cached_system_font: None,
             in_media_query: false,

components/style/values/computed/mod.rs

@@ -174,12 +174,11 @@ impl<'a> Context<'a> {
         F: FnOnce(&Context) -> R
     {
         let mut conditions = RuleCacheConditions::default();
-        let default_values = device.default_computed_values();
         let provider = get_metrics_provider_for_product();
 
         let context = Context {
             is_root_element: false,
-            builder: StyleBuilder::for_inheritance(device, default_values, None),
+            builder: StyleBuilder::for_inheritance(device, None, None),
             font_metrics_provider: &provider,
             cached_system_font: None,
             in_media_query: true,

components/style/values/specified/box.rs

@@ -217,6 +217,16 @@ impl Display {
         }
     }
 
+    /// Returns true if the value is `Contents`
+    #[inline]
+    pub fn is_contents(&self) -> bool {
+        match *self {
+            #[cfg(feature = "gecko")]
+            Display::Contents => true,
+            _ => false,
+        }
+    }
+
     /// Returns true if the value is `None`
     #[inline]
     pub fn is_none(&self) -> bool {

ports/geckolib/glue.rs

@@ -1997,10 +1997,11 @@ pub extern "C" fn Servo_FontFeatureValuesRule_GetValueText(rule: RawServoFontFea
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null: ServoStyleContextBorrowedOrNull,
+pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(
-                                                          pseudo_tag: *mut nsAtom,
+    parent_style_or_null: ServoStyleContextBorrowedOrNull,
-                                                          raw_data: RawServoStyleSetBorrowed)
+    pseudo_tag: *mut nsAtom,
-     -> ServoStyleContextStrong {
+    raw_data: RawServoStyleSetBorrowed,
+) -> ServoStyleContextStrong {
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
     let guards = StylesheetGuards::same(&guard);
@@ -2299,7 +2300,7 @@ fn get_pseudo_style(
     Some(style.unwrap_or_else(|| {
         StyleBuilder::for_inheritance(
             doc_data.stylist.device(),
-            styles.primary(),
+            Some(styles.primary()),
             Some(pseudo),
         ).build()
     }))
@@ -2318,11 +2319,10 @@ pub extern "C" fn Servo_ComputedValues_Inherit(
     let atom = Atom::from(pseudo_tag);
     let pseudo = PseudoElement::from_anon_box_atom(&atom)
         .expect("Not an anon-box? Gah!");
-    let device = data.stylist.device();
 
     let mut style = StyleBuilder::for_inheritance(
         data.stylist.device(),
-        parent_style_context.unwrap_or_else(|| device.default_computed_values()),
+        parent_style_context,
         Some(&pseudo)
     );
 
@@ -3647,22 +3647,20 @@ fn simulate_compute_values_failure(_: &PropertyValuePair) -> bool {
     false
 }
 
-fn create_context<'a>(
+fn create_context_for_animation<'a>(
     per_doc_data: &'a PerDocumentStyleDataImpl,
     font_metrics_provider: &'a FontMetricsProvider,
     style: &'a ComputedValues,
     parent_style: Option<&'a ComputedValues>,
-    pseudo: Option<&'a PseudoElement>,
     for_smil_animation: bool,
     rule_cache_conditions: &'a mut RuleCacheConditions,
 ) -> Context<'a> {
     Context {
         is_root_element: false,
-        builder: StyleBuilder::for_derived_style(
+        builder: StyleBuilder::for_animation(
             per_doc_data.stylist.device(),
             style,
             parent_style,
-            pseudo,
         ),
         font_metrics_provider: font_metrics_provider,
         cached_system_font: None,
@@ -3740,14 +3738,12 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(
     let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data());
     let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x);
 
-    let pseudo = style.pseudo();
     let mut conditions = Default::default();
-    let mut context = create_context(
+    let mut context = create_context_for_animation(
         &data,
         &metrics,
         &style,
         parent_style,
-        pseudo.as_ref(),
         /* for_smil_animation = */ false,
         &mut conditions,
     );
@@ -3849,14 +3845,12 @@ pub extern "C" fn Servo_GetAnimationValues(
     let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data());
     let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x);
 
-    let pseudo = style.pseudo();
     let mut conditions = Default::default();
-    let mut context = create_context(
+    let mut context = create_context_for_animation(
         &data,
         &metrics,
         &style,
         parent_style,
-        pseudo.as_ref(),
         /* for_smil_animation = */ true,
         &mut conditions,
     );
@@ -3893,14 +3887,12 @@ pub extern "C" fn Servo_AnimationValue_Compute(
     let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data());
     let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x);
 
-    let pseudo = style.pseudo();
     let mut conditions = Default::default();
-    let mut context = create_context(
+    let mut context = create_context_for_animation(
         &data,
         &metrics,
         style,
         parent_style,
-        pseudo.as_ref(),
         /* for_smil_animation = */ false,
         &mut conditions,
     );
@@ -4598,18 +4590,11 @@ pub extern "C" fn Servo_ComputeColor(
             let computed_color = match raw_data {
                 Some(raw_data) => {
                     let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
-                    let metrics = get_metrics_provider_for_product();
+                    let device = data.stylist.device();
-                    let mut conditions = Default::default();
+                    let quirks_mode = data.stylist.quirks_mode();
-                    let context = create_context(
+                    Context::for_media_query_evaluation(device, quirks_mode, |context| {
-                        &data,
+                        specified_color.to_computed_color(Some(&context))
-                        &metrics,
+                    })
-                        data.stylist.device().default_computed_values(),
-                        /* parent_style = */ None,
-                        /* pseudo = */ None,
-                        /* for_smil_animation = */ false,
-                        &mut conditions,
-                    );
-                    specified_color.to_computed_color(Some(&context))
                 }
                 None => {
                     specified_color.to_computed_color(None)