Get all font unittests running with DirectWrite on Windows 7+

Get all font unittests running with DirectWrite on Windows 7+

Fixes as per below:-
1. Remove the addition of the fLeading value when calculating the height for the font
   with DirectWrite. fAscent + fDescent is the height of the font and adding the fLeading
   value to it returns the spacing between lines which is not what we are looking for.

2. The FontListTest.Fonts_GetHeight_GetBaseline unittest has a condition which basically validates
   whether the difference between the font height and the baseline is different for Arial and Symbol
   fonts. This fails for DirectWrite and fails for GDI with font sizes like 50, etc. Replaced this check
   with a check for the font heights are different.

3. Reworked the PlatformFontWinTest.DeriveFontWithHeight test to ensure it passes for DirectWrite and GDI.

BUG=442010
R=msw

Committed: https://crrev.com/58063de6b256e760e838af217bbc992b64f1cb59
Cr-Commit-Position: refs/heads/master@{#311178}

[ananta, 2015-01-09 23:06:07]

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

[msw, 2015-01-09 23:40:22]

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

https://codereview.chromium.org/844083002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:269: // for ascent and descent.
nit: "for the baseline (ascent) and height."

https://codereview.chromium.org/844083002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:280: EXPECT_EQ(std::max(font1.GetHeight() -
font1.GetBaseline(),
nit: adjust this to also just check the height instead of the descent (and
perhaps reorder it after the Baseline check, to match the EXPECT_NE pair above).

https://codereview.chromium.org/844083002/diff/1/ui/gfx/platform_font_win_uni...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:54: // With DirectWrite the height of the
expected font calculated above may be
I might just nuke AdjustFontSizeForHeight and expected_font, and just ensure
that DeriveFontWithHeight does what it promises for this range of heights,
something like below. Then we can just remove the weird comparison against the
test function and these comments. Bonus points for moving any tests that work
cross-platform to a new platform_font_unittest.cc file. What do you think?

for (int i = -10; i < 10; i++) {
  const int target_height = base_font.GetHeight() + i;
  Font derived_font = platform_font->DeriveFontWithHeight(target_height, 0);
  EXPECT_LE(derived_font.GetHeight(), target_height);
  // A derived Font one size larger should exceed the target height.
  EXPECT_GT(derived_font.Derive(1, 0).GetHeight(), target_height);
  EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
            derived_font.GetActualFontNameForTesting());
  EXPECT_EQ(0, derived_font.GetStyle());

  ... Ditto for Font::BOLD ...
  ... Delete the bottom with |rederived_font| ...
}

[ananta, 2015-01-10 00:43:01]

Patchset #4 (id:60001) has been deleted

[ananta, 2015-01-10 00:43:15]

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

https://codereview.chromium.org/844083002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:269: // for ascent and descent.
On 2015/01/09 23:40:22, msw wrote:
> nit: "for the baseline (ascent) and height."

Done.

https://codereview.chromium.org/844083002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:280: EXPECT_EQ(std::max(font1.GetHeight() -
font1.GetBaseline(),
On 2015/01/09 23:40:22, msw wrote:
> nit: adjust this to also just check the height instead of the descent (and
> perhaps reorder it after the Baseline check, to match the EXPECT_NE pair
above).

Done.

https://codereview.chromium.org/844083002/diff/1/ui/gfx/platform_font_win_uni...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:54: // With DirectWrite the height of the
expected font calculated above may be
On 2015/01/09 23:40:22, msw wrote:
> I might just nuke AdjustFontSizeForHeight and expected_font, and just ensure
> that DeriveFontWithHeight does what it promises for this range of heights,
> something like below. Then we can just remove the weird comparison against the
> test function and these comments. Bonus points for moving any tests that work
> cross-platform to a new platform_font_unittest.cc file. What do you think?
> 
> for (int i = -10; i < 10; i++) {
>   const int target_height = base_font.GetHeight() + i;
>   Font derived_font = platform_font->DeriveFontWithHeight(target_height, 0);
>   EXPECT_LE(derived_font.GetHeight(), target_height);
>   // A derived Font one size larger should exceed the target height.
>   EXPECT_GT(derived_font.Derive(1, 0).GetHeight(), target_height);
>   EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
>             derived_font.GetActualFontNameForTesting());
>   EXPECT_EQ(0, derived_font.GetStyle());
> 
>   ... Ditto for Font::BOLD ...
>   ... Delete the bottom with |rederived_font| ...
> }

