Apply hinting in SkiaTextRenderer::SetFontRenderParams().

Apply hinting in SkiaTextRenderer::SetFontRenderParams().

Make SetFontRenderParams() pass the hinting setting through
to SkPaint instead of exposing a separate SetFontHinting()
method. Also add a default c'tor for FontRenderParams and
pass its autohinter setting through to SkPaint as well.

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

[Daniel Erat, 2014-07-10 23:46:35]

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:68: params->autohinter = renderer;
so this is pretty gross. SkiaTextRenderer wasn't passing the setting through to
SkPaint previously, but android_webview/native/aw_settings.cc and
chrome/browser/renderer_preferences_util.cc _do_ appear to have been passing it
to the renderer. on linux, it'd default to true since that's what we set it to
for chrome os above.

my larger followup change that attaches a FontRenderParams struct to each
gfx::PlatformFont makes us get the autohinter setting from FontConfig, at which
case i'm hopeful this code can go. (although we'll then get the opportunity to
discover how many users have the autohinter turned on in FontConfig
accidentally.)

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:31: params_->hinting =
FontRenderParams::HINTING_MEDIUM;
we weren't setting these explicitly here, but i believe that these are the
default skia settings

[msw, 2014-07-11 00:29:10]

lgtm with optional nits.

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:68: params->autohinter = renderer;
On 2014/07/10 23:46:35, Daniel Erat wrote:
> so this is pretty gross. SkiaTextRenderer wasn't passing the setting through
to
> SkPaint previously, but android_webview/native/aw_settings.cc and
> chrome/browser/renderer_preferences_util.cc _do_ appear to have been passing
it
> to the renderer. on linux, it'd default to true since that's what we set it to
> for chrome os above.
> 
> my larger followup change that attaches a FontRenderParams struct to each
> gfx::PlatformFont makes us get the autohinter setting from FontConfig, at
which
> case i'm hopeful this code can go. (although we'll then get the opportunity to
> discover how many users have the autohinter turned on in FontConfig
> accidentally.)

Acknowledged.

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_win.cc:31: params_->hinting =
FontRenderParams::HINTING_MEDIUM;
On 2014/07/10 23:46:35, Daniel Erat wrote:
> we weren't setting these explicitly here, but i believe that these are the
> default skia settings

Acknowledged.

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:211: switch (params.hinting) {
optional nit: add a helper to convert the enum (case x: return y).
(it's sad that we have separate gfx, skia, and content enums for this...)

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/render_text_pango...
ui/gfx/render_text_pango.cc:384: const FontRenderParams& render_params =
GetDefaultFontRenderParams();
optional nit: inline |render_params| here, like other impls.

[Daniel Erat, 2014-07-11 03:15:02]

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:211: switch (params.hinting) {
On 2014/07/11 00:29:10, msw wrote:
> optional nit: add a helper to convert the enum (case x: return y).
> (it's sad that we have separate gfx, skia, and content enums for this...)

sure, added the helper.

this makes me sad too. do you think that switching FontRenderParams to use the
skia enum for hinting would make sense?

(it'd be nice if the subpixel rendering enum could be unified too, but
FontRenderParams tracks the color component order too, while i think that
SkPaint just uses a bool -- maybe it tracks the order somewhere else.)

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/render_text_pango...
ui/gfx/render_text_pango.cc:384: const FontRenderParams& render_params =
GetDefaultFontRenderParams();
On 2014/07/11 00:29:10, msw wrote:
> optional nit: inline |render_params| here, like other impls.

Done.

[Daniel Erat, 2014-07-11 03:15:05]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-11 03:18:56]

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

[msw, 2014-07-11 04:40:57]

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

https://codereview.chromium.org/387743002/diff/20001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:211: switch (params.hinting) {
On 2014/07/11 03:15:01, Daniel Erat wrote:
> On 2014/07/11 00:29:10, msw wrote:
> > optional nit: add a helper to convert the enum (case x: return y).
> > (it's sad that we have separate gfx, skia, and content enums for this...)
> 
> sure, added the helper.
> 
> this makes me sad too. do you think that switching FontRenderParams to use the
> skia enum for hinting would make sense?

That seems reasonable.

> (it'd be nice if the subpixel rendering enum could be unified too, but
> FontRenderParams tracks the color component order too, while i think that
> SkPaint just uses a bool -- maybe it tracks the order somewhere else.)

I'm not sure, but it may just assume RGB, I'm not sure how common BGR is.
SkBitmapDevice disables this if kN32_SkColorType != fBitmap.colorType().

https://codereview.chromium.org/387743002/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/387743002/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:161: SkPaint::Hinting
FontRenderParamsHintingToSkPaintHinting(
You'll need a return statement after the switch.

[commit-bot: I haz the power, 2014-07-11 05:06:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31804)

[commit-bot: I haz the power, 2014-07-11 05:14:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 05:14:24]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Daniel Erat, 2014-07-11 13:13:45]

https://codereview.chromium.org/387743002/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/387743002/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:161: SkPaint::Hinting
FontRenderParamsHintingToSkPaintHinting(
On 2014/07/11 04:40:57, msw wrote:
> You'll need a return statement after the switch.

whoops, of course. done.

[Daniel Erat, 2014-07-11 13:13:52]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-11 13:14:53]

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

[commit-bot: I haz the power, 2014-07-11 17:54:34]

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-11 18:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 18:59:15]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Daniel Erat, 2014-07-11 19:09:31]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-11 19:10:04]

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

[commit-bot: I haz the power, 2014-07-11 21:21:25]

Change committed as 282703