Revert of Relanding this with font test fixes for gdi. (patchset #7 id:120001...

Revert of Relanding this with font test fixes for gdi. (patchset #7 id:120001 of https://codereview.chromium.org/853553002/)

Reason for revert:
still fails on XP

https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/34996/steps/gfx_unittests/logs/DeriveFontWithHeight

Original issue's description:
> 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}

TBR=msw@chromium.org,ananta@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=442010

Review URL: https://codereview.chromium.org/847283003

Cr-Commit-Position: refs/heads/master@{#311424}

ui/gfx/font_list_unittest.cc

@@ -266,24 +266,24 @@ TEST(FontListTest, Fonts_GetHeight_GetBaseline) {
   EXPECT_EQ(font1.GetBaseline(), font_list1.GetBaseline());
 
   // If there are two different fonts, the font list returns the max value
-  // for the baseline (ascent) and height.
+  // for ascent and descent.
   Font font2("Symbol", 16);
   ASSERT_EQ("symbol",
             base::StringToLowerASCII(font2.GetActualFontNameForTesting()));
   EXPECT_NE(font1.GetBaseline(), font2.GetBaseline());
-  // TODO(ananta): Find a size and font pair with reliably distinct descents.
-  EXPECT_NE(font1.GetHeight(), font2.GetHeight());
+  EXPECT_NE(font1.GetHeight() - font1.GetBaseline(),
+            font2.GetHeight() - font2.GetBaseline());
   std::vector<Font> fonts;
   fonts.push_back(font1);
   fonts.push_back(font2);
   FontList font_list_mix(fonts);
   // ascent of FontList == max(ascent of Fonts)
-  EXPECT_EQ(std::max(font1.GetBaseline(), font2.GetBaseline()),
-            font_list_mix.GetBaseline());
-  // descent of FontList == max(descent of Fonts)
   EXPECT_EQ(std::max(font1.GetHeight() - font1.GetBaseline(),
                      font2.GetHeight() - font2.GetBaseline()),
             font_list_mix.GetHeight() - font_list_mix.GetBaseline());
+  // descent of FontList == max(descent of Fonts)
+  EXPECT_EQ(std::max(font1.GetBaseline(), font2.GetBaseline()),
+            font_list_mix.GetBaseline());
 }
 
 TEST(FontListTest, Fonts_DeriveWithHeightUpperBound) {

ui/gfx/platform_font_win.cc

@@ -211,45 +211,33 @@ PlatformFontWin::PlatformFontWin(const std::string& font_name,
 
 Font PlatformFontWin::DeriveFontWithHeight(int height, int style) {
   DCHECK_GE(height, 0);
+  if (GetHeight() == height && GetStyle() == style)
+    return Font(this);
+
+  // CreateFontIndirect() doesn't return the largest size for the given height
+  // when decreasing the height. Iterate to find it.
+  if (GetHeight() > height) {
+    const int min_font_size = GetMinimumFontSize();
+    Font font = DeriveFont(-1, style);
+    int font_height = font.GetHeight();
+    int font_size = font.GetFontSize();
+    while (font_height > height && font_size != min_font_size) {
+      font = font.Derive(-1, style);
+      if (font_height == font.GetHeight() && font_size == font.GetFontSize())
+        break;
+      font_height = font.GetHeight();
+      font_size = font.GetFontSize();
+    }
+    return font;
+  }
 
-  // Create a font with a height near that of the target height.
   LOGFONT font_info;
   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
   font_info.lfHeight = height;
   SetLogFontStyle(style, &font_info);
 
   HFONT hfont = CreateFontIndirect(&font_info);
-  Font font(new PlatformFontWin(CreateHFontRef(hfont)));
-
-  // Respect the minimum font size constraint.
-  const int min_font_size = GetMinimumFontSize();
-
-  // Used to avoid shrinking the font and expanding it.
-  bool ran_shrink_loop = false;
-
-  // CreateFontIndirect() doesn't return the largest size for the given height
-  // when decreasing the height. Iterate to find it.
-  while ((font.GetHeight() > height) &&
-         (font.GetFontSize() >= min_font_size)) {
-    ran_shrink_loop = true;
-    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;
-  }
-
-  while ((!ran_shrink_loop && font.GetHeight() <= height) ||
-         (font.GetFontSize() < min_font_size)) {
-    Font derived_font = font.Derive(1, style);
-    if ((derived_font.GetHeight() > height) &&
-        (font.GetFontSize() >= min_font_size))
-      break;
-    DCHECK_GT(derived_font.GetFontSize(), font.GetFontSize());
-    font = derived_font;
-  }
-  return font;
+  return DeriveWithCorrectedSize(hfont);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -435,6 +423,37 @@ PlatformFontWin::HFontRef* PlatformFontWin::CreateHFontRefFromGDI(
                       ave_char_width, style);
 }
 
+Font PlatformFontWin::DeriveWithCorrectedSize(HFONT base_font) {
+  base::win::ScopedGetDC screen_dc(NULL);
+  gfx::ScopedSetMapMode mode(screen_dc, MM_TEXT);
+
+  base::win::ScopedGDIObject<HFONT> best_font(base_font);
+  TEXTMETRIC best_font_metrics;
+  GetTextMetricsForFont(screen_dc, best_font, &best_font_metrics);
+
+  LOGFONT font_info;
+  GetObject(base_font, sizeof(LOGFONT), &font_info);
+
+  // Set |lfHeight| to negative value to indicate it's the size, not the height.
+  font_info.lfHeight =
+      -(best_font_metrics.tmHeight - best_font_metrics.tmInternalLeading);
+
+  do {
+    // Increment font size. Prefer font with greater size if its height isn't
+    // greater than height of base font.
+    font_info.lfHeight = AdjustFontSize(font_info.lfHeight, 1);
+    base::win::ScopedGDIObject<HFONT> font(CreateFontIndirect(&font_info));
+    TEXTMETRIC font_metrics;
+    GetTextMetricsForFont(screen_dc, font, &font_metrics);
+    if (font_metrics.tmHeight > best_font_metrics.tmHeight)
+      break;
+    best_font.Set(font.release());
+    best_font_metrics = font_metrics;
+  } while (true);
+
+  return Font(new PlatformFontWin(CreateHFontRef(best_font.release())));
+}
+
 // static
 PlatformFontWin::HFontRef* PlatformFontWin::CreateHFontRefFromSkia(
     HFONT gdi_font,
@@ -499,7 +518,8 @@ PlatformFontWin::HFontRef* PlatformFontWin::CreateHFontRefFromSkia(
   // The calculations below are similar to those in the CreateHFontRef
   // function. The height, baseline and cap height are rounded up to ensure
   // that they match up closely with GDI.
-  const int height = std::ceil(skia_metrics.fDescent - skia_metrics.fAscent);
+  const int height = std::ceil(
+      skia_metrics.fDescent - skia_metrics.fAscent + skia_metrics.fLeading);
   const int baseline = std::max<int>(1, std::ceil(-skia_metrics.fAscent));
   const int cap_height = std::ceil(paint.getTextSize() *
       static_cast<double>(dwrite_font_metrics.capHeight) /

ui/gfx/platform_font_win.h

@@ -49,10 +49,11 @@ class GFX_EXPORT PlatformFontWin : public PlatformFont {
   // name could not be retrieved, returns GetFontName().
   std::string GetLocalizedFontName() const;
 
-  // 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|.
+  // Returns a derived Font with the specified |style| and with height at most
+  // |height|. If the height and style of the receiver already match, it is
+  // returned. Otherwise, the returned Font will have the largest size such that
+  // its height is less than or equal to |height| (since there may not exist a
+  // size that matches the exact |height| specified).
   Font DeriveFontWithHeight(int height, int style);
 
   // Overridden from PlatformFont:
@@ -171,6 +172,10 @@ class GFX_EXPORT PlatformFontWin : public PlatformFont {
   static HFontRef* CreateHFontRefFromGDI(HFONT font,
                                          const TEXTMETRIC& font_metrics);
 
+  // Returns a largest derived Font whose height does not exceed the height of
+  // |base_font|.
+  static Font DeriveWithCorrectedSize(HFONT base_font);
+
   // Creates and returns a new HFontRef from the specified HFONT using metrics
   // from skia. Currently this is only used if we use DirectWrite for font
   // metrics.

ui/gfx/platform_font_win_unittest.cc

@@ -17,32 +17,72 @@
 
 namespace gfx {
 
+namespace {
+
+// Returns a font based on |base_font| with height at most |target_height| and
+// font size maximized. Returns |base_font| if height is already equal.
+gfx::Font AdjustFontSizeForHeight(const gfx::Font& base_font,
+                                  int target_height) {
+  Font expected_font = base_font;
+  if (base_font.GetHeight() < target_height) {
+    // Increase size while height is <= |target_height|.
+    Font larger_font = base_font.Derive(1, 0);
+    while (larger_font.GetHeight() <= target_height) {
+      expected_font = larger_font;
+      larger_font = larger_font.Derive(1, 0);
+    }
+  } else if (expected_font.GetHeight() > target_height) {
+    // Decrease size until height is <= |target_height|.
+    do {
+      expected_font = expected_font.Derive(-1, 0);
+    } while (expected_font.GetHeight() > target_height);
+  }
+  return expected_font;
+}
+
+}  // namespace
+
 TEST(PlatformFontWinTest, DeriveFontWithHeight) {
+  // TODO(ananta): Fix this test for DirectWrite. http://crbug.com/442010
+  if (gfx::win::IsDirectWriteEnabled())
+    return;
+
   const Font base_font;
   PlatformFontWin* platform_font =
       static_cast<PlatformFontWin*>(base_font.platform_font());
 
   for (int i = -10; i < 10; i++) {
     const int target_height = base_font.GetHeight() + i;
+    Font expected_font = AdjustFontSizeForHeight(base_font, target_height);
+    ASSERT_LE(expected_font.GetHeight(), target_height);
 
     Font derived_font = platform_font->DeriveFontWithHeight(target_height, 0);
-    EXPECT_LE(derived_font.GetHeight(), target_height);
-    EXPECT_GT(derived_font.Derive(1, 0).GetHeight(), target_height);
-    EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
-              derived_font.GetActualFontNameForTesting());
+    EXPECT_EQ(expected_font.GetFontName(), derived_font.GetFontName());
+    EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize());
+    EXPECT_LE(expected_font.GetHeight(), target_height);
     EXPECT_EQ(0, derived_font.GetStyle());
 
     derived_font = platform_font->DeriveFontWithHeight(target_height,
                                                        Font::BOLD);
-    EXPECT_LE(derived_font.GetHeight(), target_height);
-    EXPECT_GT(derived_font.Derive(1, 0).GetHeight(), target_height);
-    EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
-              derived_font.GetActualFontNameForTesting());
+    EXPECT_EQ(expected_font.GetFontName(), derived_font.GetFontName());
+    EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize());
+    EXPECT_LE(expected_font.GetHeight(), target_height);
     EXPECT_EQ(Font::BOLD, derived_font.GetStyle());
+
+    // Test that deriving from the new font has the expected result.
+    Font rederived_font = derived_font.Derive(1, 0);
+    expected_font = Font(derived_font.GetFontName(),
+                         derived_font.GetFontSize() + 1);
+    EXPECT_EQ(expected_font.GetFontName(), rederived_font.GetFontName());
+    EXPECT_EQ(expected_font.GetFontSize(), rederived_font.GetFontSize());
+    EXPECT_EQ(expected_font.GetHeight(), rederived_font.GetHeight());
   }
 }
 
 TEST(PlatformFontWinTest, DeriveFontWithHeight_Consistency) {
+  // TODO(ananta): Fix this test for DirectWrite. http://crbug.com/442010
+  if (gfx::win::IsDirectWriteEnabled())
+    return;
   gfx::Font arial_12("Arial", 12);
   ASSERT_GT(16, arial_12.GetHeight());
   gfx::Font derived_1 = static_cast<PlatformFontWin*>(
@@ -157,4 +197,5 @@ TEST(PlatformFontWinTest, Metrics_SkiaVersusGDI) {
   }
 }
 
+
 }  // namespace gfx