Changeset 224034 in webkit

Timestamp:

Oct 26, 2017 11:51:33 AM (6 years ago)

Author:

Antti Koivisto

Message:

Remove unnecessary whitespace invalidation logic from RenderTreeUpdater
​https://bugs.webkit.org/show_bug.cgi?id=178786

Reviewed by Zalan Bujtas.

RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded is a somewhat complex
and confusing function for figuring out if some whitespace-only text node might need to have its
rendering status recomputed. However actually computing if a text renderer is needed is not expensive.
We can simply do it for all whitespace nodes after a sibling mutation.

This also removes a set that could have stale renderer pointers in it (they were never dereferenced).

• rendering/RenderBlock.cpp:

(WebCore::RenderBlock::addChildIgnoringContinuation):

Fix a display:contents bug exposed by this change. With display:contents a text renderer may have an anonymous
inline wrapper and we need to take it into account when the text renderer is the beforeChild.

Tested by imported/w3c/web-platform-tests/css/css-display-3/display-contents-state-change-001.html

• style/RenderTreeUpdater.cpp:

(WebCore::RenderTreeUpdater::updateRenderTree):

Call updateTextRenderer() for all whitespace-only text nodes after a change in siblings.
In normal update case it just figures out quickly (by calling textRendererIsNeeded)
that there are no changes and bails out.

(WebCore::RenderTreeUpdater::updateElementRenderer):
(WebCore::RenderTreeUpdater::updateTextRenderer):
(WebCore::RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded): Deleted.

No longer needed. Just mark that there have been changes to siblings instead.

• style/RenderTreeUpdater.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderBlock.cpp (modified) (1 diff)
• style/RenderTreeUpdater.cpp (modified) (7 diffs)
• style/RenderTreeUpdater.h (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-10-26  Antti Koivisto  <antti@apple.com>
+
+        Remove unnecessary whitespace invalidation logic from RenderTreeUpdater
+        https://bugs.webkit.org/show_bug.cgi?id=178786
+
+        Reviewed by Zalan Bujtas.
+
+        RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded is a somewhat complex
+        and confusing function for figuring out if some whitespace-only text node might need to have its
+        rendering status recomputed. However actually computing if a text renderer is needed is not expensive.
+        We can simply do it for all whitespace nodes after a sibling mutation.
+
+        This also removes a set that could have stale renderer pointers in it (they were never dereferenced).
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::addChildIgnoringContinuation):
+
+            Fix a display:contents bug exposed by this change. With display:contents a text renderer may have an anonymous
+            inline wrapper and we need to take it into account when the text renderer is the beforeChild.
+
+            Tested by imported/w3c/web-platform-tests/css/css-display-3/display-contents-state-change-001.html
+
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateRenderTree):
+
+            Call updateTextRenderer() for all whitespace-only text nodes after a change in siblings.
+            In normal update case it just figures out quickly (by calling textRendererIsNeeded)
+            that there are no changes and bails out.
+
+        (WebCore::RenderTreeUpdater::updateElementRenderer):
+        (WebCore::RenderTreeUpdater::updateTextRenderer):
+        (WebCore::RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded): Deleted.
+
+            No longer needed. Just mark that there have been changes to siblings instead.
+
+        * style/RenderTreeUpdater.h:
+
 2017-10-26  Myles C. Maxfield  <mmaxfield@apple.com>
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -555 +555 @@
 
         if (beforeChildContainer->isAnonymous()) {
+            if (beforeChildContainer->isInline()) {
+                ASSERT(RenderText::findByDisplayContentsInlineWrapperCandidate(*beforeChildContainer) == beforeChild);
+                addChild(WTFMove(newChild), beforeChildContainer);
+                return;
+            }
             // If the requested beforeChild is not one of our children, then this is because
             // there is an anonymous container within this object that contains the beforeChild.

trunk/Source/WebCore/style/RenderTreeUpdater.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -175 +175 @@
             auto& text = downcast<Text>(node);
             auto* textUpdate = m_styleUpdate->textUpdate(text);
-            if ((parent().updates && parent().updates->update.change == Style::Detach) || textUpdate || m_invalidatedWhitespaceOnlyTextSiblings.contains(&text))
+            bool didCreateParent = parent().updates && parent().updates->update.change == Style::Detach;
+            bool mayNeedUpdateWhitespaceOnlyRenderer = parent().didCreateOrDestroyChildRenderer && text.containsOnlyWhitespace();
+            if (didCreateParent || textUpdate || mayNeedUpdateWhitespaceOnlyRenderer)
                 updateTextRenderer(text, textUpdate);
 
@@ -212 +214 @@
 
     popParentsToDepth(0);
-
-    m_invalidatedWhitespaceOnlyTextSiblings.clear();
 }
 
@@ -314 +314 @@
         auto teardownType = update.style->display() == NONE ? TeardownType::RendererUpdateCancelingAnimations : TeardownType::RendererUpdate;
         tearDownRenderers(element, teardownType);
+
+        parent().didCreateOrDestroyChildRenderer = true;
     }
 
@@ -327 +329 @@
             element.willAttachRenderers();
         createRenderer(element, RenderStyle::clone(*update.style));
-        invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(element);
+
+        parent().didCreateOrDestroyChildRenderer = true;
         return;
     }
@@ -481 +484 @@
         }
         tearDownRenderer(text);
-        invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(text);
+        parent().didCreateOrDestroyChildRenderer = true;
         return;
     }
@@ -487 +490 @@
         return;
     createTextRenderer(text, renderTreePosition(), textUpdate);
-    invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(text);
-}
-
-void RenderTreeUpdater::invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(Node& current)
-{
-    // FIXME: This needs to traverse in composed tree order.
-
-    // This function finds sibling text renderers where the results of textRendererIsNeeded may have changed as a result of
-    // the current node gaining or losing the renderer. This can only affect white space text nodes.
-    for (Node* sibling = current.nextSibling(); sibling; sibling = sibling->nextSibling()) {
-        if (is<Element>(*sibling)) {
-            if (m_styleUpdate->elementUpdates(downcast<Element>(*sibling)))
-                return;
-            // Text renderers beyond rendered elements can't be affected.
-            if (sibling->renderer())
-                return;
-            continue;
-        }
-        if (!is<Text>(*sibling))
-            continue;
-        Text& textSibling = downcast<Text>(*sibling);
-        if (m_styleUpdate->textUpdate(textSibling))
-            return;
-        if (!textSibling.containsOnlyWhitespace())
-            continue;
-        m_invalidatedWhitespaceOnlyTextSiblings.add(&textSibling);
-    }
+    parent().didCreateOrDestroyChildRenderer = true;
 }
 

trunk/Source/WebCore/style/RenderTreeUpdater.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -62 +62 @@
     void updateElementRenderer(Element&, const Style::ElementUpdate&);
     void createRenderer(Element&, RenderStyle&&);
-    void invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(Node&);
     void updateBeforeDescendants(Element&, const Style::ElementUpdates*);
     void updateAfterDescendants(Element&, const Style::ElementUpdates*);
@@ -72 +71 @@
         const Style::ElementUpdates* updates { nullptr };
         std::optional<RenderTreePosition> renderTreePosition;
+
+        bool didCreateOrDestroyChildRenderer { false };
         RenderObject* previousChildRenderer { nullptr };
 
@@ -96 +97 @@
     Vector<Parent> m_parentStack;
 
-    HashSet<Text*> m_invalidatedWhitespaceOnlyTextSiblings;
-
     std::unique_ptr<GeneratedContent> m_generatedContent;
 };