Fixes vertical alignment of RenderTextWin.

Fixes vertical alignment of RenderTextWin.

I don't see the same issue of crbug.com/244323 on Windows so far, but theoretically this CL fixes the same possible issue on Windows.

By the way, this CL is effective for the case that some of fonts are unexpectedly smaller than the default font.  Not effective for the case that some of fonts are unexpectedly larger than the default font.  So, this CL doesn't fix crbug.com/146236 .

BUG=244323
TEST=none

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

[Yuki, 2013-07-22 15:01:15]

Could you review this CL?

For the issue 146236, we need another fix.
I found Hindi font is (unexpectedly?) greater than the specified font size.  I'm
now investigating why.

Cheers,
Yuki Shiino

[msw, 2013-07-23 23:43:29]

I can repro http://crbug.com/244323 on Windows ToT using the EN-US Windows
Classic Theme, which uses Tahoma 8 by default. There, inputting 'あ' shows the
char higher than a shared font baseline, and then inputting 'a' lowers the 'あ'
char down to the shared baseline; the movement of the character is jarring and
unwanted. I applied this patch locally and it solves this issue correctly; 'あ'
uses the shared baseline before entering the 'a' character; well done. You're
right that this doesn't fix http://crbug.com/146236, so I'll look into that
later (or feel free to help out or comment there if you have ideas). I commented
to this effect with a picture at http://crbug.com/244323#c13
LGTM; thanks!

[commit-bot: I haz the power, 2013-07-24 07:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/19734003/1

[commit-bot: I haz the power, 2013-07-24 10:13:06]

Change committed as 213383

[Alexei Svitkine (slow), 2013-07-24 15:29:48]

This could also use some tests.

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win...
ui/gfx/render_text_win.cc:492: common_baseline_ = font_list().GetBaseline();
This value will be clobbered unless the text is empty, so your CL is changing
the behaviour of GetBaseline() for when there is no text.

Which isn't necessarily bad, but we should have clear expectations for how that
works and accompanying tests.

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win...
ui/gfx/render_text_win.cc:568: int descent = font_list().GetHeight() -
font_list().GetBaseline();
I'm not sure it makes sense to start with the metrics of the font_list, after
all - none of the fonts in it may end up actually being used by the string due
to fallback.

Why not just use the fonts of the runs?

[msw, 2013-07-24 18:02:36]

Agreed, tests would be nice; Yuki, could you oblige, and also consider Alexei's
comment w.r.t. the empty text? Please correct me on the other comment if I'm
wrong.

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win...
ui/gfx/render_text_win.cc:568: int descent = font_list().GetHeight() -
font_list().GetBaseline();
On 2013/07/24 15:29:48, Alexei Svitkine wrote:
> I'm not sure it makes sense to start with the metrics of the font_list, after
> all - none of the fonts in it may end up actually being used by the string due
> to fallback.
> 
> Why not just use the fonts of the runs?

Using just the fonts of the runs and not including the selected 'primary' font
is the bug. If you enter a character that uses a fallback font with a smaller
baseline/height, then enter a character that uses the primary font with a larger
baseline/height, it will shift the baseline. Granted, there are still other
related defects, since new fallback fonts can arrive on any keystroke; I'm not
sure how we'll fix that, but we should try, and this seems like a reasonable
step forward (always support the baseline/height of the primary font).

[Alexei Svitkine (slow), 2013-07-24 18:12:28]

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://chromiumcodereview.appspot.com/19734003/diff/1/ui/gfx/render_text_win...
ui/gfx/render_text_win.cc:568: int descent = font_list().GetHeight() -
font_list().GetBaseline();
On 2013/07/24 18:02:36, msw wrote:
> On 2013/07/24 15:29:48, Alexei Svitkine wrote:
> > I'm not sure it makes sense to start with the metrics of the font_list,
after
> > all - none of the fonts in it may end up actually being used by the string
due
> > to fallback.
> > 
> > Why not just use the fonts of the runs?
> 
> Using just the fonts of the runs and not including the selected 'primary' font
> is the bug. If you enter a character that uses a fallback font with a smaller
> baseline/height, then enter a character that uses the primary font with a
larger
> baseline/height, it will shift the baseline. Granted, there are still other
> related defects, since new fallback fonts can arrive on any keystroke; I'm not
> sure how we'll fix that, but we should try, and this seems like a reasonable
> step forward (always support the baseline/height of the primary font).

Ah, I understand now. Thanks for the explanation - I think the code should have
a comment that also mentions this too (in addition to tests).

[Yuki, 2013-07-25 14:32:40]

I've send out http://crrev.com/20287002
Could you review that CL?

https://codereview.chromium.org/19734003/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/19734003/diff/1/ui/gfx/render_text_win.cc#new...
ui/gfx/render_text_win.cc:492: common_baseline_ = font_list().GetBaseline();
On 2013/07/24 15:29:48, Alexei Svitkine wrote:
> This value will be clobbered unless the text is empty, so your CL is changing
> the behaviour of GetBaseline() for when there is no text.
> 
> Which isn't necessarily bad, but we should have clear expectations for how
that
> works and accompanying tests.

Done.

https://codereview.chromium.org/19734003/diff/1/ui/gfx/render_text_win.cc#new...
ui/gfx/render_text_win.cc:568: int descent = font_list().GetHeight() -
font_list().GetBaseline();
On 2013/07/24 18:12:28, Alexei Svitkine wrote:
> On 2013/07/24 18:02:36, msw wrote:
> > On 2013/07/24 15:29:48, Alexei Svitkine wrote:
> > > I'm not sure it makes sense to start with the metrics of the font_list,
> after
> > > all - none of the fonts in it may end up actually being used by the string
> due
> > > to fallback.
> > > 
> > > Why not just use the fonts of the runs?
> > 
> > Using just the fonts of the runs and not including the selected 'primary'
font
> > is the bug. If you enter a character that uses a fallback font with a
smaller
> > baseline/height, then enter a character that uses the primary font with a
> larger
> > baseline/height, it will shift the baseline. Granted, there are still other
> > related defects, since new fallback fonts can arrive on any keystroke; I'm
not
> > sure how we'll fix that, but we should try, and this seems like a reasonable
> > step forward (always support the baseline/height of the primary font).
> 
> Ah, I understand now. Thanks for the explanation - I think the code should
have
> a comment that also mentions this too (in addition to tests).

Done.