[LayoutNG] Fix safe-to-break offset and its use at start of wrapped lines

This patch fixes safe-to-break offset vector in ShapeResult:
1. Fixed before the first glyph of each run to be safe-to-break.
2. Fixed before the last glyph of each run not to force safe-to-break.
3. Fixed non-cluster boundaries were off by one.

Also related, two fixes are made to NGLineBreaker:
1. When the whole item can fit, it does not consider when the start of
   the item is not safe-to-break. The optimized code path was removed
   to fix this. If this seems to hit performance, we can bring it back
   with safe-to-break support.
2. Add |start_should_be_safe| argument to ShapingLineBreaker to make
   the start of wrapped line safe-to-break, but not the middle of a
   wrapped line (i.e., when there are previous items.)

HarfBuzzShaperTest.SafeToBreakLatinDiscretionaryLigatures was fixed
to match to what HarfBuzz returns as HB_GLYPH_FLAG_UNSAFE_TO_BREAK.

This change causes a crash in
fast/inline/absolute-positioned-inline-in-centred-block.html
This is from an issue in HarfBuzzShaper, tracked in issue 817271.

Bug: 816614
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Id55ea10f0b6f879981e3725b7cac027b39f3f213
Reviewed-on: https://chromium-review.googlesource.com/939942
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539839}

third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

@@ -248,7 +248,6 @@ crbug.com/591099 css1/color_and_background/background_position.html [ Failure ]
 crbug.com/591099 css1/color_and_background/background_repeat.html [ Failure ]
 crbug.com/591099 css1/color_and_background/color.html [ Failure ]
 crbug.com/591099 css1/conformance/forward_compatible_parsing.html [ Failure ]
-crbug.com/591099 css1/font_properties/font_variant.html [ Crash ]
 crbug.com/591099 css1/formatting_model/horizontal_formatting.html [ Failure ]
 crbug.com/591099 css1/formatting_model/replaced_elements.html [ Failure ]
 crbug.com/591099 css1/formatting_model/vertical_formatting.html [ Failure ]
@@ -2176,7 +2175,6 @@ crbug.com/591099 fast/block/margin-collapse/webkit-margin-collapse-container.htm
 crbug.com/591099 fast/block/margin-collapse/webkit-margin-collapse-separate-position.html [ Failure ]
 crbug.com/591099 fast/block/margin-collapse/webkit-margin-collapse-siblings.html [ Failure ]
 crbug.com/591099 fast/block/over-constrained-auto-margin.html [ Failure ]
-crbug.com/816614 fast/block/positioning/056.html [ Failure ]
 crbug.com/591099 fast/block/positioning/059.html [ Failure ]
 crbug.com/591099 fast/block/positioning/abspos-auto-left-and-width-change-parent-margin-left.html [ Failure ]
 crbug.com/591099 fast/block/positioning/auto-height-with-top-and-bottom.html [ Failure ]
@@ -3133,7 +3131,7 @@ crbug.com/591099 fast/inline-block/baseline-vertical.html [ Failure ]
 crbug.com/591099 fast/inline-block/contenteditable-baseline.html [ Failure ]
 crbug.com/714962 fast/inline-block/tricky-baseline.html [ Failure ]
 crbug.com/591099 fast/inline-block/vertical-align-top-and-bottom-2.html [ Failure ]
-crbug.com/591099 fast/inline/absolute-positioned-inline-in-centred-block.html [ Failure ]
+crbug.com/591099 fast/inline/absolute-positioned-inline-in-centred-block.html [ Crash Failure ]
 crbug.com/591099 fast/inline/bpm-inline-ancestors.html [ Failure ]
 crbug.com/714962 fast/inline/continuation-outlines-with-layers-2.html [ Failure ]
 crbug.com/591099 fast/inline/continuation-outlines-with-layers.html [ Failure ]
@@ -4098,7 +4096,6 @@ crbug.com/714962 fast/text/international/plane2.html [ Failure ]
 crbug.com/714962 fast/text/international/rtl-selection-rect-with-fallback.html [ Failure ]
 crbug.com/796943 fast/text/international/shape-across-elements-simple.html [ Pass ]
 crbug.com/591099 fast/text/international/text-combine-image-test.html [ Failure ]
-crbug.com/591099 fast/text/international/unicode-bidi-plaintext-line-wrap.html [ Failure ]
 crbug.com/591099 fast/text/justify-ideograph-complex.html [ Failure ]
 crbug.com/591099 fast/text/justify-ideograph-simple.html [ Failure ]
 crbug.com/591099 fast/text/justify-ideograph-vertical.html [ Failure ]

