Get all font unittests running with DirectWrite on Windows 7+

ui/gfx/platform_font_win_unittest.cc

Index: ui/gfx/platform_font_win_unittest.cc
diff --git a/ui/gfx/platform_font_win_unittest.cc b/ui/gfx/platform_font_win_unittest.cc
index dc4ac5eb54bf92896c92c339f6f22da41b265202..57136406df9a833d2dd298a22f8cdf4ba54b6536 100644
--- a/ui/gfx/platform_font_win_unittest.cc
+++ b/ui/gfx/platform_font_win_unittest.cc
@@ -17,72 +17,41 @@
 
 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_EQ(expected_font.GetFontName(), derived_font.GetFontName());
-    EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize());
-    EXPECT_LE(expected_font.GetHeight(), target_height);
+    const int target_height = base_font.GetHeight() + i;
+    Font derived_font = platform_font->DeriveFontWithHeight(target_height, 0);
+    // Ideally a derived font should have its height less or equal to the

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

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]

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:32]

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]

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.

+    // target height. However for font heights like 17 with DirectWrite we
+    // end up getting a font with height 18. We just check that the derived
+    // font height is at most 1 px off.
+    EXPECT_LE(abs(derived_font.GetHeight() - target_height), 1);
+    // A derived Font one size larger should ideally exceed the target height.
+    // However with DirectWrite with bigger heights like 24+, the derived font
+    // ends up being the same height as the target height.
+    EXPECT_GE(derived_font.Derive(1, 0).GetHeight(), target_height);
+    EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
+              derived_font.GetActualFontNameForTesting());
     EXPECT_EQ(0, derived_font.GetStyle());
-
     derived_font = platform_font->DeriveFontWithHeight(target_height,
                                                        Font::BOLD);
-    EXPECT_EQ(expected_font.GetFontName(), derived_font.GetFontName());
-    EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize());
-    EXPECT_LE(expected_font.GetHeight(), target_height);
+    // Please refer above as to why we validate that the difference between
+    // the derived font height and desired font height is at most 1.
+    EXPECT_LE(abs(derived_font.GetHeight() - target_height), 1);
+    // Please refer above as to why we use EXPECT_GE to check the derived font
+    // height.
+    EXPECT_GE(derived_font.Derive(1, 0).GetHeight(), target_height);
+    EXPECT_EQ(platform_font->GetActualFontNameForTesting(),
+              derived_font.GetActualFontNameForTesting());
     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*>(
@@ -197,5 +166,4 @@ TEST(PlatformFontWinTest, Metrics_SkiaVersusGDI) {
   }
 }
 
-
 }  // namespace gfx