servo: Merge #19666 - style: Simplify the skip item based display fixup adjustment (from emilio:root-and-item); r=upsuper
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 31 Dec 2017 07:01:10 -0600
changeset 449301 2984920ed0a895bc502dde04a38196f924c06d8b
parent 449300 894c4edfb0fb95fb4d9889175c00df46eb2989b1
child 449302 239bb57dde414fa47cb275b94ac2a5f6ee6e32ef
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersupsuper
bugs1405635
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #19666 - style: Simplify the skip item based display fixup adjustment (from emilio:root-and-item); r=upsuper In practice the only NAC that possibly inherits from a grid or flex container are pseudos. In Gecko, if the root element is an item container, custom anon content would also sometimes incorrectly inherit from that (see bug 1405635), but that's fixed in Stylo. We remove the IS_ROOT_ELEMENT blockification from the "skip display fixup" check, since the root element is never NAC or anything like that, so there's no need for the check. This also fixes some reparenting fishiness related to pseudo-elements. We were only skipping the fixup when reparenting anon boxes, not when reparenting normal element styles, nor when reparenting other pseudo styles which are not anon boxes. Source-Repo: https://github.com/servo/servo Source-Revision: 1970e82b0d310128eabe8466d39d42cc20e7ae4b
servo/components/layout_thread/dom_wrapper.rs
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/style_adjuster.rs
servo/components/style/style_resolver.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/layout_thread/dom_wrapper.rs
+++ b/servo/components/layout_thread/dom_wrapper.rs
@@ -455,20 +455,16 @@ impl<'le> TElement for ServoLayoutElemen
     fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| {
                 &(*(d.ptr.get() as *mut StyleData)).element_data
             })
         }
     }
 
-    fn skip_root_and_item_based_display_fixup(&self) -> bool {
-        false
-    }
-
     unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags) {
         self.element.insert_selector_flags(flags);
     }
 
     fn has_selector_flags(&self, flags: ElementSelectorFlags) -> bool {
         self.element.has_selector_flags(flags)
     }
 
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -665,21 +665,16 @@ pub trait TElement
         self.get_data().map(|x| x.borrow())
     }
 
     /// Mutably borrows the ElementData.
     fn mutate_data(&self) -> Option<AtomicRefMut<ElementData>> {
         self.get_data().map(|x| x.borrow_mut())
     }
 
-    /// Whether we should skip any root- or item-based display property
-    /// blockification on this element.  (This function exists so that Gecko
-    /// native anonymous content can opt out of this style fixup.)
-    fn skip_root_and_item_based_display_fixup(&self) -> bool;
-
     /// Sets selector flags, which indicate what kinds of selectors may have
     /// matched on this element and therefore what kind of work may need to
     /// be performed when DOM state changes.
     ///
     /// This is unsafe, like all the flag-setting methods, because it's only safe
     /// to call with exclusive access to the element. When setting flags on the
     /// parent during parallel traversal, we use SequentialTask to queue up the
     /// set to run after the threads join.
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -1251,29 +1251,16 @@ impl<'le> TElement for GeckoElement<'le>
 
             // Perform a mutable borrow of the data in debug builds. This
             // serves as an assertion that there are no outstanding borrows
             // when we destroy the data.
             debug_assert!({ let _ = data.borrow_mut(); true });
         }
     }
 
