Stop combining text runs which are connected by 'COMMON' blocks.

ui/gfx/render_text_harfbuzz.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text_harfbuzz.h"
 
 #include <limits>
 #include <set>
 
 #include "base/i18n/bidi_line_iterator.h"
@@ skipping 72 matching lines @@
     const bool block_break = current_block != first_block &&
         (first_block_unusual || IsUnusualBlockCode(current_block));
     if (block_break || current_char == '\n' ||
         first_bracket != IsBracket(current_char)) {
       return run_start + iter.array_pos();
     }
   }
   return run_break;
 }
 
-// If the given scripts match, returns the one that isn't USCRIPT_COMMON or
-// USCRIPT_INHERITED, i.e. the more specific one. Otherwise returns
-// USCRIPT_INVALID_CODE.
+// If the given scripts match, returns the one that isn't USCRIPT_INHERITED,
+// i.e. the more specific one. Otherwise returns USCRIPT_INVALID_CODE.
 UScriptCode ScriptIntersect(UScriptCode first, UScriptCode second) {
-  if (first == second ||
-      (second > USCRIPT_INVALID_CODE && second <= USCRIPT_INHERITED)) {
+  if (first == second || second == USCRIPT_INHERITED)
     return first;
-  }
-  if (first > USCRIPT_INVALID_CODE && first <= USCRIPT_INHERITED)
+  if (first == USCRIPT_INHERITED)
     return second;
   return USCRIPT_INVALID_CODE;
 }
 
 // Writes the script and the script extensions of the character with the
 // Unicode |codepoint|. Returns the number of written scripts.
 int GetScriptExtensions(UChar32 codepoint, UScriptCode* scripts) {
   UErrorCode icu_error = U_ZERO_ERROR;
   // ICU documentation incorrectly states that the result of
   // |uscript_getScriptExtensions| will contain the regular script property.
@@ skipping 46 matching lines @@
                    size_t length,
                    UScriptCode* script) {
   DCHECK_GT(length, 0U);
 
   UScriptCode scripts[kMaxScripts] = { USCRIPT_INVALID_CODE };
 
   base::i18n::UTF16CharIterator char_iterator(text.c_str() + start, length);
   size_t scripts_size = GetScriptExtensions(char_iterator.get(), scripts);
   *script = scripts[0];
 
   while (char_iterator.Advance()) {

[Jun Mukai, 2015/04/29 21:09:07]

Hm, why not just doing
if (u_isUWhiteSpace(char_iterator.get()))
  continue;
?
You don't have to compute ScriptSetIntersect for whitespaces, anyways it
shouldn't separate the text runs.

[xdai1, 2015/05/05 18:27:49]

As I said in the previous comment to msw@, this is depending on how we want to
break the text into runs. For example, suppose we have text "abc defg". If we
use  "continue;" here, then we'll get two runs: "abc " and "defg", but if we
fake out |scripts| and |scripts_size|, then we'll get one single run: "abc
defg". I think these two approaches are both fine. But since the previous one
makes more sense, I'll accept your suggestion and use "continue;" here.

     ScriptSetIntersect(char_iterator.get(), scripts, &scripts_size);
+    // Special handling to merge white space into the previous run.
+    if (u_isUWhiteSpace(char_iterator.get())) {
+      scripts[0] = *script;
+      scripts_size = 1U;
+    }
     if (scripts_size == 0U)
       return char_iterator.array_pos();
     *script = scripts[0];
   }
 
   return length;
 }
 
 // A port of hb_icu_script_to_script because harfbuzz on CrOS is built without
 // hb-icu. See http://crbug.com/356929
@@ skipping 52 matching lines @@
         min_baseline_(min_baseline),
         min_height_(min_height),
         multiline_(multiline),
         word_wrap_behavior_(word_wrap_behavior),
         text_(text),
         words_(words),
         run_list_(run_list),
         text_x_(0),
         line_x_(0),
         max_descent_(0),
-        max_ascent_(0) {
+        max_ascent_(0),
+        next_word_start_pos_(0),
+        word_width_(0) {
     DCHECK_EQ(multiline_, (words_ != nullptr));
     AdvanceLine();
   }
 
   // Breaks the run at given |run_index| into Line structs.
   void AddRun(int run_index) {
     const internal::TextRunHarfBuzz* run = run_list_.runs()[run_index];
     base::char16 first_char = text_[run->range.start()];
+
+    if (word_wrap_behavior_ == TRUNCATE_LONG_WORDS && words_) {
+      // If the run has been processed, skip it.
+      size_t current_word_start_pos =
+          words_->GetBreak(run->range.start())->first;
+      if (current_word_start_pos < next_word_start_pos_)
+        return;
+    }
+
     if (multiline_ && first_char == '\n') {
       AdvanceLine();
     } else if (multiline_ && (line_x_ + SkFloatToScalar(run->width)) >
                max_width_) {
       BreakRun(run_index);
     } else {
       AddSegment(run_index, run->range, run->width);
     }
   }
 
@@ skipping 49 matching lines @@
   bool BreakRunAtWidth(const internal::TextRunHarfBuzz& run,
                        size_t start_char,
                        SkScalar* width,
                        size_t* end_char,
                        size_t* next_char) {
     DCHECK(words_);
     DCHECK(run.range.Contains(Range(start_char, start_char + 1)));
     SkScalar available_width = max_width_ - line_x_;
     BreakList<size_t>::const_iterator word = words_->GetBreak(start_char);
     BreakList<size_t>::const_iterator next_word = word + 1;
-    // Width from |std::max(word->first, start_char)| to the current character.
-    SkScalar word_width = 0;
+    next_word_start_pos_ =

[Jun Mukai, 2015/04/29 21:09:07]

Probably word_end_pos_ would be a better name?

[xdai1, 2015/05/05 18:27:49]

Done.

+        (next_word == words_->breaks().end()) ? text_.size() : next_word->first;
     *width = 0;
 
     Range char_range;
     SkScalar truncated_width = 0;
     for (size_t i = start_char; i < run.range.end(); i += char_range.length()) {
       // |word| holds the word boundary at or before |i|, and |next_word| holds
       // the word boundary right after |i|. Advance both |word| and |next_word|
       // when |i| reaches |next_word|.
       if (next_word != words_->breaks().end() && i >= next_word->first) {
-        if (*width > available_width) {

[Jun Mukai, 2015/04/29 21:09:07]

I am not sure if removing this block works correctly.  Consider the input is
"aaaaa-b ccc" and the given width is for 3 characters, behavior is
IGNORE_LONG_WORDS.

In that case:
- first run "aaaaa" runs and no problem
- second run "-" has no problem
- third run "b ccc". It first scan "b ", it's *width is already bigger than
available_width, it needs to return true here, otherwise it continues to show
"ccc" in the same line.

[xdai1, 2015/05/05 18:27:49]

In third run it won't come inside this inner if because it can't pass the outer
if (next_word == words_->breaks().end()).

I also tested it by writing a test case and it works as expected. It creates
three lines (because it contains three words: [aaaaa-], [b ], [ccc]):
aaaaa-
b
ccc
Actually it just postpones returning at line 387.

[Jun Mukai, 2015/05/06 17:38:52]

For line 387, the behavior has to be TRUNCATE_LONG_WORDS. Consider the behavior
is IGNORE_LONG_WORDS.  In the case I commented above,
- *width > available_width, but
- line_x_ > 0 and word_width_ > *width
- word_wrap_behavior_ is neither WRAP_LONG_WORDS nor TRUNCATE_LONG_WORDS

Therefore, it doesn't match any cases in the if-clause of line 365-388.  It
eventually arrives here.

[xdai1, 2015/05/06 20:23:44]

In this case when it comes to "b", it will return in line 375. Because:
1) "b " is a single run, it doesn't combined into the previous "aaaaa-" run.
2) line_x_ != 0 && word_width_ == *width
I also added this test case to RenderTextTest.Multiline_LineBreakerBehavior.

[Jun Mukai, 2015/05/06 20:47:55]

No, "b " and "ccc" must be in a single run, they are only separated by a
whitespace.
word_width_ shouldn't equal to *width.  *width starts from 0 in each iteration
of this method, but word_width_ should start from 'aaaaa-' (oh, if hyphen "-"
splits into words, think that "aaaaa'b ccc").
My point is, think that a run contains multiple words (like "b ccc"), and its
first word ("b ") is actually a part of a larger word including previous runs
("aaaaa'"), and it's involved by linebreaks.
I don't get how your code deals with such cases.

-          DCHECK_NE(WRAP_LONG_WORDS, word_wrap_behavior_);
-          *next_char = i;
-          if (word_wrap_behavior_ != TRUNCATE_LONG_WORDS)
-            *end_char = *next_char;
-          else
-            *width = truncated_width;
-          return true;
-        }
         word = next_word++;
-        word_width = 0;
+        next_word_start_pos_ = (next_word == words_->breaks().end())
+                                   ? text_.size()
+                                   : next_word->first;
+        word_width_ = 0;
       }
 
       Range glyph_range;
       run.GetClusterAt(i, &char_range, &glyph_range);
       DCHECK_LT(0U, char_range.length());
 
       SkScalar char_width = ((glyph_range.end() >= run.glyph_count)
                                  ? SkFloatToScalar(run.width)
                                  : run.positions[glyph_range.end()].x()) -
                             run.positions[glyph_range.start()].x();
 
       *width += char_width;
-      word_width += char_width;
+      word_width_ += char_width;
 
       // TODO(mukai): implement ELIDE_LONG_WORDS.
       if (*width > available_width) {
-        if (line_x_ != 0 || word_width < *width) {
+        if ((line_x_ != 0 && word_width_ <= *width) ||
+            (line_x_ == 0 && word_width_ < *width)) {
           // Roll back one word.
-          *width -= word_width;
+          *width -= word_width_;
           *next_char = std::max(word->first, start_char);
           *end_char = *next_char;
+          word_width_ = 0;
           return true;
         } else if (word_wrap_behavior_ == WRAP_LONG_WORDS) {
           if (char_width < *width) {
             // Roll back one character.
             *width -= char_width;
             *next_char = i;
           } else {
             // Continue from the next character.
             *next_char = i + char_range.length();
           }
           *end_char = *next_char;
           return true;
+        } else if (word_wrap_behavior_ == TRUNCATE_LONG_WORDS) {
+          *width = truncated_width;
+          *next_char = next_word_start_pos_;
+          return true;
         }
       } else {
         *end_char = char_range.end();
         truncated_width = *width;
       }
     }
 
     if (word_wrap_behavior_ == TRUNCATE_LONG_WORDS)
       *width = truncated_width;
     *end_char = *next_char = run.range.end();
@@ skipping 31 matching lines @@
       line->size.set_height(std::max(min_height_, max_descent_ + max_ascent_));
       line->baseline =
           std::max(min_baseline_, SkScalarRoundToInt(max_ascent_));
       line->preceding_heights = std::ceil(total_size_.height());
       total_size_.set_height(total_size_.height() + line->size.height());
       total_size_.set_width(std::max(total_size_.width(), line->size.width()));
     }
     max_descent_ = 0;
     max_ascent_ = 0;
     line_x_ = 0;
