Remove redundant inline text boxes for empty combined text
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Sep 2018 17:02:33 +0000 (17:02 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Sep 2018 17:02:33 +0000 (17:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189119

Reviewed by Zalan Bujtas.

Source/WebCore:

We should consider inline text boxes that have a combined text renderer (RenderCombineText)
whose composed string is empty as "redundant" just as we do for inline text boxes that have
a non-combined text renderer that have zero length so that we remove them. Such boxes are
visibly empty and do not take up space visually. By removing them we reduce memory and make
it easier to reason about the line box tree.

Currently RenderBlockFlow::computeBlockDirectionPositionsForLine() tests if an inline text
box is empty by checking if it has a zero length (InlineTextBox::len()). However an inline
text box associated with a RenderCombineText always has length 1 regardless of whether the
composed string it represents is the empty string. Instead we should expose a way to check
if an inline text box is visually empty and have RenderBlockFlow::computeBlockDirectionPositionsForLine()
query the inline text box for this answer.

* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::hasTextContent const): Added. Returns whether an inline text box
has text content. We do not need to consider hypenation since hypens are an embellishment (i.e.
they are not part of the markup of the page).
(WebCore::InlineTextBox::paint): Write in terms of hasTextContent().
(WebCore::InlineTextBox::subdivideAndResolveStyle): Assert that WebCore::subdivide() always
returns a non-empty list of subdivisions. A non-empty text box should always have at least
one subdivision, say for the unmarked text. I left the existing conditonal (though marked
it as UNLIKELY()) so as to be forgiving and avoid a bad user experience should WebCore::subdivide()
return an empty vector in a non-debug build.
* rendering/InlineTextBox.h:
* rendering/RenderBlockLineLayout.cpp:
(WebCore::RenderBlockFlow::computeBlockDirectionPositionsForLine): Write in terms of InlineTextBox::hasTextContent()
so that we remove empty inline text boxes associated with combined text.
* rendering/RenderText.cpp:
(WebCore::RenderText::positionLineBox): Write in terms of InlineTextBox::hasTextContent().

LayoutTests:

Update expected result now that we do not create an inline text box associated with combined text
when we do not have any combined text to render.

* fast/text/text-combine-surroundContents-crash-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/InlineTextBox.h
Source/WebCore/rendering/RenderBlockLineLayout.cpp
Source/WebCore/rendering/RenderText.cpp

index 0d7fe72..78d1e12 100644 (file)
@@ -1,3 +1,15 @@
+2018-09-04  Daniel Bates  <dabates@apple.com>
+
+        Remove redundant inline text boxes for empty combined text
+        https://bugs.webkit.org/show_bug.cgi?id=189119
+
+        Reviewed by Zalan Bujtas.
+
+        Update expected result now that we do not create an inline text box associated with combined text
+        when we do not have any combined text to render.
+
+        * fast/text/text-combine-surroundContents-crash-expected.txt:
+
 2018-09-04  Zan Dobersek  <zdobersek@igalia.com> and Ms2ger  <Ms2ger@igalia.com>
 
         Implement support for passing ImageBitmap to texImage2D/texSubImage2D
index 8fcf441..53f12c6 100644 (file)
@@ -4,8 +4,7 @@ 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 20x20
-          text run at (0,0) width 20: "\x{FFFC}"
+        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}"
index d63bca0..7d7eb70 100644 (file)
@@ -1,3 +1,40 @@
+2018-09-04  Daniel Bates  <dabates@apple.com>
+
+        Remove redundant inline text boxes for empty combined text
+        https://bugs.webkit.org/show_bug.cgi?id=189119
+
+        Reviewed by Zalan Bujtas.
+
+        We should consider inline text boxes that have a combined text renderer (RenderCombineText)
+        whose composed string is empty as "redundant" just as we do for inline text boxes that have
+        a non-combined text renderer that have zero length so that we remove them. Such boxes are
+        visibly empty and do not take up space visually. By removing them we reduce memory and make
+        it easier to reason about the line box tree.
+
+        Currently RenderBlockFlow::computeBlockDirectionPositionsForLine() tests if an inline text
+        box is empty by checking if it has a zero length (InlineTextBox::len()). However an inline
+        text box associated with a RenderCombineText always has length 1 regardless of whether the
+        composed string it represents is the empty string. Instead we should expose a way to check
+        if an inline text box is visually empty and have RenderBlockFlow::computeBlockDirectionPositionsForLine()
+        query the inline text box for this answer.
+
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::hasTextContent const): Added. Returns whether an inline text box
+        has text content. We do not need to consider hypenation since hypens are an embellishment (i.e.
+        they are not part of the markup of the page).
+        (WebCore::InlineTextBox::paint): Write in terms of hasTextContent().
+        (WebCore::InlineTextBox::subdivideAndResolveStyle): Assert that WebCore::subdivide() always
+        returns a non-empty list of subdivisions. A non-empty text box should always have at least
+        one subdivision, say for the unmarked text. I left the existing conditonal (though marked
+        it as UNLIKELY()) so as to be forgiving and avoid a bad user experience should WebCore::subdivide()
+        return an empty vector in a non-debug build.
+        * rendering/InlineTextBox.h:
+        * rendering/RenderBlockLineLayout.cpp:
+        (WebCore::RenderBlockFlow::computeBlockDirectionPositionsForLine): Write in terms of InlineTextBox::hasTextContent()
+        so that we remove empty inline text boxes associated with combined text.
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::positionLineBox): Write in terms of InlineTextBox::hasTextContent().
+
 2018-09-04  Zan Dobersek  <zdobersek@igalia.com> and Ms2ger  <Ms2ger@igalia.com>
 
         Implement support for passing ImageBitmap to texImage2D/texSubImage2D
