Linux: splice together RendererPreferences and SkFontHost

Linux: splice together RendererPreferences and SkFontHost

Dan Erat has been working down, plumbing the user's font settings from
XSETTINGS in the UI thread of the browser to RendererPreferences in
the renderer. At the same time, I've been working upwards:
implementing subpixel rendering support in Skia and adding an
interface to it in WebCore.

This patch joins the two together. It adds an API surface to the
WebKit API for configuring the font preferences and adds code to
translate from RendererPreferences to that API. It's a little
unfortunate that we need four sets of enums around the code base for
things like the hinting level, but each is supposed to be self
contained and adding a dependancy would much more grevious.

BUG=12179

[darin (slow to review), 2009-07-21 22:33:29]

http://codereview.chromium.org/159152/diff/1001/1002
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/159152/diff/1001/1002#newcode2711
Line 2711: WebFontPreferences::setHinting(
can you break this code out into a render_view_linux.cc?

maybe just define a method on RenderView that is #if defined(OS_LINUX), and then
implement that in render_view_linux.cc along with the static helper functions
above.

http://codereview.chromium.org/159152/diff/1001/1003
File webkit/api/public/linux/WebFontPreferences.h (right):

http://codereview.chromium.org/159152/diff/1001/1003#newcode39
Line 39: class WebFontPreferences {
How about calling this WebFontRendering instead?  I like including the term
"rendering" so that this is not conflated in the future to include font
selection rules, font size preferences, etc.

http://codereview.chromium.org/159152/diff/1001/1003#newcode41
Line 41: // Fonts
---------------------------------------------------------------
nit: this comment seems unnecessary.  there is only one section and the name of
the class already has the word font in it.

http://codereview.chromium.org/159152/diff/1001/1003#newcode58
Line 58: 
nit: kill the new line above setSmoothing for consistency with the others.

http://codereview.chromium.org/159152/diff/1001/1003#newcode72
Line 72: };
nit: add a new line after the };

[agl, 2009-07-21 23:07:04]

http://codereview.chromium.org/159152/diff/1001/1002
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/159152/diff/1001/1002#newcode2711
Line 2711: WebFontPreferences::setHinting(
On 2009/07/21 22:33:29, darin wrote:
> can you break this code out into a render_view_linux.cc?
> 
> maybe just define a method on RenderView that is #if defined(OS_LINUX), and
then
> implement that in render_view_linux.cc along with the static helper functions
> above.

Done.

http://codereview.chromium.org/159152/diff/1001/1003
File webkit/api/public/linux/WebFontPreferences.h (right):

http://codereview.chromium.org/159152/diff/1001/1003#newcode39
Line 39: class WebFontPreferences {
On 2009/07/21 22:33:29, darin wrote:
> How about calling this WebFontRendering instead?  I like including the term
> "rendering" so that this is not conflated in the future to include font
> selection rules, font size preferences, etc.

Done.

http://codereview.chromium.org/159152/diff/1001/1003#newcode41
Line 41: // Fonts
---------------------------------------------------------------
On 2009/07/21 22:33:29, darin wrote:
> nit: this comment seems unnecessary.  there is only one section and the name
of
> the class already has the word font in it.

Done.

http://codereview.chromium.org/159152/diff/1001/1003#newcode58
Line 58: 
On 2009/07/21 22:33:29, darin wrote:
> nit: kill the new line above setSmoothing for consistency with the others.

Done.

http://codereview.chromium.org/159152/diff/1001/1003#newcode72
Line 72: };
On 2009/07/21 22:33:29, darin wrote:
> nit: add a new line after the };

Done.

[darin (slow to review), 2009-07-21 23:14:46]

LGTM

http://codereview.chromium.org/159152/diff/1017/1020
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159152/diff/1017/1020#newcode618
Line 618: #if !defined(OS_LINUX)
sorry to nit pick, but it would be nice to reverse the sense here.  that way if
there ever needs to be branches for other ports, it will be natural to add #elif
defined(OS_FOO)

[agl, 2009-07-21 23:17:19]

Thanks. Waiting for WebKit side to be reviewed before landing.

http://codereview.chromium.org/159152/diff/1017/1020
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/159152/diff/1017/1020#newcode618
Line 618: #if !defined(OS_LINUX)
On 2009/07/21 23:14:47, darin wrote:
> sorry to nit pick, but it would be nice to reverse the sense here.  that way
if
> there ever needs to be branches for other ports, it will be natural to add
#elif
> defined(OS_FOO)

Done.

[agl, 2009-07-22 00:19:04]

updated to use Skia enums

[darin (slow to review), 2009-07-22 14:22:50]

LGTM

http://codereview.chromium.org/159152/diff/22/1030
File webkit/api/public/linux/WebFontRendering.h (right):

http://codereview.chromium.org/159152/diff/22/1030#newcode46
Line 46: WEBKIT_API static void setAntiAlias(bool on);
nit: i think webkit style would be to not name these bool parameters since the
meaning of the bool is pretty obvious.