Use fallback font in PlatformFontPango::DeriveFont().

Use fallback font in PlatformFontPango::DeriveFont().

Make DeriveFont() fall back to "sans" if it doesn't find an
SkTypeface matching the requested family, as
InitFromDetails() already did.

Also make InitFromDetails() pass through the requested
style.

BUG=394860
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284125

[Daniel Erat, 2014-07-18 01:16:20]

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.cc
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:47: family->c_str(),
static_cast<SkTypeface::Style>(skia_style)));
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.

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:52: kFallbackFontFamilyName,
static_cast<SkTypeface::Style>(skia_style)));
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]

lgtm

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.cc
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:47: family->c_str(),
static_cast<SkTypeface::Style>(skia_style)));
On 2014/07/18 01:16:19, Daniel Erat wrote:
> 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.

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

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:52: kFallbackFontFamilyName,
static_cast<SkTypeface::Style>(skia_style)));
On 2014/07/18 01:16:19, Daniel Erat wrote:
> i'm less sure about this one. should i unconditionally use SkTypeface::kNormal
> here to increase the chances of a match?

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

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:172: // If the size changed, get updated rendering
settings.
Should we also do this if the family (or style) changed?

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

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.cc
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:52: kFallbackFontFamilyName,
static_cast<SkTypeface::Style>(skia_style)));
On 2014/07/18 03:58:27, msw wrote:
> On 2014/07/18 01:16:19, Daniel Erat wrote:
> > i'm less sure about this one. should i unconditionally use
SkTypeface::kNormal
> > here to increase the chances of a match?
> 
> I suppose you could add another if (!typeface) block, or watch for crashes.

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.

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:172: // If the size changed, get updated rendering
settings.
On 2014/07/18 03:58:27, msw wrote:
> Should we also do this if the family (or style) changed?

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).

[Daniel Erat, 2014-07-18 14:53:46]

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.cc
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:52: kFallbackFontFamilyName,
static_cast<SkTypeface::Style>(skia_style)));
On 2014/07/18 04:49:59, Daniel Erat wrote:
> On 2014/07/18 03:58:27, msw wrote:
> > On 2014/07/18 01:16:19, Daniel Erat wrote:
> > > i'm less sure about this one. should i unconditionally use
> SkTypeface::kNormal
> > > here to increase the chances of a match?
> > 
> > I suppose you could add another if (!typeface) block, or watch for crashes.
> 
> 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.

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.

[Daniel Erat, 2014-07-18 14:54:05]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-18 14:54:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/399113005/20001

[commit-bot: I haz the power, 2014-07-18 16:53:48]

Change committed as 284125

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

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.cc
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/399113005/diff/1/ui/gfx/platform_font_pango.c...
ui/gfx/platform_font_pango.cc:52: kFallbackFontFamilyName,
static_cast<SkTypeface::Style>(skia_style)));
On 2014/07/18 14:53:45, Daniel Erat wrote:
> On 2014/07/18 04:49:59, Daniel Erat wrote:
> > On 2014/07/18 03:58:27, msw wrote:
> > > On 2014/07/18 01:16:19, Daniel Erat wrote:
> > > > i'm less sure about this one. should i unconditionally use
> > SkTypeface::kNormal
> > > > here to increase the chances of a match?
> > > 
> > > I suppose you could add another if (!typeface) block, or watch for
crashes.
> > 
> > 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.
> 
> 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.

Nice! Good call testing that behavior!