third_party/WebKit/LayoutTests/TestExpectations

@@ -395,13 +395,13 @@ crbug.com/714962 virtual/layout_ng/fast/inline/positionedLifetime.html [ Failure
 crbug.com/714962 virtual/layout_ng/fast/inline/bpm-inline-ancestors.html [ Failure ]
 
 ### Image/text failures also on LayoutNG
-crbug.com/714962 virtual/layout_ng/fast/inline/absolute-positioned-inline-in-centred-block.html [ Failure ]
+crbug.com/714962 virtual/layout_ng/fast/inline/absolute-positioned-inline-in-centred-block.html [ Crash Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/continuation-outlines-with-layers.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/continuation-outlines.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/drawStyledEmptyInlines.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/inline-borders-with-bidi-override.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/inline-focus-ring-under-absolute-enclosing-relative-div.html [ Failure ]
-crbug.com/714962 virtual/layout_ng/fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html [ Failure ]
+crbug.com/714962 virtual/layout_ng/fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html [ Crash Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/nested-text-descendants.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/out-of-flow-objects-and-whitespace-after-empty-inline.html [ Failure ]
 crbug.com/714962 virtual/layout_ng/fast/inline/outline-continuations.html [ Failure ]

third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t1505-c524-font-var-00-b-expected.txt

 layer at (0,0) size 800x600
   LayoutView at (0,0) size 800x600
-layer at (0,0) size 800x88
-  LayoutNGBlockFlow {HTML} at (0,0) size 800x88
-    LayoutNGBlockFlow {BODY} at (8,16) size 784x56
+layer at (0,0) size 800x108
+  LayoutNGBlockFlow {HTML} at (0,0) size 800x108
+    LayoutNGBlockFlow {BODY} at (8,16) size 784x76
       LayoutNGBlockFlow {P} at (0,0) size 784x20 [color=#000080]
         LayoutText {#text} at (0,0) size 268x19
           text run at (0,0) width 268: "This Paragraph should be in Small Caps."
-      LayoutNGBlockFlow {P} at (0,36) size 784x20 [color=#000080]
-        LayoutText {#text} at (0,0) size 460x19
-          text run at (0,0) width 460: "This Sentence should be in Small Caps. "
+      LayoutNGBlockFlow {P} at (0,36) size 784x40 [color=#000080]
+        LayoutText {#text} at (0,0) size 259x19
+          text run at (0,0) width 259: "This Sentence should be in Small Caps."
         LayoutInline {SPAN} at (0,0) size 210x19
-          LayoutText {#text} at (460,0) size 210x19
-            text run at (460,0) width 210: "This Sentence should be Normal."
+          LayoutText {#text} at (0,20) size 210x19
+            text run at (0,20) width 210: "This Sentence should be Normal."
         LayoutText {#text} at (0,0) size 0x0

third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node_test.cc

@@ -104,6 +104,7 @@ class NGInlineNodeTest : public NGLayoutTest {
 
     scoped_refptr<NGConstraintSpace> constraint_space =
         NGConstraintSpaceBuilder(WritingMode::kHorizontalTb, icb_size)
+            .SetAvailableSize({LayoutUnit::Max(), LayoutUnit(-1)})
             .ToConstraintSpace(WritingMode::kHorizontalTb);
     scoped_refptr<NGLayoutResult> result =
         NGInlineLayoutAlgorithm(node, *constraint_space).Layout();

third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc

@@ -318,21 +318,6 @@ NGLineBreaker::LineBreakState NGLineBreaker::HandleText(
   NGInlineItemResult* item_result = AddItem(item, item_results);
   LayoutUnit available_width = line_.AvailableWidth();
 
-  // If the start offset is at the item boundary, try to add the entire item.
-  if (offset_ == item.StartOffset()) {
-    item_result->inline_size =
-        item.TextShapeResult()->SnappedWidth().ClampNegativeToZero();
-    LayoutUnit next_position = line_.position + item_result->inline_size;
-    if (!auto_wrap_ || next_position <= available_width) {
-      item_result->shape_result = item.TextShapeResult();
-      item_result->may_break_inside = auto_wrap_;
-      ComputeCanBreakAfter(item_result);
-      line_.position = next_position;
-      MoveToNextOf(item);
-      return state;
-    }
-  }
-
   if (auto_wrap_) {
     // Try to break inside of this text item.
     BreakText(item_result, item, available_width - line_.position, line_info);
@@ -361,7 +346,6 @@ NGLineBreaker::LineBreakState NGLineBreaker::HandleText(
   // Add the rest of the item if !auto_wrap.
   // Because the start position may need to reshape, run ShapingLineBreaker
   // with max available width.
-  DCHECK_NE(offset_, item.StartOffset());
   BreakText(item_result, item, LayoutUnit::Max(), line_info);
   DCHECK_EQ(item_result->end_offset, item.EndOffset());
   DCHECK(!item_result->may_break_inside);
@@ -392,7 +376,8 @@ void NGLineBreaker::BreakText(NGInlineItemResult* item_result,
   available_width = std::max(LayoutUnit(0), available_width);
   ShapingLineBreaker::Result result;
   scoped_refptr<ShapeResult> shape_result =
-      breaker.ShapeLine(item_result->start_offset, available_width, &result);
+      breaker.ShapeLine(item_result->start_offset, available_width,
+                        offset_ == line_info->StartOffset(), &result);
   DCHECK_GT(shape_result->NumCharacters(), 0u);
   if (result.is_hyphenated) {
     AppendHyphen(item, line_info);

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp

@@ -896,8 +896,8 @@ TEST_F(HarfBuzzShaperTest, SafeToBreakLatinDiscretionaryLigatures) {
   scoped_refptr<ShapeResult> result = shaper.Shape(&testFont, TextDirection::kLtr);
   EXPECT_EQ(6u, result->NextSafeToBreakOffset(1));    // After CA ligature.
   EXPECT_EQ(6u, result->NextSafeToBreakOffset(6));    // After CA ligature.
-  EXPECT_EQ(9u, result->NextSafeToBreakOffset(7));    // Before RA ligature.
-  EXPECT_EQ(9u, result->NextSafeToBreakOffset(9));    // Before RA ligature.
+  EXPECT_EQ(11u, result->NextSafeToBreakOffset(7));   // At end of string.
+  EXPECT_EQ(11u, result->NextSafeToBreakOffset(9));   // At end of string.
   EXPECT_EQ(11u, result->NextSafeToBreakOffset(10));  // At end of string.
 
   // Add zero-width spaces at the safe to break offsets.

third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp

@@ -528,12 +528,12 @@ float HarfBuzzPositionToFloat(hb_position_t value) {
 bool IsSafeToBreakBefore(const hb_glyph_info_t* glyph_infos,
                          unsigned num_glyphs,
                          unsigned i) {
-  // At the end of the run.
-  if (i == num_glyphs - 1)
+  // Before the first glyph is safe to break.
+  if (!i)
     return true;
 
   // Not at a cluster boundary.
-  if (glyph_infos[i].cluster == glyph_infos[i + 1].cluster)
+  if (glyph_infos[i].cluster == glyph_infos[i - 1].cluster)
     return false;
 
   // The HB_GLYPH_FLAG_UNSAFE_TO_BREAK flag is set for all glyphs in a

third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp

@@ -216,6 +216,7 @@ inline scoped_refptr<ShapeResult> ShapingLineBreaker::Shape(TextDirection direct
 scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeLine(
     unsigned start,
     LayoutUnit available_space,
+    bool start_should_be_safe,
     ShapingLineBreaker::Result* result_out) {
   DCHECK_GE(available_space, LayoutUnit(0));
   unsigned range_start = result_->StartIndexForResult();
@@ -239,12 +240,15 @@ scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeLine(
   unsigned candidate_break =
       result_->OffsetForPosition(end_position, false) + range_start;
 
+  unsigned first_safe =
+      start_should_be_safe ? result_->NextSafeToBreakOffset(start) : start;
+  DCHECK_GE(first_safe, start);
   if (candidate_break >= range_end) {
     // The |result_| does not have glyphs to fill the available space,
     // and thus unable to compute. Return the result up to range_end.
     DCHECK_EQ(candidate_break, range_end);
     result_out->break_offset = range_end;
-    return ShapeToEnd(start, start_position, range_end);
+    return ShapeToEnd(start, first_safe, range_end);
   }
 
   // candidate_break should be >= start, but rounding errors can chime in when
@@ -261,7 +265,7 @@ scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeLine(
     // measure beyond it.
     if (break_opportunity >= range_end) {
       result_out->break_offset = range_end;
-      return ShapeToEnd(start, start_position, range_end);
+      return ShapeToEnd(start, first_safe, range_end);
     }
   }
   DCHECK_GT(break_opportunity, start);
@@ -270,8 +274,6 @@ scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeLine(
   // the start and the next safe-to-break boundary needs to be reshaped and the
   // available space adjusted to take the reshaping into account.
   scoped_refptr<ShapeResult> line_start_result;
-  unsigned first_safe = result_->NextSafeToBreakOffset(start);
-  DCHECK_GE(first_safe, start);
   if (first_safe != start) {
     if (first_safe >= break_opportunity) {
       // There is no safe-to-break, reshape the whole range.
@@ -361,24 +363,27 @@ scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeLine(
 // Shape from the specified offset to the end of the ShapeResult.
 // If |start| is safe-to-break, this copies the subset of the result.
 scoped_refptr<ShapeResult> ShapingLineBreaker::ShapeToEnd(unsigned start,
-                                                   LayoutUnit start_position,
-                                                   unsigned range_end) {
-  unsigned first_safe = result_->NextSafeToBreakOffset(start);
+                                                          unsigned first_safe,
+                                                          unsigned range_end) {
+  DCHECK_GE(start, result_->StartIndexForResult());
+  DCHECK_LT(start, range_end);
   DCHECK_GE(first_safe, start);
+  DCHECK_EQ(range_end, result_->EndIndexForResult());
 
-  scoped_refptr<ShapeResult> line_result;
-  TextDirection direction = result_->Direction();
+  // If |start| is safe-to-break, no reshape is needed.
   if (first_safe == start) {
-    // If |start| is safe-to-break no reshape is needed.
-    line_result = result_->SubRange(start, range_end);
-  } else if (first_safe < range_end) {
-    // Otherwise reshape to |first_safe|, then copy the rest.
-    line_result = Shape(direction, start, first_safe);
-    result_->CopyRange(first_safe, range_end, line_result.get());
-  } else {
-    // If no safe-to-break offset is found in range, reshape the entire range.
-    line_result = Shape(direction, start, range_end);
+    return result_->SubRange(start, range_end);
   }
+
+  // If no safe-to-break offset is found in range, reshape the entire range.
+  TextDirection direction = result_->Direction();
+  if (first_safe >= range_end) {
+    return Shape(direction, start, range_end);
+  }
+
+  // Otherwise reshape to |first_safe|, then copy the rest.
+  scoped_refptr<ShapeResult> line_result = Shape(direction, start, first_safe);
+  result_->CopyRange(first_safe, range_end, line_result.get());
   return line_result;
 }
 

third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.h

@@ -47,6 +47,7 @@ class PLATFORM_EXPORT ShapingLineBreaker final {
   struct Result {
     STACK_ALLOCATED();
 
+    // Indicates the resulting break offset.
     unsigned break_offset;
 
     // True if the break is hyphenated, either by automatic hyphenation or
@@ -58,10 +59,17 @@ class PLATFORM_EXPORT ShapingLineBreaker final {
 
   // Shapes a line of text by finding a valid and appropriate break opportunity
   // based on the shaping results for the entire paragraph.
-  // The output parameter breakOffset indicates the resulting break offset.
+  // |start_should_be_safe| is true for the beginning of each wrapped line, but
+  // is false for subsequent ShapeResults.
   scoped_refptr<ShapeResult> ShapeLine(unsigned start_offset,
-                                LayoutUnit available_space,
-                                Result* result_out);
+                                       LayoutUnit available_space,
+                                       bool start_should_be_safe,
+                                       Result* result_out);
+  scoped_refptr<ShapeResult> ShapeLine(unsigned start_offset,
+                                       LayoutUnit available_space,
+                                       Result* result_out) {
+    return ShapeLine(start_offset, available_space, true, result_out);
+  }
 
   // Disable breaking at soft hyphens (U+00AD).
   bool IsSoftHyphenEnabled() const { return is_soft_hyphen_enabled_; }
@@ -87,8 +95,8 @@ class PLATFORM_EXPORT ShapingLineBreaker final {
 
   scoped_refptr<ShapeResult> Shape(TextDirection, unsigned start, unsigned end);
   scoped_refptr<ShapeResult> ShapeToEnd(unsigned start,
-                                 LayoutUnit start_position,
-                                 unsigned range_end);
+                                        unsigned first_safe,
+                                        unsigned range_end);
 
   const HarfBuzzShaper* shaper_;
   const Font* font_;