Attachment 348510 Details for Bug 189119 – Patch

[patch] Patch

bug-189119-20180830110912.patch (text/plain), 8.48 KB, created by Daniel Bates on 2018-08-30 11:09:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-08-30 11:09:12 PDT

Size: 8.48 KB

patch

obsolete

>Subversion Revision: 235381
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2673bda26340d546a9b715aade6a0a240a7e5d05..57a304f7c2f6e30b4d7b799cb74de175be4eed0a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2018-08-30  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 NOBODY (OOPS!).
>+
>+        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::isEmpty const): Added. Returns whether an inline text box is empty.
>+        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 isEmpty().
>+        (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::isEmpty()
>+        so that we remove empty inline text boxes associated with combined text.
>+        * rendering/RenderText.cpp:
>+        (WebCore::RenderText::positionLineBox): Write in terms of InlineTextBox::isEmpty().
>+
> 2018-08-29  Daniel Bates  <dabates@apple.com>
> 
>         REGRESSION (r226138): WebCore::subdivide() may return an empty an empty vector; Web process can crash when performing find in Epiphany
>diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp
>index 59a7565652cfe8c1d65fba60bd469deb2c326c10..b5ed38c78a3673abe926dd2a1b8761b861cbd1dd 100644
>--- a/Source/WebCore/rendering/InlineTextBox.cpp
>+++ b/Source/WebCore/rendering/InlineTextBox.cpp
>@@ -76,6 +76,15 @@ InlineTextBox::~InlineTextBox()
>     TextPainter::removeGlyphDisplayList(*this);
> }
> 
>+bool InlineTextBox::isEmpty() const
>+{
>+    if (!m_len)
>+        return true;
>+    if (auto* combinedText = this->combinedText())
>+        return combinedText->combinedStringForRendering().isEmpty();
>+    return false;
>+}
>+
> void InlineTextBox::markDirty(bool dirty)
> {
>     if (dirty) {
>@@ -437,7 +446,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 || isEmpty())
>         return;
> 
>     ASSERT(paintInfo.phase != PaintPhase::SelfOutline && paintInfo.phase != PaintPhase::ChildOutlines);
>@@ -792,7 +801,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.
>diff --git a/Source/WebCore/rendering/InlineTextBox.h b/Source/WebCore/rendering/InlineTextBox.h
>index 9156bac891ef2078c53a61aef6f4eec7871fdfad..e079d36180a7d9f5d2b297801bf75a63b932d72b 100644
>--- a/Source/WebCore/rendering/InlineTextBox.h
>+++ b/Source/WebCore/rendering/InlineTextBox.h
>@@ -57,6 +57,8 @@ public:
>     void setNextTextBox(InlineTextBox* n) { m_nextTextBox = n; }
>     void setPreviousTextBox(InlineTextBox* p) { m_prevTextBox = p; }
> 
>+    bool isEmpty() const;
>+
>     // FIXME: These accessors should ASSERT(!isDirty()). See https://bugs.webkit.org/show_bug.cgi?id=97264
>     unsigned start() const { return m_start; }
>     unsigned end() const { return m_len ? m_start + m_len - 1 : m_start; }
>diff --git a/Source/WebCore/rendering/RenderBlockLineLayout.cpp b/Source/WebCore/rendering/RenderBlockLineLayout.cpp
>index b188a46e269b5c7087a1625d1731fbd5fb9b1134..5cf26a69ea81b8a117e8f3ee6ab3fd7c17c4976e 100644
>--- a/Source/WebCore/rendering/RenderBlockLineLayout.cpp
>+++ b/Source/WebCore/rendering/RenderBlockLineLayout.cpp
>@@ -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.isEmpty();
>         } else if (is<RenderBox>(renderer)) {
>             downcast<RenderBox>(renderer).positionLineBox(downcast<InlineElementBox>(*run->box()));
>             inlineBoxIsRedundant = renderer.isOutOfFlowPositioned();
>diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
>index ff05b72d26fe4967534011ceaadcfd3f4fd4caf2..4e166530675da738b736cbed6f5e12731e3b0c92 100644
>--- a/Source/WebCore/rendering/RenderText.cpp
>+++ b/Source/WebCore/rendering/RenderText.cpp
>@@ -1304,7 +1304,7 @@ std::unique_ptr<InlineTextBox> RenderText::createTextBox()
> 
> void RenderText::positionLineBox(InlineTextBox& textBox)
> {
>-    if (!textBox.len())
>+    if (textBox.isEmpty())
>         return;
>     m_containsReversedText |= !textBox.isLeftToRightDirection();
> }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 3ec5d8cb8e51748e91e764549322a0c3fedf5787..dfb2b2bea21bcc9265c1d9e0bbddea49fea955f9 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-08-30  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 NOBODY (OOPS!).
>+
>+        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-08-29  Daniel Bates  <dabates@apple.com>
> 
>         REGRESSION (r226138): WebCore::subdivide() may return an empty an empty vector; Web process can crash when performing find in Epiphany
>diff --git a/LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt b/LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt
>index 8fcf441a4c67b324eac5001e9a98b9c1eb344391..53f12c69bce8d7262ebfa72b26b75a85bbf0ed2a 100644
>--- a/LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt
>+++ b/LayoutTests/fast/text/text-combine-surroundContents-crash-expected.txt
>@@ -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}"

Flags:

zalan: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189119: 348448 | 348510