Enable subpixel positioning for internal display with dsf larger than 1.0

Enable subpixel positioning for internal display with dsf larger than 1.0
Remove enable-browser-text-subpixel-positioning flag.

BUG=388844
TEST=covered by tests

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

[oshima, 2014-08-19 22:16:02]

Changed to pass device scale factor as discussed offline.
Let me know if this is different from what you had in your mind.

[Daniel Erat, 2014-08-19 22:32:26]

https://codereview.chromium.org/485873002/diff/140001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/485873002/diff/140001/ash/display/display_man...
ash/display/display_manager.h:125: // Initiailzes font related params that
depends on display
nit: Initializes

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:103: GFX_EXPORT void
SetDeviceScaleFactorForInternalDisplay(
mind renaming this to mention fonts so it's easier to tell what it's doing?

gfx::SetFontRenderParamsDeviceScaleFactor();

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:107: GFX_EXPORT bool
GetBrowserTextSubpixelPoisitioningEnabled();
i don't think you need this; you should be able to just call
GetFontRenderParams() and check subpixel_positioning

(also, there's a typo here: should be Positioning)

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux.cc:34: const float
kMinDsfToEnableSubpixelPositioning = 1.2f;
forget if i sent my previous comment asking this: why 1.2? can we just use it
for anything above 1.0? that's what blink does.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:276: TEST_F(FontRenderParamsTest,
ForceSubpixelPositioning) {
this should only be for OS_CHROMEOS, right?

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:283:
CreateFontconfigEditStanza("rgba", "const", "rgb") +
do you need this? it seems to be outside the scope of the code that you're
testing, and you aren't checking the subpixel_rendering field either

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:287: // more information.
this comment seems wrong

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:292:
SetDeviceScaleFactorForInternalDisplay(1.0f);
what's the reason for this? note that the cache gets reset between tests.

you could also try sending a different query after setting it to 1.0 and
checking that subpixel positioning isn't enabled.

[oshima, 2014-08-20 01:41:13]

https://codereview.chromium.org/485873002/diff/140001/ash/display/display_man...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/485873002/diff/140001/ash/display/display_man...
ash/display/display_manager.h:125: // Initiailzes font related params that
depends on display
On 2014/08/19 22:32:25, Daniel Erat wrote:
> nit: Initializes

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.h (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:103: GFX_EXPORT void
SetDeviceScaleFactorForInternalDisplay(
On 2014/08/19 22:32:25, Daniel Erat wrote:
> mind renaming this to mention fonts so it's easier to tell what it's doing?
> 
> gfx::SetFontRenderParamsDeviceScaleFactor();

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params.h:107: GFX_EXPORT bool
GetBrowserTextSubpixelPoisitioningEnabled();
On 2014/08/19 22:32:25, Daniel Erat wrote:
> i don't think you need this; you should be able to just call
> GetFontRenderParams() and check subpixel_positioning
> 
> (also, there's a typo here: should be Positioning)

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux.cc:34: const float
kMinDsfToEnableSubpixelPositioning = 1.2f;
On 2014/08/19 22:32:25, Daniel Erat wrote:
> forget if i sent my previous comment asking this: why 1.2? can we just use it
> for anything above 1.0? that's what blink does.

Done. I just removed const and use 1.0f as it's more obvious with 1.0f.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:276: TEST_F(FontRenderParamsTest,
ForceSubpixelPositioning) {
On 2014/08/19 22:32:25, Daniel Erat wrote:
> this should only be for OS_CHROMEOS, right?

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:283:
CreateFontconfigEditStanza("rgba", "const", "rgb") +
On 2014/08/19 22:32:25, Daniel Erat wrote:
> do you need this? it seems to be outside the scope of the code that you're
> testing, and you aren't checking the subpixel_rendering field either

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:287: // more information.
On 2014/08/19 22:32:25, Daniel Erat wrote:
> this comment seems wrong

Done.

https://codereview.chromium.org/485873002/diff/140001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:292:
SetDeviceScaleFactorForInternalDisplay(1.0f);
On 2014/08/19 22:32:25, Daniel Erat wrote:
> what's the reason for this? note that the cache gets reset between tests.
> 
> you could also try sending a different query after setting it to 1.0 and
> checking that subpixel positioning isn't enabled.

This is necessary so that other test does not use this value.
Setting it back does not change the value of params.subpixel_positioning as it
cashes the result.

And we never set it back, so I don't think we need to test that scenario.

https://codereview.chromium.org/485873002/diff/200001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/200001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1396:
gfx::ClearFontRenderParamsCacheForTest();
test was failing due to cache. This fixed my problem.

[Daniel Erat, 2014-08-20 04:21:00]

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: template <const char* VALUE, bool
INTERNAL>
i don't love using a template here instead of putting all of this inside of a
single test with a helper method that does the setup and returns whether
subpixel positioning is enabled. is the issue that you can't change the switches
repeatedly within a single test?

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1415: bool
IsTextSubpixelPoisitioningEnabled() {
you have the same typo as before here; should be "Positioning" (starts with
"Pos" instead of "Pois")

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1422: ASSERT_EQ(1.0f,
all of these should be ASSERT_DOUBLE_EQ, i think

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux.cc:29: // instead of hinting.
i'd remove "instead of hinting" here; i don't think that the two are necessarily
mutually exclusive

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:281: {
you don't need this block, do you?

[oshima, 2014-08-20 18:36:21]

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: template <const char* VALUE, bool
INTERNAL>
On 2014/08/20 04:20:59, Daniel Erat wrote:
> i don't love using a template here instead of putting all of this inside of a
> single test with a helper method that does the setup and returns whether
> subpixel positioning is enabled. is the issue that you can't change the
switches
> repeatedly within a single test?

Can you elaborate why you don't like this?

It's possible to create a helper method which does init process, but I prefer
the test works in the same way as others. I think that's more important than
avoiding template.

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1415: bool
IsTextSubpixelPoisitioningEnabled() {
On 2014/08/20 04:20:59, Daniel Erat wrote:
> you have the same typo as before here; should be "Positioning" (starts with
> "Pos" instead of "Pois")

Done.

https://codereview.chromium.org/485873002/diff/220001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1422: ASSERT_EQ(1.0f,
On 2014/08/20 04:20:59, Daniel Erat wrote:
> all of these should be ASSERT_DOUBLE_EQ, i think

Done.

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux.cc:29: // instead of hinting.
On 2014/08/20 04:21:00, Daniel Erat wrote:
> i'd remove "instead of hinting" here; i don't think that the two are
necessarily
> mutually exclusive

Done.

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/220001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:281: {
On 2014/08/20 04:21:00, Daniel Erat wrote:
> you don't need this block, do you?

Done.

[Daniel Erat, 2014-08-20 20:14:42]

https://codereview.chromium.org/485873002/diff/240001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/240001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: template <const char* VALUE, bool
INTERNAL>
i thought that there's a general push to avoid templates when possible in
chromium, although i wasn't able to find anything saying this in the style
guide. almost all of the templates that i've seen are used to specialize a class
or function for a given _type_, not to pass values like this code is doing. it
feels tricky and uncommon to me, and i think that using a helper method would be
easier to understand.

https://codereview.chromium.org/485873002/diff/240001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/240001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:278:
SetFontRenderParamsDeviceScaleFactor(1.25f);
nit: do a query before you increase it to 1.25 and check that subpixel
positioning is disabled?

[oshima, 2014-08-20 21:54:40]

https://codereview.chromium.org/485873002/diff/240001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/240001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: template <const char* VALUE, bool
INTERNAL>
On 2014/08/20 20:14:42, Daniel Erat wrote:
> i thought that there's a general push to avoid templates when possible in
> chromium, although i wasn't able to find anything saying this in the style
> guide. almost all of the templates that i've seen are used to specialize a
class
> or function for a given _type_, not to pass values like this code is doing. it
> feels tricky and uncommon to me, and i think that using a helper method would
be
> easier to understand.

okay, uploaded new patch. I hope you like it this time!

https://codereview.chromium.org/485873002/diff/240001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/240001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux_unittest.cc:278:
SetFontRenderParamsDeviceScaleFactor(1.25f);
On 2014/08/20 20:14:42, Daniel Erat wrote:
> nit: do a query before you increase it to 1.25 and check that subpixel
> positioning is disabled?

Done.

[Daniel Erat, 2014-08-20 22:04:53]

thanks, lgtm

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1390: // A helper class that sets the
display configuration and start ash.
nit: s/start/starts/

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1435:
Shell::GetScreen()->GetPrimaryDisplay().device_scale_factor());
nit: i think these are usually indented four spaces beyond the previous line
instead of eight

[oshima, 2014-08-20 22:26:58]

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1390: // A helper class that sets the
display configuration and start ash.
On 2014/08/20 22:04:52, Daniel Erat wrote:
> nit: s/start/starts/

Done.

https://codereview.chromium.org/485873002/diff/300001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1435:
Shell::GetScreen()->GetPrimaryDisplay().device_scale_factor());
On 2014/08/20 22:04:52, Daniel Erat wrote:
> nit: i think these are usually indented four spaces beyond the previous line
> instead of eight

all fixed.

[oshima, 2014-08-20 22:27:59]

asvitkine@ : owners review for font related changes in ui/gfx

[Alexei Svitkine (slow), 2014-08-21 15:36:38]

LGTM % comments

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: namespace {
Nit: Add blank line after this.

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1409: TearDown();
Nit: Isn't TearDown() called automatically by gtest?

Same for SetUp().

Can you double check that they aren't being called twice for you? (If they are,
remove from ctor and dtor).

https://codereview.chromium.org/485873002/diff/320001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/485873002/diff/320001/ui/gfx/font_render_para...
ui/gfx/font_render_params_linux.cc:29: float
device_scale_factor_for_internal_display = 1.0f;
Nit: add g_ prefix

[Daniel Erat, 2014-08-21 15:49:04]

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1409: TearDown();
On 2014/08/21 15:36:38, Alexei Svitkine wrote:
> Nit: Isn't TearDown() called automatically by gtest?
> 
> Same for SetUp().
> 
> Can you double check that they aren't being called twice for you? (If they
are,
> remove from ctor and dtor).

whoops, i missed this. i think that you're right; these should only need to be
called explicitly if a test class overrides them.

[oshima, 2014-08-21 16:36:20]

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1389: namespace {
On 2014/08/21 15:36:38, Alexei Svitkine wrote:
> Nit: Add blank line after this.

Done.

https://codereview.chromium.org/485873002/diff/320001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1409: TearDown();
On 2014/08/21 15:49:04, Daniel Erat wrote:
> On 2014/08/21 15:36:38, Alexei Svitkine wrote:
> > Nit: Isn't TearDown() called automatically by gtest?
> > 
> > Same for SetUp().
> > 
> > Can you double check that they aren't being called twice for you? (If they
> are,
> > remove from ctor and dtor).
> 
> whoops, i missed this. i think that you're right; these should only need to be
> called explicitly if a test class overrides them.

No, SetUp/TearDown are called from test framework (TestSuite?).
I'm creating an test instance my own, and that's why I'm calling them myself.

I added comment as discussed with dan.

[oshima, 2014-08-21 16:36:30]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-21 16:37:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/485873002/340001

[commit-bot: I haz the power, 2014-08-21 21:47:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-08-21 22:05:16]

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

[commit-bot: I haz the power, 2014-08-21 22:05:17]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[oshima, 2014-08-21 22:24:09]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-21 22:27:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/485873002/340001

[commit-bot: I haz the power, 2014-08-21 23:06:40]

Committed patchset #10 (340001) as 291227