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

ui/gfx/render_text_win.cc

 // Copyright (c) 2012 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/i18n/break_iterator.h"
 #include "base/logging.h"
@@ skipping 594 matching lines @@
     SCRIPT_FONTPROPERTIES font_properties;
     memset(&font_properties, 0, sizeof(font_properties));
     font_properties.cBytes = sizeof(SCRIPT_FONTPROPERTIES);
     ScriptGetFontProperties(cached_hdc_, &run->script_cache, &font_properties);
 
     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) {
+      bool glyphs_missing = false;
       run->glyphs.reset(new WORD[max_glyphs]);
       run->visible_attributes.reset(new SCRIPT_VISATTR[max_glyphs]);
       hr = ScriptShape(cached_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;
+        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);
+      // If there are no missing glyphs, we're done.

[msw, 2012/04/12 19:58:32]

nit: Try to avoid "we" and similar words, here and elsewhere.
Consider // Skip font substitution if there are no missing glyphs.

[Alexei Svitkine (slow), 2012/04/12 20:40:24]

Done.

+      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

[msw, 2012/04/12 19:58:32]

nit: Remove the comma after "first,".

[Alexei Svitkine (slow), 2012/04/12 20:40:24]

Done.

+      // 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);

[msw, 2012/04/12 19:58:32]

This 3-line block is duplicated with line 685-687.
I know that was already the case, but can you combine them?

[Alexei Svitkine (slow), 2012/04/12 20:40:24]

Done.

           ScriptFreeCache(&run->script_cache);
           SelectObject(cached_hdc_, run->font.GetNativeFont());
+          continue;
         }
+      }
 
-        tried_fallback = true;
-      } else {
+      // We didn't find a font using a meta file, try to find one using font

[msw, 2012/04/12 19:58:32]

nit: "We"

[Alexei Svitkine (slow), 2012/04/12 20:40:24]

Done.

+      // 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++);
+      DeriveFontIfNecessary(font_size, run->font_style, &run->font);
+      ScriptFreeCache(&run->script_cache);
+      SelectObject(cached_hdc_, run->font.GetNativeFont());
     }
     DCHECK(SUCCEEDED(hr));
     string_size_.set_height(std::max(string_size_.height(),
                                      run->font.GetHeight()));
     common_baseline_ = std::max(common_baseline_, run->font.GetBaseline());
 
     if (run->glyph_count > 0) {
       run->advance_widths.reset(new int[run->glyph_count]);
       run->offsets.reset(new GOFFSET[run->glyph_count]);
       hr = ScriptPlace(cached_hdc_,
@@ skipping 80 matching lines @@
     internal::TextRun* run) {
   size_t caret = IndexOfAdjacentGrapheme(run->range.end(), CURSOR_BACKWARD);
   return SelectionModel(caret, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateRenderText() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx