ui/gfx: Allow for font-specific rendering settings.

ui/gfx: Allow for font-specific rendering settings.

Let implementations attach a FontRenderParams to a
gfx::PlatformFont rather than using a single default
everywhere. On Linux and Chrome OS, query FontConfig for
font-specific rendering settings.

BUG=125235, 375824
TBR=jorgelo@chromium.org

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

[Daniel Erat, 2014-07-11 17:15:29]

i haven't sent this to the bots yet since it's based on another CL that's still
in the CQ, but it seems to work on chrome os and linux.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:74: std::string* family_out);
returning the font family here seems a bit out of place, but we'd have to do two
identical fontconfig queries instead of one otherwise.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:30: bool QueryFontConfig(
this essentially just combines the code from LoadDefaults() and
PlatformFontPango's FindBestMatchFontFamilyName() (although it also grabs some
additional settings from fontconfig now)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:204: *family_out = (*family_list)[0];
i didn't bother adding fallback code like this for any other implementations
since PlatformFontPango is the only class that calls GetCustomFontRenderParams()

[Daniel Erat, 2014-07-11 18:08:41]

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:157: GetCustomFontRenderParams(false,
&family_list, &new_size, NULL, NULL);
calling this is only necessary when |size_delta| is nonzero. i'll update this in
the next revision.

[Daniel Erat, 2014-07-11 23:45:02]

(in case you're waiting for the next revision: i was holding off on uploading it
until you've had a chance to read the comments on the current revision. let me
know if you have.) :-)

[msw, 2014-07-12 00:27:34]

