Bug 1393098 part 0: Refactor logic (add helper bool & reduce GetParent calls), in nsTextFrame::CharacterDataChanged. r?jfkthame draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 31 Aug 2017 10:45:36 -0700
changeset 656786 7ff12753990e4ffd88c4d30e8e43d45fd5030520
parent 656783 4984da22242841a5d84c4e5fd866e93a450d9723
child 656787 3a59e1e8d9bd814b13c4be363a0ace95e3a07cdb
push id77318
push userdholbert@mozilla.com
push dateThu, 31 Aug 2017 17:45:48 +0000
reviewersjfkthame
bugs1393098
milestone57.0a1
Bug 1393098 part 0: Refactor logic (add helper bool & reduce GetParent calls), in nsTextFrame::CharacterDataChanged. r?jfkthame This patch doesn't affect behavior at all -- it just adjusts the logic slightly. Specifically, this patch: (a) Changes some code that currently tracks a frame, to now instead track that frame's parent, since we only ever call GetParent() on it anyway. (b) Drops a null-check that becomes unnecessary as a result of that change. (It was only there to protect us from calling GetParent() on a null pointer during the first loop iteration, and now that's not a risk since we're tracking the parent itself, and a null value will fail the equality comparison and do the right thing.) (c) Captures the "are ancestors already aware of a reflow request for my subtree" if-condition in a named boolean helper-variable. (d) Adds/improves documentation. MozReview-Commit-ID: 7dEflfiERYB
layout/generic/nsTextFrame.cpp
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -4856,34 +4856,50 @@ nsTextFrame::CharacterDataChanged(Charac
   while (true) {
     next = textFrame->GetNextContinuation();
     if (!next || next->GetContentOffset() > int32_t(aInfo->mChangeStart))
       break;
     textFrame = next;
   }
 
   int32_t endOfChangedText = aInfo->mChangeStart + aInfo->mReplaceLength;
-  nsTextFrame* lastDirtiedFrame = nullptr;
+
+  // Parent of the last frame that we passed to FrameNeedsReflow.
+  // (For subsequent frames with this same parent, we can just set their
+  // dirty bit without bothering to call FrameNeedsReflow again.)
+  nsIFrame* lastDirtiedFrameParent = nullptr;
 
   nsIPresShell* shell = PresContext()->GetPresShell();
   do {
     // textFrame contained deleted text (or the insertion point,
     // if this was a pure insertion).
     textFrame->mState &= ~TEXT_WHITESPACE_FLAGS;
     textFrame->ClearTextRuns();
-    if (!lastDirtiedFrame ||
-        lastDirtiedFrame->GetParent() != textFrame->GetParent()) {
+
+    nsIFrame* parentOfTextFrame = textFrame->GetParent();
+    bool areAncestorsAwareOfReflowRequest = false;
+    if (lastDirtiedFrameParent == parentOfTextFrame) {
+      // An earlier iteration of this loop already called
+      // FrameNeedsReflow for a sibling of |textFrame|.
+      areAncestorsAwareOfReflowRequest = true;
+    } else {
+      lastDirtiedFrameParent = parentOfTextFrame;
+    }
+
+    if (!areAncestorsAwareOfReflowRequest) {
       // Ask the parent frame to reflow me.
       shell->FrameNeedsReflow(textFrame, nsIPresShell::eStyleChange,
                               NS_FRAME_IS_DIRTY);
-      lastDirtiedFrame = textFrame;
     } else {
-      // if the parent is a block, we're cheating here because we should
-      // be marking our line dirty, but we're not. nsTextFrame::SetLength
-      // will do that when it gets called during reflow.
+      // We already called FrameNeedsReflow on behalf of an earlier sibling,
+      // so we can just mark this frame as dirty and don't need to bother
+      // telling its ancestors.
+      // Note: if the parent is a block, we're cheating here because we should
+      // be marking our line dirty, but we're not. nsTextFrame::SetLength will
+      // do that when it gets called during reflow.
       textFrame->AddStateBits(NS_FRAME_IS_DIRTY);
     }
     textFrame->InvalidateFrame();
 
     // Below, frames that start after the deleted text will be adjusted so that
     // their offsets move with the trailing unchanged text. If this change
     // deletes more text than it inserts, those frame offsets will decrease.
     // We need to maintain the invariant that mContentOffset is non-decreasing