Improve text elision in TruncateString().

ui/gfx/text_elider.cc

Index: ui/gfx/text_elider.cc
diff --git a/ui/gfx/text_elider.cc b/ui/gfx/text_elider.cc
index 7619f86f9295bdc6e2d5cf7d74da367904e4709d..a751340c8e15244e294e89bbfb273cb945021a34 100644
--- a/ui/gfx/text_elider.cc
+++ b/ui/gfx/text_elider.cc
@@ -752,7 +752,8 @@ int ElideRectangleText(const base::string16& input,
 base::string16 TruncateString(const base::string16& string,
                               size_t length,
                               BreakType break_type) {
-  DCHECK(break_type == CHARACTER_BREAK || break_type == WORD_BREAK);
+  const bool word_break = break_type == WORD_BREAK;
+  DCHECK(word_break || (break_type == CHARACTER_BREAK));
 
   if (string.size() <= length)
     // String fits, return it.

[msw, 2015/09/17 18:42:44]

unrelated nit: please move the comment above the if statement or add curlies;
ditto for the other two early returns below.

[Peter Kasting, 2015/09/19 02:43:25]

Fixed by exchanging these for some EOL comments (which I rewrote slightly).

@@ -762,35 +763,35 @@ base::string16 TruncateString(const base::string16& string,
     // No room for the elide string, return an empty string.
     return base::string16();
 
-  size_t max = length - 1;
-
   // Added to the end of strings that are too big.
   static const base::char16 kElideString[] = { 0x2026, 0 };
 
-  if (max == 0)
+  if (length == 1)
     // Just enough room for the elide string.
     return kElideString;
 
-  int32_t index = static_cast<int32_t>(max);
-  if (break_type == WORD_BREAK) {
-    // Use a line iterator to find the first boundary.
+  int32_t index = static_cast<int32_t>(length) - (word_break ? 0 : 1);

[msw, 2015/09/17 18:42:44]

I'd be a lot more comfortable with this code using iterators to set indices and
avoiding +/-1 increments that might chop surrogates. It shouldn't be too hard to
rewrite the char iterator loop to start at |length| instead of |length-1|, but
the successful word break code path might need to return early; WDYT?

[Peter Kasting, 2015/09/17 19:39:03]

I tried a couple times yesterday to rewrite the char iterator loop and couldn't
do it correctly.  Also, depending on whether you're getting into it from the
word break loop or not, you want to start at (length) or (length - 1).  Just
changing that to (length + 1) and (length) doesn't help avoid potential
surrogate breaking.

I might be able to do a more extensive rewrite of this function that gets all
this more right.  I wouldn't mind addressing all your
non-surrogate-breaking-related comments, checking this in (since the existing
code already suffers from these issues, so we wouldn't be making them worse, but
we would at least improve test coverage and fix some bugs), and then doing the
rewrite pass as a second CL.  Does that seem OK?

[msw, 2015/09/17 20:45:10]

That seems fine, except lines 804 and 814 seem to potentially introduce new
opportunities to break surrogates.

[Peter Kasting, 2015/09/17 20:58:05]

804 is definitely a regression I need to fix.  I meant to comment about that
before and forgot, but basically I need to restore the use of
char_iterator.next() there instead of adding 1.

814 can break a surrogate, but it's the same break point (length - 1) we already
have a problem with a couple other places in this code.  I don't really consider
that a new bug; I think the right fix there is basically going to be the same as
all the other places, which is pretty much, at the top of the function we need
to use a character iterator to find the right character break before length, and
then use that everywhere.  So I'd rather fix that at the same time as I fix the
similar instances.

[msw, 2015/09/17 21:06:26]

OK, sgtm

[Peter Kasting, 2015/09/19 02:43:25]

Fixed line 804 by restoring the next() call.

+  if (word_break) {
+    // Use a word iterator to find the first boundary.
     UErrorCode status = U_ZERO_ERROR;
     scoped_ptr<icu::BreakIterator> bi(
-        icu::RuleBasedBreakIterator::createLineInstance(
+        icu::RuleBasedBreakIterator::createWordInstance(
             icu::Locale::getDefault(), status));
     if (U_FAILURE(status))
-      return string.substr(0, max) + kElideString;
+      return string.substr(0, length - 1) + kElideString;

[msw, 2015/09/17 18:42:44]

It's unrelated (okay for follow-up), but I think this error case should still
try to use the character iterator codepath below instead of potentially chopping
a surrogate pair or similar.

[Peter Kasting, 2015/09/19 02:43:25]

Yep, will address in the followup.

     bi->setText(string.c_str());
     index = bi->preceding(index);

[msw, 2015/09/17 18:42:44]

nit: consider using |index = bi->preceding(length);| here and just
unconditionally initializing index to |length - 1| on line 773

[Peter Kasting, 2015/09/19 02:43:25]

Done.

     if (index == icu::BreakIterator::DONE || index == 0) {
-      // We either found no valid line break at all, or one right at the
+      // We either found no valid word break at all, or one right at the
       // beginning of the string. Go back to the end; we'll have to break in the
       // middle of a word.
-      index = static_cast<int32_t>(max);
+      index = static_cast<int32_t>(length - 1);
     }
   }
 
-  // Use a character iterator to find the previous non-whitespace character.
+  // By this point, |index| should point at the character that's a candidate for
+  // replacing with an ellipsis.  Use a character iterator to check previous
+  // characters and stop as soon as we find a previous non-whitespace character.
   icu::StringCharacterIterator char_iterator(string.c_str());
   char_iterator.setIndex(index);
   while (char_iterator.hasPrevious()) {
@@ -798,21 +799,22 @@ base::string16 TruncateString(const base::string16& string,
     if (!(u_isspace(char_iterator.current()) ||
           u_charType(char_iterator.current()) == U_CONTROL_CHAR ||
           u_charType(char_iterator.current()) == U_NON_SPACING_MARK)) {
-      // Not a whitespace character. Advance the iterator so that we
-      // include the current character in the truncated string.
-      char_iterator.next();
-      break;
+      // Not a whitespace character.  Truncate to everything up to and including
+      // this character, plus the ellipsis.

[msw, 2015/09/17 18:42:44]

nit: s/plus/and append/

[Peter Kasting, 2015/09/19 02:43:25]

Done.

+      return string.substr(0, char_iterator.getIndex() + 1) + kElideString;
     }
   }
-  if (char_iterator.hasPrevious()) {
-    // Found a valid break point.
-    index = char_iterator.getIndex();
-  } else {
-    // String has leading whitespace, return the elide string.
-    return kElideString;
-  }
 
-  return string.substr(0, index) + kElideString;
+  // Couldn't find a previous non-whitespace character.  If we were originally
+  // word-breaking, and index != length - 1, then the string is |index|
+  // whitespace characters followed by a word we're trying to break in the midst
+  // of, and we can fit at least one character of the word in the elided string.
+  // Do that rather than just returning an ellipsis.
+  if (word_break && (index != static_cast<int32_t>(length - 1)))
+    return string.substr(0, length - 1) + kElideString;

[msw, 2015/09/17 18:42:44]

This should use a char iterator to avoid chopping a surrogate pair.

+
+  // Trying to break after only whitespace, elide all of it.
+  return kElideString;
 }
 
 }  // namespace gfx