Bug 1552719 - Make nsBulletFrame::Ordinal() O(1) again. r=mats

I did this instead of just (ab)using the fact that every list item has at least
one counter-increment node because:

 * I don't have the bullet frame around by the time we initially compute the
   counter increment, which means that I'd need to grow nsBlockFrame / add a
   frame property for the list item ordinal, which I think would be unfortunate.

 * It feels more consistent with the way regular CSS counters work and with the
   way we want ::marker to eventually work.

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

--HG--
extra : moz-landing-system : lando

layout/base/nsCSSFrameConstructor.cpp

@@ -3790,6 +3790,16 @@ void nsCSSFrameConstructor::ConstructFrameFromItemInternal(
     }
   }
 
+  if (computedStyle->GetPseudoType() == PseudoStyleType::marker &&
+      newFrame->IsBulletFrame()) {
+    MOZ_ASSERT(!computedStyle->StyleContent()->ContentCount());
+    auto* node = new nsCounterUseNode(nsCounterUseNode::ForLegacyBullet);
+    auto* list = mCounterManager.CounterListFor(nsGkAtoms::list_item);
+    if (node->InitBullet(list, newFrame)) {
+      CountersDirty();
+    }
+  }
+
   NS_ASSERTION(newFrame->IsFrameOfType(nsIFrame::eLineParticipant) ==
                    ((bits & FCDATA_IS_LINE_PARTICIPANT) != 0),
                "Incorrectly set FCDATA_IS_LINE_PARTICIPANT bits");

layout/base/nsCounterManager.cpp

