Bug 1427625: Optimize appends to avoid restyling unnecessarily. r=xidorn

This unfortunately doesn't fix my test-case (because we're replacing the text
content all the time and all that), but it's still worth it, since it fixes the
case we care about (the parser appending).

We could also optimize pure insertions (since in that case we can still figure
out what the old text was), but it's probably annoying and not worth the churn.

In any case, we cannot optimize anything that resembles any kind of removal,
because from there we don't know the old text in any way (and the text nodes
like to reuse string buffers and such).

We could do two other optimizations to replace / extend this one, in that order:

 * Pass the buffer and length to CharacterDataWillChange, and use that to get
   the exact old text and the new one in RestyleManager. That would make the
   optimization exact.

 * Pass some sort of Maybe<bool> mWasWhitespace down the CharacterDataChangeInfo
   which is computed like:

    HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)
      ? Some(NS_TEXT_IS_ONLY_WHITESPACE)
      : Nothing()

It's not clear to me it's going to be completely worth the churn, so I haven't
done those yet, if we see code in the wild which resembles my testcase, we can
think of doing it.

MozReview-Commit-ID: 2rTWaZti8rv

--HG--
extra : rebase_source : 7390b8740801eb7b91700bb2533c43c173ac5db9

dom/base/nsGenericDOMDataNode.cpp

@@ -911,6 +911,10 @@ nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const
   if (mText.Is2b()) {
     // The fragment contains non-8bit characters and such characters
     // are never considered whitespace.
+    //
+    // FIXME(emilio): This is not quite true in presence of the
+    // NS_MAYBE_MODIFIED_FREQUENTLY flag... But looks like we only set that on
+    // anonymous nodes, so should be fine...
     return false;
   }
 
