Changeset 273621 in webkit

Timestamp:

Feb 27, 2021, 12:24:02 AM (6 years ago)

Author:

Antti Koivisto

Message:

Render tree updates for Text node content mutations should happen during rendering update
​https://bugs.webkit.org/show_bug.cgi?id=222406

Reviewed by Simon Fraser.

Source/WebCore:

Calls to Text.insertData and similar should not mutate render tree synchronously.
Instead render tree should be updated during the next rendering update along with
any style changes.

These updates already go via RenderTreeUpdater. We just need to save the information
about which nodes need updating so the next rendering update can pick them up.

This seems to help with some performance benchmarks.

• dom/CharacterData.cpp:

(WebCore::CharacterData::parserAppendData):
(WebCore::CharacterData::setDataAndUpdate):

Move in-tree check to the updateRendererAfterContentChange and make it use isConnected().

• dom/Document.cpp:

(WebCore::Document::removedLastRef):
(WebCore::Document::resolveStyle):

Include the text update when updating the render tree.

(WebCore::Document::updateTextRenderer):

Create a text update that will get flushed during the next rendering update.

(WebCore::Document::needsStyleRecalc const):

We need to recalc if there are pending text updates.

• dom/Document.h:
• dom/Text.cpp:

(WebCore::Text::splitText):

Use updateRendererAfterContentChange instead of poking render tree directly.

(WebCore::Text::updateRendererAfterContentChange):

• rendering/updating/RenderTreeUpdater.cpp:
• style/StyleTreeResolver.cpp:

(WebCore::Style::TreeResolver::TreeResolver):
(WebCore::Style::TreeResolver::resolve):

• style/StyleTreeResolver.h:

(WebCore::Style::TreeResolver::TreeResolver):

• style/StyleUpdate.cpp:

(WebCore::Style::Update::addText):

Merge text updates.

(WebCore::Style::Update::addPossibleRoot):

• style/StyleUpdate.h:

(WebCore::Style::Update::roots const):
(WebCore::Style::Update:: const): Deleted.

Refcount the nodes since this now has longer lifetime.

• editing/CompositeEditCommand.cpp:

(WebCore::CompositeEditCommand::deleteInsignificantText):

Ensure we leave deleteInsignificantText with updated render tree. Clients expect that.
Do layout (instead of just style update) for consistency, deleteInsignificantText does one anyway in beginning.

• editing/markup.cpp:

(WebCore::replaceChildrenWithFragment):

Pending text update may ref the node so this refcount assert is not correct.

• style/StyleUpdate.cpp:

(WebCore::Style::Update::addText):

LayoutTests:

These are progressions.

• fast/text/splitText-dirty-lines-expected.txt:
• imported/blink/fast/css/first-letter-range-insert-expected.txt:

Here we were actually drawing text that didn't exist in DOM anymore.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/splitText-dirty-lines-expected.txt (modified) (1 diff)
• LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt (modified) (1 diff)
• LayoutTests/imported/blink/fast/css/first-letter-range-insert-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/CharacterData.cpp (modified) (2 diffs)
• Source/WebCore/dom/Document.cpp (modified) (4 diffs)
• Source/WebCore/dom/Document.h (modified) (1 diff)
• Source/WebCore/dom/Text.cpp (modified) (2 diffs)
• Source/WebCore/editing/CompositeEditCommand.cpp (modified) (1 diff)
• Source/WebCore/editing/markup.cpp (modified) (1 diff)
• Source/WebCore/rendering/updating/RenderTreeUpdater.cpp (modified) (1 diff)
• Source/WebCore/style/StyleTreeResolver.cpp (modified) (2 diffs)
• Source/WebCore/style/StyleTreeResolver.h (modified) (1 diff)
• Source/WebCore/style/StyleUpdate.cpp (modified) (2 diffs)
• Source/WebCore/style/StyleUpdate.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-02-27  Antti Koivisto  <antti@apple.com>
+
+        Render tree updates for Text node content mutations should happen during rendering update
+        https://bugs.webkit.org/show_bug.cgi?id=222406
+
+        Reviewed by Simon Fraser.
+
+        These are progressions.
+
+        * fast/text/splitText-dirty-lines-expected.txt:
+        * imported/blink/fast/css/first-letter-range-insert-expected.txt:
+
+        Here we were actually drawing text that didn't exist in DOM anymore.
+
 2021-02-26  Peng Liu  <peng.liu6@apple.com>
 

