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..067f19bb911af9eab6a1c0cbb3582523f958582d 100644
--- a/ui/gfx/render_text_win.cc
+++ b/ui/gfx/render_text_win.cc
@@ -7,11 +7,14 @@
 #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 "third_party/icu/source/common/unicode/utf16.h"
 #include "ui/base/text/utf16_indexing.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_fallback_win.h"
@@ -592,9 +595,26 @@ 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)
       run_break = run->range.start() + max_run_length;

[Alexei Svitkine (slow), 2013/09/04 14:42:01]

I noticed that this logic doesn't check for whether it's splitting the run at a
surrogate pair when it's clamping the text (it should be checking
ui::IsValidCodePointIndex()).

Can you fix it or add a TODO here?

[ckocagil, 2013/09/04 21:35:46]

Done, fixed.

+
+    // Break runs between any two characters that are not in the same code
+    // block. http://crbug.com/278913
+    base::i18n::UTF16CharIterator iter(&text());
+    iter.SetPosition(run->range.start());

[Alexei Svitkine (slow), 2013/09/04 14:42:01]

This should use GetLayoutText(), not text().

Also, I think you can avoid adding SetPosition() API by instead doing this:

  const wchar_t* run_text = &(GetLayoutText()[run->range.start()]);
  UTF16CharIterator iter(run_text, run->range.length());

(And adjusting the math below appropriately - i.e. adding run->range.start() to
iter.array_pos()).

[ckocagil, 2013/09/04 21:35:46]

Done, throughout the function. There was another use of text() in this function
(from one of my CLs before), it probably has to be changed as well.
Done.

+    const UBlockCode first_block = ublock_getCode(iter.get());
+    while (iter.Advance() &&
+           static_cast<size_t>(iter.array_pos()) < run_break) {
+      if (ublock_getCode(iter.get()) != first_block) {
+        DCHECK_LE(static_cast<size_t>(iter.array_pos()), run_break);
+        DCHECK_GT(static_cast<size_t>(iter.array_pos()), run->range.start());
+        run_break = iter.array_pos();
+        break;
+      }
+    }
+
     style.UpdatePosition(LayoutIndexToTextIndex(run_break));
     if (script_item_break == run_break)
       script_item++;