-    #[inline]
-    fn skip_root_and_item_based_display_fixup(&self) -> bool {
-        if !self.is_native_anonymous() {
-            return false;
-        }
-
-        if let Some(p) = self.implemented_pseudo_element() {
-            return p.skip_item_based_display_fixup();
-        }
-
-        self.is_root_of_native_anonymous_subtree()
-    }
-
     unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags) {
         debug_assert!(!flags.is_empty());
         self.set_flags(selector_flags_to_node_flags(flags));
     }
 
     fn has_selector_flags(&self, flags: ElementSelectorFlags) -> bool {
         let node_flags = selector_flags_to_node_flags(flags);
         (self.flags() & node_flags) == node_flags
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -3055,42 +3055,38 @@ static CASCADE_PROPERTY: [CascadePropert
     % endfor
 ];
 
 bitflags! {
     /// A set of flags to tweak the behavior of the `cascade` function.
     pub struct CascadeFlags: u8 {
         /// Whether to inherit all styles from the parent. If this flag is not
         /// present, non-inherited styles are reset to their initial values.
-        const INHERIT_ALL = 1;
-
-        /// Whether to skip any display style fixup for root element, flex/grid
-        /// item, and ruby descendants.
-        const SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP = 1 << 1;
+        const INHERIT_ALL = 1 << 0;
 
         /// Whether to only cascade properties that are visited dependent.
-        const VISITED_DEPENDENT_ONLY = 1 << 2;
+        const VISITED_DEPENDENT_ONLY = 1 << 1;
 
         /// Whether the given element we're styling is the document element,
         /// that is, matches :root.
         ///
         /// Not set for native anonymous content since some NAC form their own
         /// root, but share the device.
         ///
         /// This affects some style adjustments, like blockification, and means
         /// that it may affect global state, like the Device's root font-size.
-        const IS_ROOT_ELEMENT = 1 << 3;
+        const IS_ROOT_ELEMENT = 1 << 2;
 
         /// Whether we're computing the style of a link, either visited or
         /// unvisited.
-        const IS_LINK = 1 << 4;
+        const IS_LINK = 1 << 3;
 
         /// Whether we're computing the style of a link element that happens to
         /// be visited.
-        const IS_VISITED_LINK = 1 << 5;
+        const IS_VISITED_LINK = 1 << 4;
     }
 }
 
 /// Performs the CSS cascade, computing new styles for an element from its parent style.
 ///
 /// The arguments are:
 ///
 ///   * `device`: Used to get the initial viewport and other external state.
--- a/servo/components/style/style_adjuster.rs
+++ b/servo/components/style/style_adjuster.rs
@@ -23,16 +23,22 @@ pub struct StyleAdjuster<'a, 'b: 'a> {
 
 impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     /// Trivially constructs a new StyleAdjuster.
     #[inline]
     pub fn new(style: &'a mut StyleBuilder<'b>) -> Self {
         StyleAdjuster { style }
     }
 
+    /// Whether to skip any display style fixup flex/grid item, and ruby
+    /// descendants.
+    fn skip_item_based_display_fixup(&self) -> bool {
+        self.style.pseudo.as_ref().map_or(false, |p| p.skip_item_based_display_fixup())
+    }
+
     /// <https://fullscreen.spec.whatwg.org/#new-stacking-layer>
     ///
     ///    Any position value other than 'absolute' and 'fixed' are
     ///    computed to 'absolute' if the element is in a top layer.
     ///
     fn adjust_for_top_layer(&mut self) {
         if !self.style.out_of_flow_positioned() && self.style.in_top_layer() {
             self.style.mutate_box().set_position(Position::Absolute);
@@ -61,20 +67,21 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         macro_rules! blockify_if {
             ($if_what:expr) => {
                 if !blockify {
                     blockify = $if_what;
                 }
             }
         }
 
-        if !flags.contains(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
-            blockify_if!(flags.contains(CascadeFlags::IS_ROOT_ELEMENT));
-            blockify_if!(layout_parent_style.get_box().clone_display().is_item_container());
-        }
+        blockify_if!(flags.contains(CascadeFlags::IS_ROOT_ELEMENT));
+        blockify_if!(
+            !self.skip_item_based_display_fixup() &&
+            layout_parent_style.get_box().clone_display().is_item_container()
+        );
 
         let is_item_or_root = blockify;
 
         blockify_if!(self.style.floated());
         blockify_if!(self.style.out_of_flow_positioned());
 
         if !blockify {
             return;
@@ -442,28 +449,26 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     /// * propagate the line break suppression flag,
     /// * inlinify block descendants,
     /// * suppress border and padding for ruby level containers,
     /// * correct unicode-bidi.
     #[cfg(feature = "gecko")]
     fn adjust_for_ruby(
         &mut self,
         layout_parent_style: &ComputedValues,
-        flags: CascadeFlags,
     ) {
-        use properties::CascadeFlags;
         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();
         // Check whether line break should be suppressed for this element.
         if self.should_suppress_linebreak(layout_parent_style) {
             self.style.flags.insert(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK);
             // Inlinify the display type if allowed.
-            if !flags.contains(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
+            if !self.skip_item_based_display_fixup() {
                 let inline_display = self_display.inlinify();
                 if self_display != inline_display {
                     self.style.mutate_box().set_display(inline_display);
                 }
             }
         }
         // Suppress border and padding for ruby level containers.
         // This is actually not part of the spec. It is currently unspecified
@@ -589,13 +594,13 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
             self.adjust_for_alignment(layout_parent_style);
         }
         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, flags);
+            self.adjust_for_ruby(layout_parent_style);
         }
         self.set_bits();
     }
 }
--- a/servo/components/style/style_resolver.rs
+++ b/servo/components/style/style_resolver.rs
@@ -537,21 +537,16 @@ where
         debug_assert!(
             self.element.implemented_pseudo_element().is_none() || pseudo.is_none(),
             "Pseudo-elements can't have other pseudos!"
         );
         debug_assert!(pseudo.map_or(true, |p| p.is_eager()));
 
         let mut cascade_flags = CascadeFlags::empty();
 
-        if self.element.skip_root_and_item_based_display_fixup() ||
-           pseudo.map_or(false, |p| p.skip_item_based_display_fixup()) {
-            cascade_flags.insert(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP);
-        }
-
         if pseudo.is_none() && self.element.is_link() {
             cascade_flags.insert(CascadeFlags::IS_LINK);
             if self.element.is_visited_link() &&
                 self.context.shared.visited_styles_enabled {
                 cascade_flags.insert(CascadeFlags::IS_VISITED_LINK);
             }
         }
 
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -2014,22 +2014,21 @@ pub extern "C" fn Servo_ComputedValues_G
     };
 
     let rule_node = data.stylist.rule_node_for_precomputed_pseudo(
         &guards,
         &pseudo,
         page_decls,
     );
 
