Fix clipping RenderTextHarfBuzz text on Windows 8.

Fix clipping RenderTextHarfBuzz text on Windows 8.

Round run widths on Windows without subpixel positioning.
(same as the Linux fix for the similar Issue 402374, etc.)

This fixes all clipped Win8 text seen in Issue 424484.
I see no visible effect on already-correct Win7/8 text.
See comparison screenshots at http://crbug.com/424484#c12

BUG=424484
TEST=Win7/8 text looks good with and without ClearType.
R=pkasting@chromium.org,ckocagil@chromium.org,asvitkine@chromium.org

Committed: https://crrev.com/7bdf3eb7cce5411af3b2703f33dcef1e1307af20
Cr-Commit-Position: refs/heads/master@{#303726}

[Peter Kasting, 2014-11-06 23:14:47]

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

[Peter Kasting, 2014-11-06 23:14:48]

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

https://codereview.chromium.org/707013004/diff/20001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1098: offset.set_x((offset.x() + kLeftOfCenterPadding) /
2);
I don't understand why the previous code was causing problems assuming |width|
is at least as large at display_rect().width() and display_rect().width()
actually contains the full extent of the text.

The only difference between the old code and the new is that, if |width| has an
odd number of extra pixels beyond what's necessary for diaply_rect(), the old
code puts the extra pixel on the left, and the new on the right.  But in neither
case should we actually be truncating something visible.  That should only
happen if one of the two conditions I mention above is violated.  And if those
are violated, it seems like the right fix is elsewhere rather than here.

Maybe I just don't understand what the code is doing.  Where is the logic flaw
above?

[msw, 2014-11-06 23:48:17]

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

[msw, 2014-11-06 23:48:44]

Please take a fresh look; thanks!

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

https://codereview.chromium.org/707013004/diff/20001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:1098: offset.set_x((offset.x() + kLeftOfCenterPadding) /
2);
On 2014/11/06 23:14:47, Peter Kasting wrote:
> I don't understand why the previous code was causing problems assuming |width|
> is at least as large at display_rect().width() and display_rect().width()
> actually contains the full extent of the text.
> 
> The only difference between the old code and the new is that, if |width| has
an
> odd number of extra pixels beyond what's necessary for diaply_rect(), the old
> code puts the extra pixel on the left, and the new on the right.  But in
neither
> case should we actually be truncating something visible.  That should only
> happen if one of the two conditions I mention above is violated.  And if those
> are violated, it seems like the right fix is elsewhere rather than here.
> 
> Maybe I just don't understand what the code is doing.  Where is the logic flaw
> above?

You're right, width+1 in RenderTextHarfBuzz::GetStringSizeF works.
That's a better temporary hack to use until we can dig deeper.

[Peter Kasting, 2014-11-06 23:51:26]

It would be nice to at least understand what's going on that causes this
problem, even if we decide the real fix is too nasty to be merged.

Adding 1 to GetStringSizeF() everywhere will fix this problem, but I worry it
might cause other ones, e.g. adding single pixels of space on one side of text
runs everywhere they're displayed, which in some cases could look pretty weird.

[msw, 2014-11-06 23:57:43]

On 2014/11/06 23:51:26, Peter Kasting wrote:
> It would be nice to at least understand what's going on that causes this
> problem, even if we decide the real fix is too nasty to be merged.
> 
> Adding 1 to GetStringSizeF() everywhere will fix this problem, but I worry it
> might cause other ones, e.g. adding single pixels of space on one side of text
> runs everywhere they're displayed, which in some cases could look pretty
weird.

I don't have oodles of time for this... The defect may be related to subpixel
positioning/rendering settings. The +1 hack only applies to the full RenderText
content width, not individual runs therein, and only applies to Windows 8 with
RenderTextHarfBuzz, which otherwise clips text, so it shouldn't really add any
padding.

This hack will give us room to investigate the underlying defect without
worrying about reverting RTHB on Windows 8 for M-40.

[Peter Kasting, 2014-11-07 00:02:09]

On 2014/11/06 23:57:43, msw wrote:
> I don't have oodles of time for this... The defect may be related to subpixel
> positioning/rendering settings. The +1 hack only applies to the full
RenderText
> content width, not individual runs therein, and only applies to Windows 8 with
> RenderTextHarfBuzz, which otherwise clips text, so it shouldn't really add any
> padding.