DeriveFontWithHeight is windows specific at the moment. I left the
implementation in the platform_font_win_unittest.cc file. Changed the test on
the lines you mentioned. However the checks are a touch different. Comments in
the test should clarify this.

[msw, 2015-01-10 01:50:15]

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:279: // descent of FontList == max(descent of
Fonts)
The baseline is actually the ascent, not descent.

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:283: // The height of the font list should be the
max height of the fonts in it.
Oh darn, my earlier advice was wrong. I remember why this isn't true! Because
FontList applies baseline centering, the ascent and decent will be the max of
the constituent values. The height will not necessarily be the max of its
constituent values. Revert the checks here and make sure they pass.

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win_unittest.cc:28: // Ideally a derived font should have
its height less or equal to the
I'm confused about how either of these behaviors are possible... Shouldn't the
PlatformFont metrics match the gfx::Font metrics exactly? Aren't both just
retrieving the DW metrics? How can this function return something taller than
the target, or smaller than the largest font for that height?

[ananta, 2015-01-10 02:05:49]

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:279: // descent of FontList == max(descent of
Fonts)
On 2015/01/10 01:50:15, msw wrote:
> The baseline is actually the ascent, not descent.

Done.

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:283: // The height of the font list should be the
max height of the fonts in it.
On 2015/01/10 01:50:15, msw wrote:
> Oh darn, my earlier advice was wrong. I remember why this isn't true! Because
> FontList applies baseline centering, the ascent and decent will be the max of
> the constituent values. The height will not necessarily be the max of its
> constituent values. Revert the checks here and make sure they pass.

Done.

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win_unittest.cc:28: // Ideally a derived font should have
its height less or equal to the
On 2015/01/10 01:50:15, msw wrote:
> I'm confused about how either of these behaviors are possible... Shouldn't the
> PlatformFont metrics match the gfx::Font metrics exactly? Aren't both just
> retrieving the DW metrics? How can this function return something taller than
> the target, or smaller than the largest font for that height?

Yes. They both retrieve the DW metrics. However with DirectWrite we have 1 px
discrepancies in heights.

For e.g for height 17 the ascent descent returned by GDI is 14 and 3. Skia
(DirectWrite) returns 3.26 and 14.028. Adding these gives us 17.29. We ceil as
we want to account for partial pixels in the height. This caused some text
rendering problems in previous iterations. Ceiling gives us 18 which is off by
1.

[msw, 2015-01-10 02:28:33]

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win_unittest.cc:28: // Ideally a derived font should have
its height less or equal to the
On 2015/01/10 02:05:49, ananta wrote:
> On 2015/01/10 01:50:15, msw wrote:
> > I'm confused about how either of these behaviors are possible... Shouldn't
the
> > PlatformFont metrics match the gfx::Font metrics exactly? Aren't both just
> > retrieving the DW metrics? How can this function return something taller
than
> > the target, or smaller than the largest font for that height?
> 
> Yes. They both retrieve the DW metrics. However with DirectWrite we have 1 px
> discrepancies in heights.
> 
> For e.g for height 17 the ascent descent returned by GDI is 14 and 3. Skia
> (DirectWrite) returns 3.26 and 14.028. Adding these gives us 17.29. We ceil as
> we want to account for partial pixels in the height. This caused some text
> rendering problems in previous iterations. Ceiling gives us 18 which is off by
> 1.