-    let cascade_flags = CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP;
     data.stylist.precomputed_values_for_pseudo_with_rule_node(
         &guards,
         &pseudo,
         parent_style_or_null.map(|x| &*x),
-        cascade_flags,
+        CascadeFlags::empty(),
         &metrics,
         rule_node
     ).into()
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_ResolvePseudoStyle(element: RawGeckoElementBorrowed,
                                            pseudo_type: CSSPseudoElementType,
@@ -3615,20 +3614,27 @@ pub extern "C" fn Servo_ReparentStyle(
     let guard = global_style_data.shared_lock.read();
     let doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow();
     let inputs = CascadeInputs::new_from_style(style_to_reparent);
     let metrics = get_metrics_provider_for_product();
     let pseudo = style_to_reparent.pseudo();
     let element = element.map(GeckoElement);
 
     let mut cascade_flags = CascadeFlags::empty();
-    if style_to_reparent.is_anon_box() {
-        cascade_flags.insert(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP);
-    }
     if let Some(element) = element {
+        // NOTE(emilio): This relies on element.is_some() => pseudo.is_none(),
+        // which the caller guarantees, fortunately. But this doesn't handle the
+        // IS_ROOT_ELEMENT flag correctly!
+        //
+        // I think it's not possible to wrap a root element in a first-line
+        // frame (and reparenting only happens for ::first-line and its
+        // descendants), so this may be fine...
+        //
+        // We should just get rid of all these flags which pass element / pseudo
+        // state.
         if element.is_link() {
             cascade_flags.insert(CascadeFlags::IS_LINK);
             if element.is_visited_link() && doc_data.visited_styles_enabled() {
                 cascade_flags.insert(CascadeFlags::IS_VISITED_LINK);
             }
         };
     }