Improve RenderTextWin font fallback.

ui/gfx/render_text_win.cc

 // Copyright (c) 2011 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/logging.h"
 #include "base/stl_util.h"
@@ skipping 273 matching lines @@
   return RenderText::GetRightSelectionModel(selection, break_type);
 }
 
 SelectionModel RenderTextWin::LeftEndSelectionModel() {
   if (text().empty())
     return SelectionModel(0, 0, SelectionModel::LEADING);
 
   EnsureLayout();
   size_t cursor = base::i18n::IsRTL() ? text().length() : 0;
   internal::TextRun* run = runs_[visual_to_logical_[0]];
-  bool rtl = run->script_analysis.fRTL;
-  size_t caret = rtl ? run->range.end() - 1 : run->range.start();
-  SelectionModel::CaretPlacement placement =
-      rtl ? SelectionModel::TRAILING : SelectionModel::LEADING;
+  size_t caret;
+  SelectionModel::CaretPlacement placement;
+  if (run->script_analysis.fRTL) {
+    caret = IndexOfAdjacentGrapheme(run->range.end(), false);
+    placement = SelectionModel::TRAILING;
+  } else {
+    caret = run->range.start();
+    placement = SelectionModel::LEADING;
+  }
   return SelectionModel(cursor, caret, placement);
 }
 
 SelectionModel RenderTextWin::RightEndSelectionModel() {
   if (text().empty())
     return SelectionModel(0, 0, SelectionModel::LEADING);
 
   EnsureLayout();
   size_t cursor = base::i18n::IsRTL() ? 0 : text().length();
   internal::TextRun* run = runs_[visual_to_logical_[runs_.size() - 1]];
-  bool rtl = run->script_analysis.fRTL;
-  size_t caret = rtl ? run->range.start() : run->range.end() - 1;
-  SelectionModel::CaretPlacement placement =
-      rtl ? SelectionModel::LEADING : SelectionModel::TRAILING;
+  size_t caret;
+  SelectionModel::CaretPlacement placement;
+  if (run->script_analysis.fRTL) {
+    caret = run->range.start();
+    placement = SelectionModel::LEADING;
+  } else {
+    caret = IndexOfAdjacentGrapheme(run->range.end(), false);
+    placement = SelectionModel::TRAILING;
+  }
   return SelectionModel(cursor, caret, placement);
 }
 
 void RenderTextWin::GetSubstringBounds(size_t from,
                                        size_t to,
                                        std::vector<Rect>* bounds) {
   DCHECK(!needs_layout_);
   ui::Range range(from, to);
   DCHECK(ui::Range(0, text().length()).Contains(range));
   Point display_offset(GetUpdatedDisplayOffset());
@@ skipping 132 matching lines @@
     size_t byte_length = run->glyph_count * sizeof(WORD);
     canvas_skia->drawPosText(run->glyphs.get(), byte_length, &pos[0], paint);
 
     if (run->strike || run->underline)
       DrawTextRunDecorations(canvas_skia, paint, *run, run_x, y);
   }
 }
 
 size_t RenderTextWin::IndexOfAdjacentGrapheme(size_t index, bool next) {
   EnsureLayout();
+
+  if (text().empty())
+    return 0;
+
+  if (index >= text().length()) {
+    if (next || index > text().length()) {
+      return text().length();
+    } else {
+      // If the requested |index| is past the end, use the index of the last

[xji, 2011/12/01 20:19:15]

is the comment apply? till here,  "index <= text().length()".

[Alexei Svitkine (slow), 2011/12/01 20:58:58]

Good catch, the comment meant to say is "at the end". Fixed.

+      // character to find the grapheme.
+      index = text().length() - 1;
+      if (IsCursorablePosition(index))
+        return index;
+    }
+  }
+
   size_t run_index = GetRunContainingPosition(index);
-  internal::TextRun* run = run_index < runs_.size() ? runs_[run_index] : NULL;
-  int start = run ? run->range.start() : 0;
-  int length = run ? run->range.length() : text().length();
+  DCHECK(run_index < runs_.size());
+  internal::TextRun* run = runs_[run_index];
+  int start = run->range.start();
+  int length = run->range.length();
   int ch = index - start;
-  WORD cluster = run ? run->logical_clusters[ch] : 0;
+  WORD cluster = run->logical_clusters[ch];
 
   if (!next) {
-    do {
+    // If |ch| is the start of the run, use the preceding run, if any.
+    if (ch == 0) {
+      if (run_index == 0)
+        return 0;
+      run = runs_[run_index - 1];
+      start = run->range.start();

[xji, 2011/12/01 20:19:15]

you did not seem to use the 'start'.

[Alexei Svitkine (slow), 2011/12/01 20:58:58]

It's used at the return point. Also, just keeping (run, start, length)
consistent with each other to avoid confusion.

+      length = run->range.length();
+      ch = length - 1;
+    } else if (run->logical_clusters[ch - 1] != run->logical_clusters[ch]) {

[xji, 2011/12/01 20:19:15]

So, the old code does not handle 'ch==0' correctly, right?

It took me quite long to figure out why you need this "else if" instead of
"else".

I can not think of anything better except duplicate loop as following (hopefully
the same functionality). Not sure whether it is more readable.

if (!next) {
  // If |ch| is the start of the run, use the preceding run, if any.
  if (ch == 0) {
    if (run_index == 0)
      return 0;
    run = runs_[run_index - 1];
    length = run->range.length();
    ch = length - 1;
    cluster = run->logical_clusters[ch]; 
    while (ch > 0 && run->logical_clusters[ch - 1] == cluster);
      ch--;
  } else {
   do {
      ch--;
    } while (ch >= 0 && run->logical_clusters[ch] == cluster);
  }
}

[Alexei Svitkine (slow), 2011/12/01 20:58:58]

Correct. This is covered by the unit test.
I feel that is less readable, but maybe I'm biased because I wrote the other
version of the code. ;) Let's see what Mike thinks.

+      // If |ch| is at a grapheme boundary, use the previous grapheme.
       ch--;
-    } while (ch >= 0 && run && run->logical_clusters[ch] == cluster);
+    }
+
+    // Find the start of the grapheme.
+    while (ch > 0 && run->logical_clusters[ch - 1] == run->logical_clusters[ch])

[xji, 2011/12/01 20:19:15]

Is the logic correct here?

for example: ABCDE, where "ABCD" is one grapheme, when index is at ABCDE|,
'ch' is set to "ABCD|E" at line 509. then, will 'ch' be continually decrease to
'|ABCDE' in this loop?

should it check against cluster? (and add 'cluster = run->logical_clusters[ch]'
in 'ch==0' branch?

And actually, in the above "else if", "ch--" should be the returned value (no
need to continue in loop).

[Alexei Svitkine (slow), 2011/12/01 20:58:58]

Let me change your example to ABCD|e|f, since ABCD|e will be handled by a
special case above and won't reach this code. We also assume everything is
within a single run, else line 509 won't be executed because ch == 0 if there
was a run boundary.

The index passed in would be: ABCDe|f. Line 509 will execute because
cluster('e') != cluster('f'). So now, it will point to: ABCD|ef.

Then ch > 0 is true, so we check "run->logical_clusters[ch - 1] ==
run->logical_clusters[ch]", which is equivalent to "cluster('D') ==
cluster('e')". This is FALSE, so the loop will not execute, and the correct
value will be returned.
No, that is absolutely needed. Just "ch--" will not produce a grapheme-boundary
result. That "else if" is checking essentially IsCursorablePosition(index). If
it is cursorable, then |ch| is at a grapheme boundary and thus the return value
should be the start of the previous grapheme. So we decrement 1 to get to the
last character of the previous grapheme, and then loop to get to the start of
that grapheme.

[xji, 2011/12/01 22:56:06]

Ah, my bad, mis-matched the index and its character.
thanks for the explanation. 
Then, why line 507 is "else if" instead of "else"?

+      ch--;
   } else {
-    while (ch < length && run && run->logical_clusters[ch] == cluster)
+    while (ch < length && run->logical_clusters[ch] == cluster)
       ch++;
   }
+
   return std::max(std::min(ch, length) + start, 0);
 }
 
 void RenderTextWin::ItemizeLogicalText() {
   STLDeleteContainerPointers(runs_.begin(), runs_.end());
   runs_.clear();
   string_width_ = 0;
   if (text().empty())
     return;
 
@@ skipping 49 matching lines @@
 }
 
 void RenderTextWin::LayoutVisualText() {
   HRESULT hr = E_FAIL;
   base::win::ScopedCreateDC hdc(CreateCompatibleDC(NULL));
   std::vector<internal::TextRun*>::const_iterator run_iter;
   for (run_iter = runs_.begin(); run_iter < runs_.end(); ++run_iter) {
     internal::TextRun* run = *run_iter;
     size_t run_length = run->range.length();
     const wchar_t* run_text = &(text()[run->range.start()]);
+    bool tried_fallback = false;
 
     // Select the font desired for glyph generation.
     SelectObject(hdc, run->font.GetNativeFont());
 
     run->logical_clusters.reset(new WORD[run_length]);
     run->glyph_count = 0;
     // Max glyph guess: http://msdn.microsoft.com/en-us/library/dd368564.aspx
     size_t max_glyphs = static_cast<size_t>(1.5 * run_length + 16);
     while (max_glyphs < kMaxGlyphs) {
       run->glyphs.reset(new WORD[max_glyphs]);
       run->visible_attributes.reset(new SCRIPT_VISATTR[max_glyphs]);
       hr = ScriptShape(hdc,
                        &run->script_cache,
                        run_text,
                        run_length,
                        max_glyphs,
                        &(run->script_analysis),
                        run->glyphs.get(),
                        run->logical_clusters.get(),
                        run->visible_attributes.get(),
                        &(run->glyph_count));
       if (hr == E_OUTOFMEMORY) {
         max_glyphs *= 2;
       } else if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
-        // TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can crash
-        //            on certain surrogate pairs with SCRIPT_UNDEFINED.
-        //            See https://bugzilla.mozilla.org/show_bug.cgi?id=341500
-        //            And http://maxradi.us/documents/uniscribe/
-        if (run->script_analysis.eScript == SCRIPT_UNDEFINED)
-          break;
+         // Only try font fallback if it hasn't yet been attempted for this run.
+         if (tried_fallback) {
+           // TODO(msw): Don't use SCRIPT_UNDEFINED. Apparently Uniscribe can
+           //            crash on certain surrogate pairs with SCRIPT_UNDEFINED.
+           //            See https://bugzilla.mozilla.org/show_bug.cgi?id=341500
+           //            And http://maxradi.us/documents/uniscribe/
+           run->script_analysis.eScript = SCRIPT_UNDEFINED;
+           break;
+         }
 
         // The run's font doesn't contain the required glyphs, use an alternate.
         if (ChooseFallbackFont(hdc, run->font, run_text, run_length,
                                &run->font)) {
           ScriptFreeCache(&run->script_cache);
           SelectObject(hdc, run->font.GetNativeFont());
         }
 
-        run->script_analysis.eScript = SCRIPT_UNDEFINED;
+        tried_fallback = true;
       } else {
         break;
       }
     }
     DCHECK(SUCCEEDED(hr));
 
     if (run->glyph_count > 0) {
       run->advance_widths.reset(new int[run->glyph_count]);
       run->offsets.reset(new GOFFSET[run->glyph_count]);
       hr = ScriptPlace(hdc,
@@ skipping 144 matching lines @@
   internal::TextRun* next = runs_[visual_to_logical_[visual_index + 1]];
   return next->script_analysis.fRTL ? LastSelectionModelInsideRun(next) :
                                       FirstSelectionModelInsideRun(next);
 }
 
 RenderText* RenderText::CreateRenderText() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx