linux: Avoid using unset Fontconfig properties.

linux: Avoid using unset Fontconfig properties.

Check the return values from FcPatternGet*() so unset
properties won't override the defaults from
LinuxFontDelegate. Most properties have default values
assigned to them by Fontconfig, but subpixel rendering
notably doesn't.

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

[msw, 2014-07-21 22:19:48]

This CL fixes issue 395014 (but not Issue 395090) for me.
That's the result we expected, afaik.

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:14: #include "ui/gfx/pango_util.h"
Should this (and parts of [Test|Linux]FontDelegate) be exclusive to USE_PANGO
builds? And/or is this test file excluded from non-pango configurations?

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:80: const LinuxFontDelegate*
orig_font_delegate_;
optional nit: original

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:220: FontRenderParams params =
GetCustomFontRenderParams(
nit: use GetDefaultFontRenderParams.

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:239: FontRenderParams params =
GetCustomFontRenderParams(
nit: use GetDefaultFontRenderParams.

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:267: FontRenderParams params =
GetCustomFontRenderParams(
nit: use GetDefaultFontRenderParams.

[Daniel Erat, 2014-07-21 22:34:33]

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:80: const LinuxFontDelegate*
orig_font_delegate_;
On 2014/07/21 22:19:48, msw wrote:
> optional nit: original

Done.

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:220: FontRenderParams params =
GetCustomFontRenderParams(
On 2014/07/21 22:19:48, msw wrote:
> nit: use GetDefaultFontRenderParams.

i'm actually wondering if i should just delete that method.
GetDefaultWebKitFontRenderParams() makes sense; it is (or at least was) used to
get default settings for web fonts. but we should be able to call
GetCustomFontRenderParams() everywhere else, since we know which font we're
going to render.

[msw, 2014-07-21 22:41:43]

lgtm with the hanging question about pango

[Daniel Erat, 2014-07-21 22:54:42]

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:14: #include "ui/gfx/pango_util.h"
On 2014/07/21 22:19:48, msw wrote:
> Should this (and parts of [Test|Linux]FontDelegate) be exclusive to USE_PANGO
> builds? And/or is this test file excluded from non-pango configurations?

whoops, sorry for missing this the first time through. :-(

that's a good question. i believe that desktop linux will always link in pango,
as pango is used by gtk+ to configure fonts and lay out text -- the Gtk2UI impl
of LinuxFontDelegate needs pango to get the default font. do you have any plans
to get rid of PlatformFontPango when switching to RenderTextHarfbuzz? if so, i
haven't seen any signs of them. :-)

font_render_params_linux.cc calls LinuxFontDelegate to get the default params,
so everything that uses font_render_params_linux.cc (just desktop linux and
chrome os, as far as i'm aware) is already pulling in pango.

[msw, 2014-07-21 23:38:30]

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
ui/gfx/font_render_params_linux_unittest.cc:14: #include "ui/gfx/pango_util.h"
On 2014/07/21 22:54:42, Daniel Erat wrote:
> On 2014/07/21 22:19:48, msw wrote:
> > Should this (and parts of [Test|Linux]FontDelegate) be exclusive to
USE_PANGO
> > builds? And/or is this test file excluded from non-pango configurations?
> 
> whoops, sorry for missing this the first time through. :-(
> 
> that's a good question. i believe that desktop linux will always link in
pango,
> as pango is used by gtk+ to configure fonts and lay out text -- the Gtk2UI
impl
> of LinuxFontDelegate needs pango to get the default font. do you have any
plans
> to get rid of PlatformFontPango when switching to RenderTextHarfbuzz? if so, i
> haven't seen any signs of them. :-)
> 
> font_render_params_linux.cc calls LinuxFontDelegate to get the default params,
> so everything that uses font_render_params_linux.cc (just desktop linux and
> chrome os, as far as i'm aware) is already pulling in pango.

Okay, I'm not actually worried beyond breaking any configs (Ozone or something?)
that might have been building this test file on Linux without Pango. You can
proceed if that's not a legit concern.

[Daniel Erat, 2014-07-22 13:34:10]

On 2014/07/21 23:38:30, msw wrote:
>
https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
> File ui/gfx/font_render_params_linux_unittest.cc (right):
> 
>
https://codereview.chromium.org/407763003/diff/1/ui/gfx/font_render_params_li...
> ui/gfx/font_render_params_linux_unittest.cc:14: #include "ui/gfx/pango_util.h"
> On 2014/07/21 22:54:42, Daniel Erat wrote:
> > On 2014/07/21 22:19:48, msw wrote:
> > > Should this (and parts of [Test|Linux]FontDelegate) be exclusive to
> USE_PANGO
> > > builds? And/or is this test file excluded from non-pango configurations?
> > 
> > whoops, sorry for missing this the first time through. :-(
> > 
> > that's a good question. i believe that desktop linux will always link in
> pango,
> > as pango is used by gtk+ to configure fonts and lay out text -- the Gtk2UI
> impl
> > of LinuxFontDelegate needs pango to get the default font. do you have any
> plans
> > to get rid of PlatformFontPango when switching to RenderTextHarfbuzz? if so,
i
> > haven't seen any signs of them. :-)
> > 
> > font_render_params_linux.cc calls LinuxFontDelegate to get the default
params,
> > so everything that uses font_render_params_linux.cc (just desktop linux and
> > chrome os, as far as i'm aware) is already pulling in pango.
> 
> Okay, I'm not actually worried beyond breaking any configs (Ozone or
something?)
> that might have been building this test file on Linux without Pango. You can
> proceed if that's not a legit concern.

hmm, this indeed fails to build on linux_ecs_ozone. i think that what's going on
is that that build includes linux_font_delegate.h, but that file just
forward-declares ScopedPangoFontDeclaration without compiling any concrete
implementations. the simplest fix is for me to just avoid compiling this test
when !defined(USE_PANGO), i think.

[Daniel Erat, 2014-07-22 14:22:06]

The CQ bit was checked by derat@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-22 16:20:31]

Change committed as 284697

[behdad_google, 2014-07-22 18:27:23]

FWIW not setting subpixel order by default makes sense and is intended.  You
can't guess the user's display subpixel order.  Guessing wrong is catastrophic. 
Whereas for other settings, it's ok to just choose one setting.

[Daniel Erat, 2014-07-22 18:30:55]

On 2014/07/22 18:27:23, behdad_google wrote:
> FWIW not setting subpixel order by default makes sense and is intended.  You
> can't guess the user's display subpixel order.  Guessing wrong is
catastrophic. 
> Whereas for other settings, it's ok to just choose one setting.

why doesn't antialiasing have a default?