[LayoutNG] Misc fixes in ShapingLineBreaker

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

Index: third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
diff --git a/third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp b/third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
index 5206620abd964b923089b5c2b7b48b99b87f3495..24692ffa77cf3392d428cf6f5e5926724d583616 100644
--- a/third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
+++ b/third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
@@ -12,16 +12,15 @@
 
 namespace blink {
 
-ShapingLineBreaker::ShapingLineBreaker(const HarfBuzzShaper* shaper,
-                                       const Font* font,
-                                       const ShapeResult* result,
-                                       const AtomicString locale,
-                                       LineBreakType break_type)
+ShapingLineBreaker::ShapingLineBreaker(
+    const HarfBuzzShaper* shaper,
+    const Font* font,
+    const ShapeResult* result,
+    const LazyLineBreakIterator* break_iterator)
     : shaper_(shaper),
       font_(font),
       result_(result),
-      locale_(locale),
-      break_type_(break_type) {
+      break_iterator_(break_iterator) {
   text_ = String(shaper->GetText(), shaper->TextLength());
 }
 
@@ -51,6 +50,10 @@ unsigned NextSafeToBreakBefore(const UChar* text,
   return offset;
 }
 
+LayoutUnit FlipRtl(LayoutUnit value, TextDirection direction) {
+  return direction == TextDirection::kRtl ? -value : value;
+}
+
 }  // namespace
 
 // Shapes a line of text by finding a valid and appropriate break opportunity
@@ -86,35 +89,52 @@ PassRefPtr<ShapeResult> ShapingLineBreaker::ShapeLine(
     unsigned start,
     LayoutUnit available_space,
     unsigned* break_offset) {
+  unsigned range_start = result_->CharacterStartIndex();
+  unsigned range_end = range_start + result_->NumCharacters();
+  DCHECK_GE(start, range_start);
+  DCHECK_LT(start, range_end);
+
   // The start position in the original shape results.
-  LayoutUnit start_position = result_->SnappedStartPositionForOffset(start);
+  float start_position_float = result_->PositionForOffset(start - range_start);
+  LayoutUnit start_position = LayoutUnit::FromFloatFloor(start_position_float);
   TextDirection direction = result_->Direction();
 
+  // Find a candidate break opportunity by identifying the last offset before
+  // exceeding the available space and the determine the closest valid break
+  // preceding the candidate.
+  LayoutUnit end_position = LayoutUnit::FromFloatCeil(start_position_float) +
+                            FlipRtl(available_space, direction);
+  unsigned candidate_break =
+      result_->OffsetForPosition(end_position,
+                                 ShapeResult::kOutsideAsStartOrEnd) +
+      range_start;
+  DCHECK_GE(candidate_break, start);
+  unsigned break_opportunity =
+      break_iterator_->PreviousBreakOpportunity(candidate_break, start);
+  if (break_opportunity <= start) {
+    break_opportunity = break_iterator_->NextBreakOpportunity(
+        std::max(candidate_break, start + 1));
+  }
+  DCHECK_GT(break_opportunity, start);
+
   // If the start offset is not at a safe-to-break boundary the content between
   // the start and the next safe-to-break boundary needs to be reshaped and the
   // available space adjusted to take the reshaping into account.
   RefPtr<ShapeResult> line_start_result;
   unsigned first_safe =
       NextSafeToBreakBefore(shaper_->GetText(), shaper_->TextLength(), start);
-  if (first_safe != start) {
+  DCHECK_GE(first_safe, start);
+  // Reshape takes place only when first_safe is before the break opportunity.

[eae, 2017/05/12 18:45:49]

I considered this case and wanted to fix it but failed to create a test case
where it would fail which is why I left it out of the original version. Did you
manage to come up with a test for it? If so, yay!

[kojii, 2017/05/12 19:08:13]

Yes, the Han character in your ShapeLineArabicThaiHanLatinBreakAll() hit this
when I added some new cases. Thank you for the test!

+  // Otherwise reshape will be part of line_end_result.
+  if (first_safe != start && first_safe < break_opportunity) {
     LayoutUnit original_width =
-        result_->SnappedEndPositionForOffset(first_safe) - start_position;
+        FlipRtl(result_->SnappedEndPositionForOffset(first_safe - range_start) -
+                    start_position,
+                direction);
     line_start_result = shaper_->Shape(font_, direction, start, first_safe);
     available_space += line_start_result->SnappedWidth() - original_width;
   }
 
-  // Find a candidate break opportunity by identifying the last offset before
-  // exceeding the available space and the determine the closest valid break
-  // preceding the candidate.
-  LazyLineBreakIterator break_iterator(text_, locale_, break_type_);
-  LayoutUnit end_position = start_position + available_space;
-  unsigned candidate_break = result_->OffsetForPosition(end_position, false);
-  unsigned break_opportunity =
-      break_iterator.PreviousBreakOpportunity(candidate_break, start);
-  if (break_opportunity <= start) {
-    break_opportunity = break_iterator.NextBreakOpportunity(candidate_break);
-  }
-
   RefPtr<ShapeResult> line_end_result;
   unsigned last_safe = break_opportunity;
   while (break_opportunity > start) {
@@ -125,17 +145,26 @@ PassRefPtr<ShapeResult> ShapingLineBreaker::ShapeLine(
     unsigned previous_safe = std::max(
         PreviousSafeToBreakAfter(shaper_->GetText(), start, break_opportunity),
         start);
+    DCHECK_LE(previous_safe, break_opportunity);
     if (previous_safe != break_opportunity) {
       LayoutUnit safe_position =
-          result_->SnappedStartPositionForOffset(previous_safe);
-      while (break_opportunity > previous_safe && previous_safe > start) {
+          result_->SnappedStartPositionForOffset(previous_safe - range_start);
+      while (break_opportunity > previous_safe && previous_safe >= start) {
         line_end_result =
-            shaper_->Shape(font_, direction, previous_safe, break_opportunity);
-        if (safe_position + line_end_result->SnappedWidth() <= end_position)
+            shaper_->Shape(font_, direction, previous_safe,
+                           std::min(break_opportunity, range_end));
+        if (line_end_result->SnappedWidth() <=
+            FlipRtl(end_position - safe_position, direction))
           break;
+        // Doesn't fit after the reshape. Try previous break opportunity, or
+        // overflow if there were none.
+        unsigned previous_break_opportunity =
+            break_iterator_->PreviousBreakOpportunity(break_opportunity - 1,
+                                                      start);
+        if (previous_break_opportunity <= start)
+          break;
+        break_opportunity = previous_break_opportunity;
         line_end_result = nullptr;
-        break_opportunity = break_iterator.PreviousBreakOpportunity(
-            break_opportunity - 1, start);
       }
     }
 
@@ -146,7 +175,7 @@ PassRefPtr<ShapeResult> ShapingLineBreaker::ShapeLine(
 
     // No suitable break opportunity, not exceeding the available space,
     // found. Choose the next valid one even though it will overflow.
-    break_opportunity = break_iterator.NextBreakOpportunity(candidate_break);
+    break_opportunity = break_iterator_->NextBreakOpportunity(candidate_break);
   }
 
   // Create shape results for the line by copying from the re-shaped result (if
@@ -160,6 +189,10 @@ PassRefPtr<ShapeResult> ShapingLineBreaker::ShapeLine(
   if (line_end_result)
     line_end_result->CopyRange(last_safe, max_length, line_result.Get());
 
+  DCHECK_GT(break_opportunity, start);
+  DCHECK_EQ(std::min(break_opportunity, range_end) - start,
+            line_result->NumCharacters());
+
   *break_offset = break_opportunity;
   return line_result.Release();
 }