[LFC][IFC] LineBreaker should not hold on to the lastWrapOpportunity inline item
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Feb 2020 19:05:29 +0000 (19:05 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Feb 2020 19:05:29 +0000 (19:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207707
<rdar://problem/59427376>

Reviewed by Antti Koivisto.

LineBreaker only needs a flag indicating that there's a wrapping opportunity on the line.
LineLayoutContext needs to know what the position is (as an InlineItem) so that it could initiate a revert if needed.

* layout/inlineformatting/InlineLineBreaker.cpp:
(WebCore::Layout::LineBreaker::shouldWrapInlineContent):
(WebCore::Layout::LineBreaker::tryWrappingInlineContent const):
(WebCore::Layout::LineBreaker::wordBreakBehavior const):
* layout/inlineformatting/InlineLineBreaker.h:
* layout/inlineformatting/LineLayoutContext.cpp:
(WebCore::Layout::LineLayoutContext::layoutLine):
(WebCore::Layout::LineLayoutContext::tryAddingFloatItem):
(WebCore::Layout::LineLayoutContext::tryAddingInlineItems):
(WebCore::Layout::LineLayoutContext::rebuildLine):
(WebCore::Layout::LineLayoutContext::rebuildLineForRevert): Deleted.
* layout/inlineformatting/LineLayoutContext.h:
(WebCore::Layout::LineLayoutContext::InlineItemRange::size const):

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

Source/WebCore/ChangeLog
Source/WebCore/layout/inlineformatting/InlineLineBreaker.cpp
Source/WebCore/layout/inlineformatting/InlineLineBreaker.h
Source/WebCore/layout/inlineformatting/LineLayoutContext.cpp
Source/WebCore/layout/inlineformatting/LineLayoutContext.h

index 3d0fc17..2b503a0 100644 (file)
@@ -1,3 +1,28 @@
+2020-02-13  Zalan Bujtas  <zalan@apple.com>
+
+        [LFC][IFC] LineBreaker should not hold on to the lastWrapOpportunity inline item
+        https://bugs.webkit.org/show_bug.cgi?id=207707
+        <rdar://problem/59427376>
+
+        Reviewed by Antti Koivisto.
+
+        LineBreaker only needs a flag indicating that there's a wrapping opportunity on the line.
+        LineLayoutContext needs to know what the position is (as an InlineItem) so that it could initiate a revert if needed.
+
+        * layout/inlineformatting/InlineLineBreaker.cpp:
+        (WebCore::Layout::LineBreaker::shouldWrapInlineContent):
+        (WebCore::Layout::LineBreaker::tryWrappingInlineContent const):
+        (WebCore::Layout::LineBreaker::wordBreakBehavior const):
+        * layout/inlineformatting/InlineLineBreaker.h:
+        * layout/inlineformatting/LineLayoutContext.cpp:
+        (WebCore::Layout::LineLayoutContext::layoutLine):
+        (WebCore::Layout::LineLayoutContext::tryAddingFloatItem):
+        (WebCore::Layout::LineLayoutContext::tryAddingInlineItems):
+        (WebCore::Layout::LineLayoutContext::rebuildLine):
+        (WebCore::Layout::LineLayoutContext::rebuildLineForRevert): Deleted.
+        * layout/inlineformatting/LineLayoutContext.h:
+        (WebCore::Layout::LineLayoutContext::InlineItemRange::size const):
+
 2020-02-13  Adrian Perez de Castro  <aperez@igalia.com>
 
         Non-unified build fixes mid February 2020 edition
index d4c004c..9e8c2c9 100644 (file)
@@ -139,9 +139,11 @@ LineBreaker::Result LineBreaker::shouldWrapInlineContent(const RunList& candidat
                     return true;
                 return shouldKeepBeginningOfLineWhitespace(candidateItem.style());
             };
-            auto& inlineItem = candidateRuns[*lastLineWrapOpportunityIndex].inlineItem;
-            if (isEligibleLineWrapOpportunity(inlineItem))
-                m_lastWrapOpportunity = &inlineItem;
+            auto& lastWrapOpportunityCandidateItem = candidateRuns[*lastLineWrapOpportunityIndex].inlineItem;
+            if (isEligibleLineWrapOpportunity(lastWrapOpportunityCandidateItem)) {
+                result.lastWrapOpportunityItem = &lastWrapOpportunityCandidateItem;
+                m_hasWrapOpportunityAtPreviousPosition = true;
+            }
         }
     }
     return result;
@@ -200,14 +202,14 @@ LineBreaker::Result LineBreaker::tryWrappingInlineContent(const RunList& candida
     }
     // If we are not allowed to break this overflowing content, we still need to decide whether keep it or push it to the next line.
     if (lineStatus.lineIsEmpty) {
-        ASSERT(!m_lastWrapOpportunity);
+        ASSERT(!m_hasWrapOpportunityAtPreviousPosition);
         return { Result::Action::Keep, IsEndOfLine::No };
     }
     // Now either wrap here or at an earlier position, or not wrap at all.
     if (isContentWrappingAllowed(candidateContent))
         return { Result::Action::Push, IsEndOfLine::Yes };
-    if (m_lastWrapOpportunity)
-        return { Result::Action::Revert, IsEndOfLine::Yes, { }, m_lastWrapOpportunity };
+    if (m_hasWrapOpportunityAtPreviousPosition)
+        return { Result::Action::RevertToLastWrapOpportunity, IsEndOfLine::Yes };
     return { Result::Action::Keep, IsEndOfLine::No };
 }
 
@@ -286,10 +288,10 @@ LineBreaker::WordBreakRule LineBreaker::wordBreakBehavior(const RenderStyle& sty
         return WordBreakRule::NoBreak;
     // For compatibility with legacy content, the word-break property also supports a deprecated break-word keyword.
     // When specified, this has the same effect as word-break: normal and overflow-wrap: anywhere, regardless of the actual value of the overflow-wrap property.
-    if (style.wordBreak() == WordBreak::BreakWord && !m_lastWrapOpportunity)
+    if (style.wordBreak() == WordBreak::BreakWord && !m_hasWrapOpportunityAtPreviousPosition)
         return WordBreakRule::AtArbitraryPosition;
     // OverflowWrap::Break: An otherwise unbreakable sequence of characters may be broken at an arbitrary point if there are no otherwise-acceptable break points in the line.
-    if (style.overflowWrap() == OverflowWrap::Break && !m_lastWrapOpportunity)
+    if (style.overflowWrap() == OverflowWrap::Break && !m_hasWrapOpportunityAtPreviousPosition)
         return WordBreakRule::AtArbitraryPosition;
 
     if (!n_hyphenationIsDisabled && style.hyphens() == Hyphens::Auto && canHyphenate(style.locale()))
index bedc030..fd32109 100644 (file)
@@ -53,7 +53,7 @@ public:
             Keep, // Keep content on the current line.
             Split, // Partial content is on the current line.
             Push, // Content is pushed to the next line.
-            Revert // The current content overflows and can't get wrapped. The line needs to be reverted back to the last line wrapping opportunity.
+            RevertToLastWrapOpportunity // The current content overflows and can't get wrapped. The line needs to be reverted back to the last line wrapping opportunity.
         };
         struct PartialTrailingContent {
             size_t trailingRunIndex { 0 };
@@ -63,7 +63,7 @@ public:
         Action action { Action::Keep };
         IsEndOfLine isEndOfLine { IsEndOfLine::No };
         Optional<PartialTrailingContent> partialTrailingContent { };
-        const InlineItem* revertTo { nullptr };
+        const InlineItem* lastWrapOpportunityItem { nullptr };
     };
 
     struct Run {
@@ -111,7 +111,7 @@ private:
     bool isContentWrappingAllowed(const ContinuousContent&) const;
 
     bool n_hyphenationIsDisabled { false };
-    const InlineItem* m_lastWrapOpportunity { nullptr };
+    bool m_hasWrapOpportunityAtPreviousPosition { false };
 };
 
 inline LineBreaker::Run::Run(const InlineItem& inlineItem, InlineLayoutUnit logicalWidth)