Are you sure it always clips text in these cases?  Maybe it only clips
sometimes?

Also, are you sure this is only on Windows 8?  Or did we simply not notice yet
on other platforms due to e.g. the Windows 8 default font being different or
something, so the cases where this occurs are different?

Without understanding what's going on, it seems like it's hard to answer these
questions definitively.

I'm not really in a position to approve or deny this CL.  I'm just trying to get
you to worry about the right things.  It's up to you and Cem what you want to
do.

[msw, 2014-11-07 00:31:54]

On 2014/11/07 00:02:09, Peter Kasting wrote:
> On 2014/11/06 23:57:43, msw wrote:
> > I don't have oodles of time for this... The defect may be related to
subpixel
> > positioning/rendering settings. The +1 hack only applies to the full
> RenderText
> > content width, not individual runs therein, and only applies to Windows 8
with
> > RenderTextHarfBuzz, which otherwise clips text, so it shouldn't really add
any
> > padding.
> 
> Are you sure it always clips text in these cases?  Maybe it only clips
> sometimes?
> 
> Also, are you sure this is only on Windows 8?  Or did we simply not notice yet
> on other platforms due to e.g. the Windows 8 default font being different or
> something, so the cases where this occurs are different?
> 
> Without understanding what's going on, it seems like it's hard to answer these
> questions definitively.
> 
> I'm not really in a position to approve or deny this CL.  I'm just trying to
get
> you to worry about the right things.  It's up to you and Cem what you want to
> do.

Those are valid points that I'll have to investigate after branch, if I have a
time. Perhaps I'll demote the bug back to rb-stable. Cem, if you think the
severity here really warrants rb-beta, you can disable rthb on Win8 I guess.

[msw, 2014-11-11 03:26:47]

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

[msw, 2014-11-11 03:27:32]

msw@chromium.org changed reviewers:
+ asvitkine@chromium.org
- sky@chromium.org

[msw, 2014-11-11 03:34:09]

I tried a number of other changes locally (ceil'ing
GetStringSizeF/GetContentWidth, using enclosing bounds on GetGraphemeBounds,
adjusting the display offset calculations, tweaking some int/float comparisons),
but those weren't complete or caused other defects (like causing omnibox results
to elide).

I found that rounding the run widths on Windows (like we already do on Linux for
a similar defect) covers the known related defects without changing the
appearance of already-correct text. That seems like the most reasonable fix for
now. Please take a look; thanks!

[ckocagil, 2014-11-11 15:49:49]

On 2014/11/11 03:34:09, msw wrote:
> I tried a number of other changes locally (ceil'ing
> GetStringSizeF/GetContentWidth, using enclosing bounds on GetGraphemeBounds,
> adjusting the display offset calculations, tweaking some int/float
comparisons),
> but those weren't complete or caused other defects (like causing omnibox
results
> to elide).
> 
> I found that rounding the run widths on Windows (like we already do on Linux
for
> a similar defect) covers the known related defects without changing the
> appearance of already-correct text. That seems like the most reasonable fix
for
> now. Please take a look; thanks!

Are you sure that this doesn't change the appearance of any string at all?

Can you locally test this by comparing screenshots of the Chrome hotdog menus?

[Alexei Svitkine (slow), 2014-11-11 16:10:17]

code change looks good - the fewer ifdefs between platforms the better - but as
Cem says, assuming it doesn't actually regress anything.

[msw, 2014-11-11 20:16:30]

I posted screenshots at http://crbug.com/424484#c12
The fix shifts all defective text over one pixel, and leaves other text as-is. I
don't see any such differences on corresponding Windows 7 instances.

[ckocagil, 2014-11-11 20:19:27]

On 2014/11/11 20:16:30, msw wrote:
> I posted screenshots at http://crbug.com/424484#c12
> The fix shifts all defective text over one pixel, and leaves other text as-is.
I
> don't see any such differences on corresponding Windows 7 instances.

lgtm!

[Alexei Svitkine (slow), 2014-11-11 20:31:04]

lgtm

[msw, 2014-11-11 20:44:49]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-11-11 20:45:44]

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

[commit-bot: I haz the power, 2014-11-11 21:48:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-11 21:48:53]

Patchset 5 (id:??) landed as
https://crrev.com/7bdf3eb7cce5411af3b2703f33dcef1e1307af20
Cr-Commit-Position: refs/heads/master@{#303726}