Bug 1498943 - Remove PseudoElement::inherits_all. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 15 Oct 2018 03:13:09 +0000
changeset 489614 69252f7e6d81935bacf4f12e45aa055026328a50
parent 489613 434e41d68832d1cabc346286fe6874c0dd6f34c7
child 489615 5aff488dd9e943c985dc7ffed98d26e46c71dfab
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersheycam
bugs1498943
milestone64.0a1
Bug 1498943 - Remove PseudoElement::inherits_all. r=heycam I plan to change servo to use all: inherit on its UA sheet. I hope the patch below should make it good enough performance-wise. And also, it's probably broken so I don't think it's worth supporting it specially. Differential Revision: https://phabricator.services.mozilla.com/D8686
servo/components/style/gecko/pseudo_element.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/servo/selector_parser.rs
servo/components/style/stylist.rs
--- a/servo/components/style/gecko/pseudo_element.rs
+++ b/servo/components/style/gecko/pseudo_element.rs
@@ -52,25 +52,16 @@ impl PseudoElement {
 
         if self.is_precomputed() {
             return PseudoElementCascadeType::Precomputed;
         }
 
         PseudoElementCascadeType::Lazy
     }
 
-    /// Whether cascading this pseudo-element makes it inherit all properties,
-    /// even reset ones.
-    ///
-    /// This is used in Servo for anonymous boxes, though it's likely broken.
-    #[inline]
-    pub fn inherits_all(&self) -> bool {
-        false
-    }
-
     /// Whether the pseudo-element should inherit from the default computed
     /// values instead of from the parent element.
     ///
     /// This is not the common thing, but there are some pseudos (namely:
     /// ::backdrop), that shouldn't inherit from the parent element.
     pub fn inherits_from_default_values(&self) -> bool {
         matches!(*self, PseudoElement::Backdrop)
     }
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -3293,25 +3293,16 @@ impl<'a> StyleBuilder<'a> {
         #[cfg(feature = "gecko")]
         debug_assert!(parent_style.is_none() ||
                       ::std::ptr::eq(parent_style.unwrap(),
                                      parent_style_ignoring_first_line.unwrap()) ||
                       parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine));
         let reset_style = device.default_computed_values();
         let inherited_style = parent_style.unwrap_or(reset_style);
         let inherited_style_ignoring_first_line = parent_style_ignoring_first_line.unwrap_or(reset_style);
-        // FIXME(bz): inherits_all seems like a fundamentally broken idea.  I'm
-        // 99% sure it should give incorrect behavior for table anonymous box
-        // backgrounds, for example.  This code doesn't attempt to make it play
-        // nice with inherited_style_ignoring_first_line.
-        let reset_style = if pseudo.map_or(false, |p| p.inherits_all()) {
-            inherited_style
-        } else {
-            reset_style
-        };
 
         let flags = inherited_style.flags.inherited();
 
         StyleBuilder {
             device,
             inherited_style,
             inherited_style_ignoring_first_line,
             reset_style,
--- a/servo/components/style/servo/selector_parser.rs
+++ b/servo/components/style/servo/selector_parser.rs
@@ -216,53 +216,16 @@ impl PseudoElement {
             PseudoElement::ServoAnonymousTableRow |
             PseudoElement::ServoAnonymousTableCell |
             PseudoElement::ServoAnonymousBlock |
             PseudoElement::ServoInlineBlockWrapper |
             PseudoElement::ServoInlineAbsolute => PseudoElementCascadeType::Precomputed,
         }
     }
 
-    /// For most (but not all) anon-boxes, we inherit all values from the
-    /// parent, this is the hook in the style system to allow this.
-    ///
-    /// FIXME(emilio): It's likely that this is broken in a variety of
-    /// situations, and what it really wants is just inherit some reset
-    /// properties...  Also, I guess it just could do all: inherit on the
-    /// stylesheet, though chances are that'd be kinda slow if we don't cache
-    /// them...
-    pub fn inherits_all(&self) -> bool {
-        match *self {
-            PseudoElement::After |
-            PseudoElement::Before |
-            PseudoElement::Selection |
-            PseudoElement::DetailsContent |
-            PseudoElement::DetailsSummary |
-            // Anonymous table flows shouldn't inherit their parents properties in order
-            // to avoid doubling up styles such as transformations.
-            PseudoElement::ServoAnonymousTableCell |
-            PseudoElement::ServoAnonymousTableRow |
-            PseudoElement::ServoText |
-            PseudoElement::ServoInputText => false,
-
-            // For tables, we do want style to inherit, because TableWrapper is
-            // responsible for handling clipping and scrolling, while Table is
-            // responsible for creating stacking contexts.
-            //
-            // StackingContextCollectionFlags makes sure this is processed
-            // properly.
-            PseudoElement::ServoAnonymousTable |
-            PseudoElement::ServoAnonymousTableWrapper |
-            PseudoElement::ServoTableWrapper |
-            PseudoElement::ServoAnonymousBlock |
-            PseudoElement::ServoInlineBlockWrapper |
-            PseudoElement::ServoInlineAbsolute => true,
-        }
-    }
-
     /// Covert non-canonical pseudo-element to canonical one, and keep a
     /// canonical one as it is.
     pub fn canonical(&self) -> PseudoElement {
         self.clone()
     }
 
     /// Stub, only Gecko needs this
     pub fn pseudo_info(&self) {
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -609,24 +609,16 @@ impl Stylist {
             return maybe;
         }
 
         f(&self.cascade_data.author) || f(&self.cascade_data.user)
     }
 
     /// Computes the style for a given "precomputed" pseudo-element, taking the
     /// universal rules and applying them.
-    ///
-    /// If `inherit_all` is true, then all properties are inherited from the
-    /// parent; otherwise, non-inherited properties are reset to their initial
-    /// values. The flow constructor uses this flag when constructing anonymous
-    /// flows.
-    ///
-    /// TODO(emilio): The type parameter could go away with a void type
-    /// implementing TElement.
     pub fn precomputed_values_for_pseudo<E>(
         &self,
         guards: &StylesheetGuards,
         pseudo: &PseudoElement,
         parent: Option<&ComputedValues>,
         font_metrics: &FontMetricsProvider,
     ) -> Arc<ComputedValues>
     where