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 341 matching lines @@
       if (!bounds.empty() && rect.SharesEdgeWith(bounds.back())) {
         rect = rect.Union(bounds.back());
         bounds.pop_back();
       }
       bounds.push_back(rect);
     }
   }
   return bounds;
 }
 
-bool RenderTextWin::IsCursorablePosition(size_t position) {
+bool RenderTextWin::IsCursorablePosition(size_t position, bool is_trailing) {
   if (position == 0 || position == text().length())
     return true;
 
   size_t run_index = GetRunContainingPosition(position);
   if (run_index >= runs_.size())
     return false;
 
   internal::TextRun* run = runs_[run_index];
+  if (is_trailing && !run->script_analysis.fRTL)

[msw, 2011/11/29 20:42:00]

Shouldn't this be done before |GetRunContainingPosition(position);|?
What if the caret's |position| is trailing the last character of an RTL
run/string? Seems like potential out of bounds indexing...
I'm generally confused by this part of the CL; perhaps my check for cursorable
positions via comparing logical clusters is incorrect/insufficient?
At the same time, it seems odd that we were(Win)/are(Linux) checking the caret
position as cursorable without considering the LEADING/TRAILING placement.

+    position++;
   size_t start = run->range.start();
   if (position == start)
     return true;
   return run->logical_clusters[position - start] !=
          run->logical_clusters[position - start - 1];
 }
 
 void RenderTextWin::UpdateLayout() {
   // TODO(msw): Skip complex processing if ScriptIsComplex returns false.
   ItemizeLogicalText();
@@ skipping 78 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 216 matching lines @@
     Rect r(GetUpdatedCursorBounds());
     canvas->DrawRectInt(kCursorColor, r.x(), r.y(), r.width(), r.height());
   }
 }
 
 RenderText* RenderText::CreateRenderText() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx