Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property. r?dholbert,heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 26 Jul 2017 15:26:19 +0200
changeset 618417 983c6ce26cee78668a5693b1a563b8ca56618f8d
parent 618416 9f4aac621592b9a63cb43a2cee2bee40a387337c
child 618418 338b18c8c9741fc74dce2506ea16b4cd694cab74
push id71323
push userbmo:emilio+bugs@crisal.io
push dateMon, 31 Jul 2017 12:35:01 +0000
reviewersdholbert, heycam
bugs1384542
milestone56.0a1
Bug 1384542: Get rid of GetParentAllowServo in implementation of CSS 'justify-items' property. r?dholbert,heycam MozReview-Commit-ID: 4qydjqSoVXs
layout/generic/nsBlockFrame.cpp
layout/style/GeckoStyleContext.cpp
layout/style/nsComputedDOMStyle.cpp
layout/style/nsRuleNode.cpp
layout/style/nsStyleStruct.cpp
layout/style/nsStyleStruct.h
--- 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/GeckoStyleContext.cpp
+++ b/layout/style/GeckoStyleContext.cpp
@@ -392,16 +392,17 @@ GeckoStyleContext::GetUniqueStyleData(co
 #define UNIQUE_CASE(c_)                                                       \
   case eStyleStruct_##c_:                                                     \
     result = new (presContext) nsStyle##c_(                                   \
       * static_cast<const nsStyle##c_ *>(current));                           \
     break;
 
   UNIQUE_CASE(Font)
   UNIQUE_CASE(Display)
+  UNIQUE_CASE(Position)
   UNIQUE_CASE(Text)
   UNIQUE_CASE(TextReset)
   UNIQUE_CASE(Visibility)
 
 #undef UNIQUE_CASE
 
   default:
     NS_ERROR("Struct type not supported.  Please find another way to do this if you can!");
@@ -860,16 +861,33 @@ GeckoStyleContext::ApplyStyleFixups(bool
         text->mTextAlign == NS_STYLE_TEXT_ALIGN_MOZ_CENTER ||
         text->mTextAlign == NS_STYLE_TEXT_ALIGN_MOZ_RIGHT)
     {
       nsStyleText* uniqueText = GET_UNIQUE_STYLE_DATA(Text);
       uniqueText->mTextAlign = NS_STYLE_TEXT_ALIGN_START;
     }
   }
 
+  // Fixup the "justify-items: auto" value based on our parent style here if
+  // needed.
+  //
+  // Note that this only pulls a unique struct in the case the parent has the
+  // "legacy" modifier (which is not the default), and the computed value would
+  // change as a result.
+  //
+  // We check the parent first just to avoid unconditionally pulling the
+  // nsStylePosition struct on every style context.
+  if (mParent &&
+      mParent->StylePosition()->mJustifyItems & NS_STYLE_JUSTIFY_LEGACY &&
+      StylePosition()->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO &&
+      StylePosition()->mJustifyItems != mParent->StylePosition()->mJustifyItems) {
+    nsStylePosition* uniquePosition = GET_UNIQUE_STYLE_DATA(Position);
+    uniquePosition->mJustifyItems = mParent->StylePosition()->mJustifyItems;
+  }
+
   // CSS2.1 section 9.2.4 specifies fixups for the 'display' property of
   // the root element.  We can't implement them in nsRuleNode because we
   // don't want to store all display structs that aren't 'block',
   // 'inline', or 'table' in the style context tree on the off chance
   // that the root element has its style reresolved later.  So do them
   // here if needed, by changing the style data, so that other code
   // doesn't get confused by looking at the style data.
   if (!mParent &&
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -4690,18 +4690,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
@@ -8654,31 +8654,38 @@ 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);
   }
 
+  // NOTE(emilio): Even though "auto" technically depends on the parent style
+  // context, most of the time it'll resolve to "normal".  So, we
+  // optimistically assume here that it does resolve to "normal", and we
+  // handle the other cases in ApplyStyleFixups. This way, position structs
+  // can be cached in the default/common case.
+  pos->mJustifyItems =
+    pos->mSpecifiedJustifyItems == NS_STYLE_JUSTIFY_AUTO
+      ? NS_STYLE_JUSTIFY_NORMAL : pos->mSpecifiedJustifyItems;
+
   // justify-self: enum, inherit, initial
   SetValue(*aRuleData->ValueForJustifySelf(),
            pos->mJustifySelf, conditions,
            SETVAL_ENUMERATED | SETVAL_UNSET_INITIAL,
            parentPos->mJustifySelf,
            NS_STYLE_JUSTIFY_AUTO);
 
   // flex-basis: auto, length, percent, enum, calc, inherit, initial
--- 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 set to
+  // mSpecifiedJustifyItems, except when the latter is AUTO -- in that case,
+  // mJustifyItems is set to NORMAL, or to the parent style context's
+  // mJustifyItems if it has the legacy flag.
+  //
+  // This last part 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