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

ui/gfx/render_text_harfbuzz.cc

Index: ui/gfx/render_text_harfbuzz.cc
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 32a9e554f60b47127cef9d426aef71a9f410adfa..0cfe31f0388bee413e04b6ff3e2af1f72106438f 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -90,15 +90,12 @@ size_t FindRunBreakingCharacter(const base::string16& text,
   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;
 }
@@ -167,6 +164,9 @@ int ScriptInterval(const base::string16& text,
 
   while (char_iterator.Advance()) {
     ScriptSetIntersect(char_iterator.get(), scripts, &scripts_size);
+    // Special handling to merge white space into the previous run.
+    if (u_isUWhiteSpace(char_iterator.get()))
+      continue;
     if (scripts_size == 0U)
       return char_iterator.array_pos();
     *script = scripts[0];
@@ -239,7 +239,9 @@ class HarfBuzzLineBreaker {
         text_x_(0),
         line_x_(0),
         max_descent_(0),
-        max_ascent_(0) {
+        max_ascent_(0),
+        word_end_pos_(0),
+        word_width_(0) {
     DCHECK_EQ(multiline_, (words_ != nullptr));
     AdvanceLine();
   }
@@ -248,6 +250,20 @@ class HarfBuzzLineBreaker {
   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.
+      BreakList<size_t>::const_iterator next_word =
+          words_->GetBreak(run->range.start()) + 1;
+      size_t current_word_end_pos = next_word == words_->breaks().end()
+          ? text_.size()
+          : next_word->first;
+      if (current_word_end_pos <= word_end_pos_ && first_char != '\n') {

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

This will ignore too much if TRUNCATE_LONG_WORDS is set and the run contains
multiple words?
Think about the example like "averyverylongword's test". The "s " should be
truncated but "test" should appear in the next line.

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

Because of the way we handle the white space, it won't get this trouble. For
this example, there are four runs here: "averyverylongword", "'", "s ", "test",
and two words here: "averyverylongword's " and "test". So it only ignore "'" and
"s ", but doesn't ignore the run "test".

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

Wait. Do we split runs by whitespaces?  Don't we want to concatenate them due to
the performance reason ckocagil pointed out?

+        word_width_ = 0;
+        return;
+      }
+    }
+
     if (multiline_ && first_char == '\n') {
       AdvanceLine();
     } else if (multiline_ && (line_x_ + SkFloatToScalar(run->width)) >
@@ -317,8 +333,8 @@ class HarfBuzzLineBreaker {
     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;
+    word_end_pos_ =
+        (next_word == words_->breaks().end()) ? text_.size() : next_word->first;
     *width = 0;
 
     Range char_range;
@@ -328,17 +344,11 @@ class HarfBuzzLineBreaker {
       // 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) {
-          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;
+        word_end_pos_ = (next_word == words_->breaks().end())
+                                   ? text_.size()
+                                   : next_word->first;
+        word_width_ = 0;
       }
 
       Range glyph_range;
@@ -351,15 +361,17 @@ class HarfBuzzLineBreaker {
                             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) {
@@ -371,6 +383,12 @@ class HarfBuzzLineBreaker {
             *next_char = i + char_range.length();
           }
           *end_char = *next_char;
+          word_width_ = 0;
+          return true;
+        } else if (word_wrap_behavior_ == TRUNCATE_LONG_WORDS) {
+          *width = truncated_width;
+          *next_char = word_end_pos_;
+          word_width_ = 0;
           return true;
         }
       } else {
@@ -423,6 +441,7 @@ class HarfBuzzLineBreaker {
     max_descent_ = 0;
     max_ascent_ = 0;
     line_x_ = 0;
+    word_width_ = 0;
     lines_.push_back(internal::Line());
   }
 
@@ -468,6 +487,30 @@ class HarfBuzzLineBreaker {
     }
     text_x_ += SkFloatToScalar(width);
     line_x_ += SkFloatToScalar(width);
+
+    // Calculate |word_width_|.
+    if (words_) {
+      BreakList<size_t>::const_iterator word_for_current_run =

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

This isn't correct if the segment contains more than two words.
Consider example of "foo bar baz, quox".  For the first segment, |char_range| is
(0, 11) but the word_width should be computed from the last word "baz" whose
range is (8, 11)

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

Because of the way we handle white space, there are 5 runs here: "foo ", "bar ",
"baz", ", ", "quox". Since we add runs one by one, so it should be fine here?

+          words_->GetBreak(char_range.start());
+      BreakList<size_t>::const_iterator word_for_next_run =
+          (char_range.end() == text_.size())
+          ? words_->breaks().end()
+          : words_->GetBreak(char_range.end());
+      if (word_width_ == 0 && word_for_current_run == word_for_next_run) {
+        Range tmp_range;
+        Range glyph_range;
+        for (size_t pos = char_range.start(); pos < char_range.end(); ++pos) {

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

TextRunHarfBuzz::CharRangeToGlyphRange() can be used.

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

Done.

+          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 (word_for_current_run != word_for_next_run) {
+        word_width_ = 0;
+      }
+    }
   }
 
   const SkScalar max_width_;
@@ -489,6 +532,13 @@ class HarfBuzzLineBreaker {
   float max_descent_;
   float max_ascent_;
 
+  // Stores the end position of the current word in process.
+  size_t word_end_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_;