From fa8f88074507066c0dd61fcdf6e4154a89d0ba70 Mon Sep 17 00:00:00 2001 From: ananta Date: Wed, 14 Jan 2015 16:56:11 -0800 Subject: [PATCH] Relanding this with font test fixes for gdi font tests on XP 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. 5. Return a derived font from the DeriveFontWithHeight function if it matches the font size. 5. Ensure that the requested_font_size_ member in the HFontRef structure is set to the correct font size by going through the text metrics API. This is required because for small font sizes, the size as reported by LOGFONT is not correct. For e.g. if the height is -1, the actual font size is 2 on my XP machine. This causes the DeriveFontWithHeight test to fail. BUG=442010 R=msw Review URL: https://codereview.chromium.org/849043002 Cr-Commit-Position: refs/heads/master@{#311594} --- ui/gfx/font_list_unittest.cc | 12 ++--- ui/gfx/platform_font_win.cc | 100 ++++++++++++++++------------------- ui/gfx/platform_font_win.h | 13 ++--- ui/gfx/platform_font_win_unittest.cc | 57 +++----------------- 4 files changed, 63 insertions(+), 119 deletions(-) diff --git a/ui/gfx/font_list_unittest.cc b/ui/gfx/font_list_unittest.cc index 6dcbb855d083..826eefa5e208 100644 --- a/ui/gfx/font_list_unittest.cc +++ b/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 ascent and descent. + // for the baseline (ascent) and height. Font font2("Symbol", 16); ASSERT_EQ("symbol", base::StringToLowerASCII(font2.GetActualFontNameForTesting())); EXPECT_NE(font1.GetBaseline(), font2.GetBaseline()); - EXPECT_NE(font1.GetHeight() - font1.GetBaseline(), - font2.GetHeight() - font2.GetBaseline()); + // TODO(ananta): Find a size and font pair with reliably distinct descents. + EXPECT_NE(font1.GetHeight(), font2.GetHeight()); std::vector 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) { diff --git a/ui/gfx/platform_font_win.cc b/ui/gfx/platform_font_win.cc index 5e9d2537e5a1..f2b809b84b93 100644 --- a/ui/gfx/platform_font_win.cc +++ b/ui/gfx/platform_font_win.cc @@ -211,33 +211,47 @@ 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); - return DeriveWithCorrectedSize(hfont); + 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; + + // Iterate to find the largest font with a height <= |height|. + while ((font.GetHeight() > height) && + (font.GetFontSize() >= min_font_size)) { + ran_shrink_loop = true; + Font derived_font = font.Derive(-1, style); + // Break the loop if the derived font is too small or hasn't shrunk at all. + if ((derived_font.GetFontSize() < min_font_size) || + ((derived_font.GetFontSize() == font.GetFontSize()) && + (derived_font.GetHeight() == font.GetHeight()))) + break; + font = derived_font; + } + + while ((!ran_shrink_loop && font.GetHeight() <= height) || + (font.GetFontSize() < min_font_size)) { + Font derived_font = font.Derive(1, style); + // Break the loop if the derived font is too large or hasn't grown at all. + if (((derived_font.GetHeight() > height) && + (font.GetFontSize() >= min_font_size)) || + ((derived_font.GetFontSize() == font.GetFontSize()) && + (derived_font.GetHeight() == font.GetHeight()))) + break; + font = derived_font; + } + return font; } //////////////////////////////////////////////////////////////////////////////// @@ -423,37 +437,6 @@ 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 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 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, @@ -518,8 +501,7 @@ 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 + skia_metrics.fLeading); + const int height = std::ceil(skia_metrics.fDescent - skia_metrics.fAscent); const int baseline = std::max(1, std::ceil(-skia_metrics.fAscent)); const int cap_height = std::ceil(paint.getTextSize() * static_cast(dwrite_font_metrics.capHeight) / @@ -572,8 +554,16 @@ PlatformFontWin::HFontRef::HFontRef(HFONT hfont, LOGFONT font_info; GetObject(hfont_, sizeof(LOGFONT), &font_info); font_name_ = base::UTF16ToUTF8(base::string16(font_info.lfFaceName)); - if (font_info.lfHeight < 0) - requested_font_size_ = -font_info.lfHeight; + + // Retrieve the font size from the GetTextMetrics API instead of referencing + // it from the LOGFONT structure. This is because the height as reported by + // the LOGFONT structure is not always correct. For small fonts with size 1 + // the LOGFONT structure reports the height as -1, while the actual font size + // is different. (2 on my XP machine). + base::win::ScopedGetDC screen_dc(NULL); + TEXTMETRIC font_metrics = {0}; + PlatformFontWin::GetTextMetricsForFont(screen_dc, hfont_, &font_metrics); + requested_font_size_ = font_metrics.tmHeight - font_metrics.tmInternalLeading; } int PlatformFontWin::HFontRef::GetDluBaseX() { diff --git a/ui/gfx/platform_font_win.h b/ui/gfx/platform_font_win.h index 11f94d4d2ee8..6f7bc21b2081 100644 --- a/ui/gfx/platform_font_win.h +++ b/ui/gfx/platform_font_win.h @@ -49,11 +49,10 @@ 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 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). + // 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|. Font DeriveFontWithHeight(int height, int style); // Overridden from PlatformFont: @@ -172,10 +171,6 @@ 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. diff --git a/ui/gfx/platform_font_win_unittest.cc b/ui/gfx/platform_font_win_unittest.cc index dc4ac5eb54bf..c582a3292af0 100644 --- a/ui/gfx/platform_font_win_unittest.cc +++ b/ui/gfx/platform_font_win_unittest.cc @@ -17,72 +17,32 @@ 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(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); + 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(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); + 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(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( @@ -197,5 +157,4 @@ TEST(PlatformFontWinTest, Metrics_SkiaVersusGDI) { } } - } // namespace gfx -- 2.11.4.GIT