Sorry for the heavy comments, this is a heavy patch!

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_list.h#newco...
ui/gfx/font_list.h:115: // Returns an object describing how the primary font
should be rendered.
Can we avoid this? I'd rather users explicitly call GetPrimaryFont if they need
the params for that font. (Otherwise users might mistakenly apply the primary
font's settings to other fonts in the list)

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:63: // Returns the system's default parameters for
WebKit font rendering.
optional [followup] nit: update the WebKit name and comment here too.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:70: bool renderer,
nit: clarify this in name |for_web_contents|? or in comment.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:74: std::string* family_out);
On 2014/07/11 17:15:29, Daniel Erat wrote:
> returning the font family here seems a bit out of place, but we'd have to do
two
> identical fontconfig queries instead of one otherwise.

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:76: // Returns the system's default parameters for
WebKit subpixel positioning.
optional [followup] nit: update the WebKit name and comment here too. Or maybe
the *one* caller could just use GetDefaultWebKitFontRenderParams instead?

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:30: bool QueryFontConfig(
On 2014/07/11 17:15:29, Daniel Erat wrote:
> this essentially just combines the code from LoadDefaults() and
> PlatformFontPango's FindBestMatchFontFamilyName() (although it also grabs some
> additional settings from fontconfig now)

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:45: FcPatternAdd(pattern, FC_FAMILY, value,
FcTrue /* append */);
nit: FcPatternAddString might be simpler.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:52: FcPatternAdd(pattern, FC_PIXEL_SIZE,
value, FcTrue /* append */);
nit: FcPatternAddDouble might be simpler

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:58: FcPatternAdd(pattern, FC_SIZE, value,
FcTrue /* append */);
nit: FcPatternAddInteger might be simpler.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:64: FcPattern* match = FcFontMatch(0,
pattern, &result);
nit: s/0/NULL/

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:65: if (!match)
Should we DCHECK this or is it okay to not actually find any matches?

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:87: FcPatternGetInteger(match, FC_HINT_STYLE,
0, &fc_hint_style) ==
Does it make sense to offer a default hinting setting other than HINTING_NONE if
FC_HINTING is true but FC_HINT_STYLE is not set? Similarly, I'm not sure what's
intended if FC_HINTING is false, but FC_HINT_STYLE is (eg.) FC_HINT_FULL.
Perhaps your current logic requiring both settings to offer any hinting is
reasonable. (why does FontConfig have a separate bool and enum for this?)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:91: switch (fc_hint_style) {
optional nit: add a conversion helper.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:110: switch (fc_rgba) {
optional nit: add a conversion helper.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:175: params.antialiasing = true;
These reasonable defaults previously applied to Desktop Linux Aura as well,
should they continue to do so?

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:180: // Query Fontconfig to get the family
name and subpixel rendering setting.
Why don't we respect the other FontConfig settings on Chrome OS? Should we be
passing in |params| to use its values as defaults?

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:186: const LinuxFontDelegate* delegate =
LinuxFontDelegate::instance();
Can we remove LinuxFontDelegate (or most if it) now that we get the right
settings from FontConfig? (if so, that would be fine as a TODO + follow-up)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:204: *family_out = (*family_list)[0];
On 2014/07/11 17:15:29, Daniel Erat wrote:
> i didn't bother adding fallback code like this for any other implementations
> since PlatformFontPango is the only class that calls
GetCustomFontRenderParams()

Acknowledged.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:72: pango_font_description_get_size(native_font) /
PANGO_SCALE;
Hmm, can you consolidate this with pango_util's GetPangoFontSizeInPixels?
(perhaps only send/accept pixel sizes in GetCustomFontRenderParams?)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:157: GetCustomFontRenderParams(false,
&family_list, &new_size, NULL, NULL);
On 2014/07/11 18:08:40, Daniel Erat wrote:
> calling this is only necessary when |size_delta| is nonzero. i'll update this
in
> the next revision.

Really? What if the style (bold/italic) differs?
Please add a comment explaining why that's the case.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.h (right):

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:26: PlatformFontPango(const std::string& font_name,
int font_size);
nit: denote if this is pixel or point size in a comment.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:67: int size,
nit: denote if this is pixel or point size in the comment.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:77: int font_size,
nit: denote if this is pixel or point size in the comment.
(maybe readers should know the implicit unit of measurement?)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
Should there be a TODO here for any per-font settings?

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
nit: Can we have a static (lazy init) struct to return here?

[Daniel Erat, 2014-07-12 01:55:33]

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_list.h#newco...
ui/gfx/font_list.h:115: // Returns an object describing how the primary font
should be rendered.
On 2014/07/12 00:27:31, msw wrote:
> Can we avoid this? I'd rather users explicitly call GetPrimaryFont if they
need
> the params for that font. (Otherwise users might mistakenly apply the primary
> font's settings to other fonts in the list)

sure, i just did this to match the style of the rest of FontList

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:63: // Returns the system's default parameters for
WebKit font rendering.
On 2014/07/12 00:27:31, msw wrote:
> optional [followup] nit: update the WebKit name and comment here too.

added a todo and will do it in a later change (since it requires touching
additional files)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:70: bool renderer,
On 2014/07/12 00:27:32, msw wrote:
> nit: clarify this in name |for_web_contents|? or in comment.

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:76: // Returns the system's default parameters for
WebKit subpixel positioning.
On 2014/07/12 00:27:31, msw wrote:
> optional [followup] nit: update the WebKit name and comment here too. Or maybe
> the *one* caller could just use GetDefaultWebKitFontRenderParams instead?

done (my favorite part is "WebKit" in the earlier function's name vs. "Webkit"
in this function)

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:45: FcPatternAdd(pattern, FC_FAMILY, value,
FcTrue /* append */);
On 2014/07/12 00:27:33, msw wrote:
> nit: FcPatternAddString might be simpler.

nice! i'd missed these

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:52: FcPatternAdd(pattern, FC_PIXEL_SIZE,
value, FcTrue /* append */);
On 2014/07/12 00:27:33, msw wrote:
> nit: FcPatternAddDouble might be simpler

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:58: FcPatternAdd(pattern, FC_SIZE, value,
FcTrue /* append */);
On 2014/07/12 00:27:32, msw wrote:
> nit: FcPatternAddInteger might be simpler.

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:64: FcPattern* match = FcFontMatch(0,
pattern, &result);
On 2014/07/12 00:27:32, msw wrote:
> nit: s/0/NULL/

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:65: if (!match)
On 2014/07/12 00:27:33, msw wrote:
> Should we DCHECK this or is it okay to not actually find any matches?

the documentation is silent, of course, but the implementation looks like it
returns NULL if it fails to find a config (maybe if the caller forgot to
initialize fontconfig?) or if FcFontSetMatchInternal() returns NULL. that method
looks like it returns NULL if it fails to find a match, so it's probably better
to handle this instead of DCHECK-ing.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:87: FcPatternGetInteger(match, FC_HINT_STYLE,
0, &fc_hint_style) ==
On 2014/07/12 00:27:32, msw wrote:
> Does it make sense to offer a default hinting setting other than HINTING_NONE
if
> FC_HINTING is true but FC_HINT_STYLE is not set? Similarly, I'm not sure
what's
> intended if FC_HINTING is false, but FC_HINT_STYLE is (eg.) FC_HINT_FULL.
> Perhaps your current logic requiring both settings to offer any hinting is
> reasonable. (why does FontConfig have a separate bool and enum for this?)

(to make it possible for users to choose self-contradicting settings? :-P)

the pango implementation is at
https://git.gnome.org/browse/pango/tree/pango/pangoft2.c#n162 -- maybe we should
just match that:

- read FC_ANTIALIAS bool; default to true if unset
- read FC_HINTING bool; default to true if unset
- read FC_HINT_STYLE int; default to full if unset
- read FC_AUTOHINT bool; default to false if unset

it looks like the hinting bool being false trumps the hinting style enum there,
too.

in general, i'm not particularly worried about us needing to fall back to
defaults for any of these. fontconfig ships with a default config, and i suspect
that users are almost always overlaying crazy stuff on top of it via
~/.fonts.conf instead of deleting stuff from it.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:91: switch (fc_hint_style) {
On 2014/07/12 00:27:33, msw wrote:
> optional nit: add a conversion helper.

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:110: switch (fc_rgba) {
On 2014/07/12 00:27:32, msw wrote:
> optional nit: add a conversion helper.

Done.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:175: params.antialiasing = true;
On 2014/07/12 00:27:32, msw wrote:
> These reasonable defaults previously applied to Desktop Linux Aura as well,
> should they continue to do so?

no, i don't think so. i just meant "reasonable for chrome os" when i wrote that
comment, and i was a bit scared that someone would inadvertently change chrome
os while trying to change the linux defaults in the future.

the defaults that i added in FontRenderParam's c'tor line up with these, so i
don't think that linux should be affected.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:180: // Query Fontconfig to get the family
name and subpixel rendering setting.
On 2014/07/12 00:27:33, msw wrote:
> Why don't we respect the other FontConfig settings on Chrome OS? Should we be
> passing in |params| to use its values as defaults?

i'd actually like for us to get away from using fontconfig at all on chrome os
-- among other things, it makes it difficult to get deterministic font rendering
when running a chrome os build of chrome on goobuntu.

i regrettably made us use fontconfig to communicate that subpixel rendering
should be disabled on chromeboxes long ago; perhaps i should've added a flag
instead. i think that we're also dependent on it for specifying alternate fonts
to use for differents scripts, though.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:186: const LinuxFontDelegate* delegate =
LinuxFontDelegate::instance();
On 2014/07/12 00:27:33, msw wrote:
> Can we remove LinuxFontDelegate (or most if it) now that we get the right
> settings from FontConfig? (if so, that would be fine as a TODO + follow-up)

i'll add a TODO about this since i honestly wasn't sure about it. GtkSettings
exposes rendering settings, but i believe that GTK queries fontconfig when
drawing widgets, so i'm really not sure where the settings come into play, if
anywhere.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:72: pango_font_description_get_size(native_font) /
PANGO_SCALE;
On 2014/07/12 00:27:33, msw wrote:
> Hmm, can you consolidate this with pango_util's GetPangoFontSizeInPixels?
> (perhaps only send/accept pixel sizes in GetCustomFontRenderParams?)

i don't think so. i was hoping to go this route at first, but fontconfig seems
to only e.g. apply point-based matches to queries using points, even if given a
pixel-based size that should fall within the range.

you can see this using https://github.com/derat/font-config-info. when i have a
fontconfig match for a "size" (i.e. points) >= 0 and <= 9 that disables
antialiasing, i see the following:

./font-config-info -f 'Arial 8'  # i.e. 8-point
...
Fontconfig (Arial 8):
requested size       8 points at 96.00 DPI
family               Arial
pixelsize            8.33 pixels
size                 8 points
antialias            0
hinting              1
autohint             0
hintstyle            1 (slight)
rgba                 5 (none)

./font-config-info -f 'Arial 4px'
...
Fontconfig (Arial 4px):
requested size       4.00 pixels at 96.00 DPI
family               Arial
pixelsize            4.00 pixels
size                 [no match]
antialias            1
hinting              1
autohint             0
hintstyle            3 (full)
rgba                 5 (none)

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:157: GetCustomFontRenderParams(false,
&family_list, &new_size, NULL, NULL);
On 2014/07/12 00:27:33, msw wrote:
> On 2014/07/11 18:08:40, Daniel Erat wrote:
> > calling this is only necessary when |size_delta| is nonzero. i'll update
this
> in
> > the next revision.
> 
> Really? What if the style (bold/italic) differs?
> Please add a comment explaining why that's the case.

to be clear, i mean that the call to GetCustomFontRenderParams() is only
necessary if the size changed, since we only pass the family list and the size
to it. we can just use the previously-queried params otherwise.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
File ui/gfx/platform_font_pango.h (right):

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:26: PlatformFontPango(const std::string& font_name,
int font_size);
On 2014/07/12 00:27:33, msw wrote:
> nit: denote if this is pixel or point size in a comment.

renamed the argument

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:67: int size,
On 2014/07/12 00:27:33, msw wrote:
> nit: denote if this is pixel or point size in the comment.

renamed

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.h:77: int font_size,
On 2014/07/12 00:27:33, msw wrote:
> nit: denote if this is pixel or point size in the comment.
> (maybe readers should know the implicit unit of measurement?)

renamed

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
On 2014/07/12 00:27:33, msw wrote:
> nit: Can we have a static (lazy init) struct to return here?

the windows implementation of GetDefaultFontRenderParams() returns a reference
to a lazily-initialized singleton, if i'm reading your code correctly :-P

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
On 2014/07/12 00:27:33, msw wrote:
> Should there be a TODO here for any per-font settings?

i wasn't sure if windows supports this. font_render_params_win.cc looks like
it's grabbing global settings via SystemParametersInfo().

[msw, 2014-07-12 02:26:41]

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_list.h#newco...
ui/gfx/font_list.h:115: // Returns an object describing how the primary font
should be rendered.
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:31, msw wrote:
> > Can we avoid this? I'd rather users explicitly call GetPrimaryFont if they
> need
> > the params for that font. (Otherwise users might mistakenly apply the
primary
> > font's settings to other fonts in the list)
> 
> sure, i just did this to match the style of the rest of FontList

Yeah, it's a gray area, but good to avoid non-obvious implicit primary font use.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:63: // Returns the system's default parameters for
WebKit font rendering.
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:31, msw wrote:
> > optional [followup] nit: update the WebKit name and comment here too.
> 
> added a todo and will do it in a later change (since it requires touching
> additional files)

sgtm, thanks

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:65: if (!match)
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > Should we DCHECK this or is it okay to not actually find any matches?
> 
> the documentation is silent, of course, but the implementation looks like it
> returns NULL if it fails to find a config (maybe if the caller forgot to
> initialize fontconfig?) or if FcFontSetMatchInternal() returns NULL. that
method
> looks like it returns NULL if it fails to find a match, so it's probably
better
> to handle this instead of DCHECK-ing.

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:87: FcPatternGetInteger(match, FC_HINT_STYLE,
0, &fc_hint_style) ==
On 2014/07/12 01:55:33, Daniel Erat wrote:
> On 2014/07/12 00:27:32, msw wrote:
> > Does it make sense to offer a default hinting setting other than
HINTING_NONE
> if
> > FC_HINTING is true but FC_HINT_STYLE is not set? Similarly, I'm not sure
> what's
> > intended if FC_HINTING is false, but FC_HINT_STYLE is (eg.) FC_HINT_FULL.
> > Perhaps your current logic requiring both settings to offer any hinting is
> > reasonable. (why does FontConfig have a separate bool and enum for this?)
> 
> (to make it possible for users to choose self-contradicting settings? :-P)
> 
> the pango implementation is at
> https://git.gnome.org/browse/pango/tree/pango/pangoft2.c#n162 -- maybe we
should
> just match that:
> 
> - read FC_ANTIALIAS bool; default to true if unset
> - read FC_HINTING bool; default to true if unset
> - read FC_HINT_STYLE int; default to full if unset
> - read FC_AUTOHINT bool; default to false if unset
> 
> it looks like the hinting bool being false trumps the hinting style enum
there,
> too.
> 
> in general, i'm not particularly worried about us needing to fall back to
> defaults for any of these. fontconfig ships with a default config, and i
suspect
> that users are almost always overlaying crazy stuff on top of it via
> ~/.fonts.conf instead of deleting stuff from it.

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:175: params.antialiasing = true;
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:32, msw wrote:
> > These reasonable defaults previously applied to Desktop Linux Aura as well,
> > should they continue to do so?
> 
> no, i don't think so. i just meant "reasonable for chrome os" when i wrote
that
> comment, and i was a bit scared that someone would inadvertently change chrome
> os while trying to change the linux defaults in the future.
> 
> the defaults that i added in FontRenderParam's c'tor line up with these, so i
> don't think that linux should be affected.

Ah, ok. Should we remove these explicit assignments, given the new ctor?

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:180: // Query Fontconfig to get the family
name and subpixel rendering setting.
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > Why don't we respect the other FontConfig settings on Chrome OS? Should we
be
> > passing in |params| to use its values as defaults?
> 
> i'd actually like for us to get away from using fontconfig at all on chrome os
> -- among other things, it makes it difficult to get deterministic font
rendering
> when running a chrome os build of chrome on goobuntu.
> 
> i regrettably made us use fontconfig to communicate that subpixel rendering
> should be disabled on chromeboxes long ago; perhaps i should've added a flag
> instead. i think that we're also dependent on it for specifying alternate
fonts
> to use for differents scripts, though.

Can you add a comment here? The reasons for limited FC use are non-obvious.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:186: const LinuxFontDelegate* delegate =
LinuxFontDelegate::instance();
On 2014/07/12 01:55:32, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > Can we remove LinuxFontDelegate (or most if it) now that we get the right
> > settings from FontConfig? (if so, that would be fine as a TODO + follow-up)
> 
> i'll add a TODO about this since i honestly wasn't sure about it. GtkSettings
> exposes rendering settings, but i believe that GTK queries fontconfig when
> drawing widgets, so i'm really not sure where the settings come into play, if
> anywhere.

Acknowledged.

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:72: pango_font_description_get_size(native_font) /
PANGO_SCALE;
On 2014/07/12 01:55:33, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > Hmm, can you consolidate this with pango_util's GetPangoFontSizeInPixels?
> > (perhaps only send/accept pixel sizes in GetCustomFontRenderParams?)
> 
> i don't think so. i was hoping to go this route at first, but fontconfig seems
> to only e.g. apply point-based matches to queries using points, even if given
a
> pixel-based size that should fall within the range.
> 
> you can see this using https://github.com/derat/font-config-info. when i have
a
> fontconfig match for a "size" (i.e. points) >= 0 and <= 9 that disables
> antialiasing, i see the following:
> 
> ./font-config-info -f 'Arial 8'  # i.e. 8-point
> ...
> Fontconfig (Arial 8):
> requested size       8 points at 96.00 DPI
> family               Arial
> pixelsize            8.33 pixels
> size                 8 points
> antialias            0
> hinting              1
> autohint             0
> hintstyle            1 (slight)
> rgba                 5 (none)
> 
> ./font-config-info -f 'Arial 4px'
> ...
> Fontconfig (Arial 4px):
> requested size       4.00 pixels at 96.00 DPI
> family               Arial
> pixelsize            4.00 pixels
> size                 [no match]
> antialias            1
> hinting              1
> autohint             0
> hintstyle            3 (full)
> rgba                 5 (none)

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:157: GetCustomFontRenderParams(false,
&family_list, &new_size, NULL, NULL);
On 2014/07/12 01:55:33, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > On 2014/07/11 18:08:40, Daniel Erat wrote:
> > > calling this is only necessary when |size_delta| is nonzero. i'll update
> this
> > in
> > > the next revision.
> > 
> > Really? What if the style (bold/italic) differs?
> > Please add a comment explaining why that's the case.
> 
> to be clear, i mean that the call to GetCustomFontRenderParams() is only
> necessary if the size changed, since we only pass the family list and the size
> to it. we can just use the previously-queried params otherwise.

Acknowledged.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
On 2014/07/12 01:55:33, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > nit: Can we have a static (lazy init) struct to return here?
> 
> the windows implementation of GetDefaultFontRenderParams() returns a reference
> to a lazily-initialized singleton, if i'm reading your code correctly :-P

Ah, right.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:196: return GetDefaultFontRenderParams();
On 2014/07/12 01:55:33, Daniel Erat wrote:
> On 2014/07/12 00:27:33, msw wrote:
> > Should there be a TODO here for any per-font settings?
> 
> i wasn't sure if windows supports this. font_render_params_win.cc looks like
> it's grabbing global settings via SystemParametersInfo().

Perhaps it's not supported, it's fine as-is without a TODO.

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:27: // Converts a Fontconfig FC_HINT_STYLE
value to a FontRenderParams::Hinting
optional nit: omit "a" and "value" x2 for a one-liner.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:29: FontRenderParams::Hinting
FontconfigHintStyleToFontRenderParamsHinting(
optional nit: consider a shorter name?

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:31: switch (hint_style) {
This needs a return statement after the switch.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:39: // Converts a Fontconfig FC_RGBA value to
a FontRenderParams::SubpixelRendering
optional nit: omit "a" and "value" x2 for a one-liner.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:42:
FontconfigRgbaToFontRenderParamsSubpixelRendering(int rgba) {
optional nit: consider a shorter name?

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:43: switch (rgba) {
This needs a return statement after the switch.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:57: const int* point_size
nit: missing comma

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:155: if (size_delta) {
nit: explicitly check != 0

[Daniel Erat, 2014-07-12 02:44:39]

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

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:175: params.antialiasing = true;
On 2014/07/12 02:26:40, msw wrote:
> On 2014/07/12 01:55:32, Daniel Erat wrote:
> > On 2014/07/12 00:27:32, msw wrote:
> > > These reasonable defaults previously applied to Desktop Linux Aura as
well,
> > > should they continue to do so?
> > 
> > no, i don't think so. i just meant "reasonable for chrome os" when i wrote
> that
> > comment, and i was a bit scared that someone would inadvertently change
chrome
> > os while trying to change the linux defaults in the future.
> > 
> > the defaults that i added in FontRenderParam's c'tor line up with these, so
i
> > don't think that linux should be affected.
> 
> Ah, ok. Should we remove these explicit assignments, given the new ctor?

i'd rather continue to be explicit for chrome os, since we know what we want the
settings to be there. i'm more comfortable starting with the defaults from the
c'tor for desktop linux, since there are no "right" settings there and it's very
likely that whatever we use will be overridden by gtk and/or fontconfig.

https://codereview.chromium.org/382273002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:180: // Query Fontconfig to get the family
name and subpixel rendering setting.
On 2014/07/12 02:26:40, msw wrote:
> On 2014/07/12 01:55:32, Daniel Erat wrote:
> > On 2014/07/12 00:27:33, msw wrote:
> > > Why don't we respect the other FontConfig settings on Chrome OS? Should we
> be
> > > passing in |params| to use its values as defaults?
> > 
> > i'd actually like for us to get away from using fontconfig at all on chrome
os
> > -- among other things, it makes it difficult to get deterministic font
> rendering
> > when running a chrome os build of chrome on goobuntu.
> > 
> > i regrettably made us use fontconfig to communicate that subpixel rendering
> > should be disabled on chromeboxes long ago; perhaps i should've added a flag
> > instead. i think that we're also dependent on it for specifying alternate
> fonts
> > to use for differents scripts, though.
> 
> Can you add a comment here? The reasons for limited FC use are non-obvious.

done. also added a TODO, since i'm still not wholly convinced. maybe it'd be
better if chrome os just used the same fontconfig-dependent path as desktop
linux. i need to talk to jungshik about this to see what his plans are.

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:27: // Converts a Fontconfig FC_HINT_STYLE
value to a FontRenderParams::Hinting
On 2014/07/12 02:26:41, msw wrote:
> optional nit: omit "a" and "value" x2 for a one-liner.

Done.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:29: FontRenderParams::Hinting
FontconfigHintStyleToFontRenderParamsHinting(
On 2014/07/12 02:26:41, msw wrote:
> optional nit: consider a shorter name?

i just changed these to "ConvertFontconfig____"; they're in an anon namespace
and return enums, so hopefully it isn't ambiguous

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:31: switch (hint_style) {
On 2014/07/12 02:26:41, msw wrote:
> This needs a return statement after the switch.

do we use compilers that don't understand the default case? gcc doesn't
complain, at least.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:39: // Converts a Fontconfig FC_RGBA value to
a FontRenderParams::SubpixelRendering
On 2014/07/12 02:26:41, msw wrote:
> optional nit: omit "a" and "value" x2 for a one-liner.

Done.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:42:
FontconfigRgbaToFontRenderParamsSubpixelRendering(int rgba) {
On 2014/07/12 02:26:41, msw wrote:
> optional nit: consider a shorter name?

Done.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:43: switch (rgba) {
On 2014/07/12 02:26:41, msw wrote:
> This needs a return statement after the switch.

same here

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:57: const int* point_size
On 2014/07/12 02:26:41, msw wrote:
> nit: missing comma

ugh, broke this in post-compile reindenting

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:155: if (size_delta) {
On 2014/07/12 02:26:41, msw wrote:
> nit: explicitly check != 0

heh, was curious which way you'd go here

[msw, 2014-07-12 17:57:11]

LGTM with an optional nit (and modulo switch return errors).

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:29: FontRenderParams::Hinting
FontconfigHintStyleToFontRenderParamsHinting(
On 2014/07/12 02:44:38, Daniel Erat wrote:
> On 2014/07/12 02:26:41, msw wrote:
> > optional nit: consider a shorter name?
> 
> i just changed these to "ConvertFontconfig____"; they're in an anon namespace
> and return enums, so hopefully it isn't ambiguous

Nice.

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:31: switch (hint_style) {
On 2014/07/12 02:44:38, Daniel Erat wrote:
> On 2014/07/12 02:26:41, msw wrote:
> > This needs a return statement after the switch.
> 
> do we use compilers that don't understand the default case? gcc doesn't
> complain, at least.

I don't make the clang rules, I just fix the clang errors :)
Feel free to land them as-is if you don't get any errors.

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:155: if (size_delta) {
On 2014/07/12 02:44:38, Daniel Erat wrote:
> On 2014/07/12 02:26:41, msw wrote:
> > nit: explicitly check != 0
> 
> heh, was curious which way you'd go here

I prefer explicit checks for everything but bools and pointers.
What's your general preference for these things?

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

https://codereview.chromium.org/382273002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:100: params_out->hinting =
ConvertFontconfigHintStyle(fc_hint_style);
optional nit: You could probably assume that FcPatternGet* doesn't modify the
value without returning FcResultMatch, then, since you start with reasonable
values for |fc_hint_style| and |fc_rgba|, you could always set |params_out|
using the converted Fontconfig values. Like:

FcBool fc_hinting = 0;
int fc_hint_style = FC_HINT_NONE;
FcPatternGetBool(match, FC_HINTING, 0, &fc_hinting);
if (fc_hinting)
  FcPatternGetInteger(match, FC_HINT_STYLE, 0, &fc_hint_style);
params_out->hinting = ConvertFontconfigHintStyle(fc_hint_style);

int fc_rgba = FC_RGBA_UNKNOWN;
FcPatternGetInteger(match, FC_RGBA, 0, &fc_rgba);
params_out->subpixel_rendering = ConvertFontconfigRgba(fc_rgba);

[Daniel Erat, 2014-07-14 14:59:16]

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:31: switch (hint_style) {
On 2014/07/12 17:57:11, msw wrote:
> On 2014/07/12 02:44:38, Daniel Erat wrote:
> > On 2014/07/12 02:26:41, msw wrote:
> > > This needs a return statement after the switch.
> > 
> > do we use compilers that don't understand the default case? gcc doesn't
> > complain, at least.
> 
> I don't make the clang rules, I just fix the clang errors :)
> Feel free to land them as-is if you don't get any errors.

cool (seems to build locally with clang)

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

https://codereview.chromium.org/382273002/diff/40001/ui/gfx/platform_font_pan...
ui/gfx/platform_font_pango.cc:155: if (size_delta) {
On 2014/07/12 17:57:11, msw wrote:
> On 2014/07/12 02:44:38, Daniel Erat wrote:
> > On 2014/07/12 02:26:41, msw wrote:
> > > nit: explicitly check != 0
> > 
> > heh, was curious which way you'd go here
> 
> I prefer explicit checks for everything but bools and pointers.
> What's your general preference for these things?

i'm mostly the same (especially if i happen to be writing go code on the side,
since it's stricter about this)

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

https://codereview.chromium.org/382273002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:64: // TODO(derat): s/WebKit/Blink/
changed my mind here and updated this comment to suggest
GetDefaultFontRenderParamsForWebContents() so it matches the param to
GetCustom...()

https://codereview.chromium.org/382273002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params.h:69: // it will be updated to contain the recommended
font family from |family_list|.
added a TODO here to also accept a style, since we can pass bold/italic to
fontconfig as well

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

https://codereview.chromium.org/382273002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:100: params_out->hinting =
ConvertFontconfigHintStyle(fc_hint_style);
On 2014/07/12 17:57:11, msw wrote:
> optional nit: You could probably assume that FcPatternGet* doesn't modify the
> value without returning FcResultMatch, then, since you start with reasonable
> values for |fc_hint_style| and |fc_rgba|, you could always set |params_out|
> using the converted Fontconfig values. Like:
> 
> FcBool fc_hinting = 0;
> int fc_hint_style = FC_HINT_NONE;
> FcPatternGetBool(match, FC_HINTING, 0, &fc_hinting);
> if (fc_hinting)
>   FcPatternGetInteger(match, FC_HINT_STYLE, 0, &fc_hint_style);
> params_out->hinting = ConvertFontconfigHintStyle(fc_hint_style);
> 
> int fc_rgba = FC_RGBA_UNKNOWN;
> FcPatternGetInteger(match, FC_RGBA, 0, &fc_rgba);
> params_out->subpixel_rendering = ConvertFontconfigRgba(fc_rgba);

thanks! i've confirmed that this is how fontconfig works and updated the rest of
the return values to use the same pattern.

[Daniel Erat, 2014-07-14 14:59:54]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-14 15:01:02]

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

[Daniel Erat, 2014-07-14 15:49:55]

TBR jorgelo@ for sandbox

[Daniel Erat, 2014-07-14 15:50:02]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-14 15:50:12]

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

[Daniel Erat, 2014-07-14 17:07:39]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-14 17:07:43]

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

[commit-bot: I haz the power, 2014-07-14 19:40:59]

Change committed as 283001