Okay, this must only occur when PlatformFontWin::DeriveFontWithHeight is
deriving a font with a height *taller* than the base platform font, since there
it's using CreateFontIndirect and DeriveWithCorrectedSize, instead of checking
the gfx::Font::GetHeight value and iterating. I think we should change
DeriveFontWithHeight to use the gfx::Font::GetHeight when increasing the size
exactly like it does when decreasing the size.

Perhaps, in the interest of not regressing performance too much, we could start
DeriveFontWithHeight by calling CreateFontIndirect with the target height, and
then iterating as needed to increase or decrease the size to match the intended
height.

+CC a few font-knowledgable folks for a sanity check.

[ananta, 2015-01-12 20:10:31]

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/844083002/diff/100001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win_unittest.cc:28: // Ideally a derived font should have
its height less or equal to the
On 2015/01/10 02:28:32, msw wrote:
> On 2015/01/10 02:05:49, ananta wrote:
> > On 2015/01/10 01:50:15, msw wrote:
> > > I'm confused about how either of these behaviors are possible... Shouldn't
> the
> > > PlatformFont metrics match the gfx::Font metrics exactly? Aren't both just
> > > retrieving the DW metrics? How can this function return something taller
> than
> > > the target, or smaller than the largest font for that height?
> > 
> > Yes. They both retrieve the DW metrics. However with DirectWrite we have 1
px
> > discrepancies in heights.
> > 
> > For e.g for height 17 the ascent descent returned by GDI is 14 and 3. Skia
> > (DirectWrite) returns 3.26 and 14.028. Adding these gives us 17.29. We ceil
as
> > we want to account for partial pixels in the height. This caused some text
> > rendering problems in previous iterations. Ceiling gives us 18 which is off
by
> > 1.
> 
> Okay, this must only occur when PlatformFontWin::DeriveFontWithHeight is
> deriving a font with a height *taller* than the base platform font, since
there
> it's using CreateFontIndirect and DeriveWithCorrectedSize, instead of checking
> the gfx::Font::GetHeight value and iterating. I think we should change
> DeriveFontWithHeight to use the gfx::Font::GetHeight when increasing the size
> exactly like it does when decreasing the size.
> 
> Perhaps, in the interest of not regressing performance too much, we could
start
> DeriveFontWithHeight by calling CreateFontIndirect with the target height, and
> then iterating as needed to increase or decrease the size to match the
intended
> height.
> 
> +CC a few font-knowledgable folks for a sanity check.

I changed the logic in the DeriveFontWithHeight function to handle the
increasing font size logic correctly. Removed the code which starts off by
creating the font via CreateFontIndirect as it does not add much value and also
simplifies the logic.

In any case if we create the font with the target height we end up with the same
problem where in the created font is 1 px larger.

PTAL.

[msw, 2015-01-12 21:30:55]

This looks pretty good, but can you test the performance impact quickly? Font
derivations are notoriously expensive. Please check the overall test runtime for
PlatformFontWinTest.DeriveFontWithHeight with the current code, versus with an
initial call to CreateFontIndirect at the top of the function (to reduce the
iterative DeriveFont calls). You might also print the number of DeriveFont calls
made each time the test calls PlatformFontWin::DeriveFontWithHeight for both
solutions.

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:233: // We derive a font with a size delta of 0
initially, i.e the derived font
Is this necessary given the early return at lines 214/215?

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:233: // We derive a font with a size delta of 0
initially, i.e the derived font
nit: add a blank line above.

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:240: Font derived_font = font.Derive(1, style);
I'm curious about the performance impact here, but I'm not aware of related
perftests. Perhaps an initial CreateFontIndirect would avoid 90% of the
DeriveFont iterations for both loops. Maybe Alexei or Dan have some thoughts?

[ananta, 2015-01-12 22:14:15]

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:233: // We derive a font with a size delta of 0
initially, i.e the derived font
On 2015/01/12 21:30:55, msw wrote:
> Is this necessary given the early return at lines 214/215?

Removed the comment. The function has changed a bit.

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:233: // We derive a font with a size delta of 0
initially, i.e the derived font
On 2015/01/12 21:30:55, msw wrote:
> nit: add a blank line above.

Done.

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:233: // We derive a font with a size delta of 0
initially, i.e the derived font
On 2015/01/12 21:30:55, msw wrote:
> Is this necessary given the early return at lines 214/215?

The function has changed.

https://codereview.chromium.org/844083002/diff/160001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:240: Font derived_font = font.Derive(1, style);
On 2015/01/12 21:30:55, msw wrote:
> I'm curious about the performance impact here, but I'm not aware of related
> perftests. Perhaps an initial CreateFontIndirect would avoid 90% of the
> DeriveFont iterations for both loops. Maybe Alexei or Dan have some thoughts?

Done. Removed the starting 2 lines which check for whether the current font is
of the desired height. This is as per our discussion that we want to iterate and
return the next available font with the desired height.

[msw, 2015-01-12 22:29:29]

lgtm with nits, thanks!

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:215: LOGFONT font_info;
nit: Add a comment like "Create a font with a height near that of the target
height."

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:241: Font derived_font = font.Derive(1, style);
nit: The loop above breaks if the derived font isn't any smaller than the
current font. I'm not sure if that's still a problem there, or if something
similar might happen here. If you want to be really safe, you can add a similar
break, otherwise you might want to just add a dcheck here:
DCHECK_GT(derived_font.GetFontSize(), font.GetFontSize());

[Alexei Svitkine (slow), 2015-01-12 23:00:42]

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

[Alexei Svitkine (slow), 2015-01-12 23:00:43]

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:240: while (font.GetHeight() <= height) {
Have you checked whether this extra loop doesn't cause any performance
regressions? In the past, I recall that we have had cases where bad performance
in this function resulted in a degradation of text rendering performance in UI.
Can you check whether this change is performance neutral or not?

[ananta, 2015-01-12 23:08:02]

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:215: LOGFONT font_info;
On 2015/01/12 22:29:29, msw wrote:
> nit: Add a comment like "Create a font with a height near that of the target
> height."

Done.

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:240: while (font.GetHeight() <= height) {
On 2015/01/12 23:00:43, Alexei Svitkine wrote:
> Have you checked whether this extra loop doesn't cause any performance
> regressions? In the past, I recall that we have had cases where bad
performance
> in this function resulted in a degradation of text rendering performance in
UI.
> Can you check whether this change is performance neutral or not?

We start off with a native font created with the target height which ensures
that the loop here will not have too many iterations to cause performance
issues. In the unittest runs the loop runs once at the most which is similar to
current behavior.

https://codereview.chromium.org/844083002/diff/180001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:241: Font derived_font = font.Derive(1, style);
On 2015/01/12 22:29:29, msw wrote:
> nit: The loop above breaks if the derived font isn't any smaller than the
> current font. I'm not sure if that's still a problem there, or if something
> similar might happen here. If you want to be really safe, you can add a
similar
> break, otherwise you might want to just add a dcheck here:
> DCHECK_GT(derived_font.GetFontSize(), font.GetFontSize());

Done.

[ananta, 2015-01-13 01:05:33]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-01-13 01:07:04]

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

[commit-bot: I haz the power, 2015-01-13 01:59:27]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-01-13 02:01:02]

Patchset 11 (id:??) landed as
https://crrev.com/58063de6b256e760e838af217bbc992b64f1cb59
Cr-Commit-Position: refs/heads/master@{#311178}

[jochen (gone - plz use gerrit), 2015-01-13 09:18:01]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/820173005/ by jochen@chromium.org.

The reason for reverting is: breaks PlatformFontWinTest.DeriveFontWithHeight on
xp and vista.