Use fallback font in PlatformFontPango::DeriveFont().

ui/gfx/platform_font_pango.cc

Index: ui/gfx/platform_font_pango.cc
diff --git a/ui/gfx/platform_font_pango.cc b/ui/gfx/platform_font_pango.cc
index 2c89ac5baa83d035803e66bf05c0bf492fd48ca3..717dca4680fd4e2821618c20fe1bd1e2163da266 100644
--- a/ui/gfx/platform_font_pango.cc
+++ b/ui/gfx/platform_font_pango.cc
@@ -31,6 +31,32 @@ namespace {
 // IsFallbackFontAllowed function in skia/ext/SkFontHost_fontconfig_direct.cpp.
 const char* kFallbackFontFamilyName = "sans";
 
+// Creates a SkTypeface for the passed-in Font::FontStyle and family. If a
+// fallback typeface is used instead of the requested family, |family| will be
+// updated to contain the fallback's family name.
+skia::RefPtr<SkTypeface> CreateSkTypeface(int style, std::string* family) {
+  DCHECK(family);
+
+  int skia_style = SkTypeface::kNormal;
+  if (gfx::Font::BOLD & style)
+    skia_style |= SkTypeface::kBold;
+  if (gfx::Font::ITALIC & style)
+    skia_style |= SkTypeface::kItalic;
+
+  skia::RefPtr<SkTypeface> typeface = skia::AdoptRef(SkTypeface::CreateFromName(
+      family->c_str(), static_cast<SkTypeface::Style>(skia_style)));

[Daniel Erat, 2014/07/18 01:16:19]

note that InitFromDetails() used to always pass through SkTypeface::kNormal
here, regardless of the style that was actually requested. i suspect that this
was a bug.

[msw, 2014/07/18 03:58:27]

Hmm, I wonder if we were always doing setFakeBoldText and setTextSkewX?

+  if (!typeface) {
+    // A non-scalable font such as .pcf is specified. Fall back to a default
+    // scalable font.
+    typeface = skia::AdoptRef(SkTypeface::CreateFromName(
+        kFallbackFontFamilyName, static_cast<SkTypeface::Style>(skia_style)));

[Daniel Erat, 2014/07/18 01:16:19]

i'm less sure about this one. should i unconditionally use SkTypeface::kNormal
here to increase the chances of a match?

[msw, 2014/07/18 03:58:27]

I suppose you could add another if (!typeface) block, or watch for crashes.

[Daniel Erat, 2014/07/18 04:49:59]

i think i'll just watch for crashes, since i don't want to write code based on
superstition and am not sure how to trigger this case to see if it's an actual
problem.

[Daniel Erat, 2014/07/18 14:53:45]

just to follow up on this: skia looks like it's doing a straightforward
fontconfig query in SkFontConfigInterfaceDirect::matchFamilyName() in
src/ports/SkFontConfigInterface_direct.cpp. i've verified that when i query
fontconfig for a bold/italic version of a font that only provides a regular
style, it returns the regular font, so i think that this will fall back
automatically.

[msw, 2014/07/18 17:32:26]

Nice! Good call testing that behavior!

+    CHECK(typeface) << "Could not find any font: " << family << ", "
+                    << kFallbackFontFamilyName;
+    *family = kFallbackFontFamilyName;
+  }
+  return typeface;
+}
+
 }  // namespace
 
 namespace gfx {
@@ -139,16 +165,9 @@ Font PlatformFontPango::DeriveFont(int size_delta, int style) const {
   DCHECK_GT(new_size, 0);
 
   // If the style changed, we may need to load a new face.
-  skia::RefPtr<SkTypeface> typeface = typeface_;
-  if (style != style_) {
-    int skstyle = SkTypeface::kNormal;
-    if (gfx::Font::BOLD & style)
-      skstyle |= SkTypeface::kBold;
-    if (gfx::Font::ITALIC & style)
-      skstyle |= SkTypeface::kItalic;
-    typeface = skia::AdoptRef(SkTypeface::CreateFromName(
-        font_family_.c_str(), static_cast<SkTypeface::Style>(skstyle)));
-  }
+  std::string new_family = font_family_;
+  skia::RefPtr<SkTypeface> typeface =
+      (style == style_) ? typeface_ : CreateSkTypeface(style, &new_family);
 
   // If the size changed, get updated rendering settings.

[msw, 2014/07/18 03:58:27]

Should we also do this if the family (or style) changed?

[Daniel Erat, 2014/07/18 04:49:59]

think i mentioned the style question in an earlier change (yeah, i should, after
updating GetCustomFontRenderParams() to also take the style), but you're totally
right that i should requery if new_family != font_family_ right now (and this
should also be using new_family).

   FontRenderParams render_params = font_render_params_;
@@ -159,7 +178,7 @@ Font PlatformFontPango::DeriveFont(int size_delta, int style) const {
   }
 
   return Font(new PlatformFontPango(typeface,
-                                    font_family_,
+                                    new_family,
                                     new_size,
                                     style,
                                     render_params));
@@ -254,21 +273,8 @@ void PlatformFontPango::InitFromDetails(
     const FontRenderParams& render_params) {
   DCHECK_GT(font_size_pixels, 0);
 
-  typeface_ = typeface;
   font_family_ = font_family;
-  if (!typeface_) {
-    typeface_ = skia::AdoptRef(
-        SkTypeface::CreateFromName(font_family.c_str(), SkTypeface::kNormal));
-    if (!typeface_) {
-      // A non-scalable font such as .pcf is specified. Fall back to a default
-      // scalable font.
-      typeface_ = skia::AdoptRef(SkTypeface::CreateFromName(
-          kFallbackFontFamilyName, SkTypeface::kNormal));
-      CHECK(typeface_) << "Could not find any font: " << font_family << ", "
-                       << kFallbackFontFamilyName;
-      font_family_ = kFallbackFontFamilyName;
-    }
-  }
+  typeface_ = typeface ? typeface : CreateSkTypeface(style, &font_family_);
 
   font_size_pixels_ = font_size_pixels;
   style_ = style;