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

ui/gfx/render_text_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text_win.h"
 
 #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"
 #include "ui/gfx/font_smoothing_win.h"
 #include "ui/gfx/platform_font_win.h"
 
 namespace gfx {
 
 namespace {
 
@@ skipping 560 matching lines @@
                           run->font_style, &run->font);
     run->strike = style.style(STRIKE);
     run->diagonal_strike = style.style(DIAGONAL_STRIKE);
     run->underline = style.style(UNDERLINE);
     run->script_analysis = script_item->a;
 
     // Find the next break and advance the iterators as needed.
     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++;
     run->range.set_end(run_break);
     runs_.push_back(run);
   }
 
   // Undo the temporarily applied composition underlines and selection colors.
   UndoCompositionAndSelectionStyles();
 }
@@ skipping 289 matching lines @@
   size_t position = LayoutIndexToTextIndex(run->range.end());
   position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
   return SelectionModel(position, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateInstance() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx