Enable subpixel positioning for UI

Enable subpixel positioning for UI

BUG=389649, 434860, 434843
R=ananta,msw,derat,jam,eae,sky

Committed: https://crrev.com/9a4438efc6399ff2024ffb4d7b0e087da2c6f4c0
Cr-Commit-Position: refs/heads/master@{#306346}

[ckocagil, 2014-11-20 12:17:42]

ckocagil@chromium.org changed reviewers:
+ derat@chromium.org

[ckocagil, 2014-11-20 12:20:36]

Looks like enabling subpixel positioning fixes the issues.

ananta: Does this make sense to you?

[Daniel Erat, 2014-11-20 15:32:14]

lgtm, but i don't know much about windows so you should wait for someone else's
review.

+eae as a heads-up due to his directwrite involvement (in blink).

[eae, 2014-11-20 16:35:22]

eae@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2014-11-20 16:35:22]

This more or less matches the logic we use in blink so LGTM.

[ananta, 2014-11-20 16:43:06]

lgtm

[msw, 2014-11-20 17:41:45]

Nice work, Cem; just one request.

https://codereview.chromium.org/738363002/diff/1/ui/gfx/font_render_params_wi...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/738363002/diff/1/ui/gfx/font_render_params_wi...
ui/gfx/font_render_params_win.cc:22: const FontRenderParams& GetParams(bool
for_web_contents) {
Remove |for_web_contents| and its plumbing.

[ckocagil, 2014-11-23 19:40:29]

Patchset #2 (id:20001) has been deleted

[ckocagil, 2014-11-23 19:48:06]

ckocagil@chromium.org changed reviewers:
+ jam@chromium.org

[ckocagil, 2014-11-23 20:13:37]

I had to expand the scope of this CL. Requesting another round of reviews.

msw: ui/gfx/*, textfield_unittest.cc
jam and sky: browser_main_runner.cc, examples_main.cc, run_all_unittests.cc,
direct_write.*

Summary:

1. The browser initializes DirectWrite but no other executable used to do that.
So we never actually tested the DirectWrite font provider. Added initialization
calls to gfx_unittests and views_examples.

2. Enabling subpixel positioning didn't pass tests. Improved RenderText* support
for floating point positions and changed some tests.

3. The DirectWrite font metrics provider (used for fetching font size, cap
height, baseline and such) in font_render_params_win.cc actually fails tests but
it didn't get noticed due to (1). I disabled it in this CL, so we use the GDI
metrics provider with a DirectWrite renderer. At worst this will cause incorrect
baseline, vertical text clipping or blank margins. I plan on fixing this in a
followup CL.

[ananta, 2014-11-24 01:12:58]

On 2014/11/23 20:13:37, ckocagil wrote:
> I had to expand the scope of this CL. Requesting another round of reviews.
> 
> msw: ui/gfx/*, textfield_unittest.cc
> jam and sky: browser_main_runner.cc, examples_main.cc, run_all_unittests.cc,
> direct_write.*
> 
> Summary:
> 
> 1. The browser initializes DirectWrite but no other executable used to do
that.
> So we never actually tested the DirectWrite font provider. Added
initialization
> calls to gfx_unittests and views_examples.
> 
> 2. Enabling subpixel positioning didn't pass tests. Improved RenderText*
support
> for floating point positions and changed some tests.
> 
> 3. The DirectWrite font metrics provider (used for fetching font size, cap
> height, baseline and such) in font_render_params_win.cc actually fails tests
but
> it didn't get noticed due to (1). I disabled it in this CL, so we use the GDI
> metrics provider with a DirectWrite renderer. At worst this will cause
incorrect
> baseline, vertical text clipping or blank margins. I plan on fixing this in a
> followup CL.

The gdi metrics codepath will cause all the bugs which were originally fixed to
come back. Is it possible to continue using directwrite metrics in canary so
that we can get some data from the field?.

Thanks for the tests. It would be great if we can leave the dw metrics enabled
for chrome and fix the tests in a followup

[ckocagil, 2014-11-24 02:09:02]

Patchset #3 (id:60001) has been deleted

[ckocagil, 2014-11-24 02:12:32]

On 2014/11/24 01:12:58, ananta wrote:
> The gdi metrics codepath will cause all the bugs which were originally fixed
to
> come back. Is it possible to continue using directwrite metrics in canary so
> that we can get some data from the field?.
> 
> Thanks for the tests. It would be great if we can leave the dw metrics enabled
> for chrome and fix the tests in a followup

Done. Reverted changes to platform_font_win.cc ui/gfx/test/run_all_unittests.cc.

[jam, 2014-11-24 18:15:21]

content lgtm

[msw, 2014-11-24 21:51:00]

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

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:725: return cursor_enabled_ ?
This change seems hinge on the cursor painting 1px beyond the int-enclosed text
bounds. Use std::ceil(GetStringSizeF.width()) instead of GetStringSize() to make
that behavior explicit and avoid GetStringSize(), which should probably be
deprecated. Also, perhaps add a comment about the behavior of the cursor here.

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:657: bounds.second = std::ceil(bounds.second);
Add a comment here explaining why this is necessary. Should this also std::floor
the bounds.first for the leftmost char in RTL when the cursor is enabled?

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/win/direct_write....
ui/gfx/win/direct_write.cc:70: switches::kDisableDirectWriteForUI) &&
Should this be || to preserve the old logic?

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:1454: EXPECT_GE(1, std::abs(x
- prev_x));
If I read this right, entering an RTL character in an LTR string could cause the
cursor position to shift by a pixel or less? Why is this expectation changing?
Can you avoid this?

[ckocagil, 2014-11-25 01:38:39]

ckocagil@chromium.org changed reviewers:
+ sky@chromium.org

[ckocagil, 2014-11-25 01:39:47]

+sky: ui/views/*

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

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:725: return cursor_enabled_ ?
On 2014/11/24 21:51:00, msw wrote:
> This change seems hinge on the cursor painting 1px beyond the int-enclosed
text
> bounds.

Correct.

> Use std::ceil(GetStringSizeF.width()) instead of GetStringSize() to make
> that behavior explicit and avoid GetStringSize(), which should probably be
> deprecated. Also, perhaps add a comment about the behavior of the cursor here.

Done and done.

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:657: bounds.second = std::ceil(bounds.second);
On 2014/11/24 21:51:00, msw wrote:
> Add a comment here explaining why this is necessary.

Done.

> Should this also std::floor
> the bounds.first for the leftmost char in RTL when the cursor is enabled?

Do you mean visually leftmost or logically? We handle visually rightmost for
both RTL and LTR. Visually leftmost isn't a problem since the first glyph always
starts at 0.

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/win/direct_write....
ui/gfx/win/direct_write.cc:70: switches::kDisableDirectWriteForUI) &&
On 2014/11/24 21:51:00, msw wrote:
> Should this be || to preserve the old logic?

Good catch! Done.

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:1454: EXPECT_GE(1, std::abs(x
- prev_x));
On 2014/11/24 21:51:00, msw wrote:
> If I read this right, entering an RTL character in an LTR string could cause
the
> cursor position to shift by a pixel or less? Why is this expectation changing?

When the cursor is placed between two glyphs, we place it at
std::round(boundary) which is visually better. But when it's placed at the
rightmost position we use std::ceil to ensure it doesn't overlap the last glyph.
So these values are off by one if the boundary gets rounded down.

> Can you avoid this?

I tried, but there are many assumptions in tests. I'd prefer fixing this in a
followup CL.

[msw, 2014-11-25 04:52:06]

lgtm

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:657: bounds.second = std::ceil(bounds.second);
On 2014/11/25 01:39:47, ckocagil wrote:
> On 2014/11/24 21:51:00, msw wrote:
> > Add a comment here explaining why this is necessary.
> 
> Done.
> 
> > Should this also std::floor
> > the bounds.first for the leftmost char in RTL when the cursor is enabled?
> 
> Do you mean visually leftmost or logically? We handle visually rightmost for
> both RTL and LTR. Visually leftmost isn't a problem since the first glyph
always
> starts at 0.

Acknowledged.

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/738363002/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_unittest.cc:1454: EXPECT_GE(1, std::abs(x
- prev_x));
On 2014/11/25 01:39:47, ckocagil wrote:
> On 2014/11/24 21:51:00, msw wrote:
> > If I read this right, entering an RTL character in an LTR string could cause
> the
> > cursor position to shift by a pixel or less? Why is this expectation
changing?
> 
> When the cursor is placed between two glyphs, we place it at
> std::round(boundary) which is visually better. But when it's placed at the
> rightmost position we use std::ceil to ensure it doesn't overlap the last
glyph.
> So these values are off by one if the boundary gets rounded down.
> 
> > Can you avoid this?
> 
> I tried, but there are many assumptions in tests. I'd prefer fixing this in a
> followup CL.

Add an explanatory comment and/or TODO.

[sky, 2014-12-01 15:57:22]

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write...
ui/gfx/win/direct_write.cc:68: void MaybeInitializeDirectWrite() {
What happens if this is invoked multiple times?

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write...
ui/gfx/win/direct_write.cc:77: typedef decltype(DWriteCreateFactory)*
DWriteCreateFactoryProc;
Use using for improved clarity.

[ckocagil, 2014-12-01 19:31:53]

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write.cc
File ui/gfx/win/direct_write.cc (right):

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write...
ui/gfx/win/direct_write.cc:68: void MaybeInitializeDirectWrite() {
On 2014/12/01 15:57:22, sky wrote:
> What happens if this is invoked multiple times?

We likely leak some objects (DWrite or SkFontMgr) with each call. Changed this
function to skip subsequent calls. Should I add a NOTREACHED too?

https://codereview.chromium.org/738363002/diff/120001/ui/gfx/win/direct_write...
ui/gfx/win/direct_write.cc:77: typedef decltype(DWriteCreateFactory)*
DWriteCreateFactoryProc;
On 2014/12/01 15:57:22, sky wrote:
> Use using for improved clarity.

Done.

[sky, 2014-12-01 22:45:09]

The static, LGTM

[ckocagil, 2014-12-02 00:34:14]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-12-02 00:35:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/738363002/160001

[commit-bot: I haz the power, 2014-12-02 01:01:24]

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

[commit-bot: I haz the power, 2014-12-02 01:01:25]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/88395)

[ckocagil, 2014-12-02 03:01:06]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-12-02 03:01:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/738363002/180001

[commit-bot: I haz the power, 2014-12-02 04:48:40]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2014-12-02 04:49:28]

Patchset 8 (id:??) landed as
https://crrev.com/9a4438efc6399ff2024ffb4d7b0e087da2c6f4c0
Cr-Commit-Position: refs/heads/master@{#306346}