@@ -924,6 +928,8 @@ nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const
   while (cp < end) {
     char ch = *cp;
 
+    // NOTE(emilio): If you ever change the definition of "whitespace" here, you
+    // need to change it too in RestyleManager::CharacterDataChanged.
     if (!dom::IsSpaceCharacter(ch)) {
       return false;
     }

layout/base/PresShell.cpp

@@ -4298,20 +4298,7 @@ PresShell::CharacterDataChanged(nsIDocument* aDocument,
 
   nsAutoCauseReflowNotifier crNotifier(this);
 
-  // Call this here so it only happens for real content mutations and
-  // not cases when the frame constructor calls its own methods to force
-  // frame reconstruction.
-  nsIContent *container = aContent->GetParent();
-  uint32_t selectorFlags =
-    container ? (container->GetFlags() & NODE_ALL_SELECTOR_FLAGS) : 0;
-  if (selectorFlags != 0 && !aContent->IsRootOfAnonymousSubtree()) {
-    Element* element = container->AsElement();
-    if (aInfo.mAppend && !aContent->GetNextSibling())
-      mPresContext->RestyleManager()->RestyleForAppend(element, aContent);
-    else
-      mPresContext->RestyleManager()->RestyleForInsertOrChange(element, aContent);
-  }
-
+  mPresContext->RestyleManager()->CharacterDataChanged(aContent, aInfo);
   mFrameConstructor->CharacterDataChanged(aContent, aInfo);
   VERIFY_STYLE_TREE;
 }

layout/base/RestyleManager.cpp

@@ -21,6 +21,7 @@
 #include "nsStyleUtil.h"
 #include "StickyScrollContainer.h"
 #include "mozilla/EffectSet.h"
+#include "mozilla/IntegerRange.h"
 #include "mozilla/ViewportFrame.h"
 #include "SVGObserverUtils.h"
 #include "SVGTextFrame.h"
@@ -52,27 +53,6 @@ RestyleManager::ContentInserted(nsINode* aContainer, nsIContent* aChild)
 
 void
 RestyleManager::ContentAppended(nsIContent* aContainer, nsIContent* aFirstNewContent)
-{
-  RestyleForAppend(aContainer, aFirstNewContent);
-}
-
-void
-RestyleManager::RestyleForEmptyChange(Element* aContainer)
-{
-  // In some cases (:empty + E, :empty ~ E), a change in the content of
-  // an element requires restyling its parent's siblings.
-  nsRestyleHint hint = eRestyle_Subtree;
-  nsIContent* grandparent = aContainer->GetParent();
-  if (grandparent &&
-      (grandparent->GetFlags() & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS)) {
-    hint = nsRestyleHint(hint | eRestyle_LaterSiblings);
-  }
-  PostRestyleEvent(aContainer, hint, nsChangeHint(0));
-}
-
-void
-RestyleManager::RestyleForAppend(nsIContent* aContainer,
-                                 nsIContent* aFirstNewContent)
 {
   // The container cannot be a document, but might be a ShadowRoot.
   if (!aContainer->IsElement()) {
@@ -134,6 +114,62 @@ RestyleManager::RestyleForAppend(nsIContent* aContainer,
   }
 }
 
+void
+RestyleManager::RestyleForEmptyChange(Element* aContainer)
+{
+  // In some cases (:empty + E, :empty ~ E), a change in the content of
+  // an element requires restyling its parent's siblings.
+  nsRestyleHint hint = eRestyle_Subtree;
+  nsIContent* grandparent = aContainer->GetParent();
+  if (grandparent &&
+      (grandparent->GetFlags() & NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS)) {
+    hint = nsRestyleHint(hint | eRestyle_LaterSiblings);
+  }
+  PostRestyleEvent(aContainer, hint, nsChangeHint(0));
+}
+
+void
+RestyleManager::MaybeRestyleForEdgeChildChange(Element* aContainer,
+                                               nsIContent* aChangedChild)
+{
+  MOZ_ASSERT(aContainer->GetFlags() & NODE_HAS_EDGE_CHILD_SELECTOR);
+  MOZ_ASSERT(aChangedChild->GetParent() == aContainer);
+  // restyle the previously-first element child if it is after this node
+  bool passedChild = false;
+  for (nsIContent* content = aContainer->GetFirstChild();
+       content;
+       content = content->GetNextSibling()) {
+    if (content == aChangedChild) {
+      passedChild = true;
+      continue;
+    }
+    if (content->IsElement()) {
+      if (passedChild) {
+        PostRestyleEvent(content->AsElement(), eRestyle_Subtree,
+                         nsChangeHint(0));
+      }
+      break;
+    }
+  }
+  // restyle the previously-last element child if it is before this node
+  passedChild = false;
+  for (nsIContent* content = aContainer->GetLastChild();
+       content;
+       content = content->GetPreviousSibling()) {
+    if (content == aChangedChild) {
+      passedChild = true;
+      continue;
+    }
+    if (content->IsElement()) {
+      if (passedChild) {
+        PostRestyleEvent(content->AsElement(), eRestyle_Subtree,
+                         nsChangeHint(0));
+      }
+      break;
+    }
+  }
+}
+
 // Needed since we can't use PostRestyleEvent on non-elements (with
 // eRestyle_LaterSiblings or nsRestyleHint(eRestyle_Subtree |
 // eRestyle_LaterSiblings) as appropriate).
@@ -153,6 +189,132 @@ RestyleSiblingsStartingWith(RestyleManager* aRestyleManager,
   }
 }
 
+template<typename CharT>
+bool
+WhitespaceOnly(const CharT* aBuffer, size_t aUpTo)
+{
+  for (auto index : IntegerRange(aUpTo)) {
+    if (!dom::IsSpaceCharacter(aBuffer[index])) {
+      return false;
+    }
+  }
+  return true;
+}
+
+template<typename CharT>
+bool
+WhitespaceOnlyChangedOnAppend(const CharT* aBuffer,
+                              size_t aOldLength,
+                              size_t aNewLength)
+{
+  MOZ_ASSERT(aOldLength < aNewLength);
+  if (!WhitespaceOnly(aBuffer, aOldLength)) {
+    // The old text was already not whitespace-only.
+    return false;
+  }
+
+  return !WhitespaceOnly(aBuffer + aOldLength, aNewLength - aOldLength);
+}
+
+static bool
+HasAnySignificantSibling(Element* aContainer, nsIContent* aChild)
+{
+  MOZ_ASSERT(aChild->GetParent() == aContainer);
+  for (nsIContent* child = aContainer->GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    if (child == aChild) {
+      continue;
+    }
+    // We don't know whether we're testing :empty or :-moz-only-whitespace,
+    // so be conservative and assume :-moz-only-whitespace (i.e., make
+    // IsSignificantChild less likely to be true, and thus make us more
+    // likely to restyle).
+    if (nsStyleUtil::IsSignificantChild(child, true, false)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+void
+RestyleManager::CharacterDataChanged(nsIContent* aContent,
+                                     const CharacterDataChangeInfo& aInfo)
+{
+  nsINode* parent = aContent->GetParentNode();
+  MOZ_ASSERT(parent, "How were we notified of a stray node?");
+
+  uint32_t slowSelectorFlags = parent->GetFlags() & NODE_ALL_SELECTOR_FLAGS;
+  if (!(slowSelectorFlags & (NODE_HAS_EMPTY_SELECTOR |
+                             NODE_HAS_EDGE_CHILD_SELECTOR))) {
+    // Nothing to do, no other slow selector can change as a result of this.
+    return;
+  }
+
+  if (!aContent->IsNodeOfType(nsINode::eTEXT)) {
+    // Doesn't matter to styling (could be a processing instruction or a
+    // comment), it can't change whether any selectors match or don't.
+    return;
+  }
+
+
+  if (MOZ_UNLIKELY(!parent->IsElement())) {
+    MOZ_ASSERT(parent->IsShadowRoot());
+    return;
+  }
+
+  if (MOZ_UNLIKELY(aContent->IsRootOfAnonymousSubtree())) {
+    // This is an anonymous node and thus isn't in child lists, so isn't taken
+    // into account for selector matching the relevant selectors here.
+    return;
+  }
+
+  // Handle appends specially since they're common and we can know both the old
+  // and the new text exactly.
+  //
+  // TODO(emilio): This could be made much more general if :-moz-only-whitespace
+  // / :-moz-first-node and :-moz-last-node didn't exist. In that case we only
+  // need to know whether we went from empty to non-empty, and that's trivial to
+  // know, with CharacterDataChangeInfo...
+  if (!aInfo.mAppend) {
+    // FIXME(emilio): This restyles unnecessarily if the text node is the only
+    // child of the parent element. Fortunately, it's uncommon to have such
+    // nodes and this not being an append.
+    //
+    // See the testcase in bug 1427625 for a test-case that triggers this.
+    RestyleForInsertOrChange(parent->AsElement(), aContent);
+    return;
+  }
+
+  const nsTextFragment* text = aContent->GetText();
+
+  const size_t oldLength = aInfo.mChangeStart;
+  const size_t newLength = text->GetLength();
+
+  const bool emptyChanged = !oldLength && newLength;
+
+  const bool whitespaceOnlyChanged = text->Is2b()
+    ? WhitespaceOnlyChangedOnAppend(text->Get2b(), oldLength, newLength)
+    : WhitespaceOnlyChangedOnAppend(text->Get1b(), oldLength, newLength);
+
+  if (!emptyChanged && !whitespaceOnlyChanged) {
+    return;
+  }
+
+  if (slowSelectorFlags & NODE_HAS_EMPTY_SELECTOR) {
+    if (!HasAnySignificantSibling(parent->AsElement(), aContent)) {
+      // We used to be empty, restyle the parent.
+      RestyleForEmptyChange(parent->AsElement());
+      return;
+    }
+  }
+
+  if (slowSelectorFlags & NODE_HAS_EDGE_CHILD_SELECTOR) {
+    MaybeRestyleForEdgeChildChange(parent->AsElement(), aContent);
+  }
+}
+
 // Restyling for a ContentInserted or CharacterDataChanged notification.
 // This could be used for ContentRemoved as well if we got the
 // notification before the removal happened (and sometimes
@@ -176,23 +338,13 @@ RestyleManager::RestyleForInsertOrChange(nsINode* aContainer,
     return;
 
   if (selectorFlags & NODE_HAS_EMPTY_SELECTOR) {
-    // see whether we need to restyle the container
-    bool wasEmpty = true; // :empty or :-moz-only-whitespace
-    for (nsIContent* child = container->GetFirstChild();
-         child;
-         child = child->GetNextSibling()) {
-      if (child == aChild)
-        continue;
-      // We don't know whether we're testing :empty or :-moz-only-whitespace,
-      // so be conservative and assume :-moz-only-whitespace (i.e., make
-      // IsSignificantChild less likely to be true, and thus make us more
-      // likely to restyle).
-      if (nsStyleUtil::IsSignificantChild(child, true, false)) {
-        wasEmpty = false;
-        break;
-      }
-    }
+    // See whether we need to restyle the container due to :empty /
+    // :-moz-only-whitespace.
+    const bool wasEmpty = !HasAnySignificantSibling(container, aChild);
     if (wasEmpty) {
+      // FIXME(emilio): When coming from CharacterDataChanged this can restyle
+      // unnecessarily. Also can restyle unnecessarily if aChild is not
+      // significant anyway, though that's more unlikely.
       RestyleForEmptyChange(container);
       return;
     }
@@ -210,40 +362,7 @@ RestyleManager::RestyleForInsertOrChange(nsINode* aContainer,
   }
 
   if (selectorFlags & NODE_HAS_EDGE_CHILD_SELECTOR) {
-    // restyle the previously-first element child if it is after this node
-    bool passedChild = false;
-    for (nsIContent* content = container->GetFirstChild();
-         content;
-         content = content->GetNextSibling()) {
-      if (content == aChild) {
-        passedChild = true;
-        continue;
-      }
-      if (content->IsElement()) {
-        if (passedChild) {
-          PostRestyleEvent(content->AsElement(), eRestyle_Subtree,
-                           nsChangeHint(0));
-        }
-        break;
-      }
-    }
-    // restyle the previously-last element child if it is before this node
-    passedChild = false;
-    for (nsIContent* content = container->GetLastChild();
-         content;
-         content = content->GetPreviousSibling()) {
-      if (content == aChild) {
-        passedChild = true;
-        continue;
-      }
-      if (content->IsElement()) {
-        if (passedChild) {
-          PostRestyleEvent(content->AsElement(), eRestyle_Subtree,
-                           nsChangeHint(0));
-        }
-        break;
-      }
-    }
+    MaybeRestyleForEdgeChildChange(container, aChild);
   }
 }
 

layout/base/RestyleManager.h

@@ -159,14 +159,13 @@ public:
                       nsIContent* aFollowingSibling);
 
   // Restyling for a ContentInserted (notification after insertion) or
-  // for a CharacterDataChanged.  |aContainer| must be non-null; when
+  // for some CharacterDataChanged.  |aContainer| must be non-null; when
   // the container is null, no work is needed.
   void RestyleForInsertOrChange(nsINode* aContainer, nsIContent* aChild);
 
-  // Restyling for a ContentAppended (notification after insertion) or
-  // for a CharacterDataChanged.  |aContainer| must be non-null; when
-  // the container is null, no work is needed.
-  void RestyleForAppend(nsIContent* aContainer, nsIContent* aFirstNewContent);
+  // Restyle for a CharacterDataChanged notification. In practice this can only
+  // affect :empty / :-moz-only-whitespace / :-moz-first-node / :-moz-last-node.
+  void CharacterDataChanged(nsIContent*, const CharacterDataChangeInfo&);
 
   MOZ_DECL_STYLO_METHODS(GeckoRestyleManager, ServoRestyleManager)
 
@@ -223,6 +222,7 @@ protected:
   }
 
   void RestyleForEmptyChange(Element* aContainer);
+  void MaybeRestyleForEdgeChildChange(Element* aContainer, nsIContent* aChangedChild);
 
   void ContentStateChangedInternal(Element* aElement,
                                    EventStates aStateMask,