RenderTextWin: Use a font that can at least partially display the run, if possible.

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
===================================================================
--- ui/gfx/render_text_win.cc	(revision 145877)
+++ ui/gfx/render_text_win.cc	(working copy)
@@ -679,6 +679,12 @@
     size_t linked_font_index = 0;
     const std::vector<Font>* linked_fonts = NULL;
     Font original_font = run->font;
+    // Keep track of the font producing the least missing glyphs for which
+    // ScriptShape() returned S_OK. This font will be used in the case where
+    // no font is able to display all the characters in this run.
+    int least_missing_count = INT_MAX;

[msw, 2012/07/10 20:52:15]

nit: optionally rename this for more context, like
|best_partial_font_missing_glyph_count| or |least_missing_glyph_count|.

[Alexei Svitkine (slow), 2012/07/10 21:16:35]

Done.

+    Font best_partial_font = run->font;
+    bool using_best_partial_font = false;
 
     // Select the font desired for glyph generation.
     SelectObject(cached_hdc_, run->font.GetNativeFont());
@@ -711,13 +717,20 @@
       } 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
-        glyphs_missing = HasMissingGlyphs(run);
+        const int missing_count = CountCharsWithMissingGlyphs(run);
+        // Track the font that produced the least missing glyphs.
+        if (missing_count < least_missing_count) {
+          least_missing_count = missing_count;
+          best_partial_font = run->font;
+        }
+        glyphs_missing = (missing_count != 0);
       }
 
-      // Skip font substitution if there are no missing glyphs.
-      if (!glyphs_missing) {
+      // Skip font substitution if there are no missing glyphs or if the font
+      // with the least missing glyphs is being used as a last resort.
+      if (!glyphs_missing || using_best_partial_font) {
         // Save the successful fallback font that was chosen.
-        if (tried_fallback)
+        if (tried_fallback && !using_best_partial_font)
           successful_substitute_fonts_[original_font.GetFontName()] = run->font;
         break;
       }
@@ -765,8 +778,25 @@
           linked_fonts = GetLinkedFonts(run->font);
       }
 
-      // None of the linked fonts worked, break out of the loop.
+      // None of the fallback fonts were able to display the entire run.
       if (linked_font_index == linked_fonts->size()) {
+        // If there was a font that was able to partially display the run, use

[msw, 2012/07/10 20:52:15]

nit: make one-liner as "If a font could partially display the run, use that
now." or similar.

[Alexei Svitkine (slow), 2012/07/10 21:16:35]

Done.

+        // that now.
+        if (least_missing_count != INT_MAX) {
+          ApplySubstituteFont(run, best_partial_font);
+          using_best_partial_font = true;
+          continue;
+        }
+
+        // If no font was able to partially display the run, replace all glyphs

[msw, 2012/07/10 20:52:15]

In the bug, you wrote (and I agree that) "In the failure case, we should also
display the missing glyph "box" character, using the original input font." So,
I'm not certain, but shouldn't this do something slightly different? Isn't the
script_cache that of the last attempted fallback font, and not the original
font?

[Alexei Svitkine (slow), 2012/07/10 21:16:35]

You're right that this is not solving that specific issue. I would like to leave
that (more minor) problem for another CL. I think it may be a bit easier to
handle that case once I refactor the fallback code into a separate file.

[msw, 2012/07/10 21:40:24]

sgtm

+        // with |wgDefault| to ensure they don't hold garbage values.
+        SCRIPT_FONTPROPERTIES properties;
+        memset(&properties, 0, sizeof(properties));
+        properties.cBytes = sizeof(properties);
+        ScriptGetFontProperties(cached_hdc_, &run->script_cache, &properties);
+        for (int i = 0; i < run->glyph_count; ++i)
+          run->glyphs[i] = properties.wgDefault;
+
         // 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
@@ -841,7 +871,8 @@
   SelectObject(cached_hdc_, run->font.GetNativeFont());
 }
 
-bool RenderTextWin::HasMissingGlyphs(internal::TextRun* run) const {
+int RenderTextWin::CountCharsWithMissingGlyphs(internal::TextRun* run) const {
+  int chars_not_missing_glyphs = 0;

[msw, 2012/07/10 20:52:15]

Isn't this counting the number glyphs found? (not related to chars?)
If so, rename this |glyphs_found|, and rename the function too.
Otherwise, clarify exactly what's being counted.

[Alexei Svitkine (slow), 2012/07/10 21:16:35]

It is counting chars, in fact, due to how the loop is constructed. See the for
loop below; it's iterating over chars.

For each char, it finds the corresponding glyph. A glyph may cover multiple
chars (e.g. a decorated letter may be multiple chars, but a single glyph). So
this is indeed counting chars.

I believe both the function and variable are already named in a way to indicate
that. However, I have clarified the code elsewhere.

   SCRIPT_FONTPROPERTIES properties;
   memset(&properties, 0, sizeof(properties));
   properties.cBytes = sizeof(properties);
@@ -854,7 +885,7 @@
     DCHECK_LT(glyph_index, run->glyph_count);
 
     if (run->glyphs[glyph_index] == properties.wgDefault)
-      return true;
+      continue;
 
     // Windows Vista sometimes returns glyphs equal to wgBlank (instead of
     // wgDefault), with fZeroWidth set. Treat such cases as having missing
@@ -864,11 +895,13 @@
         run->visible_attributes[glyph_index].fZeroWidth &&
         !IsWhitespace(run_text[char_index]) &&
         !IsUnicodeBidiControlCharacter(run_text[char_index])) {
-      return true;
+      continue;
     }
+
+    ++chars_not_missing_glyphs;
   }
 
-  return false;
+  return run->range.length() - chars_not_missing_glyphs;

[msw, 2012/07/10 20:52:15]

Shouldn't this be "return run->glyph_count - glyphs_found;"
Perhaps DCHECK that this value is not negative.

[Alexei Svitkine (slow), 2012/07/10 21:16:35]

Nope, it shouldn't. See above.

It can't be negative because the loop iterates over |run->range.length()|
characters and can only increments |chars_not_missing_glyphs| one or zero times
each iteration. I've added a DCHECK() though, in case the code changes.

 }
 
 const std::vector<Font>* RenderTextWin::GetLinkedFonts(const Font& font) const {