content: Remove RendererPreferences font-rendering enums.

content: Remove RendererPreferences font-rendering enums.

Make the RendererPreferences message struct use enums from
gfx::FontRenderParams instead of defining its own.

This also drops the SYSTEM_DEFAULT values for hinting and
subpixel rendering. They weren't getting used anywhere.

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

[Daniel Erat, 2014-07-22 19:57:04]

msw: code review, please
jochen: approval for content and chrome
jln: security review for view_messages.h

[Daniel Erat, 2014-07-22 19:58:56]

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
File content/renderer/render_view_linux.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
content/renderer/render_view_linux.cc:19: // When anti-aliasing is off, GTK maps
all non-zero hinting settings to
perhaps FontRenderParams should also be doing this

[msw, 2014-07-22 20:58:35]

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:25:
hinting(gfx::FontRenderParams::HINTING_SLIGHT),
RendererPreferencesToSkiaHinting used Normal for default previously, is this
change to use slight okay?

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:28:
subpixel_rendering(gfx::FontRenderParams::SUBPIXEL_RENDERING_RGB),
Doesn't this conflict with the old behavior of
RendererPreferencesToSubpixelRenderingFlag and Behdad's advice:
https://codereview.chromium.org/407763003/#msg11

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
File content/renderer/render_view_linux.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
content/renderer/render_view_linux.cc:19: // When anti-aliasing is off, GTK maps
all non-zero hinting settings to
On 2014/07/22 19:58:56, Daniel Erat wrote:
> perhaps FontRenderParams should also be doing this

It'd be good to file a bug if we can repro such a defect.

[Daniel Erat, 2014-07-22 21:15:50]

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:25:
hinting(gfx::FontRenderParams::HINTING_SLIGHT),
On 2014/07/22 20:58:35, msw wrote:
> RendererPreferencesToSkiaHinting used Normal for default previously, is this
> change to use slight okay?

i believe that the default value is arbitrary since this struct is later
initialized to the values from gfx::GetDefaultWebKitFontRenderParams(). matching
FontRenderParam's defaults (as i'm doing here) seems cleaner, though.

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:28:
subpixel_rendering(gfx::FontRenderParams::SUBPIXEL_RENDERING_RGB),
On 2014/07/22 20:58:35, msw wrote:
> Doesn't this conflict with the old behavior of
> RendererPreferencesToSubpixelRenderingFlag and Behdad's advice:
> https://codereview.chromium.org/407763003/#msg11

this one also gets reset later. i'm fine with switching it to _NONE here, but i
don't think it really matters.

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
File content/renderer/render_view_linux.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/renderer/render_view...
content/renderer/render_view_linux.cc:19: // When anti-aliasing is off, GTK maps
all non-zero hinting settings to
On 2014/07/22 20:58:35, msw wrote:
> On 2014/07/22 19:58:56, Daniel Erat wrote:
> > perhaps FontRenderParams should also be doing this
> 
> It'd be good to file a bug if we can repro such a defect.

sure, http://crbug.com/396239

[msw, 2014-07-22 21:45:07]

lgtm with an optional nit to change the unused default subpixel rendering
setting to _NONE.

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:25:
hinting(gfx::FontRenderParams::HINTING_SLIGHT),
On 2014/07/22 21:15:50, Daniel Erat wrote:
> On 2014/07/22 20:58:35, msw wrote:
> > RendererPreferencesToSkiaHinting used Normal for default previously, is this
> > change to use slight okay?
> 
> i believe that the default value is arbitrary since this struct is later
> initialized to the values from gfx::GetDefaultWebKitFontRenderParams().
matching
> FontRenderParam's defaults (as i'm doing here) seems cleaner, though.

Acknowledged.

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:28:
subpixel_rendering(gfx::FontRenderParams::SUBPIXEL_RENDERING_RGB),
On 2014/07/22 21:15:50, Daniel Erat wrote:
> On 2014/07/22 20:58:35, msw wrote:
> > Doesn't this conflict with the old behavior of
> > RendererPreferencesToSubpixelRenderingFlag and Behdad's advice:
> > https://codereview.chromium.org/407763003/#msg11
> 
> this one also gets reset later. i'm fine with switching it to _NONE here, but
i
> don't think it really matters.

I guess it doesn't really matter if both here and the FontRenderParams ctor
default values should always be overwritten. But maybe both should be *_NONE
just to be safe, given Behdad's advice.

[Daniel Erat, 2014-07-23 00:06:32]

Bo, can you let me know what your plans are here?
android_webview/native/aw_settings.cc has its own code to copy fields from
FontRenderParams to RendererPreferences, along with a bunch of TODOs to
"Deduplicate with chrome/ code". Are you planning to make this file use
UpdateFromSystemSettings() from chrome/browser/renderer_preferences_util.h
anytime soon? I'll just delete the no-longer-needed conversion helpers for now.

[Daniel Erat, 2014-07-23 00:10:23]

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/413523004/diff/1/content/public/common/render...
content/public/common/renderer_preferences.cc:28:
subpixel_rendering(gfx::FontRenderParams::SUBPIXEL_RENDERING_RGB),
On 2014/07/22 21:45:07, msw wrote:
> On 2014/07/22 21:15:50, Daniel Erat wrote:
> > On 2014/07/22 20:58:35, msw wrote:
> > > Doesn't this conflict with the old behavior of
> > > RendererPreferencesToSubpixelRenderingFlag and Behdad's advice:
> > > https://codereview.chromium.org/407763003/#msg11
> > 
> > this one also gets reset later. i'm fine with switching it to _NONE here,
but
> i
> > don't think it really matters.
> 
> I guess it doesn't really matter if both here and the FontRenderParams ctor
> default values should always be overwritten. But maybe both should be *_NONE
> just to be safe, given Behdad's advice.

done

[boliu, 2014-07-23 00:10:44]

On 2014/07/23 00:06:32, Daniel Erat wrote:
> Bo, can you let me know what your plans are here?
> android_webview/native/aw_settings.cc has its own code to copy fields from
> FontRenderParams to RendererPreferences, along with a bunch of TODOs to
> "Deduplicate with chrome/ code". Are you planning to make this file use
> UpdateFromSystemSettings() from chrome/browser/renderer_preferences_util.h
> anytime soon? I'll just delete the no-longer-needed conversion helpers for
now.

Deleting it counts as deduplicating :) thanks!

android_webview lgtm

[jochen (gone - plz use gerrit), 2014-07-23 07:01:50]

lgtm

[jln (very slow on Chromium), 2014-07-23 19:04:13]

content/common/view_messages.h lgtm

[Daniel Erat, 2014-07-23 19:55:04]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-23 19:58:00]

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

[commit-bot: I haz the power, 2014-07-24 03:43:16]

Change committed as 285145

[johnme, 2014-07-24 15:54:24]

A revert of this CL has been created in
https://codereview.chromium.org/410413008/ by johnme@chromium.org.

The reason for reverting is: This caused* the following 3 layout tests to
regress[1] on WebKit Android (Nexus4).

- css1/font_properties/font_variant.html
- fast/text/chromium-linux-fontconfig-renderstyle.html
- svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg

Specifically the results[2] show that the text rendering has changed slightly,
and from manual inspection the hinting seems worse (e.g. some 1 pixel wide black
lines have become spread into 2-pixel wide gray lines).

