Fix CJK font linking size on Windows XP.

ui/gfx/render_text_win.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/render_text_win.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #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"
 
 namespace {
 
 // The maximum supported number of Uniscribe runs; a SCRIPT_ITEM is 8 bytes.
 // TODO(msw): Review memory use/failure? Max string length? Alternate approach?
 const int kGuessItems = 100;
 const int kMaxItems = 10000;
@@ skipping 83 matching lines @@
     if ((index != string16::npos) && (index + 1 != values[i].length())) {
       const std::string linked_name = UTF16ToUTF8(values[i].substr(index + 1));
       const gfx::Font linked_font(linked_name, font.GetFontSize());
       linked_fonts->push_back(linked_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) {
+// 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);
 }
 
 }  // namespace
 
 namespace gfx {
 
 namespace internal {
 
@@ skipping 418 matching lines @@
   // TODO(msw): Only break for font changes, not color etc. See TextRun comment.
   StyleRanges styles(style_ranges());
   ApplyCompositionAndSelectionStyles(&styles);
   StyleRanges::const_iterator style = styles.begin();
   SCRIPT_ITEM* script_item = &script_items[0];
   for (int run_break = 0; run_break < text_length;) {
     internal::TextRun* run = new internal::TextRun();
     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;
     run->underline = style->underline;
     run->script_analysis = script_item->a;
 
     // Find the range end and advance the structures as needed.
     int script_item_end = (script_item + 1)->iCharPos;
     int style_range_end = style->range.end();
     run_break = std::min(script_item_end, style_range_end);
@@ skipping 163 matching lines @@
     const ABC& abc = run->abc_widths;
     run->width = abc.abcA + abc.abcB + abc.abcC;
     preceding_run_widths += run->width;
   }
   string_size_.set_width(preceding_run_widths);
 }
 
 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());
 }
 
 const std::vector<Font>* RenderTextWin::GetLinkedFonts(const Font& font) const {
   const std::string& font_name = font.GetFontName();
   std::map<std::string, std::vector<Font> >::const_iterator it =
       cached_linked_fonts_.find(font_name);
   if (it != cached_linked_fonts_.end())
     return &it->second;
@@ skipping 36 matching lines @@
     internal::TextRun* run) {
   size_t caret = IndexOfAdjacentGrapheme(run->range.end(), CURSOR_BACKWARD);
   return SelectionModel(caret, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateRenderText() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx