Improve RenderTextWin font fallback.

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
===================================================================
--- ui/gfx/render_text_win.cc	(revision 112221)
+++ ui/gfx/render_text_win.cc	(working copy)
@@ -305,10 +305,15 @@
   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);
 }
 
@@ -461,21 +466,43 @@
 
 size_t RenderTextWin::IndexOfAdjacentGrapheme(size_t index, bool next) {
   EnsureLayout();
+
+  if (index < 0 || text().empty())
+    return 0;
+
+  if (index >= text().length()) {
+    if (next) {
+      return text().length();
+    } else {
+      size_t last = text().length() - 1;
+      index = IndexOfAdjacentGrapheme(last, false);
+      if (index == last)
+        return index;
+      next = true;

[msw, 2011/11/30 21:37:33]

This is a bit of trickery right here.
It looks okay, but please add explanatory comments.

[Alexei Svitkine (slow), 2011/12/01 18:17:23]

Done.

+    }
+  }
+
   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 {
       ch--;
-    } while (ch >= 0 && run && run->logical_clusters[ch] == cluster);
+    } while (ch >= 0 && run->logical_clusters[ch] == cluster);
+
+    // Now, |ch| is at the end of the previous grapheme. Find the start.
+    while (ch > 0 && run->logical_clusters[ch - 1] == run->logical_clusters[ch])
+      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);
 }
 
@@ -545,6 +572,7 @@
     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());
@@ -569,12 +597,15 @@
       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.

[msw, 2011/11/30 21:37:33]

nit: disambiguate this comment and the |tried_fallback| flag for
SCRIPT_UNDEFINED from the term "font fallback".

[Alexei Svitkine (slow), 2011/12/01 18:17:23]

Sorry, I don't understand your suggestion here. I named it 'tried_fallback' to
indicate whether we've already called |ChooseFallbackFont()| once for this text
run.

The idea being to set SCRIPT_UNDEFINED only if we did tried to find a fallback
font but still got USP_E_SCRIPT_NOT_IN_FONT. (As opposed to the previous code
which would set SCRIPT_UNDEFINED even if a suitable font was found.)

Can you elaborate on what you'd rather see for the name of this variable?

+         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,
@@ -583,7 +614,7 @@
           SelectObject(hdc, run->font.GetNativeFont());
         }
 
-        run->script_analysis.eScript = SCRIPT_UNDEFINED;
+        tried_fallback = true;
       } else {
         break;
       }