RenderText: Try fallback fonts of the Uniscribe font while shaping

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/i18n/char_iterator.h"
@@ skipping 1014 matching lines @@
     if (missing_count == 0)
       return;
     if (missing_count < best_partial_font_missing_char_count) {
       best_partial_font_missing_char_count = missing_count;
       best_partial_font = current_font;
     }
   }
 
   // Try finding a fallback font using a meta file.
   // TODO(msw|asvitkine): Support RenderText's font_list()?
+  Font uniscribe_font;
   if (GetUniscribeFallbackFont(original_font, run_text, run_length,
-                               &current_font)) {
+                               &uniscribe_font)) {
+    current_font = uniscribe_font;
     missing_count = CountCharsWithMissingGlyphs(run,
         ShapeTextRunWithFont(run, current_font));
     if (missing_count == 0) {
       successful_substitute_fonts_[original_font.GetFontName()] = current_font;
       return;
     }
     if (missing_count < best_partial_font_missing_char_count) {
       best_partial_font_missing_char_count = missing_count;
       best_partial_font = current_font;
     }
   }
 
   // Try fonts in the fallback list except the first, which is |original_font|.
   std::vector<std::string> fonts =
       GetFallbackFontFamilies(original_font.GetFontName());
   for (size_t i = 1; i < fonts.size(); ++i) {
+    current_font = Font(fonts[i], original_font.GetFontSize());

[msw, 2014/10/20 20:12:03]

The entire defect may just be that |current_font| was not updated in this loop.
Any good shaping would then use the stale value for
|successful_substitute_fonts_| or |best_partial_font|.

Can you try just adding this line and updating the ShapeTextRunWithFont call
immediately below, without trying additional fallback fonts of the Uniscribe
fallback font? (remove the loop in lines 1068-1082 below and potentially revert
the change above too). See if that fixes the issue... do you need me to test?

[Alexei Svitkine (slow), 2014/10/20 20:21:20]

From my recollection of the original motivation for the previous logic, I
suspect it won't be enough - though happy to hear otherwise.

IIRC, issue is that the good font is from the fallback list of the uniscribe
font, not of the original font. So updating current_font isn't enough I think.

[Alexei Svitkine (slow), 2014/10/20 20:25:06]

Although yeah, it looks like the code without the above line would just result
in font linking not working ... so maybe this will work too. I guess the
question is whether it will cover all the cases or not. I'd prefer we keep logic
that matches the original working behavior for the patch that's merged to stable
- and we can investigate if the second linked font query is needed separately on
TOT.

     missing_count = CountCharsWithMissingGlyphs(run,
-        ShapeTextRunWithFont(run, Font(fonts[i], original_font.GetFontSize())));
+        ShapeTextRunWithFont(run, current_font));
     if (missing_count == 0) {
       successful_substitute_fonts_[original_font.GetFontName()] = current_font;
       return;
+    }
+    if (missing_count < best_partial_font_missing_char_count) {
+      best_partial_font_missing_char_count = missing_count;
+      best_partial_font = current_font;
+    }
+  }
+
+  // Try fonts in the fallback list of the Uniscribe font.
+  fonts = GetFallbackFontFamilies(uniscribe_font.GetFontName());
+  for (size_t i = 1; i < fonts.size(); ++i) {
+    current_font = Font(fonts[i], original_font.GetFontSize());
+    missing_count = CountCharsWithMissingGlyphs(run,
+      ShapeTextRunWithFont(run, current_font));

[Alexei Svitkine (slow), 2014/10/20 20:14:04]

Nit: Indent 2 more.

+    if (missing_count == 0) {
+      successful_substitute_fonts_[original_font.GetFontName()] = current_font;
+      return;
     }
     if (missing_count < best_partial_font_missing_char_count) {
       best_partial_font_missing_char_count = missing_count;
       best_partial_font = current_font;
     }
   }
 
   // If a font was able to partially display the run, use that now.
   if (best_partial_font_missing_char_count < static_cast<int>(run_length)) {
     // Re-shape the run only if |best_partial_font| differs from the last font.
@@ skipping 156 matching lines @@
   size_t position = LayoutIndexToTextIndex(run->range.end());
   position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
   return SelectionModel(position, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateNativeInstance() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx