Bug 1782597 Part 2 - Use null to indicate page value equal to the auto value for CSS named pages r=dholbert

This applies both to the individual mStartPageValue and mEndPageValue fields
of the nsIFrame::PageValues struct, and for the nsIFrame::PageValuesProperty
being null to indicate both mStartPageValue and mEndPageValue are auto.

Fetching this is handled by nsIFrame::GetStartPageValue and
nsIFrame::GetEndPageValue, which also ensure the use of first-in-flow frames.

Differential Revision: https://phabricator.services.mozilla.com/D157873

layout/base/nsCSSFrameConstructor.cpp

@@ -1413,6 +1413,11 @@ nsCSSFrameConstructor::AutoFrameConstructionPageName::
                "Page name should not have been set");
     return;
   }
+#ifdef DEBUG
+  MOZ_ASSERT(!aFrame->mWasVisitedByAutoFrameConstructionPageName,
+             "Frame should only have been visited once");
+  aFrame->mWasVisitedByAutoFrameConstructionPageName = true;
+#endif
 
   EnsureAutoPageName(aState, aFrame->GetParent());
   mNameToRestore = aState.mAutoPageNameValue;
@@ -1421,20 +1426,6 @@ nsCSSFrameConstructor::AutoFrameConstructionPageName::
              "Page name should have been found by EnsureAutoPageName");
   MaybeApplyPageName(aState, aFrame->StylePage()->mPage);
   aFrame->SetAutoPageValue(aState.mAutoPageNameValue);
-  // Ensure that the PageValuesProperty field has been created.
-  // Before layout.css.named_pages.enabled is prefed on by default, we should
-  // investigate making this property optional, and have a missing property
-  // indicate a default root page-name or an auto page-name.
-  //
-  // When we make this property optional, we should add a debug-only flag on
-  // nsIFrame to indicate it was visited by this logic, as currently we assert
-  // the existence of this property to validate that.
-  nsIFrame::PageValues* pageValues =
-      aFrame->GetProperty(nsIFrame::PageValuesProperty());
-  if (!pageValues) {
-    pageValues = new nsIFrame::PageValues();
-    aFrame->AddProperty(nsIFrame::PageValuesProperty(), pageValues);
-  }
 }
 
 nsCSSFrameConstructor::AutoFrameConstructionPageName::
@@ -9367,6 +9358,51 @@ static bool FrameHasOnlyPlaceholderNextSiblings(const nsIFrame* aFrame) {
   return !nextSibling;
 }
 
+static void SetPageValues(nsIFrame* const aFrame,
+                          const nsAtom* const aAutoValue,
+                          const nsAtom* const aStartValue,
+                          const nsAtom* const aEndValue) {
+  MOZ_ASSERT(aAutoValue, "Auto page value should never be null");
+  MOZ_ASSERT(aStartValue || aEndValue, "Should not have called with no values");
+  nsIFrame::PageValues* pageValues =
+      aFrame->GetProperty(nsIFrame::PageValuesProperty());
+
+  if (aStartValue) {
+    if (aStartValue == aAutoValue) {
+      // If the page value struct already exists, set the start value to null
+      // to indicate the auto value.
+      if (pageValues) {
+        pageValues->mStartPageValue = nullptr;
+      }
+    } else {
+      // The start value is not auto, so we need to store it, creating the
+      // page values struct if it does not already exist.
+      if (!pageValues) {
+        pageValues = new nsIFrame::PageValues();
+        aFrame->SetProperty(nsIFrame::PageValuesProperty(), pageValues);
+      }
+      pageValues->mStartPageValue = aStartValue;
+    }
+  }
+  if (aEndValue) {
+    if (aEndValue == aAutoValue) {
+      // If the page value struct already exists, set the end value to null
+      // to indicate the auto value.
+      if (pageValues) {
+        pageValues->mEndPageValue = nullptr;
+      }
+    } else {
+      // The end value is not auto, so we need to store it, creating the
+      // page values struct if it does not already exist.
+      if (!pageValues) {
+        pageValues = new nsIFrame::PageValues();
+        aFrame->SetProperty(nsIFrame::PageValuesProperty(), pageValues);
+      }
+      pageValues->mEndPageValue = aEndValue;
+    }
+  }
+}
+
 inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
     nsFrameConstructorState& aState, FrameConstructionItemList& aItems,
     nsContainerFrame* aParentFrame, bool aParentIsWrapperAnonBox,
@@ -9450,6 +9486,15 @@ inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
     // Set the start/end page values while iterating the frame list, to walk
     // up the frame tree only once after iterating the frame list.
     // This also avoids extra property lookups on these frames.
+    MOZ_ASSERT(aState.mAutoPageNameValue == aParentFrame->GetAutoPageValue(),
+               "aState.mAutoPageNameValue should have been equivalent to "
+               "the auto value stored on our parent frame.");
+    // Even though we store null for page values that equal the "auto" resolved
+    // value on frames, we always want startPageValue/endPageValue to be the
+    // actual atoms reflecting the start/end values. This is because when we
+    // propagate the values up the frame tree, we will need to compare them to
+    // the auto value for each ancestor. This value might be different than the
+    // auto value for this frame.
     const nsAtom* startPageValue = nullptr;
     const nsAtom* endPageValue = nullptr;
     for (nsIFrame* f : aFrameList) {
@@ -9484,21 +9529,29 @@ inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
               : aState.mAutoPageNameValue;
       nsIFrame::PageValues* pageValues =
           f->GetProperty(nsIFrame::PageValuesProperty());
-      if (!pageValues) {
+      // If this frame has any children, it will already have had its page
-        pageValues = new nsIFrame::PageValues();
+      // values set at this point. However, if no page values have been set,
-        f->AddProperty(nsIFrame::PageValuesProperty(), pageValues);
+      // we must ensure that the appropriate PageValuesProperty value has been
-      }
+      // set.
-      MOZ_ASSERT(!pageValues->mStartPageValue == !pageValues->mEndPageValue,
+      // If the page name is equal to the auto value, then PageValuesProperty
-                 "Both or neither mStartPageValue and mEndPageValue should "
+      // should remain null to indicate that the start/end values are both
-                 "have been set");
+      // equal to the auto value.
-      if (!pageValues->mStartPageValue) {
+      if (pageNameAtom != aState.mAutoPageNameValue && !pageValues) {
-        pageValues->mStartPageValue = pageNameAtom;
+        pageValues = new nsIFrame::PageValues{pageNameAtom, pageNameAtom};
-        pageValues->mEndPageValue = pageNameAtom;
+        f->SetProperty(nsIFrame::PageValuesProperty(), pageValues);
       }
+      // We don't want to use GetStartPageValue() or GetEndPageValue(), as each
+      // requires a property lookup which we can avoid here.
       if (!startPageValue) {
-        startPageValue = pageValues->mStartPageValue;
+        startPageValue = (pageValues && pageValues->mStartPageValue)
+                             ? pageValues->mStartPageValue.get()
+                             : aState.mAutoPageNameValue;
       }
-      endPageValue = pageValues->mEndPageValue;
+      endPageValue = (pageValues && pageValues->mEndPageValue)
+                         ? pageValues->mEndPageValue.get()
+                         : aState.mAutoPageNameValue;
+      MOZ_ASSERT(startPageValue && endPageValue,
+                 "Should have found start/end page value");
     }
     MOZ_ASSERT(!startPageValue == !endPageValue,
                "Should have set both or neither page values");
@@ -9507,7 +9560,8 @@ inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
       // end page values.
       // As we go, if we find that, for a frame, we are not contributing one of
       // the start/end page values, then our subtree will not contribute this
-      // value from that frame onward.
+      // value from that frame onward. startPageValue/endPageValue are set to
+      // null to indicate this.
       // Stop iterating when we are not contributing either start or end
       // values, when we hit the root frame (no parent), or when we find a
       // frame that is not a block frame.
@@ -9517,35 +9571,36 @@ inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
            ancestorFrame = ancestorFrame->GetParent()) {
         MOZ_ASSERT(!ancestorFrame->GetPrevInFlow(),
                    "Should not have fragmentation yet");
-
+        MOZ_ASSERT(ancestorFrame->mWasVisitedByAutoFrameConstructionPageName,
-        nsIFrame::PageValues* const ancestorPageValues =
+                   "Frame should have been visited by "
-            ancestorFrame->GetProperty(nsIFrame::PageValuesProperty());
+                   "AutoFrameConstructionPageName");
-
+        {
-        if (!ancestorPageValues) {
+          // Get what the auto value is, based on this frame's parent.
-          break;
+          // For the root frame, `auto` resolves to the empty atom.
+          const nsContainerFrame* const parent = ancestorFrame->GetParent();
+          const nsAtom* const parentAuto = MOZ_LIKELY(parent)
+                                               ? parent->GetAutoPageValue()
+                                               : nsGkAtoms::_empty;
+          SetPageValues(ancestorFrame, parentAuto, startPageValue,
+                        endPageValue);
         }
-
-        // Set start/end values if we are still contributing those values.
         // Once we stop contributing start/end values, we know there is a
         // sibling subtree that contributed that value to our shared parent
         // instead of our starting frame's subtree. This means once
-        // startPageValue/endPageValue becomes null, it should stay null and
+        // startPageValue/endPageValue becomes null, indicating that we are no
-        // we no longer need to check for siblings in that direction.
+        // longer contributing that page value, it should stay null and we no
-        if (startPageValue) {
+        // longer need to check for siblings in that direction.
-          ancestorPageValues->mStartPageValue = startPageValue;
+        if (startPageValue &&
-          if (!FrameHasOnlyPlaceholderPrevSiblings(ancestorFrame)) {
+            !FrameHasOnlyPlaceholderPrevSiblings(ancestorFrame)) {
           startPageValue = nullptr;
         }
-        }
+        if (endPageValue &&
-        if (endPageValue) {
+            !FrameHasOnlyPlaceholderNextSiblings(ancestorFrame)) {
-          ancestorPageValues->mEndPageValue = endPageValue;
-          if (!FrameHasOnlyPlaceholderNextSiblings(ancestorFrame)) {
           endPageValue = nullptr;
         }
       }
     }
   }
-  }
 
   if (aParentIsWrapperAnonBox) {
     for (nsIFrame* f : aFrameList) {

layout/generic/nsBlockFrame.cpp

@@ -2823,21 +2823,14 @@ void nsBlockFrame::ReflowDirtyLines(BlockReflowState& aState) {
     if (canBreakForPageNames && (!aState.mReflowInput.mFlags.mIsTopOfPage ||
                                  !aState.IsAdjacentWithBStart())) {
       const nsIFrame* const frame = line->mFirstChild;
-      if (const nsIFrame* prevFrame = frame->GetPrevSibling()) {
+      if (const nsIFrame* const prevFrame = frame->GetPrevSibling()) {
-        if (const nsIFrame::PageValues* const pageValues =
+        if (!frame->IsPlaceholderFrame() && !prevFrame->IsPlaceholderFrame() &&
-                frame->FirstInFlow()->GetProperty(
+            frame->GetStartPageValue() != prevFrame->GetEndPageValue()) {
-                    nsIFrame::PageValuesProperty())) {
-          const nsIFrame::PageValues* const prevPageValues =
-              prevFrame->FirstInFlow()->GetProperty(
-                  nsIFrame::PageValuesProperty());
-          if (prevPageValues &&
-              prevPageValues->mEndPageValue != pageValues->mStartPageValue) {
           shouldBreakForPageName = true;
           line->MarkDirty();
         }
       }
     }
-    }
 
     if (needToRecoverState && line->IsDirty()) {
       // We need to reconstruct the block-end margin only if we didn't

layout/generic/nsIFrame.h

@@ -588,6 +588,9 @@ class nsIFrame : public nsQueryFrame {
         mHasModifiedDescendants(false),
         mHasOverrideDirtyRegion(false),
         mMayHaveWillChangeBudget(false),
+#ifdef DEBUG
+        mWasVisitedByAutoFrameConstructionPageName(false),
+#endif
         mIsPrimaryFrame(false),
         mMayHaveTransformAnimation(false),
         mMayHaveOpacityAnimation(false),
@@ -1296,13 +1299,29 @@ class nsIFrame : public nsQueryFrame {
   // frame construction, we insert page breaks when we begin a new page box and
   // the previous page box had a different name.
   struct PageValues {
-    // mFirstChildPageName of null is used to indicate that no child has been
+    // A value of null indicates that the value is equal to what auto resolves
-    // constructed yet.
+    // to for this frame.
     RefPtr<const nsAtom> mStartPageValue = nullptr;
     RefPtr<const nsAtom> mEndPageValue = nullptr;
   };
   NS_DECLARE_FRAME_PROPERTY_DELETABLE(PageValuesProperty, PageValues)
 
+  const nsAtom* GetStartPageValue() const {
+    if (const PageValues* const values =
+            FirstInFlow()->GetProperty(PageValuesProperty())) {
+      return values->mStartPageValue;
+    }
+    return nullptr;
+  }
+
+  const nsAtom* GetEndPageValue() const {
+    if (const PageValues* const values =
+            FirstInFlow()->GetProperty(PageValuesProperty())) {
+      return values->mEndPageValue;
+    }
+    return nullptr;
+  }
+
  private:
   // The value that the CSS page-name "auto" keyword resolves to for children
   // of this frame.
@@ -5182,6 +5201,18 @@ class nsIFrame : public nsQueryFrame {
    */
   bool mMayHaveWillChangeBudget : 1;
 
+#ifdef DEBUG
+ public:
+  /**
+   * True if this frame has already been been visited by
+   * nsCSSFrameConstructor::AutoFrameConstructionPageName.
+   *
+   * This is used to assert that we have visited each frame only once, and is
+   * not useful otherwise.
+   */
+  bool mWasVisitedByAutoFrameConstructionPageName : 1;
+#endif
+
  private:
   /**
    * True if this is the primary frame for mContent.