Chromium change of sharing some WebKit API between Android/Linux.

Chromium change of sharing some WebKit API between Android/Linux.

Previously the Android-specific font rendering configurations are hardcoded in WebCore. Now change to use WebKit API.

Also passes autohinting and use_bitmaps parameters to WebKit.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145773

[Xianzhu, 2012-06-11 21:56:48]

Incomplete yet. Just for creating a CL number to be referenced in the WebKit CL.

[Xianzhu, 2012-06-14 23:27:38]

On 2012/06/14 23:26:27, Xianzhu wrote:

Now it's complete, but only buildable after the patch in webkit bug 88802 is
rolled in.

[jamesr, 2012-06-14 23:33:38]

I am not a fan of "linuxish"

http://codereview.chromium.org/10544103/diff/7001/content/content_renderer.gypi
File content/content_renderer.gypi (right):

http://codereview.chromium.org/10544103/diff/7001/content/content_renderer.gy...
content/content_renderer.gypi:180: 'renderer/render_view_linuxish.cc',
what a bizarre name - what does "linuxish" mean?  can we think of something
better, please? we have _linux, _posix, and _mac (as a counterpoint to _posix)

[Daniel Erat, 2012-06-14 23:44:20]

http://codereview.chromium.org/10544103/diff/7001/chrome/browser/renderer_pre...
File chrome/browser/renderer_preferences_util.cc (right):

http://codereview.chromium.org/10544103/diff/7001/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:53: #elif defined(OS_ANDROID)
You need to sync and merge this file; it's changed fairly drastically due to
http://crrev.com/141837.  Please add the Android defaults to
ui/gfx/font_render_params_linux.cc instead of hardcoding them here.

http://codereview.chromium.org/10544103/diff/7001/content/renderer/render_vie...
File content/renderer/render_view_linuxish.cc (left):

http://codereview.chromium.org/10544103/diff/7001/content/renderer/render_vie...
content/renderer/render_view_linuxish.cc:113:
WebFontInfo::setSubpixelPositioning(prefs.use_subpixel_positioning);
Was this a merge error?  I just added this code.

[Xianzhu, 2012-06-14 23:47:06]

On 2012/06/14 23:33:38, jamesr wrote:
> I am not a fan of "linuxish"
> 
>
http://codereview.chromium.org/10544103/diff/7001/content/content_renderer.gypi
> File content/content_renderer.gypi (right):
> 
>
http://codereview.chromium.org/10544103/diff/7001/content/content_renderer.gy...
> content/content_renderer.gypi:180: 'renderer/render_view_linuxish.cc',
> what a bizarre name - what does "linuxish" mean?  can we think of something
> better, please? we have _linux, _posix, and _mac (as a counterpoint to _posix)

In build/filename_rules.gyp, 'linuxish' is used to indicate directories or files
used when 'OS=="android" or OS=="linux" or OS=="openbsd" or OS=="freebsd"'. In
WebKit WebFontRendering's implementation depends harfbuzz which is included when
'use_x11=1 or OS=="android"' so it seems to equal to 'linuxish' in Chromium's
term.

[jamesr, 2012-06-14 23:48:03]

That's terrible.  Why not posix and !mac?

[Xianzhu, 2012-06-15 00:05:00]

http://codereview.chromium.org/10544103/diff/7001/chrome/browser/renderer_pre...
File chrome/browser/renderer_preferences_util.cc (right):

http://codereview.chromium.org/10544103/diff/7001/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:53: #elif defined(OS_ANDROID)
On 2012/06/14 23:44:20, Daniel Erat wrote:
> You need to sync and merge this file; it's changed fairly drastically due to
> http://crrev.com/141837.  Please add the Android defaults to
> ui/gfx/font_render_params_linux.cc instead of hardcoding them here.

Sorry I overlooked your newest change. It's great to have a more proper place to
initialize the default values. :)

Will change this soon.

http://codereview.chromium.org/10544103/diff/7001/content/renderer/render_vie...
File content/renderer/render_view_linuxish.cc (left):

http://codereview.chromium.org/10544103/diff/7001/content/renderer/render_vie...
content/renderer/render_view_linuxish.cc:113:
WebFontInfo::setSubpixelPositioning(prefs.use_subpixel_positioning);
On 2012/06/14 23:44:20, Daniel Erat wrote:
> Was this a merge error?  I just added this code.

