Fix CJK font linking size on Windows XP.

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
===================================================================
--- ui/gfx/render_text_win.cc	(revision 133692)
+++ ui/gfx/render_text_win.cc	(working copy)
@@ -13,6 +13,7 @@
 #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"
@@ -116,14 +117,41 @@
   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) {
+// Chooses the largest font size for |font| such that its height does not exceed

[msw, 2012/04/25 22:07:31]

Add a DCHECK at the bottom of the function to ensure the new font height doesn't
exceed |font_height|.

+// |font_height|. |font_style| is used for the style when deriving the font.
+void DeriveFontWithHeight(int font_height, int font_style, gfx::Font* font) {

[msw, 2012/04/25 22:07:31]

This probably needs test coverage.

[Alexei Svitkine (slow), 2012/04/25 23:19:31]

Do you mean the function itself, or the whole RenderText path that ends up
calling this? Unfortunately, our XP bots don't have consistent CJK support - so
we can't really test the RenderText path.

As for the function itself, it would be pretty straight forward to write a unit
test for it, except for the fact it's in an anonymous namespace. One option is
to move it to a member of RenderTextWin and test it from a friend class.

What do you think?

[msw, 2012/04/25 23:35:41]

I meant the CJK rendertext codepath, very sad that that's not feasible because
of XP try/buildbots :'( are you sure it's not just a deterministic but crappy
level of support (as opposed to non-deterministic level of support)? What about
using RenderTextWin::ApplySubstituteFont as an entry point?

If you can't test a wider codepath easily, I guess this wonky behavior still
deserves testing and *sigh* should move to a class function.

+  if (font->GetHeight() > font_height) {
+    do {
+      *font = font->DeriveFont(-1, font_style);
+    } while (font->GetHeight() > font_height);

[msw, 2012/04/25 22:07:31]

Only code trying to emulate MS APIs could be this awful.
How expensive is each DeriveFont? Can we initially derive a decent guess to
reduce iteration?

[Alexei Svitkine (slow), 2012/04/25 23:19:31]

DeriveFont() is not very expensive anymore (but not free, either). It used to be
very bad prior to a change I made, but that's no longer the case.

I'll take a look if there's a good value for an initial guess. I'll have to
check what sizes this ends up mapping to for the various CJK languages. Note:
This is also affected by the initial font size, which (for most of the UI) comes
from the user's settings in Windows. Although, probably it's okay to make a
guess that corresponds to the default settings. It would make this function more
complex, though.

[msw, 2012/04/25 23:35:41]

Yeah, I'm dubious that it's worth your time/effort and the additional complexity
to make a guess of unknown value. Thankfully we/users can check the impact via
chrome://tracing/ down the line and adjust later if needed.

+  } else if (font->GetHeight() < font_height) {
+    gfx::Font larger_font = font->DeriveFont(1, font_style);
+    while (larger_font.GetHeight() < font_height) {

[msw, 2012/04/25 22:07:31]

Shouldn't this be '<=' ? Otherwiset we'll miss when larger_font's height is
equal to the target height.

[Alexei Svitkine (slow), 2012/04/25 23:19:31]

You're absolutely right! I had this correct before but lost it when cleaning up
my code. :\

+      *font = larger_font;
+      larger_font = larger_font.DeriveFont(1, font_style);
+    }
+  }
+}
+
+// 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) {
+  // On Windows XP, the font must be resized using |font_height| instead of
+  // |font_size| to match the GDI behavior.

[msw, 2012/04/25 22:07:31]

grammar nit: nix 'the' -> "match GDI behavior"

+  if (base::win::GetVersion() < base::win::VERSION_VISTA) {
+    DeriveFontWithHeight(font_height, font_style, font);
+    font_size = font->GetFontSize();
+  }
+
   const int kStyleMask = (gfx::Font::BOLD | gfx::Font::ITALIC);
   const int current_style = (font->GetStyle() & kStyleMask);
   const int target_style = (font_style & 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 +590,8 @@
     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 +775,9 @@
 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());
 }