Unify desktop Linux and Chrome OS font rendering params.

Unify desktop Linux and Chrome OS font rendering params.

Use the same path to get font rendering settings on desktop
Linux and Chrome OS: query the LinuxFontDelegate if it's set
and then query Fontconfig.

Also make the LinuxFontDelegate interface return a
PangoFontDescription directly (as opposed to an opaque
string that's actually a stringified PangoFontDescription)
and make it return a cached FontRenderParams struct instead
of exposing separate methods for looking up individual
properties.

Finally, add a missing build dependency on ui_test_pak, which
is needed to get the default font resource when running
gfx_unittests on Chrome OS.

BUG=125235, 393067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284255

[Daniel Erat, 2014-07-18 17:19:11]

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
File ui/gfx/linux_font_delegate.h (right):

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
ui/gfx/linux_font_delegate.h:39: virtual scoped_ptr<ScopedPangoFontDescription>
(i was concerned that the string that used to be returned by this method could
be passed to gfx::FontList, which uses a description format that's a subset of
pango's format)

[Daniel Erat, 2014-07-18 17:26:16]

On 2014/07/18 17:19:11, Daniel Erat wrote:
>
https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
> File ui/gfx/linux_font_delegate.h (right):
> 
>
https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
> ui/gfx/linux_font_delegate.h:39: virtual
scoped_ptr<ScopedPangoFontDescription>
> (i was concerned that the string that used to be returned by this method could
> be passed to gfx::FontList, which uses a description format that's a subset of
> pango's format)

notes mostly to self:
- will re-upload to resolve merge conflict after review
- will wait for https://chromium-review.googlesource.com/#/c/208860/ to go in
before committing

[msw, 2014-07-18 19:14:44]

Nice cleanup!

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (left):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:831: // TODO(derat): Remove this
debugging code if/when http://crbug.com/375824
Did this yield any useful info? Is it worth keeping at all?

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:368: g_free(hint_style);
nit: Remove the NULL checks for these two g_free calls. g_free "Frees the memory
pointed to by mem . If mem is NULL it simply returns."

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
File ui/gfx/linux_font_delegate.h (right):

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/linux_font_delega...
ui/gfx/linux_font_delegate.h:39: virtual scoped_ptr<ScopedPangoFontDescription>
On 2014/07/18 17:19:11, Daniel Erat wrote:
> (i was concerned that the string that used to be returned by this method could
> be passed to gfx::FontList, which uses a description format that's a subset of
> pango's format)

Acknowledged.

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:51: scoped_ptr<ScopedPangoFontDescription> desc;
nit: |description| shouldn't cause any extra wrapping, afaict.

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:59: CHECK(default_font_description_);
ChromeOS uses InitDefaultFontList() to set a specific
|default_font_description_| that should be used here instead of (or before
falling back upon) delegate->GetDefaultPangoFontDescription()?

Perhaps that code could be changed [later] to only set the default value on
gfx::FontList or gfx::PlatformFontPango (and remove the default value member
from the other).

[Daniel Erat, 2014-07-18 19:39:09]

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (left):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:831: // TODO(derat): Remove this
debugging code if/when http://crbug.com/375824
On 2014/07/18 19:14:43, msw wrote:
> Did this yield any useful info? Is it worth keeping at all?

no, i don't think so. i originally theorized that differences between
gtk-font-name and GtkLabel's actual style could be responsible for the font-size
issues tracked at http://crbug.com/375824, but it didn't pan out -- the only
differences i ever heard about were no-op ones like "Ubuntu 9" vs. "Ubuntu  9"
and "Tahoma Regular 8" vs. "Tahoma 8".

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:51: scoped_ptr<ScopedPangoFontDescription> desc;
On 2014/07/18 19:14:44, msw wrote:
> nit: |description| shouldn't cause any extra wrapping, afaict.

Done.

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:59: CHECK(default_font_description_);
On 2014/07/18 19:14:44, msw wrote:
> ChromeOS uses InitDefaultFontList() to set a specific
> |default_font_description_| that should be used here instead of (or before
> falling back upon) delegate->GetDefaultPangoFontDescription()?
> 
> Perhaps that code could be changed [later] to only set the default value on
> gfx::FontList or gfx::PlatformFontPango (and remove the default value member
> from the other).

chrome os doesn't use a LinuxFontDelegate (although i'm wondering if it should,
at some point). i've reordered the code to make this clearer, but let me know if
you preferred it the previous way. note that i'm still checking the delegate on
chrome os in font_render_params_linux.cc, just to avoid ifdefs there.

ui/base/resource/resource_bundle.cc has the following:

  // TODO(yukishiino): Remove SetDefaultFontDescription() once the migration to
  // the font list is done.  We will no longer need SetDefaultFontDescription()
  // after every client gets started using a FontList instead of a Font.

i'm not sure what the status of that is, though.

[msw, 2014-07-18 19:59:46]

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (left):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:831: // TODO(derat): Remove this
debugging code if/when http://crbug.com/375824
On 2014/07/18 19:39:08, Daniel Erat wrote:
> On 2014/07/18 19:14:43, msw wrote:
> > Did this yield any useful info? Is it worth keeping at all?
> 
> no, i don't think so. i originally theorized that differences between
> gtk-font-name and GtkLabel's actual style could be responsible for the
font-size
> issues tracked at http://crbug.com/375824, but it didn't pan out -- the only
> differences i ever heard about were no-op ones like "Ubuntu 9" vs. "Ubuntu  9"
> and "Tahoma Regular 8" vs. "Tahoma 8".

Acknowledged.

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:368: g_free(hint_style);
On 2014/07/18 19:14:43, msw wrote:
> nit: Remove the NULL checks for these two g_free calls. g_free "Frees the
memory
> pointed to by mem . If mem is NULL it simply returns."

ping, but you can leave them if you prefer.

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/400193004/diff/60001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:59: CHECK(default_font_description_);
On 2014/07/18 19:39:09, Daniel Erat wrote:
> On 2014/07/18 19:14:44, msw wrote:
> > ChromeOS uses InitDefaultFontList() to set a specific
> > |default_font_description_| that should be used here instead of (or before
> > falling back upon) delegate->GetDefaultPangoFontDescription()?
> > 
> > Perhaps that code could be changed [later] to only set the default value on
> > gfx::FontList or gfx::PlatformFontPango (and remove the default value member
> > from the other).
> 
> chrome os doesn't use a LinuxFontDelegate (although i'm wondering if it
should,
> at some point). i've reordered the code to make this clearer, but let me know
if
> you preferred it the previous way. note that i'm still checking the delegate
on
> chrome os in font_render_params_linux.cc, just to avoid ifdefs there.
> 
> ui/base/resource/resource_bundle.cc has the following:
> 
>   // TODO(yukishiino): Remove SetDefaultFontDescription() once the migration
to
>   // the font list is done.  We will no longer need
SetDefaultFontDescription()
>   // after every client gets started using a FontList instead of a Font.
> 
> i'm not sure what the status of that is, though.

Hmm, I think the latest patch set is a bit safer for now, but obviously isn't
the long term ideal solution.

https://codereview.chromium.org/400193004/diff/80001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/400193004/diff/80001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:87: } else {
Should this be a separate check like "if (!description.get()) ..." or something
to check if the underlying description from the LinuxFontDelegate is empty?
Perhaps it could also live outside the preprocessor blocks.

[Daniel Erat, 2014-07-18 20:26:52]

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/400193004/diff/60001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:368: g_free(hint_style);
On 2014/07/18 19:59:46, msw wrote:
> On 2014/07/18 19:14:43, msw wrote:
> > nit: Remove the NULL checks for these two g_free calls. g_free "Frees the
> memory
> > pointed to by mem . If mem is NULL it simply returns."
> 
> ping, but you can leave them if you prefer.

whoops, sorry. saw this comment, did other things, and then somehow missed it
when i came back. :-/

https://codereview.chromium.org/400193004/diff/80001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (right):

https://codereview.chromium.org/400193004/diff/80001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:87: } else {
On 2014/07/18 19:59:46, msw wrote:
> Should this be a separate check like "if (!description.get()) ..." or
something
> to check if the underlying description from the LinuxFontDelegate is empty?
> Perhaps it could also live outside the preprocessor blocks.

sure. i'd prefer to have the delegate only return non-NULL descriptions, but
pango_font_description_copy() returns NULL when passed a NULL parameter and i
can't tell whether it's possible for GtkLabel's style's font_desc to be NULL, so
it's safer to do what you suggest.

pango_font_description_from_string() only returns NULL if passed a NULL string,
so this shouldn't be necessary for the chrome os case, but it doesn't hurt to
run it there.

to make this a bit more readable, i've changed ScopePangoFontDescription() to
have a c'tor that takes a string and automatically converts it to a
PangoFontDescription, but let me know if that seems too tricky.

[msw, 2014-07-18 20:31:39]

nice! lgtm with a nit.

https://codereview.chromium.org/400193004/diff/100001/ui/gfx/pango_util.h
File ui/gfx/pango_util.h (right):

https://codereview.chromium.org/400193004/diff/100001/ui/gfx/pango_util.h#new...
ui/gfx/pango_util.h:30: description_ =
pango_font_description_from_string(str.c_str());
nit: can this be done in an initializer list?

[Daniel Erat, 2014-07-18 20:35:05]

https://codereview.chromium.org/400193004/diff/100001/ui/gfx/pango_util.h
File ui/gfx/pango_util.h (right):

https://codereview.chromium.org/400193004/diff/100001/ui/gfx/pango_util.h#new...
ui/gfx/pango_util.h:30: description_ =
pango_font_description_from_string(str.c_str());
On 2014/07/18 20:31:39, msw wrote:
> nit: can this be done in an initializer list?

yep!

[Daniel Erat, 2014-07-18 20:35:10]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-18 20:36:41]

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

[commit-bot: I haz the power, 2014-07-18 23:53:59]

Change committed as 284255