@@ -25,33 +25,45 @@ bool nsCounterUseNode::InitTextFrame(nsGenConList* aList,
                                      nsIFrame* aTextFrame) {
   nsCounterNode::InitTextFrame(aList, aPseudoFrame, aTextFrame);
 
-  nsCounterList* counterList = static_cast<nsCounterList*>(aList);
+  auto* counterList = static_cast<nsCounterList*>(aList);
   counterList->Insert(this);
   aPseudoFrame->AddStateBits(NS_FRAME_HAS_CSS_COUNTER_STYLE);
-  bool dirty = counterList->IsDirty();
-  if (!dirty) {
-    if (counterList->IsLast(this)) {
-      Calc(counterList);
-      nsAutoString contentString;
-      GetText(contentString);
-      aTextFrame->GetContent()->AsText()->SetText(contentString, false);
-    } else {
-      // In all other cases (list already dirty or node not at the end),
-      // just start with an empty string for now and when we recalculate
-      // the list we'll change the value to the right one.
-      counterList->SetDirty();
-      return true;
-    }
+  // If the list is already dirty, or the node is not at the end, just start
+  // with an empty string for now and when we recalculate the list we'll change
+  // the value to the right one.
+  if (counterList->IsDirty()) {
+    return false;
   }
-
+  if (!counterList->IsLast(this)) {
+    counterList->SetDirty();
+    return true;
+  }
+  Calc(counterList, /* aNotify = */ false);
   return false;
 }
 
+bool nsCounterUseNode::InitBullet(nsGenConList* aList, nsIFrame* aBullet) {
+  MOZ_ASSERT(aBullet->IsBulletFrame());
+  MOZ_ASSERT(aBullet->Style()->GetPseudoType() == PseudoStyleType::marker);
+  MOZ_ASSERT(mForLegacyBullet);
+  return InitTextFrame(aList, aBullet, nullptr);
+}
+
 // assign the correct |mValueAfter| value to a node that has been inserted
 // Should be called immediately after calling |Insert|.
-void nsCounterUseNode::Calc(nsCounterList* aList) {
+void nsCounterUseNode::Calc(nsCounterList* aList, bool aNotify) {
   NS_ASSERTION(!aList->IsDirty(), "Why are we calculating with a dirty list?");
   mValueAfter = nsCounterList::ValueBefore(this);
+  if (mText) {
+    nsAutoString contentString;
+    GetText(contentString);
+    mText->SetText(contentString, aNotify);
+  } else if (mForLegacyBullet) {
+    MOZ_ASSERT_IF(mPseudoFrame, mPseudoFrame->IsBulletFrame());
+    if (nsBulletFrame* f = do_QueryFrame(mPseudoFrame)) {
+      f->SetOrdinal(mValueAfter, aNotify);
+    }
+  }
 }
 
 // assign the correct |mValueAfter| value to a node that has been inserted
@@ -173,28 +185,7 @@ void nsCounterList::RecalcAll() {
   }
 
   for (nsCounterNode* node = First(); node; node = Next(node)) {
-    auto oldValue = node->mValueAfter;
-    node->Calc(this);
-
-    if (node->mType == nsCounterNode::USE) {
-      nsCounterUseNode* useNode = node->UseNode();
-      // Null-check mText, since if the frame constructor isn't
-      // batching, we could end up here while the node is being
-      // constructed.
-      if (useNode->mText) {
-        nsAutoString text;
-        useNode->GetText(text);
-        useNode->mText->SetData(text, IgnoreErrors());
-      }
-    }
-
-    if (oldValue != node->mValueAfter && node->mPseudoFrame &&
-        node->mPseudoFrame->StyleDisplay()->mDisplay ==
-            StyleDisplay::ListItem) {
-      auto* shell = node->mPseudoFrame->PresShell();
-      shell->FrameNeedsReflow(node->mPseudoFrame, IntrinsicDirty::StyleChange,
-                              NS_FRAME_IS_DIRTY);
-    }
+    node->Calc(this, /* aNotify = */ true);
   }
 }
 

layout/base/nsCounterManager.h

@@ -30,7 +30,7 @@ struct nsCounterNode : public nsGenConNode {
   Type mType;
 
   // Counter value after this node
-  int32_t mValueAfter;
+  int32_t mValueAfter = 0;
 
   // mScopeStart points to the node (usually a RESET, but not in the
   // case of an implied 'counter-reset') that created the scope for
@@ -41,7 +41,7 @@ struct nsCounterNode : public nsGenConNode {
   // Being null for a non-RESET means that it is an implied
   // 'counter-reset'.  Being null for a RESET means it has no outer
   // scope.
-  nsCounterNode* mScopeStart;
+  nsCounterNode* mScopeStart = nullptr;
 
   // mScopePrev points to the previous node that is in the same scope,
   // or for a RESET, the previous node in the scope outside of the
@@ -52,7 +52,7 @@ struct nsCounterNode : public nsGenConNode {
   // mScopeStart.  Being null for a non-RESET means that it is an
   // implied 'counter-reset'.  Being null for a RESET means it has no
   // outer scope.
-  nsCounterNode* mScopePrev;
+  nsCounterNode* mScopePrev = nullptr;
 
   inline nsCounterUseNode* UseNode();
   inline nsCounterChangeNode* ChangeNode();
@@ -64,14 +64,10 @@ struct nsCounterNode : public nsGenConNode {
   // that (reset, increment, set, use) sort in that order.
   // (This slight weirdness allows sharing a lot of code with 'quotes'.)
   nsCounterNode(int32_t aContentIndex, Type aType)
-      : nsGenConNode(aContentIndex),
-        mType(aType),
-        mValueAfter(0),
-        mScopeStart(nullptr),
-        mScopePrev(nullptr) {}
+      : nsGenConNode(aContentIndex), mType(aType) {}
 
   // to avoid virtual function calls in the common case
-  inline void Calc(nsCounterList* aList);
+  inline void Calc(nsCounterList* aList, bool aNotify);
 
   // Is this a <ol reversed> RESET node?
   inline bool IsContentBasedReset();
@@ -82,7 +78,15 @@ struct nsCounterUseNode : public nsCounterNode {
   nsString mSeparator;
 
   // false for counter(), true for counters()
-  bool mAllCounters;
+  bool mAllCounters = false;
+
+  bool mForLegacyBullet = false;
+
+  enum ForLegacyBullet { ForLegacyBullet };
+  explicit nsCounterUseNode(enum ForLegacyBullet)
+      : nsCounterNode(0, USE), mForLegacyBullet(true) {
+    mCounterStyle = nsGkAtoms::list_item;
+  }
 
   // args go directly to member variables here and of nsGenConNode
   nsCounterUseNode(nsStyleContentData::CounterFunction* aCounterFunction,
@@ -97,9 +101,12 @@ struct nsCounterUseNode : public nsCounterNode {
   virtual bool InitTextFrame(nsGenConList* aList, nsIFrame* aPseudoFrame,
                              nsIFrame* aTextFrame) override;
 
-  // assign the correct |mValueAfter| value to a node that has been inserted
+  bool InitBullet(nsGenConList* aList, nsIFrame* aBulletFrame);
+
+  // assign the correct |mValueAfter| value to a node that has been inserted,
+  // and update the value of the text node, notifying if `aNotify` is true.
   // Should be called immediately after calling |Insert|.
-  void Calc(nsCounterList* aList);
+  void Calc(nsCounterList* aList, bool aNotify);
 
   // The text that should be displayed for this counter.
   void GetText(nsString& aResult);
@@ -114,8 +121,7 @@ struct nsCounterChangeNode : public nsCounterNode {
   // |aPropIndex| is the index of the value within the list in the
   // 'counter-increment', 'counter-reset' or 'counter-set' property.
   nsCounterChangeNode(nsIFrame* aPseudoFrame, nsCounterNode::Type aChangeType,
-                      int32_t aChangeValue,
-                      int32_t aPropIndex)
+                      int32_t aChangeValue, int32_t aPropIndex)
       : nsCounterNode(  // Fake a content index for resets, increments and sets
                         // that comes before all the real content, with
                         // the resets first, in order, and then the increments
@@ -149,9 +155,9 @@ inline nsCounterChangeNode* nsCounterNode::ChangeNode() {
   return static_cast<nsCounterChangeNode*>(this);
 }
 
-inline void nsCounterNode::Calc(nsCounterList* aList) {
+inline void nsCounterNode::Calc(nsCounterList* aList, bool aNotify) {
   if (mType == USE)
-    UseNode()->Calc(aList);
+    UseNode()->Calc(aList, aNotify);
   else
     ChangeNode()->Calc(aList);
 }

layout/base/nsGenConList.h

@@ -31,8 +31,10 @@ struct nsGenConNode : public mozilla::LinkedListElement<nsGenConNode> {
   // and needed for similar cases for counters.
   const int32_t mContentIndex;
 
-  // null for 'content:no-open-quote', 'content:no-close-quote' and for
-  // counter nodes for increments and resets (rather than uses)
+  // null for:
+  //  * content: no-open-quote / content: no-close-quote
+  //  * counter nodes for increments and resets
+  //  * counter nodes for bullets (mPseudoFrame->IsBulletFrame()).
   RefPtr<nsTextNode> mText;
 
   explicit nsGenConNode(int32_t aContentIndex)
@@ -62,7 +64,13 @@ struct nsGenConNode : public mozilla::LinkedListElement<nsGenConNode> {
  protected:
   void CheckFrameAssertions() {
     NS_ASSERTION(
-        mContentIndex < int32_t(mPseudoFrame->StyleContent()->ContentCount()),
+        mContentIndex < int32_t(mPseudoFrame->StyleContent()->ContentCount()) ||
+            // Special-case for the use node created for the legacy markers,
+            // which don't use the content property.
+            (mPseudoFrame->IsBulletFrame() && mContentIndex == 0 &&
+             mPseudoFrame->Style()->GetPseudoType() ==
+                 mozilla::PseudoStyleType::marker &&
+             !mPseudoFrame->StyleContent()->ContentCount()),
         "index out of range");
     // We allow negative values of mContentIndex for 'counter-reset' and
     // 'counter-increment'.

layout/generic/nsBulletFrame.cpp

@@ -824,21 +824,6 @@ ImgDrawResult nsBulletFrame::PaintBullet(gfxContext& aRenderingContext,
                    aDisableSubpixelAA, this);
 }
 
-int32_t nsBulletFrame::Ordinal(bool aDebugFromA11y) const {
-  auto* fc = PresShell()->FrameConstructor();
-  auto* cm = fc->CounterManager();
-  auto* list = cm->CounterListFor(nsGkAtoms::list_item);
-  MOZ_ASSERT(aDebugFromA11y || (list && !list->IsDirty()));
-  nsIFrame* listItem = GetParent()->GetContent()->GetPrimaryFrame();
-  int32_t value = 0;
-  for (auto* node = list->First(); node; node = list->Next(node)) {
-    if (node->mPseudoFrame == listItem) {
-      value = node->mValueAfter;
-    }
-  }
-  return value;
-}
-
 void nsBulletFrame::GetListItemText(CounterStyle* aStyle,
                                     mozilla::WritingMode aWritingMode,
                                     int32_t aOrdinal, nsAString& aResult) {
@@ -1282,7 +1267,7 @@ void nsBulletFrame::GetSpokenText(nsAString& aText) {
       PresContext()->CounterStyleManager()->ResolveCounterStyle(
           StyleList()->mCounterStyle);
   bool isBullet;
-  style->GetSpokenCounterText(Ordinal(true), GetWritingMode(), aText, isBullet);
+  style->GetSpokenCounterText(Ordinal(), GetWritingMode(), aText, isBullet);
   if (isBullet) {
     if (!style->IsNone()) {
       aText.Append(' ');
@@ -1332,6 +1317,17 @@ void nsBulletFrame::DeregisterAndCancelImageRequest() {
   }
 }
 
+void nsBulletFrame::SetOrdinal(int32_t aOrdinal, bool aNotify) {
+  if (mOrdinal == aOrdinal) {
+    return;
+  }
+  mOrdinal = aOrdinal;
+  if (aNotify) {
+    PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange,
+                                  NS_FRAME_IS_DIRTY);
+  }
+}
+
 NS_IMPL_ISUPPORTS(nsBulletListener, imgINotificationObserver)
 
 nsBulletListener::nsBulletListener() : mFrame(nullptr) {}

layout/generic/nsBulletFrame.h

@@ -123,8 +123,8 @@ class nsBulletFrame final : public nsFrame {
   }
   void SetFontSizeInflation(float aInflation);
 
-  // aDebugFromA11y should not be used
-  int32_t Ordinal(bool aDebugFromA11y = false) const;
+  int32_t Ordinal() const { return mOrdinal; }
+  void SetOrdinal(int32_t aOrdinal, bool aNotify);
 
   already_AddRefed<imgIContainer> GetImage() const;
 
@@ -153,6 +153,9 @@ class nsBulletFrame final : public nsFrame {
   void RegisterImageRequest(bool aKnownToBeAnimated);
   void DeregisterAndCancelImageRequest();
 
+  // Requires being set via SetOrdinal.
+  int32_t mOrdinal = 0;
+
   // This is a boolean flag indicating whether or not the current image request
   // has been registered with the refresh driver.
   bool mRequestRegistered : 1;