index 582d22b..c734778 100644 (file)
@@ -262,6 +262,7 @@ LineLayoutContext::LineContent LineLayoutContext::layoutLine(LineBuilder& line,
 {
     ASSERT(m_floats.isEmpty());
     m_partialLeadingTextItem = { };
+    m_lastWrapOpportunityItem = { };
     auto lineBreaker = LineBreaker { };
     auto currentItemIndex = layoutRange.start;
     unsigned committedInlineItemCount = 0;
@@ -275,7 +276,7 @@ LineLayoutContext::LineContent LineLayoutContext::layoutLine(LineBuilder& line,
         if (lineCandidate.floatItem) {
             // Floats shrink the available horizontal space for the rest of the content, but they are not added on the line.
             auto result = tryAddingFloatItem(line, *lineCandidate.floatItem);
-            committedInlineItemCount += result.committedCount;
+            committedInlineItemCount += result.committedCount.value;
             if (result.isEndOfLine == LineBreaker::IsEndOfLine::Yes) {
                 // This float takes up all the horizontal space.
                 return close(line, layoutRange, committedInlineItemCount, { });
@@ -285,14 +286,8 @@ LineLayoutContext::LineContent LineLayoutContext::layoutLine(LineBuilder& line,
             auto& inlineContent = lineCandidate.inlineContent;
             if (!inlineContent.runs().isEmpty()) {
                 // Now check if we can put this content on the current line.
-                auto result = tryAddingInlineItems(lineBreaker, line, lineCandidate);
-                if (result.revertTo) {
-                    ASSERT(!result.committedCount);
-                    ASSERT(result.isEndOfLine == LineBreaker::IsEndOfLine::Yes);
-                    // An earlier line wrapping opportunity turned out to be the final breaking position.
-                    rebuildLineForRevert(line, *result.revertTo, layoutRange);
-                }
-                committedInlineItemCount += result.committedCount;
+                auto result = tryAddingInlineItems(lineBreaker, line, layoutRange, lineCandidate);
+                committedInlineItemCount = result.committedCount.isRevert ? result.committedCount.value : committedInlineItemCount + result.committedCount.value;  
                 if (result.isEndOfLine == LineBreaker::IsEndOfLine::Yes) {
                     // We can't place any more items on the current line.
                     return close(line, layoutRange, committedInlineItemCount, result.partialContent);
@@ -401,10 +396,10 @@ LineLayoutContext::Result LineLayoutContext::tryAddingFloatItem(LineBuilder& lin
     else
         line.moveLogicalRight(logicalWidth);
     m_floats.append(&floatItem);
-    return { LineBreaker::IsEndOfLine::No, 1 };
+    return { LineBreaker::IsEndOfLine::No, { 1, false } };
 }
 
-LineLayoutContext::Result LineLayoutContext::tryAddingInlineItems(LineBreaker& lineBreaker, LineBuilder& line, const LineCandidate& lineCandidate)
+LineLayoutContext::Result LineLayoutContext::tryAddingInlineItems(LineBreaker& lineBreaker, LineBuilder& line, const InlineItemRange& layoutRange, const LineCandidate& lineCandidate)
 {
     auto shouldDisableHyphenation = [&] {
         auto& style = root().style();
@@ -420,6 +415,8 @@ LineLayoutContext::Result LineLayoutContext::tryAddingInlineItems(LineBreaker& l
     auto& inlineContent = lineCandidate.inlineContent;
     auto& candidateRuns = inlineContent.runs();
     auto result = lineBreaker.shouldWrapInlineContent(candidateRuns, inlineContent.logicalWidth(), lineStatus);
+    if (result.lastWrapOpportunityItem)
+        m_lastWrapOpportunityItem = result.lastWrapOpportunityItem;
     if (result.action == LineBreaker::Result::Action::Keep) {
         // This continuous content can be fully placed on the current line.
         for (auto& run : candidateRuns)
@@ -427,19 +424,23 @@ LineLayoutContext::Result LineLayoutContext::tryAddingInlineItems(LineBreaker& l
         // Consume trailing line break as well.
         if (auto* lineBreakItem = inlineContent.trailingLineBreak()) {
             line.append(*lineBreakItem, 0);
-            return { LineBreaker::IsEndOfLine::Yes, candidateRuns.size() + 1 };
+            return { LineBreaker::IsEndOfLine::Yes, { candidateRuns.size() + 1, false } };
         }
-        return { result.isEndOfLine, candidateRuns.size() };
+        return { result.isEndOfLine, { candidateRuns.size(), false } };
     }
     if (result.action == LineBreaker::Result::Action::Push) {
+        ASSERT(result.isEndOfLine == LineBreaker::IsEndOfLine::Yes);
         // This continuous content can't be placed on the current line. Nothing to commit at this time.
         return { result.isEndOfLine };
     }
-    if (result.action == LineBreaker::Result::Action::Revert) {
+    if (result.action == LineBreaker::Result::Action::RevertToLastWrapOpportunity) {
+        ASSERT(result.isEndOfLine == LineBreaker::IsEndOfLine::Yes);
         // Not only this content can't be placed on the current line, but we even need to revert the line back to an earlier position.
-        return { result.isEndOfLine, 0, { }, result.revertTo };
+        ASSERT(m_lastWrapOpportunityItem);
+        return { result.isEndOfLine, { rebuildLine(line, layoutRange), true } };
     }
     if (result.action == LineBreaker::Result::Action::Split) {
+        ASSERT(result.isEndOfLine == LineBreaker::IsEndOfLine::Yes);
         // Commit the combination of full and partial content on the current line.
         ASSERT(result.partialTrailingContent);
         commitPartialContent(line, candidateRuns, *result.partialTrailingContent);
@@ -448,12 +449,12 @@ LineLayoutContext::Result LineLayoutContext::tryAddingInlineItems(LineBreaker& l
         auto trailingRunIndex = result.partialTrailingContent->trailingRunIndex;
         auto committedInlineItemCount = trailingRunIndex + 1;
         if (!result.partialTrailingContent->partialRun)
-            return { result.isEndOfLine, committedInlineItemCount };
+            return { result.isEndOfLine, { committedInlineItemCount, false } };
 
         auto partialRun = *result.partialTrailingContent->partialRun;
         auto& trailingInlineTextItem = downcast<InlineTextItem>(candidateRuns[trailingRunIndex].inlineItem);
         auto overflowLength = trailingInlineTextItem.length() - partialRun.length;
-        return { result.isEndOfLine, committedInlineItemCount, LineContent::PartialContent { partialRun.needsHyphen, overflowLength } };
+        return { result.isEndOfLine, { committedInlineItemCount, false }, LineContent::PartialContent { partialRun.needsHyphen, overflowLength } };
     }
     ASSERT_NOT_REACHED();
     return { LineBreaker::IsEndOfLine::No };
@@ -480,29 +481,29 @@ void LineLayoutContext::commitPartialContent(LineBuilder& line, const LineBreake
     }
 }
 
-void LineLayoutContext::rebuildLineForRevert(LineBuilder& line, const InlineItem& revertTo, const InlineItemRange layoutRange)
+size_t LineLayoutContext::rebuildLine(LineBuilder& line, const InlineItemRange& layoutRange)
 {
-    // This is the rare case when the line needs to be reverted to an earlier position.
+    // Clear the line and start appending the inline items closing with the last wrap opportunity run.
     line.resetContent();
-    auto inlineItemIndex = layoutRange.start;
-    InlineLayoutUnit logicalRight = { };
+    auto currentItemIndex = layoutRange.start;
+    auto logicalRight = InlineLayoutUnit { };
     if (m_partialLeadingTextItem) {
         auto logicalWidth = inlineItemWidth(*m_partialLeadingTextItem, logicalRight);
         line.append(*m_partialLeadingTextItem, logicalWidth);
         logicalRight += logicalWidth;
-        if (&revertTo == &m_partialLeadingTextItem.value())
-            return;
-        ++inlineItemIndex;
+        if (&m_partialLeadingTextItem.value() == m_lastWrapOpportunityItem)
+            return 1;
+        ++currentItemIndex;
     }
-
-    for (; inlineItemIndex < layoutRange.end; ++inlineItemIndex) {
-        auto& inlineItem = m_inlineItems[inlineItemIndex];
+    for (; currentItemIndex < layoutRange.end; ++currentItemIndex) {
+        auto& inlineItem = m_inlineItems[currentItemIndex];
         auto logicalWidth = inlineItemWidth(inlineItem, logicalRight);
         line.append(inlineItem, logicalWidth);
         logicalRight += logicalWidth;
-        if (&inlineItem == &revertTo)
-            break;
+        if (&inlineItem == m_lastWrapOpportunityItem)
+            return currentItemIndex - layoutRange.start + 1;
     }
+    return layoutRange.size();
 }
 
 }
index 8736f83..5dd02db 100644 (file)
@@ -52,6 +52,7 @@ public:
     };
     struct InlineItemRange {
         bool isEmpty() const { return start == end; }
+        size_t size() const { return end - start; }
         size_t start { 0 };
         size_t end { 0 };
     };
@@ -61,13 +62,16 @@ private:
     void nextContentForLine(LineCandidate&, unsigned inlineItemIndex, const InlineItemRange layoutRange, Optional<unsigned> overflowLength, InlineLayoutUnit currentLogicalRight);
     struct Result {
         LineBreaker::IsEndOfLine isEndOfLine { LineBreaker::IsEndOfLine::No };
-        size_t committedCount { 0 };
+        struct CommittedContentCount {
+            size_t value { 0 };
+            bool isRevert { false };
+        };
+        CommittedContentCount committedCount { };
         Optional <LineContent::PartialContent> partialContent { };
-        const InlineItem* revertTo { nullptr };
     };
     Result tryAddingFloatItem(LineBuilder&, const InlineItem& floatItem);
-    Result tryAddingInlineItems(LineBreaker&, LineBuilder&, const LineCandidate&);
-    void rebuildLineForRevert(LineBuilder&, const InlineItem& revertTo, const InlineItemRange layoutRange);
+    Result tryAddingInlineItems(LineBreaker&, LineBuilder&, const InlineItemRange& layoutRange, const LineCandidate&);
+    size_t rebuildLine(LineBuilder&, const InlineItemRange& layoutRange);
     void commitPartialContent(LineBuilder&, const LineBreaker::RunList&, const LineBreaker::Result::PartialTrailingContent&);
     LineContent close(LineBuilder&, const InlineItemRange layoutRange, unsigned committedInlineItemCount, Optional<LineContent::PartialContent>);
 
@@ -82,6 +86,7 @@ private:
     using FloatList = Vector<const InlineItem*>;
     FloatList m_floats;
     Optional<InlineTextItem> m_partialLeadingTextItem;
+    const InlineItem* m_lastWrapOpportunityItem { nullptr };
     unsigned m_successiveHyphenatedLineCount { 0 };
 };