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 "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 511 matching lines @@
   runs_.clear();
   // Make |string_size_|'s height and |common_baseline_| tall enough to draw
   // often-used characters which are rendered with fonts in the font list.
   string_size_ = Size(0, font_list().GetHeight());
   common_baseline_ = font_list().GetBaseline();
 
   // Set Uniscribe's base text direction.
   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))) {
     // Derive the array of Uniscribe script items from the logical text.
     // 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)
     return;
 
   // Temporarily apply composition underlines and selection colors.
   ApplyCompositionAndSelectionStyles();
 
   // 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());

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Nit: Use |layout_text_length| here or get rid of it and use layout_text.length()
everywhere.

[ckocagil, 2013/09/05 20:13:15]

Done, used |layout_text_length| everywhere.

   internal::StyleIterator style(empty_colors, styles());
   SCRIPT_ITEM* script_item = &script_items[0];
   const size_t max_run_length = kMaxGlyphs / 2;
   for (size_t run_break = 0; run_break < layout_text_length;) {
     internal::TextRun* run = new internal::TextRun();
     run->range.set_start(run_break);
     run->font = GetPrimaryFont();
     run->font_style = (style.style(BOLD) ? Font::BOLD : 0) |
                       (style.style(ITALIC) ? Font::ITALIC : 0);
     DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(),
                           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;
+    if (!ui::IsValidCodePointIndex(layout_text, run_break))

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Move this into the block of the above if.

[msw, 2013/09/04 22:05:38]

Should this loop? Does it need to handle multiple consecutive invalid indices?
Does this work for an empty or 1-code-point run that "isn't a valid index"?

[ckocagil, 2013/09/05 20:13:15]

Done.

[ckocagil, 2013/09/05 20:13:15]

There can't be more than 2 words in a UTF-16 character, so this shouldn't need
to loop.
I don't think we should allow runs that start or end between a surrogate pair.
But since you asked it we probably do allow it. I moved this block inside the
if, now it shouldn't disrupt runs that start or end with surrogate halves.

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

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Nit: const

[ckocagil, 2013/09/05 20:13:15]

We now modify this, see the updated code.

+    base::i18n::UTF16CharIterator iter(layout_text.c_str() + run_start,
+                                       layout_text.length() - run_start);

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

This should be run.range.length().

[ckocagil, 2013/09/05 20:13:15]

Done.

+    const UBlockCode first_block = ublock_getCode(iter.get());

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Does this work correctly if there's an empty run?

[ckocagil, 2013/09/05 20:13:15]

Fixed.

+    while (iter.Advance() &&
+           static_cast<size_t>(iter.array_pos() + run_start) < run_break) {

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Instead of doing iter.array_pos() + run_start < run_break everywhere, how about
adding a run_end var:

const int32 run_end = static_cast<int32>(run_break - run_start);
while (iter.Advance() && iter.array_pos() < run_end) {
   ...
}

And you can also simplify the DCHECKs.

[ckocagil, 2013/09/05 20:13:15]

Done. Also I removed the DCHECKs.

+      if (ublock_getCode(iter.get()) != first_block) {
+        DCHECK_LE(static_cast<size_t>(iter.array_pos() + run_start), run_break);

[msw, 2013/09/04 22:05:38]

This seems unnecessary; it's checked by the loop. I'd toss the DCHECK_GT too.

[ckocagil, 2013/09/05 20:13:15]

Done.

+        DCHECK_GT(static_cast<size_t>(iter.array_pos() + run_start),
+                  run->range.start());
+        run_break = iter.array_pos() + run_start;
+        break;

[msw, 2013/09/04 22:05:38]

I actually think the break is unnecessary, since the loop will just terminate.

[ckocagil, 2013/09/05 20:13:15]

I replaced the |run_break| use in the loop conditional, so the break is
necessary now.

+      }
+    }
+

[Alexei Svitkine (slow), 2013/09/04 21:52:36]

Add a DCHECK(ui::IsValidCodePointIndex(run_break)) here.

[ckocagil, 2013/09/05 20:13:15]

The code above tries not to disrupt runs that start or end with surrogate
halves. We should disallow such runs as I said above, but that's probably for
another CL.

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

Fair enough. Could you add a TODO and file a crbug?

     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