*: (it's the only font-related patch on the blamelist[3])

[1]:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexu...

[2]:
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Android__...

[3]:
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=%2F....

[Daniel Erat, 2014-07-24 15:58:30]

On 2014/07/24 15:54:24, johnme wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/410413008/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=johnme@chromium.org.
> 
> The reason for reverting is: This caused* the following 3 layout tests to
> regress[1] on WebKit Android (Nexus4).
> 
> - css1/font_properties/font_variant.html
> - fast/text/chromium-linux-fontconfig-renderstyle.html
> - svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
> 
> Specifically the results[2] show that the text rendering has changed slightly,
> and from manual inspection the hinting seems worse (e.g. some 1 pixel wide
black
> lines have become spread into 2-pixel wide gray lines).
> 
> *: (it's the only font-related patch on the blamelist[3])
> 
> [1]:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexu...
> 
> [2]:
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Android__...
> 
> [3]:
>
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=%2F....

i'll bet that the problem is that we run layout tests in a configuration where
some settings are unconfigured in Fontconfig, resulting in the changes to the
defaults here showing up.

[Daniel Erat, 2014-07-24 16:51:25]

On 2014/07/24 15:58:30, Daniel Erat wrote:
> On 2014/07/24 15:54:24, johnme wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/410413008/ by
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=johnme@chromium.org.
> > 
> > The reason for reverting is: This caused* the following 3 layout tests to
> > regress[1] on WebKit Android (Nexus4).
> > 
> > - css1/font_properties/font_variant.html
> > - fast/text/chromium-linux-fontconfig-renderstyle.html
> > - svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
> > 
> > Specifically the results[2] show that the text rendering has changed
slightly,
> > and from manual inspection the hinting seems worse (e.g. some 1 pixel wide
> black
> > lines have become spread into 2-pixel wide gray lines).
> > 
> > *: (it's the only font-related patch on the blamelist[3])
> > 
> > [1]:
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexu...
> > 
> > [2]:
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Android__...
> > 
> > [3]:
> >
>
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=%2F....
> 
> i'll bet that the problem is that we run layout tests in a configuration where
> some settings are unconfigured in Fontconfig, resulting in the changes to the
> defaults here showing up.

actually, it's not entirely clear to me what's happening here:

ui/gfx/font_render_params_android.cc sets all of the FontRenderParams members,
so i wouldn't expect my changes to FontRenderParams's defaults to have any
effect on android. i don't see default-constructed FontRenderParams getting used
anywhere.

android_webview/native/aw_settings.cc updates all of RendererPreferences's
font-related members based on FontRenderParams, so the change to
RendererPreferences's defaults shouldn't have any effect on this piece of code,
either. my other theory was that we could be using a default-constructed
RendererPreferences somewhere for android layout tests, but the old code used
RENDERER_PREFERENCES_HINTING_SYSTEM_DEFAULT, which would later get mapped to
SkPaint::kNormal_Hinting. i changed the default to
gfx::FontRenderParams::HINTING_MEDIUM, which also gets mapped to
SkPaint::kNormal_Hinting.

looking closely at the images from the failed tests, it seems like subpixel
positioning actually changed -- i believe that it's not enabled in the expected
images but is enabled in the actual images.

my change also changed RendererPreferences::use_subpixel_positioning's default
from false to true to match FontRenderParam's default. perhaps that change to
the default is somehow making it through into blink. blink generally disables
subpixel positioning for layout tests, but i think that that behavior can be
overridden from the browser.

i'll try just changing just RendererPreferences::use_subpixel_positioning's
default back.

[Daniel Erat, 2014-07-24 17:46:17]

On 2014/07/24 16:51:25, Daniel Erat wrote:
> On 2014/07/24 15:58:30, Daniel Erat wrote:
> > On 2014/07/24 15:54:24, johnme wrote:
> > > A revert of this CL has been created in
> > > https://codereview.chromium.org/410413008/ by
> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=johnme@chromium.org.
> > > 
> > > The reason for reverting is: This caused* the following 3 layout tests to
> > > regress[1] on WebKit Android (Nexus4).
> > > 
> > > - css1/font_properties/font_variant.html
> > > - fast/text/chromium-linux-fontconfig-renderstyle.html
> > > - svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
> > > 
> > > Specifically the results[2] show that the text rendering has changed
> slightly,
> > > and from manual inspection the hinting seems worse (e.g. some 1 pixel wide
> > black
> > > lines have become spread into 2-pixel wide gray lines).
> > > 
> > > *: (it's the only font-related patch on the blamelist[3])
> > > 
> > > [1]:
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexu...
> > > 
> > > [2]:
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Android__...
> > > 
> > > [3]:
> > >
> >
>
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=%2F....
> > 
> > i'll bet that the problem is that we run layout tests in a configuration
where
> > some settings are unconfigured in Fontconfig, resulting in the changes to
the
> > defaults here showing up.
> 
> actually, it's not entirely clear to me what's happening here:
> 
> ui/gfx/font_render_params_android.cc sets all of the FontRenderParams members,
> so i wouldn't expect my changes to FontRenderParams's defaults to have any
> effect on android. i don't see default-constructed FontRenderParams getting
used
> anywhere.
> 
> android_webview/native/aw_settings.cc updates all of RendererPreferences's
> font-related members based on FontRenderParams, so the change to
> RendererPreferences's defaults shouldn't have any effect on this piece of
code,
> either. my other theory was that we could be using a default-constructed
> RendererPreferences somewhere for android layout tests, but the old code used
> RENDERER_PREFERENCES_HINTING_SYSTEM_DEFAULT, which would later get mapped to
> SkPaint::kNormal_Hinting. i changed the default to
> gfx::FontRenderParams::HINTING_MEDIUM, which also gets mapped to
> SkPaint::kNormal_Hinting.
> 
> looking closely at the images from the failed tests, it seems like subpixel
> positioning actually changed -- i believe that it's not enabled in the
expected
> images but is enabled in the actual images.
> 
> my change also changed RendererPreferences::use_subpixel_positioning's default
> from false to true to match FontRenderParam's default. perhaps that change to
> the default is somehow making it through into blink. blink generally disables
> subpixel positioning for layout tests, but i think that that behavior can be
> overridden from the browser.
> 
> i'll try just changing just RendererPreferences::use_subpixel_positioning's
> default back.

https://codereview.chromium.org/417953006/ is the change to reland this.