Bug 1384542: Get rid of GetParentAllowServo in justify-items. r?dholbert,heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 26 Jul 2017 15:26:19 +0200
changeset 616841 36e8bb6ebc3462770b86821844b67dfef242d56c
parent 616832 95ee775201314ba1062d578b610896830773d66b
child 616842 f5b758932a7debc33193ce04e9c6dab4c5230f47
push id70828
push userbmo:emilio+bugs@crisal.io
push dateThu, 27 Jul 2017 15:03:04 +0000
reviewersdholbert, heycam
bugs1384542
milestone56.0a1
Bug 1384542: Get rid of GetParentAllowServo in justify-items. r?dholbert,heycam MozReview-Commit-ID: 4qydjqSoVXs
layout/generic/nsBlockFrame.cpp
layout/style/nsComputedDOMStyle.cpp
layout/style/nsRuleNode.cpp
layout/style/nsStyleStruct.cpp
layout/style/nsStyleStruct.h
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/longhand/position.mako.rs
servo/components/style/values/computed/mod.rs
servo/components/style/values/specified/align.rs
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -1526,23 +1526,21 @@ nsBlockFrame::Reflow(nsPresContext*     
               (value == NS_STYLE_ALIGN_AUTO));
     };
 
     // First check this frame for non-default values of the css-align properties
     // that apply to block containers.
     // Note: we check here for non-default "justify-items", though technically
     // that'd only affect rendering if some child has "justify-self:auto".
     // (It's safe to assume that's likely, since it's the default value that
