Move allow_character_break property to RenderText.

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 c931f22216e57951854e33322a597d7271c4ea37..7e2c29860b5400ec4a02e4477b8228be27b654c0 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -224,6 +224,7 @@ class HarfBuzzLineBreaker {
                       int min_baseline,
                       float min_height,
                       bool multiline,
+                      WordWrapBehavior word_wrap_behavior,
                       const base::string16& text,
                       const BreakList<size_t>* words,
                       const internal::TextRunList& run_list)
@@ -231,6 +232,7 @@ class HarfBuzzLineBreaker {
         min_baseline_(min_baseline),
         min_height_(min_height),
         multiline_(multiline),
+        word_wrap_behavior_(word_wrap_behavior),
         text_(text),
         words_(words),
         run_list_(run_list),
@@ -285,9 +287,10 @@ class HarfBuzzLineBreaker {
     // Break the run until it fits the current line.
     while (next_char < run.range.end()) {
       const size_t current_char = next_char;
+      size_t end_char = next_char;
       const bool skip_line =
-          BreakRunAtWidth(run, current_char, &width, &next_char);
-      AddSegment(run_index, Range(current_char, next_char),
+          BreakRunAtWidth(run, current_char, &width, &end_char, &next_char);
+      AddSegment(run_index, Range(current_char, end_char),
                  SkScalarToFloat(width));
       if (skip_line)
         AdvanceLine();
@@ -298,7 +301,8 @@ class HarfBuzzLineBreaker {
   // before available width using word break. If the current position is at the
   // beginning of a line, this function will not roll back to |start_char| and
   // |*next_char| will be greater than |start_char| (to avoid constructing empty
-  // lines).
+  // lines). It stores the end of the segment range to |end_char|, which can be
+  // smaller than |*next_char| if word_wrap_behavior is TRUNCATE_LONG_WORDS.

[msw, 2015/03/26 19:36:29]

nit: "smaller than |*next_char| for certain word wrapping behaviors."
(that should eventually be the case for elide too, right?)

[Jun Mukai, 2015/03/26 22:47:38]

Done.

   // Returns whether to skip the line before |*next_char|.
   // TODO(ckocagil): We might have to reshape after breaking at ligatures.
   //                 See whether resolving the TODO above resolves this too.
@@ -306,6 +310,7 @@ class HarfBuzzLineBreaker {
   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)));
@@ -317,11 +322,20 @@ class HarfBuzzLineBreaker {
     *width = 0;
 
     Range char_range;
+    bool truncated = false;
     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) {
+          DCHECK(word_wrap_behavior_ == IGNORE_LONG_WORDS ||

[msw, 2015/03/26 19:36:30]

nit: maybe just DCHECK_NE WRAP_LONG_WORDS?

[Jun Mukai, 2015/03/26 22:47:38]

Done.

+                 word_wrap_behavior_ == TRUNCATE_LONG_WORDS);
+          *next_char = i;
+          if (!truncated)
+            *end_char = *next_char;
+          return true;
+        }
         word = next_word++;
         word_width = 0;
       }
@@ -338,24 +352,35 @@ class HarfBuzzLineBreaker {
       *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;
           *next_char = std::max(word->first, start_char);
-        } else 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) {
+          *end_char = i;

[msw, 2015/03/26 19:36:30]

Shouldn't end_char be set as soon as the width exceeds the available width, and
not afterward? Otherwise, doesn't this extend end_char for each overflowing
char, so that it'll match next_char? ie. shouldn't the condition above be:
        } else if (word_wrap_behavior_ == TRUNCATE_LONG_WORDS && !truncated) {

Actually, I wonder if you can simplify the code a bit:
1) Remove this "else if" block and |truncated|.
2) After "if (*width > available_width)" add "else { *end_char = i; }".
3) Change 335 and 382 to check "if (word_wrap_behavior_ == IGNORE_LONG_WORDS)".

[Jun Mukai, 2015/03/26 22:47:38]

You are right, "!truncated" is necessary in the condition. My test case didn't
catch that. Thanks!

however I'm afraid the latter approach doesn't work, anyways truncated flag is
necessary to truncate in middle of a word, we want to record the first character
which exceeds the available width (when the behavior is TRUNCATE).

[msw, 2015/03/26 23:05:54]

Hmm, if we only set "*end_char = i;" if (*width <= available_width), as I
suggested in #2 above, and when (word_wrap_behavior_ != TRUNCATE_LONG_WORDS) for
lines 335 and 383, as I suggested in #3, wouldn't that correctly capture the
first character that exceeds the available width?

[Jun Mukai, 2015/03/27 00:31:58]

Aagh, I see. Sorry for my poor understanding, now I got it.
Fixed (to be more precise, *end_char is not i, it's char_range.end(), otherwise
it's truncated a bit more aggressively).

+          truncated = 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;
         }
-        return true;
       }
     }
 
     *next_char = run.range.end();
+    if (!truncated)
+      *end_char = *next_char;
     return false;
   }
 
@@ -448,6 +473,7 @@ class HarfBuzzLineBreaker {
   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_;
@@ -997,7 +1023,8 @@ void RenderTextHarfBuzz::EnsureLayout() {
     HarfBuzzLineBreaker line_breaker(
         display_rect().width(), font_list().GetBaseline(),
         std::max(font_list().GetHeight(), min_line_height()), multiline(),
-        GetDisplayText(), multiline() ? &GetLineBreaks() : nullptr, *run_list);
+        word_wrap_behavior(), GetDisplayText(),
+        multiline() ? &GetLineBreaks() : nullptr, *run_list);
 
     // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed.
     tracked_objects::ScopedTracker tracking_profile3(