Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10228009:
Fix CJK font linking size on Windows XP. (Closed)
|
Created:
8 years, 8 months ago by Alexei Svitkine (slow) Modified:
8 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix 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.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134600
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #
Total comments: 29
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Messages
Total messages: 29 (0 generated)
I'm so sorry... http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:120: // Chooses the largest font size for |font| such that its height does not exceed Add a DCHECK at the bottom of the function to ensure the new font height doesn't exceed |font_height|. http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:122: void DeriveFontWithHeight(int font_height, int font_style, gfx::Font* font) { This probably needs test coverage. http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:126: } while (font->GetHeight() > font_height); 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? http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:129: while (larger_font.GetHeight() < font_height) { Shouldn't this be '<=' ? Otherwiset we'll miss when larger_font's height is equal to the target height. http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:144: // |font_size| to match the GDI behavior. grammar nit: nix 'the' -> "match GDI behavior"
http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:122: void DeriveFontWithHeight(int font_height, int font_style, gfx::Font* font) { On 2012/04/25 22:07:31, msw wrote: > This probably needs test coverage. 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? http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:126: } while (font->GetHeight() > font_height); On 2012/04/25 22:07:31, msw wrote: > 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? 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. http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:129: while (larger_font.GetHeight() < font_height) { On 2012/04/25 22:07:31, msw wrote: > Shouldn't this be '<=' ? Otherwiset we'll miss when larger_font's height is > equal to the target height. You're absolutely right! I had this correct before but lost it when cleaning up my code. :\
http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:122: void DeriveFontWithHeight(int font_height, int font_style, gfx::Font* font) { On 2012/04/25 23:19:31, Alexei Svitkine wrote: > On 2012/04/25 22:07:31, msw wrote: > > This probably needs test coverage. > > 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? 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. http://codereview.chromium.org/10228009/diff/1002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:126: } while (font->GetHeight() > font_height); On 2012/04/25 23:19:31, Alexei Svitkine wrote: > On 2012/04/25 22:07:31, msw wrote: > > 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? > > 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. 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.
Looks like Win32's CreateFontIndirect() can create a font based on a given height, which makes my looping function not necessary. I'll rework this CL to expose that functionality through PlatformFontWin and make this change use that function.
Nice find!
Updated. PTAL. +sky for PlatformFontWin and ui/ui_unittests.gypi changes.
LGTM
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. This caused a test failure in label_unittest.cc that had an assumption about this. That test was already disabled on non-Windows. I've updated the test to use a font size that passes and removed the ifdef that disabled it on non-Windows platforms.
Alexei, you really rock! http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:53: ((font_style & gfx::Font::UNDERLINED) == gfx::Font::UNDERLINED); Just set the bool to the bit-wise & value (kinda like Bold). Otherwise, comparing against 0 should fit on the line above. Please make italic consistent if you do make a change here. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:213: const int height = std::max<int>(1, font_metrics.tmHeight); nice :) http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:226: return new HFontRef(font, font_size, height, baseline, ave_char_width, style); Seems like HFontRef's ctor should just take (HFONT font, TEXTMETRIC font_metrics, int style). Perhaps that's not in the scope of this CL, but this is its only callsite... http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.h File ui/gfx/platform_font_win.h (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.h... ui/gfx/platform_font_win.h:53: // its height is less than or equal to |height|. It's unclear (from the comment) why the resulting height might not exactly match the argument height, is it worth mentioning why that's the case? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... File ui/gfx/platform_font_win_unittest.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:14: TEST(PlatformFontWinTest, DeriveFontWithHeight_SameHeight) { optional: It'd be awesome if we could combine these tests and adjust the DeriveFont size increment value and the loop condition based on the iterator value (where we iterate over values base_font.GetHeight()-10 through base_font.GetHeight()+10). That might be a little tricky, so I don't blame you if you tell me to bug off :) http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:38: expected_font = expected_font.DeriveFont(-1, 0); This is slightly un-intuitive... can you explain in a comment that "// Get the expected font by incrementally deriving [smaller|larger] fonts. Use this value to check the result of DeriveFontWithHeight()." or similar (same for test below). http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:39: } while (expected_font.GetHeight() > target_height); Should we [EXPECT|ASSERT]_LE(expected_font.GetHeight(), target_height); here? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:45: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); Also EXPECT_LE(expected_font.GetHeight(), target_height); ? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:50: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); Also EXPECT_LE(expected_font.GetHeight(), target_height); ? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:66: } Should we [EXPECT|ASSERT]_LE(expected_font.GetHeight(), target_height); here? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:72: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); Also EXPECT_LE(expected_font.GetHeight(), target_height); ? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:77: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); Also EXPECT_LE(expected_font.GetHeight(), target_height); ? http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:123: void DeriveFontIfNecessary(int font_size, It would be nice to have this function take a const Font& font for the target size/height/style, then using the size/height per platform is more of an impl detail. http://codereview.chromium.org/10228009/diff/12003/ui/views/controls/label_un... File ui/views/controls/label_unittest.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/views/controls/label_un... ui/views/controls/label_unittest.cc:22: gfx::Font font(font_name, 26); Is this test fragile wrt the font size? If so, please comment.
http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:53: ((font_style & gfx::Font::UNDERLINED) == gfx::Font::UNDERLINED); On 2012/04/26 21:04:42, msw wrote: > Just set the bool to the bit-wise & value (kinda like Bold). > Otherwise, comparing against 0 should fit on the line above. > Please make italic consistent if you do make a change here. Done. I used != 0 since I don't know if passing values other than 0 and 1 would cause a problem for CreateFontIndirect(). (Note: The previous code was being extra safe - since for example if one of those flags was defined to be more than one bit, comparing against zero is not correct. But that's not the case and unlikely to change, so I've simplified it as you suggest). http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:226: return new HFontRef(font, font_size, height, baseline, ave_char_width, style); On 2012/04/26 21:04:42, msw wrote: > Seems like HFontRef's ctor should just take (HFONT font, TEXTMETRIC > font_metrics, int style). Perhaps that's not in the scope of this CL, but this > is its only callsite... I agree, but let's leave it for another CL. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.h File ui/gfx/platform_font_win.h (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.h... ui/gfx/platform_font_win.h:53: // its height is less than or equal to |height|. On 2012/04/26 21:04:42, msw wrote: > It's unclear (from the comment) why the resulting height might not exactly match > the argument height, is it worth mentioning why that's the case? I've expanded the comment to explain that. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... File ui/gfx/platform_font_win_unittest.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:14: TEST(PlatformFontWinTest, DeriveFontWithHeight_SameHeight) { On 2012/04/26 21:04:42, msw wrote: > optional: It'd be awesome if we could combine these tests and adjust the > DeriveFont size increment value and the loop condition based on the iterator > value (where we iterate over values base_font.GetHeight()-10 through > base_font.GetHeight()+10). That might be a little tricky, so I don't blame you > if you tell me to bug off :) Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:38: expected_font = expected_font.DeriveFont(-1, 0); On 2012/04/26 21:04:42, msw wrote: > This is slightly un-intuitive... can you explain in a comment that "// Get the > expected font by incrementally deriving [smaller|larger] fonts. Use this value > to check the result of DeriveFontWithHeight()." or similar (same for test > below). Added comments. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:39: } while (expected_font.GetHeight() > target_height); On 2012/04/26 21:04:42, msw wrote: > Should we [EXPECT|ASSERT]_LE(expected_font.GetHeight(), target_height); here? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:45: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); On 2012/04/26 21:04:42, msw wrote: > Also EXPECT_LE(expected_font.GetHeight(), target_height); ? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:50: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); On 2012/04/26 21:04:42, msw wrote: > Also EXPECT_LE(expected_font.GetHeight(), target_height); ? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:66: } On 2012/04/26 21:04:42, msw wrote: > Should we [EXPECT|ASSERT]_LE(expected_font.GetHeight(), target_height); here? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:72: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); On 2012/04/26 21:04:42, msw wrote: > Also EXPECT_LE(expected_font.GetHeight(), target_height); ? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:77: EXPECT_EQ(expected_font.GetFontSize(), derived_font.GetFontSize()); On 2012/04/26 21:04:42, msw wrote: > Also EXPECT_LE(expected_font.GetHeight(), target_height); ? Done. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:123: void DeriveFontIfNecessary(int font_size, On 2012/04/26 21:04:42, msw wrote: > It would be nice to have this function take a const Font& font for the target > size/height/style, then using the size/height per platform is more of an impl > detail. That won't work for the call site that applies the style to the font, the style would have to come from a parameter and not the const font. It would also be a bit confusing, because now it would take two fonts as parameters - one for the reference size, and the other for the other characteristics (e.g. font name) and the output parameter. Right now, it's clear that the single font param is the font that may be changed according to the other parameters. It's much less confusing than what you're proposing. http://codereview.chromium.org/10228009/diff/12003/ui/views/controls/label_un... File ui/views/controls/label_unittest.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/views/controls/label_un... ui/views/controls/label_unittest.cc:22: gfx::Font font(font_name, 26); On 2012/04/26 21:04:42, msw wrote: > Is this test fragile wrt the font size? If so, please comment. Done. It also turns out that the Linux failure was not just because of the size - the font name also gets mapped to "Sans" instead of keeping "Courier", so I've reverted the ifdef to make it Windows specific. :\
http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:226: return new HFontRef(font, font_size, height, baseline, ave_char_width, style); > I agree, but let's leave it for another CL. SGTM. http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/10228009/diff/12003/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:123: void DeriveFontIfNecessary(int font_size, > It's much less confusing than what you're proposing. Fair enough, I agree :), the function comment suffices to explain why it takes a height and a size, which is the minor confusing implementation detail i was trying to encapsulate. http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... File ui/gfx/platform_font_win_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:55: TEST(PlatformFontWinTest, DeriveFontWithHeight) { Awesome, and now this covers the case of DeriveFontWithHeight_SameHeight? You can probably remove that test. http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... File ui/views/controls/label_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... ui/views/controls/label_unittest.cc:19: #if defined(OS_WIN) Can't we just make the font name check Win specific (with a comment) and still check the size on Linux?
http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... File ui/gfx/platform_font_win_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:55: TEST(PlatformFontWinTest, DeriveFontWithHeight) { On 2012/04/26 22:14:13, msw wrote: > Awesome, and now this covers the case of DeriveFontWithHeight_SameHeight? You > can probably remove that test. Good point. http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... File ui/views/controls/label_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... ui/views/controls/label_unittest.cc:19: #if defined(OS_WIN) On 2012/04/26 22:14:13, msw wrote: > Can't we just make the font name check Win specific (with a comment) and still > check the size on Linux? Yes, I think so.
LGTM (with suggested comments). Send mail again when you push the new Patch Set(s), but you don't have to wait for my re-review. Thanks!
http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... File ui/gfx/platform_font_win_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/gfx/platform_font_win_u... ui/gfx/platform_font_win_unittest.cc:55: TEST(PlatformFontWinTest, DeriveFontWithHeight) { On 2012/04/26 22:14:13, msw wrote: > Awesome, and now this covers the case of DeriveFontWithHeight_SameHeight? You > can probably remove that test. Done. http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... File ui/views/controls/label_unittest.cc (right): http://codereview.chromium.org/10228009/diff/14006/ui/views/controls/label_un... ui/views/controls/label_unittest.cc:19: #if defined(OS_WIN) On 2012/04/26 22:14:13, msw wrote: > Can't we just make the font name check Win specific (with a comment) and still > check the size on Linux? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10228009/24001
Try job failure for 10228009-24001 (retry) on win_rel for step "gfx_unittests". It's a second try, previously, step "gfx_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Hmm. Looks like I had a non-default font size set, for which the new test I've added was passing. Reverting to the default font size causes the test to fail for some sizes. It seems like, contrary to Microsoft's API docs, CreateFontIndirect() doesn't always return the maximum size for the given height. For example, for height 12, the test code finds an expected font size of 10 with that height, but CreateFontIndirect() returns a font of size 9. When it fails, it's always lower by 1. Sigh.
On 2012/04/27 21:02:21, Alexei Svitkine wrote: > Hmm. Looks like I had a non-default font size set, for which the new test I've > added was passing. > > Reverting to the default font size causes the test to fail for some sizes. It > seems like, contrary to Microsoft's API docs, CreateFontIndirect() doesn't > always return the maximum size for the given height. > > For example, for height 12, the test code finds an expected font size of 10 with > that height, but CreateFontIndirect() returns a font of size 9. When it fails, > it's always lower by 1. Sigh. But the test passes on XP, which is only where we use this... Hmm.
On 2012/04/27 21:15:14, Alexei Svitkine wrote: > On 2012/04/27 21:02:21, Alexei Svitkine wrote: > > Hmm. Looks like I had a non-default font size set, for which the new test I've > > added was passing. > > > > Reverting to the default font size causes the test to fail for some sizes. It > > seems like, contrary to Microsoft's API docs, CreateFontIndirect() doesn't > > always return the maximum size for the given height. > > > > For example, for height 12, the test code finds an expected font size of 10 > with > > that height, but CreateFontIndirect() returns a font of size 9. When it fails, > > it's always lower by 1. Sigh. > > But the test passes on XP, which is only where we use this... Hmm. So the underlying impl of CreateFontIndirect changed after xp? Does this mean that it'll still do the right thing on all versions?
On Fri, Apr 27, 2012 at 6:01 PM, <msw@chromium.org> wrote: > On 2012/04/27 21:15:14, Alexei Svitkine wrote: >> >> On 2012/04/27 21:02:21, Alexei Svitkine wrote: >> > Hmm. Looks like I had a non-default font size set, for which the new >> > test > > I've >> >> > added was passing. >> > >> > Reverting to the default font size causes the test to fail for some >> > sizes. > > It >> >> > seems like, contrary to Microsoft's API docs, CreateFontIndirect() >> > doesn't >> > always return the maximum size for the given height. >> > >> > For example, for height 12, the test code finds an expected font size of >> > 10 >> with >> > that height, but CreateFontIndirect() returns a font of size 9. When it > > fails, >> >> > it's always lower by 1. Sigh. > > >> But the test passes on XP, which is only where we use this... Hmm. > > > So the underlying impl of CreateFontIndirect changed after xp? Perhaps. Or the problem may be specific to the fonts that come by default. > Does this mean that it'll still do the right thing on all versions? I think the change does the right thing in terms of the high level problem it's addressing, but the unit test is testing a superset of functionality needed by the change. After investigating this a bit more yesterday, I also think there's another problem here. If you call DeriveFont() on a font created via DeriveFontWithHeight(), then the size delta will get applied to the height and not the size. That's not correct - I'll take a look at fixing it Monday and adding a test. I still haven't decided what's the best way to fix the test failure, though. We can either relax the contract of DeriveFontWithHeight() to not guarantee that the size is maximized when the resulting height is equal (this would mean it would have to be tested differently), or we can add additional code to DeriveFontWithHeight() to workaround CreateFontIndirect() not always returning the maximum size with the given height. > > http://codereview.chromium.org/10228009/
I've updated the CL. It now passes the test in all cases and fixes the problem with the semantics of DeriveFont() being called on a font derived with DeriveFontWithHeight(), for which I've also added testing. I've fixed the issue by making DeriveFontWithHeight() iterate to find the right height in the case of when the requested height is lower than the original font's height. I think this is acceptable here, because for the specific CJK issue, the code ends up always increasing the height, so that branch doesn't get taken in practice. Mike, can you take another look?
http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:92: if (GetHeight() > height) { Should this heed the min locale height/size like AdjustFontSize? http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:93: Font font = this->DeriveFont(-1, style); Is there a name conflict or can you nix "this->"? http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h File ui/gfx/platform_font_win.h (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h... ui/gfx/platform_font_win.h:124: int requested_font_size_; If this condition is limited to tests, and it's known behavior that the font size may not match the requested size, then can the test account for the that, instead of adding this member? http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi#newco... ui/ui_unittests.gypi:112: 'base/win/hwnd_subclass_unittest.cc', What's this? Bad merge?
http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:92: if (GetHeight() > height) { On 2012/04/30 17:40:01, msw wrote: > Should this heed the min locale height/size like AdjustFontSize? No, since DeriveFont() goes through AdjustFontSize(). http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:93: Font font = this->DeriveFont(-1, style); On 2012/04/30 17:40:01, msw wrote: > Is there a name conflict or can you nix "this->"? Done. http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h File ui/gfx/platform_font_win.h (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h... ui/gfx/platform_font_win.h:124: int requested_font_size_; On 2012/04/30 17:40:01, msw wrote: > If this condition is limited to tests, and it's known behavior that the font > size may not match the requested size, then can the test account for the that, > instead of adding this member? It's not just test code, actually. I'm using it in DeriveFontWithHeight() too, in the path that doesn't use CreateFontIndirect(). I removed the reference to test code in the description. http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi#newco... ui/ui_unittests.gypi:112: 'base/win/hwnd_subclass_unittest.cc', On 2012/04/30 17:40:01, msw wrote: > What's this? Bad merge? No, I moved this up from below to alphabetize them. (And that entry was added after my last patch, so if you're viewing diffs between Patch 6 and 7, it may look odd. Look at the diff with the base file.)
http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:92: if (GetHeight() > height) { On 2012/04/30 17:49:14, Alexei Svitkine wrote: > On 2012/04/30 17:40:01, msw wrote: > > Should this heed the min locale height/size like AdjustFontSize? > > No, since DeriveFont() goes through AdjustFontSize(). So an arg |height| < min will cause an infinite while loop since |font.GetHeight()| will always exceed |height|? Please guard against this and add a test case. http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h File ui/gfx/platform_font_win.h (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.h... ui/gfx/platform_font_win.h:124: int requested_font_size_; On 2012/04/30 17:49:14, Alexei Svitkine wrote: > On 2012/04/30 17:40:01, msw wrote: > > If this condition is limited to tests, and it's known behavior that the font > > size may not match the requested size, then can the test account for the that, > > instead of adding this member? > > It's not just test code, actually. I'm using it in DeriveFontWithHeight() too, > in the path that doesn't use CreateFontIndirect(). I removed the reference to > test code in the description. Oh wow, it took me a minute to fully grok why this is needed; it's a sad state of affairs, but I don't see any clear alternative. http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi File ui/ui_unittests.gypi (right): http://codereview.chromium.org/10228009/diff/46005/ui/ui_unittests.gypi#newco... ui/ui_unittests.gypi:112: 'base/win/hwnd_subclass_unittest.cc', On 2012/04/30 17:49:14, Alexei Svitkine wrote: > On 2012/04/30 17:40:01, msw wrote: > > What's this? Bad merge? > > No, I moved this up from below to alphabetize them. (And that entry was added > after my last patch, so if you're viewing diffs between Patch 6 and 7, it may > look odd. Look at the diff with the base file.) Got it, thanks!
http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/46005/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:92: if (GetHeight() > height) { On 2012/04/30 18:55:39, msw wrote: > On 2012/04/30 17:49:14, Alexei Svitkine wrote: > > On 2012/04/30 17:40:01, msw wrote: > > > Should this heed the min locale height/size like AdjustFontSize? > > > > No, since DeriveFont() goes through AdjustFontSize(). > > So an arg |height| < min will cause an infinite while loop since > |font.GetHeight()| will always exceed |height|? Please guard against this and > add a test case. Good catch. Fixed and added test.
LGTM with optional nit. http://codereview.chromium.org/10228009/diff/35007/ui/gfx/platform_font_win.cc File ui/gfx/platform_font_win.cc (right): http://codereview.chromium.org/10228009/diff/35007/ui/gfx/platform_font_win.c... ui/gfx/platform_font_win.cc:101: while (font.GetHeight() > height && font.GetFontSize() != min_font_size) { optionally: font.GetFontSize() >= min_font_size ?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10228009/35007
Change committed as 134600 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