-    // a child would have.) We also pass in nullptr for the parent style context
-    // because an accurate parameter is slower and only necessary to detect a
-    // narrow edge case with the "legacy" keyword.
+    // a child would have.).
     const nsStylePosition* stylePosition = reflowInput->mStylePosition;
     if (!IsStyleNormalOrAuto(stylePosition->mJustifyContent) ||
         !IsStyleNormalOrAuto(stylePosition->mAlignContent) ||
-        !IsStyleNormalOrAuto(stylePosition->ComputedJustifyItems(nullptr))) {
+        !IsStyleNormalOrAuto(stylePosition->mJustifyItems)) {
       Telemetry::Accumulate(Telemetry::BOX_ALIGN_PROPS_IN_BLOCKS_FLAG, true);
     } else {
       // If not already flagged by the parent, now check justify-self of the
       // block-level child frames.
       for (nsBlockFrame::LineIterator line = LinesBegin();
            line != LinesEnd(); ++line) {
         if (line->IsBlock() &&
             !IsStyleNormalOrAuto(line->mFirstChild->StylePosition()->mJustifySelf)) {
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -4689,18 +4689,17 @@ nsComputedDOMStyle::DoGetJustifyContent(
   return val.forget();
 }
 
 already_AddRefed<CSSValue>
 nsComputedDOMStyle::DoGetJustifyItems()
 {
   RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
   nsAutoString str;
-  auto justify =
-    StylePosition()->ComputedJustifyItems(mStyleContext->GetParentAllowServo());
+  auto justify = StylePosition()->mJustifyItems;
   nsCSSValue::AppendAlignJustifyValueToString(justify, str);
   val->SetString(str);
   return val.forget();
 }
 
 already_AddRefed<CSSValue>
 nsComputedDOMStyle::DoGetJustifySelf()
 {
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -8510,16 +8510,31 @@ SetGridLine(const nsCSSValue& aValue,
       }
       item = item->mNext;
     } while (item);
     MOZ_ASSERT(!aResult.IsAuto(),
                "should have set something away from default value");
   }
 }
 
+static uint8_t
+ComputedJustifyItems(uint8_t aSpecifiedJustifiedItems,
+                     uint8_t aParentJustifyItems)
+{
+  if (aSpecifiedJustifiedItems != NS_STYLE_JUSTIFY_AUTO) {
+    return aSpecifiedJustifiedItems;
+  }
+
+  if (aParentJustifyItems & NS_STYLE_JUSTIFY_LEGACY) {
+    return aParentJustifyItems;
+  }
+
+  return NS_STYLE_JUSTIFY_NORMAL;
+}
+
 const void*
 nsRuleNode::ComputePositionData(void* aStartStruct,
                                 const nsRuleData* aRuleData,
                                 GeckoStyleContext* aContext,
                                 nsRuleNode* aHighestNode,
                                 const RuleDetail aRuleDetail,
                                 const RuleNodeCacheConditions aConditions)
 {
@@ -8654,30 +8669,36 @@ nsRuleNode::ComputePositionData(void* aS
            pos->mJustifyContent, conditions,
            SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
            parentPos->mJustifyContent,
            NS_STYLE_JUSTIFY_NORMAL);
 
   // justify-items: enum, inherit, initial
   const auto& justifyItemsValue = *aRuleData->ValueForJustifyItems();
   if (MOZ_UNLIKELY(justifyItemsValue.GetUnit() == eCSSUnit_Inherit)) {
-    if (MOZ_LIKELY(parentContext)) {
-      pos->mJustifyItems =
-        parentPos->ComputedJustifyItems(parentContext->GetParent());
-    } else {
-      pos->mJustifyItems = NS_STYLE_JUSTIFY_NORMAL;
-    }
+    pos->mSpecifiedJustifyItems =
+      MOZ_LIKELY(parentContext)
+        ? parentPos->mJustifyItems
+        : NS_STYLE_JUSTIFY_NORMAL;
     conditions.SetUncacheable();
   } else {
     SetValue(justifyItemsValue,
-             pos->mJustifyItems, conditions,
+             pos->mSpecifiedJustifyItems, conditions,
              SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
-             parentPos->mJustifyItems, // unused, we handle 'inherit' above
+             parentPos->mSpecifiedJustifyItems, // unused, we handle 'inherit' above
              NS_STYLE_JUSTIFY_AUTO);
-  }
+    if (pos->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO) {
+      // FIXME(emilio): This is kind of unfortunate because this is a reset
+      // property and AUTO is the default value... Can we do better?
+      conditions.SetUncacheable();
+    }
+  }
+
+  pos->mJustifyItems = ComputedJustifyItems(pos->mSpecifiedJustifyItems,
+                                            parentPos->mJustifyItems);
 
   // justify-self: enum, inherit, initial
   SetValue(*aRuleData->ValueForJustifySelf(),
            pos->mJustifySelf, conditions,
            SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
            parentPos->mJustifySelf,
            NS_STYLE_JUSTIFY_AUTO);
 
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1439,17 +1439,18 @@ nsStylePosition::nsStylePosition(const n
   , mGridAutoRowsMin(eStyleUnit_Auto)
   , mGridAutoRowsMax(eStyleUnit_Auto)
   , mGridAutoFlow(NS_STYLE_GRID_AUTO_FLOW_ROW)
   , mBoxSizing(StyleBoxSizing::Content)
   , mAlignContent(NS_STYLE_ALIGN_NORMAL)
   , mAlignItems(NS_STYLE_ALIGN_NORMAL)
   , mAlignSelf(NS_STYLE_ALIGN_AUTO)
   , mJustifyContent(NS_STYLE_JUSTIFY_NORMAL)
-  , mJustifyItems(NS_STYLE_JUSTIFY_AUTO)
+  , mSpecifiedJustifyItems(NS_STYLE_JUSTIFY_AUTO)
+  , mJustifyItems(NS_STYLE_JUSTIFY_NORMAL)
   , mJustifySelf(NS_STYLE_JUSTIFY_AUTO)
   , mFlexDirection(NS_STYLE_FLEX_DIRECTION_ROW)
   , mFlexWrap(NS_STYLE_FLEX_WRAP_NOWRAP)
   , mObjectFit(NS_STYLE_OBJECT_FIT_FILL)
   , mOrder(NS_STYLE_ORDER_INITIAL)
   , mFlexGrow(0.0f)
   , mFlexShrink(1.0f)
   , mZIndex(eStyleUnit_Auto)
@@ -1497,16 +1498,17 @@ nsStylePosition::nsStylePosition(const n
   , mGridAutoRowsMin(aSource.mGridAutoRowsMin)
   , mGridAutoRowsMax(aSource.mGridAutoRowsMax)
   , mGridAutoFlow(aSource.mGridAutoFlow)
   , mBoxSizing(aSource.mBoxSizing)
   , mAlignContent(aSource.mAlignContent)
   , mAlignItems(aSource.mAlignItems)
   , mAlignSelf(aSource.mAlignSelf)
   , mJustifyContent(aSource.mJustifyContent)
+  , mSpecifiedJustifyItems(aSource.mSpecifiedJustifyItems)
   , mJustifyItems(aSource.mJustifyItems)
   , mJustifySelf(aSource.mJustifySelf)
   , mFlexDirection(aSource.mFlexDirection)
   , mFlexWrap(aSource.mFlexWrap)
   , mObjectFit(aSource.mObjectFit)
   , mOrder(aSource.mOrder)
   , mFlexGrow(aSource.mFlexGrow)
   , mFlexShrink(aSource.mFlexShrink)
@@ -1628,16 +1630,22 @@ nsStylePosition::CalcDifference(const ns
   // Changing 'justify-content/items/self' might affect the positioning,
   // but it won't affect any sizing.
   if (mJustifyContent != aNewData.mJustifyContent ||
       mJustifyItems != aNewData.mJustifyItems ||
       mJustifySelf != aNewData.mJustifySelf) {
     hint |= nsChangeHint_NeedReflow;
   }
 
+  // No need to do anything if mSpecifiedJustifyItems changes, as long as
+  // mJustifyItems (tested above) is unchanged.
+  if (mSpecifiedJustifyItems != aNewData.mSpecifiedJustifyItems) {
+    hint |= nsChangeHint_NeutralChange;
+  }
+
   // 'align-content' doesn't apply to a single-line flexbox but we don't know
   // if we're a flex container at this point so we can't optimize for that.
   if (mAlignContent != aNewData.mAlignContent) {
     hint |= nsChangeHint_NeedReflow;
   }
 
   bool widthChanged = mWidth != aNewData.mWidth ||
                       mMinWidth != aNewData.mMinWidth ||
@@ -1710,42 +1718,23 @@ nsStylePosition::UsedAlignSelf(nsStyleCo
     MOZ_ASSERT(!(parentAlignItems & NS_STYLE_ALIGN_LEGACY),
                "align-items can't have 'legacy'");
     return parentAlignItems;
   }
   return NS_STYLE_ALIGN_NORMAL;
 }
 
 uint8_t
-nsStylePosition::ComputedJustifyItems(nsStyleContext* aParent) const
-{
-  if (mJustifyItems != NS_STYLE_JUSTIFY_AUTO) {
-    return mJustifyItems;
-  }
-  if (MOZ_LIKELY(aParent)) {
-    auto inheritedJustifyItems = aParent->StylePosition()->ComputedJustifyItems(
-      aParent->GetParentAllowServo());
-    // "If the inherited value of justify-items includes the 'legacy' keyword,
-    // 'auto' computes to the inherited value."  Otherwise, 'normal'.
-    if (inheritedJustifyItems & NS_STYLE_JUSTIFY_LEGACY) {
-      return inheritedJustifyItems;
-    }
-  }
-  return NS_STYLE_JUSTIFY_NORMAL;
-}
-
-uint8_t
 nsStylePosition::UsedJustifySelf(nsStyleContext* aParent) const
 {
   if (mJustifySelf != NS_STYLE_JUSTIFY_AUTO) {
     return mJustifySelf;
   }
   if (MOZ_LIKELY(aParent)) {
-    auto inheritedJustifyItems = aParent->StylePosition()->ComputedJustifyItems(
-      aParent->GetParentAllowServo());
+    auto inheritedJustifyItems = aParent->StylePosition()->mJustifyItems;
     return inheritedJustifyItems & ~NS_STYLE_JUSTIFY_LEGACY;
   }
   return NS_STYLE_JUSTIFY_NORMAL;
 }
 
 // --------------------
 // nsStyleTable
 //
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -1674,22 +1674,16 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
 
   /**
    * Return the used value for 'align-self' given our parent StyleContext
    * aParent (or null for the root).
    */
   uint8_t UsedAlignSelf(nsStyleContext* aParent) const;
 
   /**
-   * Return the computed value for 'justify-items' given our parent StyleContext
-   * aParent (or null for the root).
-   */
-  uint8_t ComputedJustifyItems(nsStyleContext* aParent) const;
-
-  /**
    * Return the used value for 'justify-self' given our parent StyleContext
    * aParent (or null for the root).
    */
   uint8_t UsedJustifySelf(nsStyleContext* aParent) const;
 
   mozilla::Position mObjectPosition;    // [reset]
   nsStyleSides  mOffset;                // [reset] coord, percent, calc, auto
   nsStyleCoord  mWidth;                 // [reset] coord, percent, enum, calc, auto
@@ -1705,24 +1699,27 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
   nsStyleCoord  mGridAutoRowsMax;       // [reset] coord, percent, enum, calc, flex
   uint8_t       mGridAutoFlow;          // [reset] enumerated. See nsStyleConsts.h
   mozilla::StyleBoxSizing mBoxSizing;   // [reset] see nsStyleConsts.h
 
   uint16_t      mAlignContent;          // [reset] fallback value in the high byte
   uint8_t       mAlignItems;            // [reset] see nsStyleConsts.h
   uint8_t       mAlignSelf;             // [reset] see nsStyleConsts.h
   uint16_t      mJustifyContent;        // [reset] fallback value in the high byte
-private:
-  friend class nsRuleNode;
-
-  // mJustifyItems should only be read via ComputedJustifyItems(), which
-  // lazily resolves its "auto" value. nsRuleNode needs direct access so
-  // it can set mJustifyItems' value when populating this struct.
+  // We cascade mSpecifiedJustifyItems, to handle the auto value, but store the
+  // computed value in mJustifyItems.
+  //
+  // They're effectively only different in this regard: mJustifyItems is
+  // effectively mSpecifiedJustifyItems, except the later is converted
+  // from AUTO to NORMAL, or to the parent style context's mJustifyItems if it
+  // has the legacy flag enabled.
+  //
+  // This last bit happens in nsStyleContext::ApplyStyleFixups.
+  uint8_t       mSpecifiedJustifyItems; // [reset] see nsStyleConsts.h
   uint8_t       mJustifyItems;          // [reset] see nsStyleConsts.h
-public:
   uint8_t       mJustifySelf;           // [reset] see nsStyleConsts.h
   uint8_t       mFlexDirection;         // [reset] see nsStyleConsts.h
   uint8_t       mFlexWrap;              // [reset] see nsStyleConsts.h
   uint8_t       mObjectFit;             // [reset] see nsStyleConsts.h
   int32_t       mOrder;                 // [reset] integer
   float         mFlexGrow;              // [reset] float
   float         mFlexShrink;            // [reset] float
   nsStyleCoord  mZIndex;                // [reset] integer, auto
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1349,18 +1349,36 @@ fn static_assert() {
                 Either::First(0)
             }
         }
     }
 
     % for kind in ["align", "justify"]:
     ${impl_simple_type_with_conversion(kind + "_content")}
     ${impl_simple_type_with_conversion(kind + "_self")}
-    ${impl_simple_type_with_conversion(kind + "_items")}
     % endfor
+    ${impl_simple_type_with_conversion("align_items")}
+
+    pub fn set_justify_items(&mut self, v: longhands::justify_items::computed_value::T) {
+        debug_assert!(v.computed.0 != ::values::specified::align::ALIGN_AUTO);
+        self.gecko.mJustifyItems = v.computed.into();
+        self.gecko.mSpecifiedJustifyItems = v.specified.into();
+    }
+
+    pub fn copy_justify_items_from(&mut self, other: &Self) {
+        self.gecko.mJustifyItems = other.gecko.mJustifyItems;
+        self.gecko.mSpecifiedJustifyItems = other.gecko.mSpecifiedJustifyItems;
+    }
+
+    pub fn clone_justify_items(&self) -> longhands::justify_items::computed_value::T {
+        longhands::justify_items::computed_value::T {
+            computed: self.gecko.mJustifyItems.into(),
+            specified: self.gecko.mSpecifiedJustifyItems.into(),
+        }
+    }
 
     pub fn set_order(&mut self, v: longhands::order::computed_value::T) {
         self.gecko.mOrder = v;
     }
 
     pub fn clone_order(&self) -> longhands::order::computed_value::T {
         self.gecko.mOrder
     }
--- a/servo/components/style/properties/longhand/position.mako.rs
+++ b/servo/components/style/properties/longhand/position.mako.rs
@@ -103,17 +103,17 @@ macro_rules! impl_align_conversions {
                               extra_prefixes="webkit",
                               animation_value_type="discrete")}
 
     #[cfg(feature = "gecko")]
     impl_align_conversions!(::values::specified::align::AlignItems);
 
     ${helpers.predefined_type(name="justify-items",
                               type="JustifyItems",
-                              initial_value="specified::JustifyItems::auto()",
+                              initial_value="computed::JustifyItems::auto()",
                               spec="https://drafts.csswg.org/css-align/#propdef-justify-items",
                               animation_value_type="discrete")}
 
     #[cfg(feature = "gecko")]
     impl_align_conversions!(::values::specified::align::JustifyItems);
 % endif
 
 // Flex item properties