index 59a7565..d1d2e28 100644 (file)
@@ -76,6 +76,17 @@ InlineTextBox::~InlineTextBox()
     TextPainter::removeGlyphDisplayList(*this);
 }
 
+bool InlineTextBox::hasTextContent() const
+{
+    if (m_len > 1)
+        return true;
+    if (auto* combinedText = this->combinedText()) {
+        ASSERT(m_len == 1);
+        return !combinedText->combinedStringForRendering().isEmpty();
+    }
+    return false;
+}
+
 void InlineTextBox::markDirty(bool dirty)
 {
     if (dirty) {
@@ -437,7 +448,7 @@ static MarkedText createMarkedTextFromSelectionInBox(const InlineTextBox& box)
 void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset, LayoutUnit /*lineTop*/, LayoutUnit /*lineBottom*/)
 {
     if (isLineBreak() || !paintInfo.shouldPaintWithinRoot(renderer()) || renderer().style().visibility() != Visibility::Visible
-        || m_truncation == cFullTruncation || paintInfo.phase == PaintPhase::Outline || !m_len)
+        || m_truncation == cFullTruncation || paintInfo.phase == PaintPhase::Outline || !hasTextContent())
         return;
 
     ASSERT(paintInfo.phase != PaintPhase::SelfOutline && paintInfo.phase != PaintPhase::ChildOutlines);
@@ -792,7 +803,8 @@ auto InlineTextBox::subdivideAndResolveStyle(const Vector<MarkedText>& textsToSu
         return { };
 
     auto markedTexts = subdivide(textsToSubdivide);
-    if (markedTexts.isEmpty())
+    ASSERT(!markedTexts.isEmpty());
+    if (UNLIKELY(markedTexts.isEmpty()))
         return { };
 
     // Compute frontmost overlapping styled marked texts.
index 9156bac..37ee5e1 100644 (file)
@@ -57,7 +57,14 @@ public:
     void setNextTextBox(InlineTextBox* n) { m_nextTextBox = n; }
     void setPreviousTextBox(InlineTextBox* p) { m_prevTextBox = p; }
 
+    bool hasTextContent() const;
+
+    // These functions do not account for combined text. For combined text this box will always have len() == 1
+    // regardless of whether the resulting composition is the empty string. Use hasTextContent() if you want to
+    // know whether this box has text content.
+    //
     // FIXME: These accessors should ASSERT(!isDirty()). See https://bugs.webkit.org/show_bug.cgi?id=97264
+    // Note len() == 1 for combined text regardless of whether the composition is empty. Use hasTextContent() to
     unsigned start() const { return m_start; }
     unsigned end() const { return m_len ? m_start + m_len - 1 : m_start; }
     unsigned len() const { return m_len; }
index b188a46..f138006 100644 (file)
@@ -1001,7 +1001,7 @@ void RenderBlockFlow::computeBlockDirectionPositionsForLine(RootInlineBox* lineB
         if (is<RenderText>(renderer)) {
             auto& inlineTextBox = downcast<InlineTextBox>(*run->box());
             downcast<RenderText>(renderer).positionLineBox(inlineTextBox);
-            inlineBoxIsRedundant = !inlineTextBox.len();
+            inlineBoxIsRedundant = !inlineTextBox.hasTextContent();
         } else if (is<RenderBox>(renderer)) {
             downcast<RenderBox>(renderer).positionLineBox(downcast<InlineElementBox>(*run->box()));
             inlineBoxIsRedundant = renderer.isOutOfFlowPositioned();
index ff05b72..54013e3 100644 (file)
@@ -1304,7 +1304,7 @@ std::unique_ptr<InlineTextBox> RenderText::createTextBox()
 
 void RenderText::positionLineBox(InlineTextBox& textBox)
 {
-    if (!textBox.len())
+    if (!textBox.hasTextContent())
         return;
     m_containsReversedText |= !textBox.isLeftToRightDirection();
 }