trunk/LayoutTests/fast/text/splitText-dirty-lines-expected.txt

@@ -16 +16 @@
 layer at (8,8) size 200x160
   RenderBlock (positioned) {DIV} at (0,0) size 200x160 [color=#008000]
-    RenderText {#text} at (0,0) size 0x0
     RenderText {#text} at (0,0) size 160x160
       text run at (0,0) width 160: "AAAA"

trunk/LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt

@@ -1 +1 @@
 layer at (0,0) size 800x600
   RenderView at (0,0) size 800x600
-layer at (0,0) size 800x76
-  RenderBlock {HTML} at (0,0) size 800x76
-    RenderBody {BODY} at (8,8) size 784x60
-      RenderBlock {DIV} at (0,0) size 20x40
-        RenderCombineText {#text} at (0,0) size 0x0
-        RenderInline {SPAN} at (0,0) size 20x0
-        RenderCombineText {#text} at (0,20) size 20x20
-          text run at (0,20) width 20: "\x{FFFC}"
-      RenderBlock (anonymous) at (0,40) size 784x0
+layer at (0,0) size 800x56
+  RenderBlock {HTML} at (0,0) size 800x56
+    RenderBody {BODY} at (8,8) size 784x40
+      RenderBlock {DIV} at (0,0) size 20x20
+        RenderInline {SPAN} at (0,0) size 0x0
+        RenderCombineText {#text} at (0,0) size 20x20
+          text run at (0,0) width 20: "\x{FFFC}"
+      RenderBlock (anonymous) at (0,20) size 784x0
         RenderText {#text} at (0,0) size 0x0
         RenderText {#text} at (0,0) size 0x0
-      RenderBlock {DIV} at (0,40) size 784x20
+      RenderBlock {DIV} at (0,20) size 784x20
         RenderText {#text} at (0,0) size 400x20
           text run at (0,0) width 400: "PASS, did not crash."

trunk/LayoutTests/imported/blink/fast/css/first-letter-range-insert-expected.txt

@@ -1 @@
-B
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-02-27  Antti Koivisto  <antti@apple.com>
+
+        Render tree updates for Text node content mutations should happen during rendering update
+        https://bugs.webkit.org/show_bug.cgi?id=222406
+
+        Reviewed by Simon Fraser.
+
+        Calls to Text.insertData and similar should not mutate render tree synchronously.
+        Instead render tree should be updated during the next rendering update along with
+        any style changes.
+
+        These updates already go via RenderTreeUpdater. We just need to save the information
+        about which nodes need updating so the next rendering update can pick them up.
+
+        This seems to help with some performance benchmarks.
+
+        * dom/CharacterData.cpp:
+        (WebCore::CharacterData::parserAppendData):
+        (WebCore::CharacterData::setDataAndUpdate):
+
+        Move in-tree check to the updateRendererAfterContentChange and make it use isConnected().
+
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef):
+        (WebCore::Document::resolveStyle):
+
+        Include the text update when updating the render tree.
+
+        (WebCore::Document::updateTextRenderer):
+
+        Create a text update that will get flushed during the next rendering update.
+
+        (WebCore::Document::needsStyleRecalc const):
+
+        We need to recalc if there are pending text updates.
+
+        * dom/Document.h:
+        * dom/Text.cpp:
+        (WebCore::Text::splitText):
+
+        Use updateRendererAfterContentChange instead of poking render tree directly.
+
+        (WebCore::Text::updateRendererAfterContentChange):
+        * rendering/updating/RenderTreeUpdater.cpp:
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::TreeResolver):
+        (WebCore::Style::TreeResolver::resolve):
+        * style/StyleTreeResolver.h:
+        (WebCore::Style::TreeResolver::TreeResolver):
+        * style/StyleUpdate.cpp:
+        (WebCore::Style::Update::addText):
+
+        Merge text updates.
+
+        (WebCore::Style::Update::addPossibleRoot):
+        * style/StyleUpdate.h:
+        (WebCore::Style::Update::roots const):
+        (WebCore::Style::Update:: const): Deleted.
+
+        Refcount the nodes since this now has longer lifetime.
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::deleteInsignificantText):
+
+        Ensure we leave deleteInsignificantText with updated render tree. Clients expect that.
+        Do layout (instead of just style update) for consistency, deleteInsignificantText does one anyway in beginning.
+
+        * editing/markup.cpp:
+        (WebCore::replaceChildrenWithFragment):
+
+        Pending text update may ref the node so this refcount assert is not correct.
+
+        * style/StyleUpdate.cpp:
+        (WebCore::Style::Update::addText):
+
 2021-02-27  Megan Gardner  <megan_gardner@apple.com>
 

trunk/Source/WebCore/dom/CharacterData.cpp

@@ -103 +103 @@
 
     ASSERT(!renderer() || is<Text>(*this));
-    if (is<Text>(*this) && parentNode())
+    if (is<Text>(*this))
         downcast<Text>(*this).updateRendererAfterContentChange(oldLength, 0);
 
@@ -183 +183 @@
 
     ASSERT(!renderer() || is<Text>(*this));
-    if (is<Text>(*this) && parentNode())
+    if (is<Text>(*this))
         downcast<Text>(*this).updateRendererAfterContentChange(offsetOfReplacedData, oldLength);
 

trunk/Source/WebCore/dom/Document.cpp

@@ -808 +808 @@
 #endif
         m_associatedFormControls.clear();
+        m_pendingRenderTreeTextUpdate = { };
 
         m_fontSelector->stopLoadingAndClearFonts();
@@ -2053 +2054 @@
         }
 
-        Style::TreeResolver resolver(*this);
+        Style::TreeResolver resolver(*this, WTFMove(m_pendingRenderTreeTextUpdate));
         auto styleUpdate = resolver.resolve();
 
@@ -2101 +2102 @@
 void Document::updateTextRenderer(Text& text, unsigned offsetOfReplacedText, unsigned lengthOfReplacedText)
 {
-    auto textUpdate = makeUnique<Style::Update>(*this);
-    textUpdate->addText(text, { offsetOfReplacedText, lengthOfReplacedText, WTF::nullopt });
-
-    updateRenderTree(WTFMove(textUpdate));
+    if (!m_pendingRenderTreeTextUpdate)
+        m_pendingRenderTreeTextUpdate = makeUnique<Style::Update>(*this);
+
+    m_pendingRenderTreeTextUpdate->addText(text, { offsetOfReplacedText, lengthOfReplacedText, WTF::nullopt });
+
+    scheduleRenderingUpdate({ });
 }
 
@@ -2116 +2119 @@
 
     if (childNeedsStyleRecalc())
+        return true;
+
+    if (m_pendingRenderTreeTextUpdate)
         return true;
 

trunk/Source/WebCore/dom/Document.h

@@ -1804 +1804 @@
     Timer m_styleRecalcTimer;
 
+    std::unique_ptr<Style::Update> m_pendingRenderTreeTextUpdate;
+
     Element* m_cssTarget { nullptr };
 

trunk/Source/WebCore/dom/Text.cpp

@@ -77 +77 @@
     document().textNodeSplit(*this);
 
-    if (renderer())
-        renderer()->setTextWithOffset(data(), 0, oldData.length());
+    updateRendererAfterContentChange(0, oldData.length());
 
     return newText;
@@ -216 +215 @@
 void Text::updateRendererAfterContentChange(unsigned offsetOfReplacedData, unsigned lengthOfReplacedData)
 {
-    ASSERT(parentNode());
+    if (!isConnected())
+        return;
+
     if (styleValidity() >= Style::Validity::SubtreeAndRenderersInvalid)
         return;

trunk/Source/WebCore/editing/CompositeEditCommand.cpp

@@ -1059 +1059 @@
         deleteInsignificantText(textNode, startOffset, endOffset);
     }
+    if (!nodes.isEmpty()) {
+        // Callers expect render tree to be in sync.
+        document().updateLayoutIgnorePendingStylesheets();
+    }
 }
 

trunk/Source/WebCore/editing/markup.cpp

@@ -1386 +1386 @@
     if (containerChild && !containerChild->nextSibling()) {
         if (is<Text>(*containerChild) && hasOneTextChild(fragment) && canUseSetDataOptimization(downcast<Text>(*containerChild), mutation)) {
-            ASSERT(!fragment->firstChild()->refCount());
             downcast<Text>(*containerChild).setData(downcast<Text>(*fragment->firstChild()).data());
             return { };

trunk/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp

@@ -103 +103 @@
 {
     ListHashSet<ContainerNode*> renderingRoots;
-    for (auto* root : update.roots()) {
+    for (auto& root : update.roots()) {
         auto* renderingRoot = findRenderingRoot(*root);
         if (!renderingRoot)

trunk/Source/WebCore/style/StyleTreeResolver.cpp

@@ -63 +63 @@
 DEFINE_ALLOCATOR_WITH_HEAP_IDENTIFIER(TreeResolverScope);
 
-TreeResolver::TreeResolver(Document& document)
+TreeResolver::TreeResolver(Document& document, std::unique_ptr<Update> update)
     : m_document(document)
+    , m_update(WTFMove(update))
 {
 }
@@ -577 +578 @@
     }
     if (!documentElement->childNeedsStyleRecalc() && !documentElement->needsStyleRecalc())
-        return nullptr;
+        return WTFMove(m_update);
 
     m_didSeePendingStylesheet = m_document.styleScope().hasPendingSheetsBeforeBody();
 
-    m_update = makeUnique<Update>(m_document);
+    if (!m_update)
+        m_update = makeUnique<Update>(m_document);
     m_scopeStack.append(adoptRef(*new Scope(m_document)));
     m_parentStack.append(Parent(m_document));

trunk/Source/WebCore/style/StyleTreeResolver.h

@@ -50 +50 @@
 class TreeResolver {
 public:
-    TreeResolver(Document&);
+    TreeResolver(Document&, std::unique_ptr<Update> = { });
     ~TreeResolver();
 

trunk/Source/WebCore/style/StyleUpdate.cpp

@@ -97 +97 @@
 void Update::addText(Text& text, Element* parent, TextUpdate&& textUpdate)
 {
-    ASSERT(!m_texts.contains(&text));
     ASSERT(composedTreeAncestors(text).first() == parent);
 
     addPossibleRoot(parent);
-    m_texts.add(&text, WTFMove(textUpdate));
+    auto result = m_texts.ensure(&text, [&] {
+        return WTFMove(textUpdate);
+    });
+    if (!result.isNewEntry) {
+        auto& entry = result.iterator->value;
+        auto startOffset = std::min(entry.offset, textUpdate.offset);
+        auto endOffset = std::max(entry.offset + entry.length, textUpdate.offset + textUpdate.length);
+        entry.offset = startOffset;
+        entry.length = endOffset - startOffset;
+        
+        ASSERT(!entry.inheritedDisplayContentsStyle || !textUpdate.inheritedDisplayContentsStyle);
+        if (!entry.inheritedDisplayContentsStyle)
+            entry.inheritedDisplayContentsStyle = WTFMove(textUpdate.inheritedDisplayContentsStyle);
+    }
 }
 
@@ -112 +124 @@
 {
     if (!element) {
-        m_roots.add(&m_document);
+        m_roots.add(m_document.ptr());
         return;
     }

trunk/Source/WebCore/style/StyleUpdate.h

@@ -70 +70 @@
     Update(Document&);
 
-    const ListHashSet<ContainerNode*>& roots() const { return m_roots; }
+    const ListHashSet<RefPtr<ContainerNode>>& roots() const { return m_roots; }
 
     const ElementUpdates* elementUpdates(const Element&) const;
@@ -91 +91 @@
     void addPossibleRoot(Element*);
 
-    Document& m_document;
-    ListHashSet<ContainerNode*> m_roots;
-    HashMap<const Element*, ElementUpdates> m_elements;
-    HashMap<const Text*, TextUpdate> m_texts;
+    Ref<Document> m_document;
+    ListHashSet<RefPtr<ContainerNode>> m_roots;
+    HashMap<RefPtr<const Element>, ElementUpdates> m_elements;
+    HashMap<RefPtr<const Text>, TextUpdate> m_texts;
 };