Fix RenderTextWin CJK font linking size on Windows XP.

On Windows XP, the new font must be picked such that the font height, not the font size, is the same as the previous font.

Add a PlatformFontWin::DeriveFontWithHeight() function that provided the functionality needed for the above and modifies PlatformFontWin to support the above by not assuming LOGFONT.lfHeight is always negative. Instead, it now gets the font size from TEXTMETRIC.

One side effect of this change is that the GetFontSize() will now return the actual font size that will be used, rather than the input parameter when creating the font. A test in label_unittest.cc depended on this and is updated as part of this CL.

BUG=122143, 105550
TEST=Run Chrome on English Windows XP with CJK languages
installed. Go to a website with a Chinese title. The size
of the Chinese text in the tab title should be the same
as in Chrome 19 (which has use_canvas_skia=0 setting).
Also, existing unit tests and newly-added platform_font_win_unittest.cc.

Review URL: http://codereview.chromium.org/10228009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@134600 0039d316-1c4b-4281-b951-d872f2087c98

ui/gfx/platform_font_win.cc

@@ -27,6 +27,14 @@ namespace {
 // font is bold.
 const int kTextMetricWeightBold = 700;
 
+// Returns the minimum font size, using the minimum size callback, if set.
+int GetMinimumFontSize() {
+  int min_font_size = 0;
+  if (gfx::PlatformFontWin::get_minimum_font_size_callback)
+    min_font_size = gfx::PlatformFontWin::get_minimum_font_size_callback();
+  return min_font_size;
+}
+
 // Returns either minimum font allowed for a current locale or
 // lf_height + size_delta value.
 int AdjustFontSize(int lf_height, int size_delta) {
@@ -35,9 +43,7 @@ int AdjustFontSize(int lf_height, int size_delta) {
   } else {
     lf_height += size_delta;
   }
-  int min_font_size = 0;
-  if (gfx::PlatformFontWin::get_minimum_font_size_callback)
-    min_font_size = gfx::PlatformFontWin::get_minimum_font_size_callback();
+  const int min_font_size = GetMinimumFontSize();
   // Make sure lf_height is not smaller than allowed min font size for current
   // locale.
   if (abs(lf_height) < min_font_size) {
@@ -47,6 +53,13 @@ int AdjustFontSize(int lf_height, int size_delta) {
   }
 }
 
+// Sets style properties on |font_info| based on |font_style|.
+void SetLogFontStyle(int font_style, LOGFONT* font_info) {
+  font_info->lfUnderline = (font_style & gfx::Font::UNDERLINED) != 0;
+  font_info->lfItalic = (font_style & gfx::Font::ITALIC) != 0;
+  font_info->lfWeight = (font_style & gfx::Font::BOLD) ? FW_BOLD : FW_NORMAL;
+}
+
 }  // namespace
 
 namespace gfx {
@@ -75,17 +88,40 @@ PlatformFontWin::PlatformFontWin(const std::string& font_name,
   InitWithFontNameAndSize(font_name, font_size);
 }
 
+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);
+    while (font.GetHeight() > height && font.GetFontSize() != min_font_size) {
+      font = font.DeriveFont(-1, style);
+    }
+    return font;
+  }
+
+  LOGFONT font_info;
+  GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
+  font_info.lfHeight = height;
+  SetLogFontStyle(style, &font_info);
+
+  HFONT hfont = CreateFontIndirect(&font_info);
+  return Font(new PlatformFontWin(CreateHFontRef(hfont)));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // PlatformFontWin, PlatformFont implementation:
 
 Font PlatformFontWin::DeriveFont(int size_delta, int style) const {
   LOGFONT font_info;
   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
-  font_info.lfHeight = AdjustFontSize(font_info.lfHeight, size_delta);
-  font_info.lfUnderline =
-      ((style & gfx::Font::UNDERLINED) == gfx::Font::UNDERLINED);
-  font_info.lfItalic = ((style & gfx::Font::ITALIC) == gfx::Font::ITALIC);
-  font_info.lfWeight = (style & gfx::Font::BOLD) ? FW_BOLD : FW_NORMAL;
+  const int requested_font_size = font_ref_->requested_font_size();
+  font_info.lfHeight = AdjustFontSize(-requested_font_size, size_delta);
+  SetLogFontStyle(style, &font_info);
 
   HFONT hfont = CreateFontIndirect(&font_info);
   return Font(new PlatformFontWin(CreateHFontRef(hfont)));
@@ -191,10 +227,11 @@ PlatformFontWin::HFontRef* PlatformFontWin::CreateHFontRef(HFONT font) {
     GetTextMetrics(screen_dc, &font_metrics);
   }
 
-  const int height = std::max(1, static_cast<int>(font_metrics.tmHeight));
-  const int baseline = std::max(1, static_cast<int>(font_metrics.tmAscent));
-  const int ave_char_width =
-      std::max(1, static_cast<int>(font_metrics.tmAveCharWidth));
+  const int height = std::max<int>(1, font_metrics.tmHeight);
+  const int baseline = std::max<int>(1, font_metrics.tmAscent);
+  const int ave_char_width = std::max<int>(1, font_metrics.tmAveCharWidth);
+  const int font_size =
+      std::max<int>(1, font_metrics.tmHeight - font_metrics.tmInternalLeading);
   int style = 0;
   if (font_metrics.tmItalic)
     style |= Font::ITALIC;
@@ -203,7 +240,7 @@ PlatformFontWin::HFontRef* PlatformFontWin::CreateHFontRef(HFONT font) {
   if (font_metrics.tmWeight >= kTextMetricWeightBold)
     style |= Font::BOLD;
 
-  return new HFontRef(font, height, baseline, ave_char_width, style);
+  return new HFontRef(font, font_size, height, baseline, ave_char_width, style);
 }
 
 PlatformFontWin::PlatformFontWin(HFontRef* hfont_ref) : font_ref_(hfont_ref) {
@@ -213,23 +250,26 @@ PlatformFontWin::PlatformFontWin(HFontRef* hfont_ref) : font_ref_(hfont_ref) {
 // PlatformFontWin::HFontRef:
 
 PlatformFontWin::HFontRef::HFontRef(HFONT hfont,
+         int font_size,
          int height,
          int baseline,
          int ave_char_width,
          int style)
     : hfont_(hfont),
+      font_size_(font_size),
       height_(height),
       baseline_(baseline),
       ave_char_width_(ave_char_width),
       style_(style),
-      dlu_base_x_(-1) {
+      dlu_base_x_(-1),
+      requested_font_size_(font_size) {
   DLOG_ASSERT(hfont);
 
   LOGFONT font_info;
   GetObject(hfont_, sizeof(LOGFONT), &font_info);
   font_name_ = UTF16ToUTF8(string16(font_info.lfFaceName));
-  DCHECK_LT(font_info.lfHeight, 0);
-  font_size_ = -font_info.lfHeight;
+  if (font_info.lfHeight < 0)
+    requested_font_size_ = -font_info.lfHeight;
 }
 
 int PlatformFontWin::HFontRef::GetDluBaseX() {

ui/gfx/platform_font_win.h

@@ -47,6 +47,13 @@ class UI_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).
+  Font DeriveFontWithHeight(int height, int style);
+
   // Overridden from PlatformFont:
   virtual Font DeriveFont(int size_delta, int style) const OVERRIDE;
   virtual int GetHeight() const OVERRIDE;
@@ -75,6 +82,7 @@ class UI_EXPORT PlatformFontWin : public PlatformFont {
     // This constructor takes control of the HFONT, and will delete it when
     // the HFontRef is deleted.
     HFontRef(HFONT hfont,
+             int font_size,
              int height,
              int baseline,
              int ave_char_width,
@@ -88,6 +96,7 @@ class UI_EXPORT PlatformFontWin : public PlatformFont {
     int style() const { return style_; }
     const std::string& font_name() const { return font_name_; }
     int font_size() const { return font_size_; }
+    int requested_font_size() const { return requested_font_size_; }
 
     // Returns the average character width in dialog units.
     int GetDluBaseX();
@@ -98,6 +107,7 @@ class UI_EXPORT PlatformFontWin : public PlatformFont {
     ~HFontRef();
 
     const HFONT hfont_;
+    const int font_size_;
     const int height_;
     const int baseline_;
     const int ave_char_width_;
@@ -106,7 +116,12 @@ class UI_EXPORT PlatformFontWin : public PlatformFont {
     // system, with an initial value of -1 meaning it hasn't yet been queried.
     int dlu_base_x_;
     std::string font_name_;
-    int font_size_;
+
+    // If the requested font size is not possible for the font, |font_size_|
+    // will be different than |requested_font_size_|. This is stored separately
+    // so that code that increases the font size in a loop will not cause the
+    // loop to get stuck on the same size.
+    int requested_font_size_;
 
     DISALLOW_COPY_AND_ASSIGN(HFontRef);
   };

ui/gfx/platform_font_win_unittest.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/gfx/platform_font_win.h"
+
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/font.h"
+
+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.DeriveFont(1, 0);
+    while (larger_font.GetHeight() <= target_height) {
+      expected_font = larger_font;
+      larger_font = larger_font.DeriveFont(1, 0);
+    }
+  } else if (expected_font.GetHeight() > target_height) {
+    // Decrease size until height is <= |target_height|.
+    do {
+      expected_font = expected_font.DeriveFont(-1, 0);
+    } while (expected_font.GetHeight() > target_height);
+  }
+  return expected_font;
+}
+
+}  // namespace
+
+TEST(PlatformFontWinTest, DeriveFontWithHeight) {
+  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);
+    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_EQ(Font::BOLD, derived_font.GetStyle());
+
+    // Test that deriving from the new font has the expected result.
+    Font rederived_font = derived_font.DeriveFont(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());
+  }
+}
+
+// Callback function used by DeriveFontWithHeight_MinSize() below.
+static int GetMinFontSize() {
+  return 10;
+}
+
+TEST(PlatformFontWinTest, DeriveFontWithHeight_MinSize) {
+  PlatformFontWin::GetMinimumFontSizeCallback old_callback =
+    PlatformFontWin::get_minimum_font_size_callback;
+  PlatformFontWin::get_minimum_font_size_callback = &GetMinFontSize;
+
+  const Font base_font;
+  const Font min_font(base_font.GetFontName(), GetMinFontSize());
+  PlatformFontWin* platform_font =
+      static_cast<PlatformFontWin*>(base_font.platform_font());
+
+  const Font derived_font =
+      platform_font->DeriveFontWithHeight(min_font.GetHeight() - 1, 0);
+  EXPECT_EQ(min_font.GetFontSize(), derived_font.GetFontSize());
+  EXPECT_EQ(min_font.GetHeight(), derived_font.GetHeight());
+
+  PlatformFontWin::get_minimum_font_size_callback = old_callback;
+}
+
+}  // namespace gfx

ui/gfx/render_text_win.cc

@@ -13,9 +13,10 @@
 #include "base/threading/thread_restrictions.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/registry.h"
+#include "base/win/windows_version.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_smoothing_win.h"
-#include "ui/gfx/platform_font.h"
+#include "ui/gfx/platform_font_win.h"
 
 namespace {
 
@@ -116,13 +117,26 @@ void QueryLinkedFontsFromRegistry(const gfx::Font& font,
   key.Close();
 }
 
-// Changes |font| to have the specified |font_size| and |font_style| if it does
-// not already. Only considers bold and italic styles, since the underlined
-// style has no effect on glyph shaping.
-void DeriveFontIfNecessary(int font_size, int font_style, gfx::Font* font) {
+// Changes |font| to have the specified |font_size| (or |font_height| on Windows
+// XP) and |font_style| if it is not the case already. Only considers bold and
+// italic styles, since the underlined style has no effect on glyph shaping.
+void DeriveFontIfNecessary(int font_size,
+                           int font_height,
+                           int font_style,
+                           gfx::Font* font) {
   const int kStyleMask = (gfx::Font::BOLD | gfx::Font::ITALIC);
-  const int current_style = (font->GetStyle() & kStyleMask);
   const int target_style = (font_style & kStyleMask);
+
+  // On Windows XP, the font must be resized using |font_height| instead of
+  // |font_size| to match GDI behavior.
+  if (base::win::GetVersion() < base::win::VERSION_VISTA) {
+    gfx::PlatformFontWin* platform_font =
+        static_cast<gfx::PlatformFontWin*>(font->platform_font());
+    *font = platform_font->DeriveFontWithHeight(font_height, target_style);
+    return;
+  }
+
+  const int current_style = (font->GetStyle() & kStyleMask);
   const int current_size = font->GetFontSize();
   if (current_style != target_style || current_size != font_size)
     *font = font->DeriveFont(font_size - current_size, font_style);
@@ -562,7 +576,8 @@ void RenderTextWin::ItemizeLogicalText() {
     run->range.set_start(run_break);
     run->font = GetFont();
     run->font_style = style->font_style;
-    DeriveFontIfNecessary(run->font.GetFontSize(), run->font_style, &run->font);
+    DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(),
+                          run->font_style, &run->font);
     run->foreground = style->foreground;
     run->strike = style->strike;
     run->diagonal_strike = style->diagonal_strike;
@@ -746,8 +761,9 @@ void RenderTextWin::LayoutVisualText() {
 void RenderTextWin::ApplySubstituteFont(internal::TextRun* run,
                                         const Font& font) {
   const int font_size = run->font.GetFontSize();
+  const int font_height = run->font.GetHeight();
   run->font = font;
-  DeriveFontIfNecessary(font_size, run->font_style, &run->font);
+  DeriveFontIfNecessary(font_size, font_height, run->font_style, &run->font);
   ScriptFreeCache(&run->script_cache);
   SelectObject(cached_hdc_, run->font.GetNativeFont());
 }

ui/ui_unittests.gypi

@@ -109,10 +109,10 @@
         ['OS == "win"', {
           'sources': [
             'base/dragdrop/os_exchange_data_win_unittest.cc',
-            # TODO(brettw) re-enable this when the dependencies on WindowImpl are fixed!
+            'base/win/hwnd_subclass_unittest.cc',
             'gfx/icon_util_unittest.cc',
             'gfx/native_theme_win_unittest.cc',
-            'base/win/hwnd_subclass_unittest.cc',
+            'gfx/platform_font_win_unittest.cc',
           ],
           'include_dirs': [
             '../..',

ui/views/controls/label_unittest.cc

@@ -16,18 +16,19 @@ namespace views {
 // All text sizing measurements (width and height) should be greater than this.
 const int kMinTextDimension = 4;
 
-#if defined(OS_WIN)
-// Courier is failing on linux because it's non scalable.
 TEST(LabelTest, FontPropertyCourier) {
   Label label;
   std::string font_name("courier");
-  gfx::Font font(font_name, 30);
+  // Note: This test is size dependent since Courier does not support all sizes.
+  gfx::Font font(font_name, 26);
   label.SetFont(font);
   gfx::Font font_used = label.font();
+#if defined(OS_WIN)
+  // On Linux, this results in "Sans" instead of "courier".
   EXPECT_EQ(font_name, font_used.GetFontName());
-  EXPECT_EQ(30, font_used.GetFontSize());
-}
 #endif
+  EXPECT_EQ(26, font_used.GetFontSize());
+}
 
 TEST(LabelTest, FontPropertyArial) {
   Label label;