Avoid extra FontRenderParams queries in RenderTextHarfBuzz.

Avoid extra FontRenderParams queries in RenderTextHarfBuzz.

gfx::Font already contains a FontRenderParams struct. Make
RenderTextHarfBuzz use it when possible instead of always
querying for a new struct.

BUG=none

Committed: https://crrev.com/faf4a0632d4aa96f0f8b31021fb353eebc5509e6
Cr-Commit-Position: refs/heads/master@{#301101}

[Daniel Erat, 2014-10-22 23:47:18]

derat@chromium.org changed reviewers:
+ ckocagil@chromium.org, msw@chromium.org

[Daniel Erat, 2014-10-22 23:47:18]

seems a bit weird that we're requerying here...

[ckocagil, 2014-10-23 05:24:48]

Avoiding gfx::Font was a conscious decision I made in order to avoid using two
different font objects: gfx::Font and SkTypeface. For the purposes of RTHB,
strings work just as well. Is this change really needed? Isn't caching
FontRenderParams at GetFontRenderParams() layer enough?

[Daniel Erat, 2014-10-23 13:27:33]

On 2014/10/23 05:24:48, ckocagil wrote:
> Avoiding gfx::Font was a conscious decision I made in order to avoid using two
> different font objects: gfx::Font and SkTypeface. For the purposes of RTHB,
> strings work just as well. Is this change really needed? Isn't caching
> FontRenderParams at GetFontRenderParams() layer enough?

i don't fully understand: RenderTextHarfBuzz was already using gfx::Font; that's
where i'm getting the FontRenderParams from. the linux implementation of
GetFontRenderParams() does caching, but it's just an optimization that i added
later. i don't think that RTHB should implicitly depend on it being there.

i also don't like having additional GetFontRenderParams() calls here, because
it's one more chance to get things wrong. (in fact, i think that the current
code _does_ get it wrong -- if the UI font was specified in terms of points
instead of pixels on linux, we need to pass the point size to Fontconfig to
ensure we get the right settings. PlatformFontPango does this correctly but we
don't have the point size after we're in RTHB.)

would you prefer that i instead pass the gfx::Font's FontRenderParams into
ShapeRunWithFont() (when available, i.e. not for fallback fonts)?

[Daniel Erat, 2014-10-23 16:30:41]

tried another approach -- mind taking another look?

[msw, 2014-10-23 18:14:22]

Either approach (patch set 2 or patch set 4) lgtm. I think using gfx::Font in
gfx::RenderText is a reasonable option, unless there's some compelling reason
not to use that; why does the the requirement for SkTypeface make us gfx::Font
averse?

One other thought: afaik ChromeOS populates the base FontList with multiple
fonts, and it seems like we ought to go through those fonts for fallback (not
just the fallbacks of the primary font), as RenderTextPango uses the whole
font_list.GetFontDescriptionString() to init Pango via SetUpPangoLayout
(although it seems to only use the FontRenderParams of the primary font, which
is likely the source of some Pango bugs):
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/pango_util....

[Daniel Erat, 2014-10-23 23:15:12]

On 2014/10/23 18:14:22, msw wrote:
> Either approach (patch set 2 or patch set 4) lgtm. I think using gfx::Font in
> gfx::RenderText is a reasonable option, unless there's some compelling reason
> not to use that; why does the the requirement for SkTypeface make us gfx::Font
> averse?

after thinking about this a bit more, i have a slight preference for not
constructing gfx::Fonts for the fallback families since it could result in us
doing some extra work that we don't actually need (since harfbuzz just wants the
family name and FontRenderParams afaik).

> One other thought: afaik ChromeOS populates the base FontList with multiple
> fonts, and it seems like we ought to go through those fonts for fallback (not
> just the fallbacks of the primary font), as RenderTextPango uses the whole
> font_list.GetFontDescriptionString() to init Pango via SetUpPangoLayout
> (although it seems to only use the FontRenderParams of the primary font, which
> is likely the source of some Pango bugs):
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/pango_util....

yeah, that seems broken. i can try to fix it in this change if you'd like.

[msw, 2014-10-23 23:20:10]

On 2014/10/23 23:15:12, Daniel Erat wrote:
> On 2014/10/23 18:14:22, msw wrote:
> > Either approach (patch set 2 or patch set 4) lgtm. I think using gfx::Font
in
> > gfx::RenderText is a reasonable option, unless there's some compelling
reason
> > not to use that; why does the the requirement for SkTypeface make us
gfx::Font
> > averse?
> 
> after thinking about this a bit more, i have a slight preference for not
> constructing gfx::Fonts for the fallback families since it could result in us
> doing some extra work that we don't actually need (since harfbuzz just wants
the
> family name and FontRenderParams afaik).

Okay, performance is a compelling reason.

> > One other thought: afaik ChromeOS populates the base FontList with multiple
> > fonts, and it seems like we ought to go through those fonts for fallback
(not
> > just the fallbacks of the primary font), as RenderTextPango uses the whole
> > font_list.GetFontDescriptionString() to init Pango via SetUpPangoLayout
> > (although it seems to only use the FontRenderParams of the primary font,
which
> > is likely the source of some Pango bugs):
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/pango_util....
> 
> yeah, that seems broken. i can try to fix it in this change if you'd like.

It'd be great to fix that in a followup (or this CL if you prefer).

[ckocagil, 2014-10-23 23:45:32]

Latest patchset lgtm.

On 2014/10/23 13:27:33, Daniel Erat wrote:
> i don't fully understand: RenderTextHarfBuzz was already using gfx::Font;
that's
> where i'm getting the FontRenderParams from.

On 2014/10/23 18:14:22, msw wrote:
> Either approach (patch set 2 or patch set 4) lgtm. I think using gfx::Font in
> gfx::RenderText is a reasonable option, unless there's some compelling reason
> not to use that; why does the the requirement for SkTypeface make us gfx::Font
> averse?

Because we don't really need a gfx::Font, other than the FontRenderParams in
this CL. gfx::Fonts own OS font handles, which (in theory) aren't free. We
currently use them in RTHB but I've been meaning to refactor sources of
gfx::Fonts wherever possible to avoid their use.

Ideally we'd either implement a PlatformFont that wraps SkTypeface
(PlatformFontPango does something similar), or create a cross-platform font
descriptor class and use it instead of concrete font refs.

On 2014/10/23 13:27:33, Daniel Erat wrote:
> i also don't like having additional GetFontRenderParams() calls here, because
> it's one more chance to get things wrong. (in fact, i think that the current
> code _does_ get it wrong

Currently we do have gfx::Font instances, so it's okay that we reuse their
existing

[ckocagil, 2014-10-23 23:45:56]

...their existing FontRenderParams.

[Daniel Erat, 2014-10-24 13:59:11]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-10-24 14:00:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/674683003/60001

[commit-bot: I haz the power, 2014-10-24 14:48:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-24 14:48:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-24 14:48:37]

Patchset 4 (id:??) landed as
https://crrev.com/faf4a0632d4aa96f0f8b31021fb353eebc5509e6
Cr-Commit-Position: refs/heads/master@{#301101}