RenderTextWin: Break runs between any two characters that are not in the same code block

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
diff --git a/ui/gfx/render_text_win.cc b/ui/gfx/render_text_win.cc
index ac018de3f446226f8e39276ee65f24ebf1e76478..5e716376c4dcd7e4e94f5c528bac479702285899 100644
--- a/ui/gfx/render_text_win.cc
+++ b/ui/gfx/render_text_win.cc
@@ -7,11 +7,13 @@
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
+#include "base/i18n/char_iterator.h"
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/windows_version.h"
+#include "third_party/icu/source/common/unicode/uchar.h"
 #include "ui/base/text/utf16_indexing.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_fallback_win.h"
@@ -543,13 +545,14 @@ void RenderTextWin::ItemizeLogicalText() {
   script_state_.uBidiLevel =
       (GetTextDirection() == base::i18n::RIGHT_TO_LEFT) ? 1 : 0;
 
-  if (text().empty())
+  const base::string16& layout_text = GetLayoutText();
+  if (layout_text.empty())
     return;
 
   HRESULT hr = E_OUTOFMEMORY;
   int script_items_count = 0;
   std::vector<SCRIPT_ITEM> script_items;
-  const size_t layout_text_length = GetLayoutText().length();
+  const size_t layout_text_length = layout_text.length();
   // Ensure that |kMaxRuns| is attempted and the loop terminates afterward.
   for (size_t runs = kGuessRuns; hr == E_OUTOFMEMORY && runs <= kMaxRuns;
        runs = std::max(runs + 1, std::min(runs * 2, kMaxRuns))) {
@@ -557,9 +560,9 @@ void RenderTextWin::ItemizeLogicalText() {
     // ScriptItemize always adds a terminal array item so that the length of
     // the last item can be derived from the terminal SCRIPT_ITEM::iCharPos.
     script_items.resize(runs);
-    hr = ScriptItemize(GetLayoutText().c_str(), layout_text_length,
-                       runs - 1, &script_control_, &script_state_,
-                       &script_items[0], &script_items_count);
+    hr = ScriptItemize(layout_text.c_str(), layout_text_length, runs - 1,
+                       &script_control_, &script_state_, &script_items[0],
+                       &script_items_count);
   }
   DCHECK(SUCCEEDED(hr));
   if (!SUCCEEDED(hr) || script_items_count <= 0)
@@ -571,7 +574,7 @@ void RenderTextWin::ItemizeLogicalText() {
   // Build the list of runs from the script items and ranged styles. Use an
   // empty color BreakList to avoid breaking runs at color boundaries.
   BreakList<SkColor> empty_colors;
-  empty_colors.SetMax(text().length());
+  empty_colors.SetMax(layout_text_length);
   internal::StyleIterator style(empty_colors, styles());
   SCRIPT_ITEM* script_item = &script_items[0];
   const size_t max_run_length = kMaxGlyphs / 2;
@@ -592,9 +595,35 @@ void RenderTextWin::ItemizeLogicalText() {
     const size_t script_item_break = (script_item + 1)->iCharPos;
     run_break = std::min(script_item_break,
                          TextIndexToLayoutIndex(style.GetRange().end()));
+
     // Clamp run lengths to avoid exceeding the maximum supported glyph count.
-    if ((run_break - run->range.start()) > max_run_length)
+    if ((run_break - run->range.start()) > max_run_length) {
       run_break = run->range.start() + max_run_length;
+      if (run_break < layout_text_length &&
+          !ui::IsValidCodePointIndex(layout_text, run_break))

[Alexei Svitkine (slow), 2013/09/05 20:32:41]

Nit: Add {}'s.

+        --run_break;
+    }
+
+    // Break runs between characters in different code blocks. This avoids using
+    // fallback fonts for more characters than needed. http://crbug.com/278913
+    if (run_break > run->range.start()) {
+      size_t run_start = run->range.start();
+      if (!ui::IsValidCodePointIndex(layout_text, run_start))

[Alexei Svitkine (slow), 2013/09/05 20:32:41]

I'm not a fan of this logic here. Decrementing run_start seems wrong to me,
since now you're overlapping with the previous run. If we can guarantee that
run_start is at a valid index (i.e. the previous run_break was at a valid
index), then this one shouldn't be necessary, I think.

[ckocagil, 2013/09/05 21:08:57]

|run_start| doesn't change |run->range.start()|. This allows CharIterator to
start with a valid char index (even if icu checks it, I would rather do it here
explicitly). Surrogate pairs that have halves in different runs will be checked
in both runs; this is intentional.

[Alexei Svitkine (slow), 2013/09/06 13:28:42]

Right, I understand this, but still am not a fan of it. It would be better if
such a case was impossible (i.e. we were guaranteed that the run start is not
the 2nd half of a surrogate pair). Otherwise, it makes the logic below quite
strange. For example, |first_block| is assigned iter.get(), but if this happened
than it would correspond to something starting at a previous run.

We should just make sure not to have run boundaries split surrogate pairs and
then this extra logic won't be necessary.

[Alexei Svitkine (slow), 2013/09/06 13:36:18]

Plus, looking at the UTF16CharIterator() code, it shouldn't crash or anything if
we pass it a fragment that splits a surrogate pair. So I'd say just remove these
isValidCodePointIndex() checks in this block and add a TODO elsewhere to ensure
we don't split surrogate pairs when splitting runs.

+        --run_start;
+      int32 run_length = static_cast<int32>(run_break - run_start);
+      if (!ui::IsValidCodePointIndex(layout_text, run_break))

[Alexei Svitkine (slow), 2013/09/05 20:32:41]

In what cases could this be the case? For example, your CL is fixing the
clamping code above, so that shouldn't result in the run_break being bad.

I guess only if Uniscribe gives us back results via the script item list? If so,
it should probably be handled at that stage.

[ckocagil, 2013/09/05 21:08:57]

I'm not sure, style ranges maybe? Do they contain code-point indices? Mike, do
you know what causes these runs or if they actually exist?

[msw, 2013/09/06 16:51:28]

Uniscribe or style ranges theoretically could, but shouldn't in practice.
It's not worth handling without repro, the TODO/DCHECK is sufficient.

+        ++run_length;
+      base::i18n::UTF16CharIterator iter(layout_text.c_str() + run_start,
+                                         run_length);
+      const UBlockCode first_block = ublock_getCode(iter.get());
+      while (iter.Advance() && iter.array_pos() < run_length) {
+        if (ublock_getCode(iter.get()) != first_block) {
+          run_break = std::min(run_break, iter.array_pos() + run_start);

[Alexei Svitkine (slow), 2013/09/05 20:32:41]

The mins is only necessary because your code above does --run_start, right?

[ckocagil, 2013/09/05 21:08:57]

No, it is necessary because of the ++run_length (this allows "iter.pos() +
run_start == run_break + 1"). I could use --run_length, but then surrogate
halves wouldn't be checked in both of the runs they are in.

+          break;
+        }
+      }
+    }
+
     style.UpdatePosition(LayoutIndexToTextIndex(run_break));
     if (script_item_break == run_break)
       script_item++;