--- a/servo/components/style/values/computed/mod.rs
+++ b/servo/components/style/values/computed/mod.rs
@@ -32,17 +32,17 @@ pub use self::color::{Color, RGBAColor};
 pub use self::effects::{BoxShadow, Filter, SimpleShadow};
 pub use self::flex::FlexBasis;
 pub use self::image::{Gradient, GradientItem, Image, ImageLayer, LineDirection, MozImageRect};
 #[cfg(feature = "gecko")]
 pub use self::gecko::ScrollSnapPoint;
 pub use self::rect::LengthOrNumberRect;
 pub use super::{Auto, Either, None_};
 #[cfg(feature = "gecko")]
-pub use super::specified::{AlignItems, AlignJustifyContent, AlignJustifySelf, JustifyItems};
+pub use super::specified::{AlignItems, AlignJustifyContent, AlignJustifySelf};
 pub use super::specified::{BorderStyle, UrlOrNone};
 pub use super::generics::grid::GridLine;
 pub use super::specified::url::SpecifiedUrl;
 pub use self::length::{CalcLengthOrPercentage, Length, LengthOrNone, LengthOrNumber, LengthOrPercentage};
 pub use self::length::{LengthOrPercentageOrAuto, LengthOrPercentageOrNone, MaxLength, MozLength, Percentage};
 pub use self::position::Position;
 pub use self::text::{InitialLetter, LetterSpacing, LineHeight, WordSpacing};
 pub use self::transform::{TimingFunction, TransformOrigin};
