Remove unnecessary whitespace invalidation logic from RenderTreeUpdater
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Oct 2017 18:51:33 +0000 (18:51 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Oct 2017 18:51:33 +0000 (18:51 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224034 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/RenderTreeUpdater.h

index bdf1337..ef2f442 100644 (file)
@@ -1,3 +1,40 @@
+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>
 
         Mark font palettes as in development
index 47bc17b..a34fb37 100644 (file)
@@ -554,6 +554,11 @@ void RenderBlock::addChildIgnoringContinuation(RenderPtr<RenderObject> newChild,
         ASSERT(beforeChildContainer);
 
         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.
             RenderElement* beforeChildAnonymousContainer = beforeChildContainer;
index fad7d51..3de720f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * 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
  * modification, are permitted provided that the following conditions
@@ -174,7 +174,9 @@ void RenderTreeUpdater::updateRenderTree(ContainerNode& root)
         if (is<Text>(node)) {
             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);
 
             storePreviousRenderer(text);
@@ -211,8 +213,6 @@ void RenderTreeUpdater::updateRenderTree(ContainerNode& root)
     }
 
     popParentsToDepth(0);
-
-    m_invalidatedWhitespaceOnlyTextSiblings.clear();
 }
 
 RenderTreePosition& RenderTreeUpdater::renderTreePosition()
@@ -313,6 +313,8 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::Ele
         // display:none cancels animations.
         auto teardownType = update.style->display() == NONE ? TeardownType::RendererUpdateCancelingAnimations : TeardownType::RendererUpdate;
         tearDownRenderers(element, teardownType);
+
+        parent().didCreateOrDestroyChildRenderer = true;
     }
 
     bool hasDisplayContents = update.style->display() == CONTENTS;
@@ -326,7 +328,8 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::Ele
         if (element.hasCustomStyleResolveCallbacks())
             element.willAttachRenderers();
         createRenderer(element, RenderStyle::clone(*update.style));
-        invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(element);
+
+        parent().didCreateOrDestroyChildRenderer = true;
         return;
     }
 
@@ -480,39 +483,13 @@ void RenderTreeUpdater::updateTextRenderer(Text& text, const Style::TextUpdate*
             return;
         }
         tearDownRenderer(text);
-        invalidateWhitespaceOnlyTextSiblingsAfterAttachIfNeeded(text);
+        parent().didCreateOrDestroyChildRenderer = true;
         return;
     }
     if (!needsRenderer)
         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;
 }
 
 void RenderTreeUpdater::storePreviousRenderer(Node& node)
index 0432a42..026355f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * 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
  * modification, are permitted provided that the following conditions
@@ -61,7 +61,6 @@ private:
     void updateTextRenderer(Text&, const Style::TextUpdate*);
     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*);
     bool textRendererIsNeeded(const Text& textNode);
@@ -71,6 +70,8 @@ private:
         Element* element { nullptr };
         const Style::ElementUpdates* updates { nullptr };
         std::optional<RenderTreePosition> renderTreePosition;
+
+        bool didCreateOrDestroyChildRenderer { false };
         RenderObject* previousChildRenderer { nullptr };
 
         Parent(ContainerNode& root);
@@ -95,8 +96,6 @@ private:
 
     Vector<Parent> m_parentStack;
 
-    HashSet<Text*> m_invalidatedWhitespaceOnlyTextSiblings;
-
     std::unique_ptr<GeneratedContent> m_generatedContent;
 };