In https://bugs.webkit.org/show_bug.cgi?id=89133 I changed
WebKit::WebFontRendering::setSubpixelPositioning() to call
WebFontInfo::setSubpixelPositioning() to avoid calling both everywhere. The
reason of changing this is that WebFontInfo uses fontconfig which is not
available on Android. Otherwise we would need the following code in several
places:

#if !defined(OS_ANDROID)
  WebFontInfo::setSubpixelPositioning(...)
#endif
  WebFontRendering::setSubpixelPositioning(...)

[Xianzhu, 2012-06-15 00:14:36]

On 2012/06/14 23:48:03, jamesr wrote:
> That's terrible.  Why not posix and !mac?

The 'linuxish' condition was added by https://codereview.chromium.org/9838033
(and later reapplication http://codereview.chromium.org/9960072) . I saw in the
first patch set '_posix' was used and then changed to '_linuxish' in Patch Set
10.

IMHO I prefer using _posix and exclude the particular file on mac.

[jamesr, 2012-06-15 01:25:36]

On 2012/06/15 00:14:36, Xianzhu wrote:
> On 2012/06/14 23:48:03, jamesr wrote:
> > That's terrible.  Why not posix and !mac?
> 
> The 'linuxish' condition was added by https://codereview.chromium.org/9838033

It looks like this was for a _VERY_ breakpad-specific concern and not intended
as a general pattern.

> (and later reapplication http://codereview.chromium.org/9960072) . I saw in
the
> first patch set '_posix' was used and then changed to '_linuxish' in Patch Set
> 10.
> 
> IMHO I prefer using _posix and exclude the particular file on mac.

I think it's worth trying to figure out a better path forward here. 
render_view_linuxish.cc will not fly for me

[jam, 2012-06-15 15:14:11]

On 2012/06/15 01:25:36, jamesr wrote:
> On 2012/06/15 00:14:36, Xianzhu wrote:
> > On 2012/06/14 23:48:03, jamesr wrote:
> > > That's terrible.  Why not posix and !mac?
> > 
> > The 'linuxish' condition was added by
https://codereview.chromium.org/9838033
> 
> It looks like this was for a _VERY_ breakpad-specific concern and not intended
> as a general pattern.
> 
> > (and later reapplication http://codereview.chromium.org/9960072) . I saw in
> the
> > first patch set '_posix' was used and then changed to '_linuxish' in Patch
Set
> > 10.
> > 
> > IMHO I prefer using _posix and exclude the particular file on mac.
> 
> I think it's worth trying to figure out a better path forward here. 
> render_view_linuxish.cc will not fly for me

+1 on the linuxish thing. That name is meaningless and doesn't give context when
one sees it.

[Xianzhu, 2012-06-24 05:15:31]

Hi, reviewers,

The 'linuxish' things have been removed in the newest patch set.
'target_conditions' is used instead. Also updated the code according to newest
WebKit changes rolled in.

Thanks,
Xianzhu

[Daniel Erat, 2012-06-24 14:51:25]

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
content/public/common/renderer_preferences.h:56: bool autohinter;
nit: mind renaming this to "use_autohinter" to match the style of the other
boolean flags?

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
File ui/gfx/font_render_params_linux.cc (right):

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
Why is the default for GTK being changed here?

[Xianzhu, 2012-06-26 12:46:44]

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
content/public/common/renderer_preferences.h:56: bool autohinter;
On 2012/06/24 14:51:25, Daniel Erat wrote:
> nit: mind renaming this to "use_autohinter" to match the style of the other
> boolean flags?

I hesitated about using 'use_'. Finally I used 'use_bitmaps' because
'bitmaps'-only looks not good and there is useBitmaps() in WebKit API, and I
used 'autohinting' which is the same name as in the original FontRenderParams.

Will it look good to you if I use 'use_autohinting' in RendererPreferences while
keep the original 'autohinting' in FontRenderParams? Or maybe I should change
'autohinting', 'pixel_positioning' and 'antialiasing' in FontRenderParams to
'use_autohinting', 'use_pixel_positioning' and 'use_antialiasing' respectively.

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
File ui/gfx/font_render_params_linux.cc (right):

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
On 2012/06/24 14:51:25, Daniel Erat wrote:
> Why is the default for GTK being changed here?

Though GTK had a different value here, the value was not used as it is not sent
to WebKit or Skia. In WebKit, autohint is set to true. To keep the original
behavior unchanged, I changed the value here to true as the value is now sent to
WebFontRendering::setAutoHint().

[Daniel Erat, 2012-06-26 14:13:29]

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
content/public/common/renderer_preferences.h:56: bool autohinter;
On 2012/06/26 12:46:45, Xianzhu wrote:
> On 2012/06/24 14:51:25, Daniel Erat wrote:
> > nit: mind renaming this to "use_autohinter" to match the style of the other
> > boolean flags?
> 
> I hesitated about using 'use_'. Finally I used 'use_bitmaps' because
> 'bitmaps'-only looks not good and there is useBitmaps() in WebKit API, and I
> used 'autohinting' which is the same name as in the original FontRenderParams.
> 
> Will it look good to you if I use 'use_autohinting' in RendererPreferences
while
> keep the original 'autohinting' in FontRenderParams? Or maybe I should change
> 'autohinting', 'pixel_positioning' and 'antialiasing' in FontRenderParams to
> 'use_autohinting', 'use_pixel_positioning' and 'use_antialiasing'
respectively.

Yes, I was suggesting changing this to use_autohinting but leaving
FontRenderParams as-is.  I don't think that slight naming differences between
the two are a problem (the code that needs to interact with both is minimal),
but within a single struct or class, I'd like to see a consistent pattern.

[Xianzhu, 2012-06-26 14:16:22]

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/14001/content/public/common/rend...
content/public/common/renderer_preferences.h:56: bool autohinter;
On 2012/06/26 14:13:29, Daniel Erat wrote:
> On 2012/06/26 12:46:45, Xianzhu wrote:
> > On 2012/06/24 14:51:25, Daniel Erat wrote:
> > > nit: mind renaming this to "use_autohinter" to match the style of the
other
> > > boolean flags?
> > 
> > I hesitated about using 'use_'. Finally I used 'use_bitmaps' because
> > 'bitmaps'-only looks not good and there is useBitmaps() in WebKit API, and I
> > used 'autohinting' which is the same name as in the original
FontRenderParams.
> > 
> > Will it look good to you if I use 'use_autohinting' in RendererPreferences
> while
> > keep the original 'autohinting' in FontRenderParams? Or maybe I should
change
> > 'autohinting', 'pixel_positioning' and 'antialiasing' in FontRenderParams to
> > 'use_autohinting', 'use_pixel_positioning' and 'use_antialiasing'
> respectively.
> 
> Yes, I was suggesting changing this to use_autohinting but leaving
> FontRenderParams as-is.  I don't think that slight naming differences between
> the two are a problem (the code that needs to interact with both is minimal),
> but within a single struct or class, I'd like to see a consistent pattern.

Done.

[Daniel Erat, 2012-06-26 14:17:12]

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
File ui/gfx/font_render_params_linux.cc (right):

http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
On 2012/06/26 12:46:45, Xianzhu wrote:
> On 2012/06/24 14:51:25, Daniel Erat wrote:
> > Why is the default for GTK being changed here?
> 
> Though GTK had a different value here, the value was not used as it is not
sent
> to WebKit or Skia. In WebKit, autohint is set to true. To keep the original
> behavior unchanged, I changed the value here to true as the value is now sent
to
> WebFontRendering::setAutoHint().

Was WebKit always turning autohinting on automatically?  If so, sorry for
missing it.  My intent was to disable autohinting on GTK, as I think that that
matches Ubuntu's default settings.

[Xianzhu, 2012-06-26 14:27:59]

On 2012/06/26 14:17:12, Daniel Erat wrote:
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> File ui/gfx/font_render_params_linux.cc (right):
> 
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
> On 2012/06/26 12:46:45, Xianzhu wrote:
> > On 2012/06/24 14:51:25, Daniel Erat wrote:
> > > Why is the default for GTK being changed here?
> > 
> > Though GTK had a different value here, the value was not used as it is not
> sent
> > to WebKit or Skia. In WebKit, autohint is set to true. To keep the original
> > behavior unchanged, I changed the value here to true as the value is now
sent
> to
> > WebFontRendering::setAutoHint().
> 
> Was WebKit always turning autohinting on automatically?  If so, sorry for
> missing it.  My intent was to disable autohinting on GTK, as I think that that
> matches Ubuntu's default settings.

Yes. Please see
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/harfbuz....
Maybe this is not the proper value for GTK. Do you think we should use 'false'
here?

[Xianzhu, 2012-06-26 14:37:27]

On 2012/06/26 14:27:59, Xianzhu wrote:
> On 2012/06/26 14:17:12, Daniel Erat wrote:
> >
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> > File ui/gfx/font_render_params_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> > ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
> > On 2012/06/26 12:46:45, Xianzhu wrote:
> > > On 2012/06/24 14:51:25, Daniel Erat wrote:
> > > > Why is the default for GTK being changed here?
> > > 
> > > Though GTK had a different value here, the value was not used as it is not
> > sent
> > > to WebKit or Skia. In WebKit, autohint is set to true. To keep the
original
> > > behavior unchanged, I changed the value here to true as the value is now
> sent
> > to
> > > WebFontRendering::setAutoHint().
> > 
> > Was WebKit always turning autohinting on automatically?  If so, sorry for
> > missing it.  My intent was to disable autohinting on GTK, as I think that
that
> > matches Ubuntu's default settings.
> 
> Yes. Please see
>
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/harfbuz....
> Maybe this is not the proper value for GTK. Do you think we should use 'false'
> here?

Danial, I also uploaded a patch which sets autohinter=false for GTK. I can't
verify because I can't access my Linux machine from China during vacation. Could
you please verify it? Thanks.

[jochen (gone - plz use gerrit), 2012-06-26 18:08:31]

http://codereview.chromium.org/10544103/diff/15012/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/15012/content/public/common/rend...
content/public/common/renderer_preferences.h:60: bool use_bitmaps;
you will need to add this to content/common/view_messages.h

[Xianzhu, 2012-06-27 01:38:51]

http://codereview.chromium.org/10544103/diff/15012/content/public/common/rend...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10544103/diff/15012/content/public/common/rend...
content/public/common/renderer_preferences.h:60: bool use_bitmaps;
On 2012/06/26 18:08:31, jochen wrote:
> you will need to add this to content/common/view_messages.h

Done.  Thanks!

[Xianzhu, 2012-06-27 23:02:26]

On 2012/06/26 14:37:27, Xianzhu wrote:
> On 2012/06/26 14:27:59, Xianzhu wrote:
> > On 2012/06/26 14:17:12, Daniel Erat wrote:
> > >
> >
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> > > File ui/gfx/font_render_params_linux.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
> > > ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
> > > On 2012/06/26 12:46:45, Xianzhu wrote:
> > > > On 2012/06/24 14:51:25, Daniel Erat wrote:
> > > > > Why is the default for GTK being changed here?
> > > > 
> > > > Though GTK had a different value here, the value was not used as it is
not
> > > sent
> > > > to WebKit or Skia. In WebKit, autohint is set to true. To keep the
> original
> > > > behavior unchanged, I changed the value here to true as the value is now
> > sent
> > > to
> > > > WebFontRendering::setAutoHint().
> > > 
> > > Was WebKit always turning autohinting on automatically?  If so, sorry for
> > > missing it.  My intent was to disable autohinting on GTK, as I think that
> that
> > > matches Ubuntu's default settings.
> > 
> > Yes. Please see
> >
>
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/harfbuz....
> > Maybe this is not the proper value for GTK. Do you think we should use
'false'
> > here?
> 
> Daniel, I also uploaded a patch which sets autohinter=false for GTK. I can't
> verify because I can't access my Linux machine from China during vacation.
Could
> you please verify it? Thanks.

Hi, Daniel, could you please verify if the effect of autohinter is correct?
Thanks.

[Daniel Erat, 2012-06-27 23:24:34]

On Wed, Jun 27, 2012 at 4:02 PM,  <wangxianzhu@chromium.org> wrote:
> On 2012/06/26 14:37:27, Xianzhu wrote:
>>
>> On 2012/06/26 14:27:59, Xianzhu wrote:
>> > On 2012/06/26 14:17:12, Daniel Erat wrote:
>> > >
>> >
>
>
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
>>
>> > > File ui/gfx/font_render_params_linux.cc (right):
>> > >
>> > >
>> >
>
>
>
http://codereview.chromium.org/10544103/diff/14001/ui/gfx/font_render_params_...
>>
>> > > ui/gfx/font_render_params_linux.cc:26: params->autohinter = true;
>> > > On 2012/06/26 12:46:45, Xianzhu wrote:
>> > > > On 2012/06/24 14:51:25, Daniel Erat wrote:
>> > > > > Why is the default for GTK being changed here?
>> > > >
>> > > > Though GTK had a different value here, the value was not used as it
>> > > > is
>
> not
>>
>> > > sent
>> > > > to WebKit or Skia. In WebKit, autohint is set to true. To keep the
>> original
>> > > > behavior unchanged, I changed the value here to true as the value is
>> > > > now
>> > sent
>> > > to
>> > > > WebFontRendering::setAutoHint().
>> > >
>> > > Was WebKit always turning autohinting on automatically?  If so, sorry
>> > > for
>> > > missing it.  My intent was to disable autohinting on GTK, as I think
>> > > that
>> that
>> > > matches Ubuntu's default settings.
>> >
>> > Yes. Please see
>> >
>
>
>
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/harfbuz....
>>
>> > Maybe this is not the proper value for GTK. Do you think we should use
>
> 'false'
>>
>> > here?
>
>
>> Daniel, I also uploaded a patch which sets autohinter=false for GTK. I
>> can't
>>
>> verify because I can't access my Linux machine from China during vacation.
>
> Could
>>
>> you please verify it? Thanks.
>
>
> Hi, Daniel, could you please verify if the effect of autohinter is correct?
> Thanks.

This is the setting used for web fonts, right?  Do you have an example
of a web font that should appear differently depending on whether the
autohinter is used or not?

> http://codereview.chromium.org/10544103/

[Xianzhu, 2012-06-28 00:02:27]

On 2012/06/27 23:24:34, Daniel Erat wrote:

> This is the setting used for web fonts, right?  Do you have an example
> of a web font that should appear differently depending on whether the
> autohinter is used or not?
> 
> > http://codereview.chromium.org/10544103/

I think it's for web fonts and fonts not matched in fontconfig, This change will
cause them to be rendered with autohinter disabled (originally enabled by
default). I think this might be applied to any font because any font could be a
web font or not matched in fontconfig, so the point might be the difference of
any font with autohinter enabled and disabled, and if the difference is
expected.

[Xianzhu, 2012-06-28 00:46:25]

On 2012/06/28 00:02:27, Xianzhu wrote:
> On 2012/06/27 23:24:34, Daniel Erat wrote:
> 
> > This is the setting used for web fonts, right?  Do you have an example
> > of a web font that should appear differently depending on whether the
> > autohinter is used or not?
> > 
> > > http://codereview.chromium.org/10544103/
> 
> I think it's for web fonts and fonts not matched in fontconfig, This change
will
> cause them to be rendered with autohinter disabled (originally enabled by
> default). I think this might be applied to any font because any font could be
a
> web font or not matched in fontconfig, so the point might be the difference of
> any font with autohinter enabled and disabled, and if the difference is
> expected.

Now I think I should not take the risk for now to change the original behavior
about autohinter which might affect some layout tests. I changed back autohinter
to true for GTK and added a TODO.

[Daniel Erat, 2012-06-28 01:13:51]

lgtm

[Xianzhu, 2012-06-29 07:36:13]

On 2012/06/28 01:13:51, Daniel Erat wrote:
> lgtm

Thanks.

It still needs approval from owners of the following dirs:
chrome/: ben@ and jam@
content/: jam@
ui/: ben@ and sky@

Thanks!

[Ben Goodger (Google), 2012-06-29 16:07:36]

ui lgtm

[commit-bot: I haz the power, 2012-07-09 17:26:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10544103/37001

[commit-bot: I haz the power, 2012-07-09 17:26:07]

Presubmit check for 10544103-37001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/common
    content/public
    content/renderer

Presubmit checks took 2.1s to calculate.

[Xianzhu, 2012-07-09 17:40:31]

Hi, any owner of content/ can review this CL? Thanks!

[abarth-chromium, 2012-07-09 17:45:43]

On 2012/07/09 17:40:31, Xianzhu wrote:
> Hi, any owner of content/ can review this CL? Thanks!

You might get a quicker response if you ask someone in particular.  Perhaps jam@
would be a good choice?

[jam, 2012-07-09 18:08:21]

content lgtm

[commit-bot: I haz the power, 2012-07-09 21:48:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10544103/48001

[commit-bot: I haz the power, 2012-07-09 23:04:49]

Change committed as 145773