@@ -391,37 +391,73 @@ impl Time {
 impl ToCss for Time {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result
         where W: fmt::Write,
     {
         write!(dest, "{}s", self.seconds())
     }
 }
 
+/// The computed value for the `justify-items` property.
+///
+/// Need to carry around both the specified and computed value to handle the
+/// special legacy keyword. Sigh
+#[cfg(feature = "gecko")]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub struct JustifyItems {
+    /// The specified value for the property. Can contain `auto`.
+    pub specified: specified::JustifyItems,
+    /// The computed value for the property. Cannot contain `auto`.
+    pub computed: specified::JustifyItems,
+}
+
+#[cfg(feature = "gecko")]
+impl ToCss for JustifyItems {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result
+        where W: fmt::Write,
+    {
+        self.computed.to_css(dest)
+    }
+}
+
+#[cfg(feature = "gecko")]
+impl JustifyItems {
+    /// Returns the `auto` value.
+    pub fn auto() -> Self {
+        Self {
+            specified: specified::JustifyItems::auto(),
+            computed: specified::JustifyItems::normal(),
+        }
+    }
+}
+
 #[cfg(feature = "gecko")]
 impl ToComputedValue for specified::JustifyItems {
     type ComputedValue = JustifyItems;
 
     // https://drafts.csswg.org/css-align/#valdef-justify-items-auto
     fn to_computed_value(&self, context: &Context) -> JustifyItems {
         use values::specified::align;
-        // If the inherited value of `justify-items` includes the `legacy` keyword, `auto` computes
-        // to the inherited value.
-        if self.0 == align::ALIGN_AUTO {
-            let inherited = context.builder.get_parent_position().clone_justify_items();
-            if inherited.0.contains(align::ALIGN_LEGACY) {
-                return inherited
-            }
-        }
-        return *self
+        let specified = *self;
+        let computed =
+            if self.0 != align::ALIGN_AUTO {
+                *self
+            } else {
+                // If the inherited value of `justify-items` includes the
+                // `legacy` keyword, `auto` computes to the inherited value,
+                // but we handle that special-case in StyleAdjuster.
+                Self::normal()
+            };
+
+        JustifyItems { specified, computed }
     }
 
     #[inline]
     fn from_computed_value(computed: &JustifyItems) -> Self {
-        *computed
+        computed.specified
     }
 }
 
 #[cfg(feature = "gecko")]
 impl ComputedValueAsSpecified for specified::AlignItems {}
 #[cfg(feature = "gecko")]
 impl ComputedValueAsSpecified for specified::AlignJustifyContent {}
 #[cfg(feature = "gecko")]
--- a/servo/components/style/values/specified/align.rs
+++ b/servo/components/style/values/specified/align.rs
@@ -287,16 +287,22 @@ pub struct JustifyItems(pub AlignFlags);
 
 impl JustifyItems {
     /// The initial value 'auto'
     #[inline]
     pub fn auto() -> Self {
         JustifyItems(ALIGN_AUTO)
     }
 
+    /// The value 'normal'
+    #[inline]
+    pub fn normal() -> Self {
+        JustifyItems(ALIGN_NORMAL)
+    }
+
     /// Whether this value has extra flags.
     #[inline]
     pub fn has_extra_flags(self) -> bool {
         self.0.intersects(ALIGN_FLAG_BITS)
     }
 }
 
 no_viewport_percentage!(JustifyItems);