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 (text().empty())
+    return 0;
+
+  if (index >= text().length()) {
+    if (next) {
+      return text().length();
+    } else {
+      size_t last = text().length() - 1;
+      index = IndexOfAdjacentGrapheme(last, false);

[xji, 2011/12/01 08:31:58]

is the reason of this special handling that:
run_index returned from GetRunContainingPosition(text.length()) >= runs_.size()
,so that the 'run' will be assigned to NULL and following ops are wrong?

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

Correct. The previous logic was incorrect in this case. By special-casing it, it
also makes the run != NULL code cleaner.

+      if (index == last)
+        return index;
+      next = true;
+    }
+  }
+
   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--;

[xji, 2011/12/01 08:31:58]

this does not seem right.
for example: ABCDE, assume CDE are in same grapheme,
if index is 3, IndexOfAdjacentGrapheme(3, false) should be 2, not 1.

It is the same for Linux.
The name is confusing, I think the purpose of this function is looking for the
previous or next grapheme boundary.

I think we should have test to cover this, or maybe the test is disabled in Win?
If there is not test coverage, we should definitely add one. 

And could you please also clarify this in the comments in render_text.h?

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

I see - this wasn't clear to me before. I've adjusted the code to fit this
behaviour and fixed the header comments to be clear about this.
I've now added unit tests for this, as promised. I also had to fix the case of
param |index| >= text().length() on Linux to return text().length().
Done.

   } 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.
+         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;
       }