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..0d72cb28a21bf77e1866f5a56ee76e1c11b134d1 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,11 @@ 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())) {

[Jun Mukai, 2015/04/28 01:57:36]

I am not so sure about other whitespace variations supports.  Did you confirm
that the Thai font can draw fullwidth space (U+3000) or some other spaces?
Otherwise directly comparing with ' ' would be safer.

[xdai1, 2015/04/29 18:23:27]

I added a test case for fullwidth space in
RenderTextTest.Multiline_LineBreakerBehavior. Based on the observation, it works
exactly the same as the normal space. I think it should be fine?

+      scripts[0] = *script;
+      scripts_size = 1U;
+    }
     if (scripts_size == 0U)
       return char_iterator.array_pos();
     *script = scripts[0];
@@ -239,7 +241,8 @@ class HarfBuzzLineBreaker {
         text_x_(0),
         line_x_(0),
         max_descent_(0),
-        max_ascent_(0) {
+        max_ascent_(0),
+        next_word_start_pos_(0) {
     DCHECK_EQ(multiline_, (words_ != nullptr));
     AdvanceLine();
   }
@@ -248,6 +251,15 @@ 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.
+      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)) >
@@ -317,8 +329,12 @@ 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;
+    next_word_start_pos_ =
+        (next_word == words_->breaks().end()) ? text_.size() : next_word->first;
+
+    // Glyph width of text_[word->first, start_char - 1].
+    SkScalar word_width =
+        start_char == 0 ? 0 : GetGlyphWidth(word->first, start_char - 1);

[Jun Mukai, 2015/04/28 01:57:36]

I think you don't have to add a method like 'GetGlyphWidth', the width in the
past runs (but in a same word) are already added and computed, you can just
remember them.

GetGlyphWidth() name and interface sounds too generic and I'm afraid it could
include tricky corner cases like mixture of RTL and LTR, in the middle of runs
and/or words. Those are not our expected cases I think. 

[xdai1, 2015/04/29 18:23:27]

Done.

     *width = 0;
 
     Range char_range;
@@ -328,16 +344,10 @@ 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++;
+        next_word_start_pos_ = (next_word == words_->breaks().end())
+                                   ? text_.size()
+                                   : next_word->first;
         word_width = 0;
       }
 
@@ -355,7 +365,8 @@ class HarfBuzzLineBreaker {
 
       // 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)) {

[Jun Mukai, 2015/04/28 01:57:36]

I don't get these conditions.  If line_x_ is 0 (i.e. this run appears at the
beginning of a line), it should never wrap the word unnecessary because it
causes to leave an empty line. Am I wrong?

[xdai1, 2015/04/29 18:23:27]

For example, "I am a student", suppose the line width is 10 (which means it
should break before "d" character). When we come to "d", line_x_ equals 0, the
word_width_ equals 4, and the *width equals 11, then we know we should roll back
one word and advance a new line. 

           // Roll back one word.
           *width -= word_width;
           *next_char = std::max(word->first, start_char);
@@ -372,6 +383,10 @@ class HarfBuzzLineBreaker {
           }
           *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();
@@ -470,6 +485,65 @@ class HarfBuzzLineBreaker {
     line_x_ += SkFloatToScalar(width);
   }
 
+  // Returns the index for the run that contains text_[pos] using binary search.
+  size_t GetRunIndexAt(size_t pos) const {
+    pos = (pos < text_.size()) ? pos : text_.size() - 1;
+    size_t low = 0, high = run_list_.size() - 1;
+    while (low <= high) {
+      size_t mid = low + (high - low) / 2;
+      if (run_list_.runs()[mid]->range.start() <= pos &&
+          run_list_.runs()[mid]->range.end() > pos)
+        return mid;
+      else if (run_list_.runs()[mid]->range.start() > pos)
+        high = mid - 1;
+      else
+        low = mid + 1;
+    }
+    return 0;
+  }
+
+  // Returns the glyph width for |text_| between [start_pos, end_pos].
+  SkScalar GetGlyphWidth(size_t start_pos, size_t end_pos) const {
+    end_pos = (end_pos < text_.size()) ? end_pos : text_.size() - 1;
+    if (start_pos > end_pos)
+      return 0;
+
+    // Finding the runs that containing |start_char| and |end_char|.
+    size_t start_run_idx = GetRunIndexAt(start_pos);
+    size_t end_run_idx = GetRunIndexAt(end_pos);
+    const internal::TextRunHarfBuzz* start_run =
+        run_list_.runs()[start_run_idx];
+    const internal::TextRunHarfBuzz* end_run = run_list_.runs()[end_run_idx];
+
+    SkScalar width = 0;
+    for (size_t i = start_run_idx; i <= end_run_idx; ++i) {
+      width += run_list_.runs()[i]->width;
+    }
+
+    Range char_range;
+    Range glyph_range;
+    for (size_t pos = start_run->range.start(); pos < start_pos; ++pos) {
+      start_run->GetClusterAt(pos, &char_range, &glyph_range);
+      SkScalar char_width =
+          ((glyph_range.end() >= start_run->glyph_count)
+               ? SkFloatToScalar(start_run->width)
+               : start_run->positions[glyph_range.end()].x()) -
+          start_run->positions[glyph_range.start()].x();
+      width -= char_width;
+    }
+
+    for (size_t pos = end_pos + 1; pos < end_run->range.end(); ++pos) {
+      end_run->GetClusterAt(pos, &char_range, &glyph_range);
+      SkScalar char_width = ((glyph_range.end() >= end_run->glyph_count)
+                                 ? SkFloatToScalar(end_run->width)
+                                 : end_run->positions[glyph_range.end()].x()) -
+                            end_run->positions[glyph_range.start()].x();
+      width -= char_width;
+    }
+
+    return width;
+  }
+
   const SkScalar max_width_;
   const int min_baseline_;
   const float min_height_;
@@ -489,6 +563,9 @@ class HarfBuzzLineBreaker {
   float max_descent_;
   float max_ascent_;
 
+  // Stores the start position of next word that need to be processed.
+  size_t next_word_start_pos_;
+
   // Size of the multiline text, not including the currently processed line.
   SizeF total_size_;