Relanding this with font test fixes for gdi.

Relanding this with font test fixes for gdi.

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.

4. Ensure that the PlatformFontWin::DeriveFontWithHeight function honors the minimum font size constraint
   in all cases.

BUG=442010
R=msw

Committed: https://crrev.com/3e05f41653bf36cce40718d8295ce2293218dab6
Cr-Commit-Position: refs/heads/master@{#311388}

[msw, 2015-01-13 21:59:57]

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:274: EXPECT_NE(font1.GetHeight(),
font2.GetHeight());
nit: can you leave this check as it was?

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:228: if (font.GetHeight() > height) {
This check could potentially return a font smaller than the min size. If
|height|=10, |min_font_size|==10, |font.GetHeight()|==11,
|font.GetFontSize()|==9, this if statement would pass; then the first Derive
statement could make: |font.GetHeight()|==10, |font.GetFontSize()|==8, which
would skip the while loop and return the |font| at size 8, smaller than the min
size of 10. Let's rewrite this loop to handle that case and match the structure
of the new loop below. Does this look right?

while ((font.GetHeight() > height) &&
       (font.GetFontSize() >= min_font_size)) {
  Font derived_font = font.Derive(-1, style);
  if ((derived_font.GetFontSize() < min_font_size) ||
      (derived_font.GetFontSize() == font.GetFontSize()))
    break;
  DCHECK_LT(derived_font.GetFontSize(), font.GetFontSize());
  font = derived_font;
}

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:242: // Respect the minimum font size constraint.
nit: move this comment just above the |min_font_size| decl.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:246: if (derived_font.GetHeight() > height &&
!min_font_size)
I think this should be:
  if ((derived_font.GetHeight() > height) &&
      (font.GetFontSize() >= min_font_size))
Otherwise, given |height|==10, |min_font_size|==5, |font.GetHeight()|=10,
|font.GetFontSize()|=7, |derived_font.GetHeight()|==11, this check as-is would
fail and we'd wind up returning the |derived_font| with a height larger than
requested, even though the |font| already passes the minimum size requirement.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.h
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.h#n...
ui/gfx/platform_font_win.h:52: // Returns a derived Font with the specified
|style| and with height at most
Update this comment to something like:
  // Returns a derived Font with the specified |style| and maximum |height|.
  // The returned Font will be the largest font size with a height <= |height|,
  // since a size with the exact specified |height| may not necessarily exist.
  // GetMinimumFontSize() may impose a font size that is taller than |height|.

[ananta, 2015-01-13 22:21:46]

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:274: EXPECT_NE(font1.GetHeight(),
font2.GetHeight());
On 2015/01/13 21:59:57, msw wrote:
> nit: can you leave this check as it was?

No. This check fails on DW and also fails with GDI for font heights like 55.

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:228: if (font.GetHeight() > height) {
On 2015/01/13 21:59:57, msw wrote:
> This check could potentially return a font smaller than the min size. If
> |height|=10, |min_font_size|==10, |font.GetHeight()|==11,
> |font.GetFontSize()|==9, this if statement would pass; then the first Derive
> statement could make: |font.GetHeight()|==10, |font.GetFontSize()|==8, which
> would skip the while loop and return the |font| at size 8, smaller than the
min
> size of 10. Let's rewrite this loop to handle that case and match the
structure
> of the new loop below. Does this look right?
> 
> while ((font.GetHeight() > height) &&
>        (font.GetFontSize() >= min_font_size)) {
>   Font derived_font = font.Derive(-1, style);
>   if ((derived_font.GetFontSize() < min_font_size) ||
>       (derived_font.GetFontSize() == font.GetFontSize()))
>     break;
>   DCHECK_LT(derived_font.GetFontSize(), font.GetFontSize());
>   font = derived_font;
> }

Yes. I put this block inside an if (font.GetHeight() > height).

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:242: // Respect the minimum font size constraint.
On 2015/01/13 21:59:57, msw wrote:
> nit: move this comment just above the |min_font_size| decl.

Done.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:246: if (derived_font.GetHeight() > height &&
!min_font_size)
On 2015/01/13 21:59:57, msw wrote:
> I think this should be:
>   if ((derived_font.GetHeight() > height) &&
>       (font.GetFontSize() >= min_font_size))
> Otherwise, given |height|==10, |min_font_size|==5, |font.GetHeight()|=10,
> |font.GetFontSize()|=7, |derived_font.GetHeight()|==11, this check as-is would
> fail and we'd wind up returning the |derived_font| with a height larger than
> requested, even though the |font| already passes the minimum size requirement.

Done.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:246: if (derived_font.GetHeight() > height &&
!min_font_size)
On 2015/01/13 21:59:57, msw wrote:
> I think this should be:
>   if ((derived_font.GetHeight() > height) &&
>       (font.GetFontSize() >= min_font_size))
> Otherwise, given |height|==10, |min_font_size|==5, |font.GetHeight()|=10,
> |font.GetFontSize()|=7, |derived_font.GetHeight()|==11, this check as-is would
> fail and we'd wind up returning the |derived_font| with a height larger than
> requested, even though the |font| already passes the minimum size requirement.

Done.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.h
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/853553002/diff/1/ui/gfx/platform_font_win.h#n...
ui/gfx/platform_font_win.h:52: // Returns a derived Font with the specified
|style| and with height at most
On 2015/01/13 21:59:57, msw wrote:
> Update this comment to something like:
>   // Returns a derived Font with the specified |style| and maximum |height|.
>   // The returned Font will be the largest font size with a height <=
|height|,
>   // since a size with the exact specified |height| may not necessarily exist.
>   // GetMinimumFontSize() may impose a font size that is taller than |height|.

Done.

[msw, 2015-01-13 22:43:59]

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:269: // for the baseline (ascent) and height.
nit: revert this comment for the height, it's actually the descent.

https://codereview.chromium.org/853553002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:274: EXPECT_NE(font1.GetHeight(),
font2.GetHeight());
On 2015/01/13 22:21:46, ananta wrote:
> On 2015/01/13 21:59:57, msw wrote:
> > nit: can you leave this check as it was?
> 
> No. This check fails on DW and also fails with GDI for font heights like 55. 

That's unfortunate, the test loses value if we can't ensure that the fontlist
descent is actually the maximum of its distinct constituent font descents.
Perhaps and a TODO comment: "// TODO: Find a size and font pair with reliably
distinct descents."

https://codereview.chromium.org/853553002/diff/40001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/853553002/diff/40001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:229: if (font.GetHeight() > height) {
Remove this if statement and the return statement it contains, we just want the
loop here. Otherwise, if |font.GetHeight()| > |height|, but |font.GetFontSize()|
< |min_font_size|, this will return the |font| as-is, smaller than the min size.

If we want to avoid running the loop below after running this loop (just for
efficiency's sake, to avoid shrinking here then re-deriving the first larger
size with a height greater than |height|), set a flag, I think like this:

  bool ran_shrink_loop = false;
  while ((font.GetHeight() > height) && 
         (font.GetFontSize() >= min_font_size)) {
    ran_shrink_loop = true;
    ...
  }

  while ((font.GetHeight() <= height && !ran_shrink_loop) ||
         (font.GetFontSize() < min_font_size)) {
    ...
  }

[ananta, 2015-01-13 23:39:40]

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

https://codereview.chromium.org/853553002/diff/1/ui/gfx/font_list_unittest.cc...
ui/gfx/font_list_unittest.cc:274: EXPECT_NE(font1.GetHeight(),
font2.GetHeight());
On 2015/01/13 22:43:59, msw wrote:
> On 2015/01/13 22:21:46, ananta wrote:
> > On 2015/01/13 21:59:57, msw wrote:
> > > nit: can you leave this check as it was?
> > 
> > No. This check fails on DW and also fails with GDI for font heights like 55.

> 
> That's unfortunate, the test loses value if we can't ensure that the fontlist
> descent is actually the maximum of its distinct constituent font descents.
> Perhaps and a TODO comment: "// TODO: Find a size and font pair with reliably
> distinct descents."

Added a TODO

https://codereview.chromium.org/853553002/diff/40001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/853553002/diff/40001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:229: if (font.GetHeight() > height) {
On 2015/01/13 22:43:59, msw wrote:
> Remove this if statement and the return statement it contains, we just want
the
> loop here. Otherwise, if |font.GetHeight()| > |height|, but
|font.GetFontSize()|
> < |min_font_size|, this will return the |font| as-is, smaller than the min
size.
> 
> If we want to avoid running the loop below after running this loop (just for
> efficiency's sake, to avoid shrinking here then re-deriving the first larger
> size with a height greater than |height|), set a flag, I think like this:
> 
>   bool ran_shrink_loop = false;
>   while ((font.GetHeight() > height) && 
>          (font.GetFontSize() >= min_font_size)) {
>     ran_shrink_loop = true;
>     ...
>   }
> 
>   while ((font.GetHeight() <= height && !ran_shrink_loop) ||
>          (font.GetFontSize() < min_font_size)) {
>     ...
>   }

I added a new function CreateFontWithHeight which creates a HFONT which matches
the minimum size. That should ensure that we at least have a Font which matches
the criteria. We can leave the if in place now? WDYT?

https://codereview.chromium.org/853553002/diff/40001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:243: (font.GetFontSize() < min_font_size)) {
I think the CreateFontWithHeight function now alleviates the need to check the
font.GetFontSize() < min_font_size here.

[msw, 2015-01-13 23:54:59]

I don't think this is the right way to go.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:524: HFONT PlatformFontWin::CreateFontWithHeight(int
height, int style) {
I don't think that this simplifies the code as you intended. Now we have a
function that creates an HFONT *similar* to what we need, using a loop, but it
still requires two loops to shrink or grow the font in the caller (since the
height of the HFONT doesn't map directly the height as reported by Skia,
right?). I'm not sure how this is better than making incremental improvements
like I suggested on the last patch set.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win.h
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:55: // GetMinimumFontSize() may impose a font size
that is taller than |height|
nit: trailing period.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:193: // The returned HFONT will be the largest font
size with a
nit: s/HFONT/font/ to fix line wrapping.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:196: // GetMinimumFontSize() may impose a font size
that is taller than |height|
nit: trailing period.

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

https://codereview.chromium.org/853553002/diff/80001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:233: while (font.GetHeight() <= height) {
This should still use something like my proposed |ran_shrink_loop| flag if we
don't want this loop to run a single useless DeriveFont (that returns the first
font with a height larger than |height|) after the shrink loop runs above.

[ananta, 2015-01-14 00:17:24]

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:524: HFONT PlatformFontWin::CreateFontWithHeight(int
height, int style) {
On 2015/01/13 23:54:59, msw wrote:
> I don't think that this simplifies the code as you intended. Now we have a
> function that creates an HFONT *similar* to what we need, using a loop, but it
> still requires two loops to shrink or grow the font in the caller (since the
> height of the HFONT doesn't map directly the height as reported by Skia,
> right?). I'm not sure how this is better than making incremental improvements
> like I suggested on the last patch set.

Reverted this part of the change. This was an experimental patch to see if it
helps in reducing the complexity in the DeriveFontWithHeight function.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win.h
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:55: // GetMinimumFontSize() may impose a font size
that is taller than |height|
On 2015/01/13 23:54:59, msw wrote:
> nit: trailing period.

Done.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:193: // The returned HFONT will be the largest font
size with a
On 2015/01/13 23:54:59, msw wrote:
> nit: s/HFONT/font/ to fix line wrapping.

Removed this function.

https://codereview.chromium.org/853553002/diff/60001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.h:196: // GetMinimumFontSize() may impose a font size
that is taller than |height|
On 2015/01/13 23:54:59, msw wrote:
> nit: trailing period.

Removed this function

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

https://codereview.chromium.org/853553002/diff/80001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:233: while (font.GetHeight() <= height) {
On 2015/01/13 23:54:59, msw wrote:
> This should still use something like my proposed |ran_shrink_loop| flag if we
> don't want this loop to run a single useless DeriveFont (that returns the
first
> font with a height larger than |height|) after the shrink loop runs above.

Done.

[msw, 2015-01-14 00:33:09]

lgtm with a nit, thanks!

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

https://codereview.chromium.org/853553002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:274: // TODO(ananta): Find a size and font pair
with reliably distinct descents."
nit: remove trailing quote.

[ananta, 2015-01-14 00:35:46]

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

https://codereview.chromium.org/853553002/diff/100001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:274: // TODO(ananta): Find a size and font pair
with reliably distinct descents."
On 2015/01/14 00:33:09, msw wrote:
> nit: remove trailing quote.

Done.

[ananta, 2015-01-14 00:36:24]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-01-14 00:37:21]

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

[commit-bot: I haz the power, 2015-01-14 01:35:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-14 01:36:55]

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

[jochen (gone - plz use gerrit), 2015-01-14 07:47:07]

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

The reason for reverting is: still fails on XP

https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build....