+    word_width_ = 0;
     lines_.push_back(internal::Line());
   }
 
   // Adds a new segment with the given properties to |lines_.back()|.
   void AddSegment(int run_index, Range char_range, float width) {
     if (char_range.is_empty()) {
       DCHECK_EQ(0, width);
       return;
     }
     const internal::TextRunHarfBuzz& run = *(run_list_.runs()[run_index]);
@@ skipping 25 matching lines @@
     if (run.is_rtl) {
       rtl_segments_.push_back(
           SegmentHandle(lines_.size() - 1, line->segments.size() - 1));
       // If this is the last segment of an RTL run, reprocess the text-space x
       // ranges of all segments from the run.
       if (char_range.end() == run.range.end())
         UpdateRTLSegmentRanges();
     }
     text_x_ += SkFloatToScalar(width);
     line_x_ += SkFloatToScalar(width);
+
+    // Calculate |word_width_|.
+    if (words_) {
+      BreakList<size_t>::const_iterator current_word =
+          (char_range.start() == 0) ? words_->breaks().begin()
+                                    : words_->GetBreak(char_range.end() - 1);
+      BreakList<size_t>::const_iterator next_word =
+          (char_range.end() == text_.size())
+              ? words_->breaks().end()
+              : words_->GetBreak(char_range.end());
+      if (word_width_ == 0 && current_word == next_word) {
+        Range tmp_range;
+        Range glyph_range;
+        for (size_t pos = current_word->first; pos < char_range.end(); ++pos) {
+          run.GetClusterAt(pos, &tmp_range, &glyph_range);
+          SkScalar char_width = ((glyph_range.end() >= run.glyph_count)
+                                     ? SkFloatToScalar(run.width)
+                                     : run.positions[glyph_range.end()].x()) -
+                                run.positions[glyph_range.start()].x();
+          word_width_ += char_width;
+        }
+      } else if (current_word != next_word) {
+        word_width_ = 0;
+      }
+    }
   }
 
   const SkScalar max_width_;
   const int min_baseline_;
   const float min_height_;
   const bool multiline_;
   const WordWrapBehavior word_wrap_behavior_;
   const base::string16& text_;
   const BreakList<size_t>* const words_;
   const internal::TextRunList& run_list_;
 
   // Stores the resulting lines.
   std::vector<internal::Line> lines_;
 
   // Text space and line space x coordinates of the next segment to be added.
   SkScalar text_x_;
   SkScalar line_x_;
 
   float max_descent_;
   float max_ascent_;
 
+  // Stores the start position of next word that need to be processed.
+  size_t next_word_start_pos_;
+  // Stores the glyph width between the current position and the start position
+  // of the current word. If the current position is pointing to the first
+  // character of the current word, |word_width_| is set to 0.
+  SkScalar word_width_;
+
   // Size of the multiline text, not including the currently processed line.
   SizeF total_size_;
 
   // The current RTL run segments, to be applied by |UpdateRTLSegmentRanges()|.
   std::vector<SegmentHandle> rtl_segments_;
 
   DISALLOW_COPY_AND_ASSIGN(HarfBuzzLineBreaker);
 };
 
 // Function object for case insensitive string comparison.
@@ skipping 977 matching lines @@
   DCHECK(!update_layout_run_list_);
   DCHECK(!update_display_run_list_);
   return text_elided() ? display_run_list_.get() : &layout_run_list_;
 }
 
 const internal::TextRunList* RenderTextHarfBuzz::GetRunList() const {
   return const_cast<RenderTextHarfBuzz*>(this)->GetRunList();
 }
 
 }  // namespace gfx