Always try metafile font fallback in the case of missing glyphs in RenderTextWin.

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
===================================================================
--- ui/gfx/render_text_win.cc	(revision 131810)
+++ ui/gfx/render_text_win.cc	(working copy)
@@ -612,6 +612,7 @@
     // 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) {
+      bool glyphs_missing = false;
       run->glyphs.reset(new WORD[max_glyphs]);
       run->visible_attributes.reset(new SCRIPT_VISATTR[max_glyphs]);
       hr = ScriptShape(cached_hdc_,
@@ -626,66 +627,60 @@
                        &(run->glyph_count));
       if (hr == E_OUTOFMEMORY) {
         max_glyphs *= 2;
+        continue;
+      } else if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
+        glyphs_missing = true;
       } else if (hr == S_OK) {
         // If |hr| is S_OK, there could still be missing glyphs in the output,
         // see: http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564.aspx
-        //
-        // If there are missing glyphs, use font linking to try to find a
-        // matching font.
-        bool glyphs_missing = false;
-        for (int i = 0; i < run->glyph_count; i++) {
+        for (int i = 0; i < run->glyph_count; ++i) {
           if (run->glyphs[i] == font_properties.wgDefault) {
             glyphs_missing = true;
             break;
           }
         }
-        // No glyphs missing - good to go.
-        if (!glyphs_missing)
-          break;
+      }
 
-        // First time through, get the linked fonts list.
-        if (linked_fonts == NULL)
-          linked_fonts = GetLinkedFonts(run->font);
+      // Skip font substitution if there are no missing glyphs.
+      if (!glyphs_missing)
+        break;
 
-        // None of the linked fonts worked - break out of the loop.
-        if (linked_font_index == linked_fonts->size())
-          break;
-
-        // Try the next linked font.
-        run->font = linked_fonts->at(linked_font_index++);
-        DeriveFontIfNecessary(font_size, run->font_style, &run->font);
-        ScriptFreeCache(&run->script_cache);
-        SelectObject(cached_hdc_, run->font.GetNativeFont());
-      } else if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
-        // 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;
-          // Reset |hr| to 0 to not trigger the DCHECK() below when a font is
-          // not found that can display the text. This is expected behavior
-          // under Windows XP without additional language packs installed and
-          // may also happen on newer versions when trying to display text in
-          // an obscure script that the system doesn't have the right font for.
-          hr = 0;
-          break;
-        }
-
-        // The run's font doesn't contain the required glyphs, use an alternate.
-        // TODO(msw): support RenderText's font_list().
+      // If there are missing glyphs, first try finding a fallback font using a
+      // meta file, if it hasn't yet been attempted for this run.
+      // TODO(msw|asvitkine): Support RenderText's font_list()?
+      if (!tried_fallback) {
+        tried_fallback = true;
         if (ChooseFallbackFont(cached_hdc_, run->font, run_text, run_length,
                                &run->font)) {
-          DeriveFontIfNecessary(font_size, run->font_style, &run->font);
-          ScriptFreeCache(&run->script_cache);
-          SelectObject(cached_hdc_, run->font.GetNativeFont());
+          UpdateRunFont(run, font_size);
+          continue;
         }
+      }
 
-        tried_fallback = true;
-      } else {
+      // Couldn't find a replacement font using the meta file, try to find one

[msw, 2012/04/12 21:32:14]

nit: consider "The meta file did not yield a replacement font,"

[Alexei Svitkine (slow), 2012/04/13 13:56:15]

Done.

+      // using font linking. First time through, get the linked fonts list.
+      if (linked_fonts == NULL)
+        linked_fonts = GetLinkedFonts(run->font);
+
+      // None of the linked fonts worked - break out of the loop.
+      if (linked_font_index == linked_fonts->size()) {
+        // 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;
+        // Reset |hr| to 0 to not trigger the DCHECK() below when a font is
+        // not found that can display the text. This is expected behavior
+        // under Windows XP without additional language packs installed and
+        // may also happen on newer versions when trying to display text in
+        // an obscure script that the system doesn't have the right font for.
+        hr = 0;
         break;
       }
+
+      // Try the next linked font.
+      run->font = linked_fonts->at(linked_font_index++);
+      UpdateRunFont(run, font_size);
     }
     DCHECK(SUCCEEDED(hr));
     string_size_.set_height(std::max(string_size_.height(),
@@ -734,6 +729,12 @@
   string_size_.set_width(preceding_run_widths);
 }
 
+void RenderTextWin::UpdateRunFont(internal::TextRun* run, int font_size) {

[msw, 2012/04/12 21:32:14]

I think it'd be a better to supply the substitute font as an argument here
(instead of |font_size|), and use the run's existing font size with
DeriveFontIfNecessary, as done with |run->font_style|.

[Alexei Svitkine (slow), 2012/04/12 22:11:43]

I agree it would be cleaner from an interface POV, but it would result in an
extra Font copy operation (since ChooseFallbackFont() would need to output to a
temp font), which (I just checked) is not cheap for PlatformFontWin. :(

(Although, I'm going to look at fixing this.)

Would you be okay with a TODO to clean this up, or do you prefer I block landing
this pending looking at making the PlatformFontWin copy constructor fast?

[Alexei Svitkine (slow), 2012/04/13 13:56:15]

Done.

+  DeriveFontIfNecessary(font_size, run->font_style, &run->font);
+  ScriptFreeCache(&run->script_cache);
+  SelectObject(cached_hdc_, run->font.GetNativeFont());
+}
+
 const std::vector<Font>* RenderTextWin::GetLinkedFonts(const Font& font) const {
   const std::string& font_name = font.GetFontName();
   std::map<std::string, std::vector<Font> >::const_iterator it =