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..7bd69cdbd5f287f8e2d6791d552d184f4d3c0e5e 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -45,13 +45,9 @@ const size_t kMaxTextLength = 10000;
 const size_t kMaxScripts = 5;
 
 // Returns true if characters of |block_code| may trigger font fallback.
-// Dingbats and emoticons can be rendered through the color emoji font file,
-// therefore it needs to be trigerred as fallbacks. See crbug.com/448909
 bool IsUnusualBlockCode(UBlockCode block_code) {
   return block_code == UBLOCK_GEOMETRIC_SHAPES ||
-         block_code == UBLOCK_MISCELLANEOUS_SYMBOLS ||
-         block_code == UBLOCK_DINGBATS ||
-         block_code == UBLOCK_EMOTICONS;
+         block_code == UBLOCK_MISCELLANEOUS_SYMBOLS;

[Jun Mukai, 2015/05/11 23:21:40]

ckocagil@ or msw@ -- is this function still necessary?

 }
 
 bool IsBracket(UChar32 character) {
@@ -90,15 +86,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;
 }
@@ -166,6 +159,9 @@ int ScriptInterval(const base::string16& text,
   *script = scripts[0];
 
   while (char_iterator.Advance()) {
+    // Special handling to merge white space into the previous run.
+    if (u_isUWhiteSpace(char_iterator.get()))
+      continue;
     ScriptSetIntersect(char_iterator.get(), scripts, &scripts_size);
     if (scripts_size == 0U)
       return char_iterator.array_pos();
@@ -239,7 +235,8 @@ class HarfBuzzLineBreaker {
         text_x_(0),
         line_x_(0),
         max_descent_(0),
-        max_ascent_(0) {
+        max_ascent_(0),
+        word_width_(0) {
     DCHECK_EQ(multiline_, (words_ != nullptr));
     AdvanceLine();
   }
@@ -250,8 +247,23 @@ class HarfBuzzLineBreaker {
     base::char16 first_char = text_[run->range.start()];
     if (multiline_ && first_char == '\n') {
       AdvanceLine();
-    } else if (multiline_ && (line_x_ + SkFloatToScalar(run->width)) >
-               max_width_) {
+      return;
+    }
+
+    if (multiline_ && line_x_ != 0) {

[Jun Mukai, 2015/05/11 23:21:40]

Why does this have to be here?
This break a run and call AdvanceLine(), which sounds like a role of BreakRun().

That way, a benefit is that you don't have to implement GetWordWidth() by
yourself, it's a part of BreakRunAtWidth().

+      // If the following word is not the first word in the current line and
+      // can't be fit into the current line, advance a new line.
+      // See kTestScenarios[2], kTestScenarios[7] and kTestScenarios[10] in
+      // RenderTextTest.Multiline_LineBreakerBehavior for example.
+      BreakList<size_t>::const_iterator word =
+          words_->GetBreak(run->range.start());
+      if (run->range.start() == word->first &&
+          line_x_ + GetWordWidth(word) > max_width_) {
+        AdvanceLine();
+      }
+    }
+
+    if (multiline_ && (line_x_ + SkFloatToScalar(run->width)) > max_width_) {
       BreakRun(run_index);
     } else {
       AddSegment(run_index, run->range, run->width);
@@ -276,6 +288,30 @@ class HarfBuzzLineBreaker {
     return &lines_[handle.first].segments[handle.second];
   }
 
+  // Gets the glyph width for |word|.
+  SkScalar GetWordWidth(BreakList<size_t>::const_iterator word) {
+    if (word == words_->breaks().end())
+      return 0;
+    size_t word_st = word->first;
+    size_t word_en =
+        (word + 1 == words_->breaks().end()) ? text_.size() : (word + 1)->first;
+    internal::TextRunList::TextRunHarfBuzzIter run_st =
+        run_list_.GetRunAt(word_st);
+    internal::TextRunList::TextRunHarfBuzzIter run_en =
+        run_list_.GetRunAt(word_en);
+    SkScalar width = 0;
+    for (auto iter = run_st; iter != run_en; iter++) {
+      Range char_range = (*iter)->range.Intersect(Range(word_st, word_en));
+      Range glyph_range = (*iter)->CharRangeToGlyphRange(char_range);
+      SkScalar char_width = ((glyph_range.end() >= (*iter)->glyph_count)
+                                 ? SkFloatToScalar((*iter)->width)
+                                 : (*iter)->positions[glyph_range.end()].x()) -
+                            (*iter)->positions[glyph_range.start()].x();
+      width += char_width;
+    }
+    return width;
+  }
+
   // Breaks a run into segments that fit in the last line in |lines_| and adds
   // them. Adds a new Line to the back of |lines_| whenever a new segment can't
   // be added without the Line's width exceeding |max_width_|.
@@ -317,8 +353,9 @@ 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;
+    size_t word_end_pos =
+        (next_word == words_->breaks().end()) ? text_.size() : next_word->first;
+    word_width_ = (start_char == word->first) ? 0 : word_width_;
     *width = 0;
 
     Range char_range;
@@ -328,17 +365,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++;
-        word_width = 0;
+        word_end_pos = (next_word == words_->breaks().end()) ? text_.size()
+                                                             : next_word->first;
+        word_width_ = 0;
       }
 
       Range glyph_range;
@@ -351,19 +381,31 @@ 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) {
-          // Roll back one word.
-          *width -= word_width;
+        if ((line_x_ != 0 && word_width_ <= *width) ||
+            (line_x_ == 0 && word_width_ < *width)) {
+          // If the run is not the first run in the current line (line_x_ != 0),
+          // rool back one word and advance a new line if the current word can't
+          // be added into the current line without exceeding |available_width|.
+          // If the run is the first run in the current line (line_x_ == 0),
+          // don't roll back if the current word width is larger than the line
+          // width to avoid constructing empty lines.
+          // See kTestScenarios[0-1] in
+          // RenderTextTest.Multiline_LineBreakerBehavior.
+          *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) {
+          if (char_width < *width ||
+              (word_width_ > *width && char_width <= *width)) {
             // Roll back one character.
+            // See kTestScenarios[8] for the first clause and kTestScenarios[9]
+            // for the second in RenderTextTest.Multiline_LineBreakerBehavior.
             *width -= char_width;
             *next_char = i;
           } else {
@@ -371,7 +413,24 @@ 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;
+          if (i < word_end_pos - 1) {
+            // Ignore all characters that belongs to the current word which has
+            // been truncated. Use kTestScenarios[5] as example. "that's good"
+            // has three runs: "that", "'", "s good". Suppose each line could
+            // fit into 4 characters, then the expected 2 lines should be:
+            // "that", "good", which means run "'" should be totally ignored and
+            // run "s good" should be partially ignored.
+            return false;
+          } else {
+            // Only advance a new line when the current word has been processed.
+            word_width_ = 0;
+            return true;
+          }
         }
       } else {
         *end_char = char_range.end();
@@ -423,6 +482,7 @@ class HarfBuzzLineBreaker {
     max_descent_ = 0;
     max_ascent_ = 0;
     line_x_ = 0;
+    word_width_ = 0;
     lines_.push_back(internal::Line());
   }
 
@@ -468,6 +528,30 @@ class HarfBuzzLineBreaker {
     }
     text_x_ += SkFloatToScalar(width);
     line_x_ += SkFloatToScalar(width);
+
+    // Calculate |word_width_|.

[Jun Mukai, 2015/05/11 23:21:40]

If you modify AddRun() to invoke always BreakRun() if |multiline_| (regardless
of run's width), then BreakRunAtWidth() can compute the word width. Then you
don't have to compute here.

Right?

+    if (multiline_) {
+      BreakList<size_t>::const_iterator word_for_range_start =
+          words_->GetBreak(char_range.start());
+      BreakList<size_t>::const_iterator word_for_range_end =
+          (char_range.end() == 0) ? words_->breaks().begin()
+                                  : words_->GetBreak(char_range.end() - 1);
+
+      if (word_for_range_start == word_for_range_end) {
+        // Current word covers the entire char_range.
+        if (word_width_ == 0) {
+          Range glyph_range = run.CharRangeToGlyphRange(char_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 {
+        // Current char_range intersects with more than one word.
+        word_width_ = char_range.end() - word_for_range_end->first;
+      }
+    }
   }
 
   const SkScalar max_width_;
@@ -489,6 +573,11 @@ class HarfBuzzLineBreaker {
   float max_descent_;
   float max_ascent_;
 
+  // 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_;
 
@@ -660,6 +749,15 @@ void TextRunList::ComputePrecedingRunWidths() {
   }
 }
 
+TextRunList::TextRunHarfBuzzIter TextRunList::GetRunAt(size_t position) const {
+  for (TextRunList::TextRunHarfBuzzIter iter = runs_.begin();
+       iter != runs_.end(); iter++) {
+    if ((*iter)->range.start() <= position && (*iter)->range.end() > position)
+      return iter;
+  }
+  return runs_.end();
+}
+
 }  // namespace internal
 
 RenderTextHarfBuzz::RenderTextHarfBuzz()