Make GetCustomFontRenderParams() pass font style.

Make GetCustomFontRenderParams() pass font style.

Include bold and italic styles in Fontconfig queries.

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

[Daniel Erat, 2014-07-19 01:34:45]

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_params.h
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:76: const int* style,
i don't plan on adding any more arguments anytime soon, but let me know if you
think that this list is unwieldy enough now (especially with all these int
pointers) that passing the query parameters in a struct would be better.

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (left):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:131: SkTypeface::kNormal, params);
this was a bug, but luckily both enums use 0 for "normal"

[Daniel Erat, 2014-07-19 01:39:01]

hmm, just noticed that there's no windows implementation of this function. it
doesn't it get called there, but i'll add a NOTIMPLEMENTED() version like what
android has all the same in the next revision.

[msw, 2014-07-19 03:11:52]

lgtm with a nit and possible cleanup suggestion.

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_params.h
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:70: // |style| optionally points to a bitmap of
Font::FontStyle values.
nit: did you mean bitmask?

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:76: const int* style,
On 2014/07/19 01:34:44, Daniel Erat wrote:
> i don't plan on adding any more arguments anytime soon, but let me know if you
> think that this list is unwieldy enough now (especially with all these int
> pointers) that passing the query parameters in a struct would be better.

Eh, it's certainly not pretty, but it seems okay.

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:120: static FontRenderParams default_params;
nit: initialize these with their decalarations by inlining LoadDefaults?

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.cc (left):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:131: SkTypeface::kNormal, params);
On 2014/07/19 01:34:44, Daniel Erat wrote:
> this was a bug, but luckily both enums use 0 for "normal"

Acknowledged; thanks for fixing it.

[Daniel Erat, 2014-07-19 14:29:07]

thanks!

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_params.h
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:70: // |style| optionally points to a bitmap of
Font::FontStyle values.
On 2014/07/19 03:11:51, msw wrote:
> nit: did you mean bitmask?

i actually wasn't sure of the correct terminology for this. i'd probably
consider Font::BOLD to be a bitmask, but i was less sure of the right term for
several of these bits OR-ed together. this stack overflow answer suggests that
it's a bitmap
(http://stackoverflow.com/questions/9728102/what-is-the-difference-between-bit...),
but i think i prefer wikipedia's usage of "bit field"
(http://en.wikipedia.org/wiki/Bit_field), so i'll switch to that if it's okay
with you.

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:120: static FontRenderParams default_params;
On 2014/07/19 03:11:52, msw wrote:
> nit: initialize these with their decalarations by inlining LoadDefaults?

Done.

[msw, 2014-07-19 17:16:22]

lgtm

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_params.h
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/403923002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:70: // |style| optionally points to a bitmap of
Font::FontStyle values.
On 2014/07/19 14:29:05, Daniel Erat wrote:
> On 2014/07/19 03:11:51, msw wrote:
> > nit: did you mean bitmask?
> 
> i actually wasn't sure of the correct terminology for this. i'd probably
> consider Font::BOLD to be a bitmask, but i was less sure of the right term for
> several of these bits OR-ed together. this stack overflow answer suggests that
> it's a bitmap
>
(http://stackoverflow.com/questions/9728102/what-is-the-difference-between-bit...),
> but i think i prefer wikipedia's usage of "bit field"
> (http://en.wikipedia.org/wiki/Bit_field), so i'll switch to that if it's okay
> with you.

Acknowledged.

[Daniel Erat, 2014-07-20 13:35:20]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-20 13:35:38]

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

[commit-bot: I haz the power, 2014-07-20 17:21:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-20 21:38:24]

Change committed as 284377