PlatformFontWin::DeriveFontWithHeight insensitive to base font's height

PlatformFontWin::DeriveFontWithHeight insensitive to base font's height

As a side effect this commit fixes also bug 149151 and allows https://codereview.chromium.org/230793003/ patch to be applied without breaking tests.

BUG=367092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279657

[Alexei Svitkine (slow), 2014-04-25 15:56:14]

Can you expand the CL description to mention which case this is specifically
fixing? (e.g. provide an example with a concrete font/size/windows config that
produces an incorrect result before and produces a better result with this
patch).

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:124: if (font_height > height) {
I'm not sure this if is needed, since the loop should naturally end due to
best_font_height <= height on next iteration.

So your current logic can just be simplified to:

Font best_font(new PlatformFontWin(CreateHFontRef(hfont)));
while (best_font.GetHeight() <= height) {
  best_font = best_font.Derive(1, style);
}

return best_font;

However, I need a bit more convincing that this is really what we want to do.
Ultimately, anything that hits this codepath (just like the loop above) will be
slow, since it will need to do multiple Derive() operations which are somewhat
expensive.

And it seems your change will force this codepath always since it actually seems
to be starting with the base font, rather than trying the CreateFontIndirect()
with the specified height for lfHeight.

(In fact, given that you're starting with the base font, your code can be
further simplified to not do the
GetObject()/SetLogFontStyle()/CreateFontIndirect()/CreateHFontRef()/etc calls
above and just init best_font with *this. However, as I say above, I'm not sure
it's the right solution.)

[Tomasz Moniuszko, 2014-05-08 14:08:17]

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:124: if (font_height > height) {
The reason for this fix is that GetTextMetrics returns the same font height
(calculated in PlatformFontWin::CreateHFontRef) for fonts of different sizes.
For instance:

Arial:
font size=12 height=15
font size=13 height=16
font size=14 height=16
font size=15 height=17

Segoe UI:
font size=17 height=23
font size=18 height=25
font size=19 height=25
font size=20 height=28

The result of PlatformFontWin::DeriveFontWithHeight call is dependent on base
font's height. For instance:

gfx::Font arial_12("Arial", 12);
gfx::Font arial_13 =
static_cast<PlatformFontWin*>(arial_12.platform_font())->DeriveFontWithHeight(16,
0);
gfx::Font arial_15("Arial", 15);
gfx::Font arial_14 =
static_cast<PlatformFontWin*>(arial_15.platform_font())->DeriveFontWithHeight(16,
0);

Both arial_13 and arial_14 fonts are the same height but they are different in
size so EXPECT_EQ(arial_13.GetHeight(), arial_14.GetHeight()); will pass but
EXPECT_EQ(arial_13.GetFontSize(), arial_14.GetFontSize()); will fail. This is
the reason why RenderTextTest.ElidedText failed in my other review:
https://codereview.chromium.org/230793003/

After applying this fix we will get Arial 14 font in both cases (if font is
derived from Arial 12 or Arial 15).

On 2014/04/25 15:56:14, Alexei Svitkine wrote:
> I'm not sure this if is needed, since the loop should naturally end due to
> best_font_height <= height on next iteration.
> 
> So your current logic can just be simplified to:
> 
> Font best_font(new PlatformFontWin(CreateHFontRef(hfont)));
> while (best_font.GetHeight() <= height) {
>   best_font = best_font.Derive(1, style);
> }
> 
> return best_font;

With this logic returned font would be too big because it's changed inside the
loop. We need to return the biggest font which height isn't greater than
requested height.

> 
> However, I need a bit more convincing that this is really what we want to do.
> Ultimately, anything that hits this codepath (just like the loop above) will
be
> slow, since it will need to do multiple Derive() operations which are somewhat
> expensive.
> 
> And it seems your change will force this codepath always since it actually
seems
> to be starting with the base font, rather than trying the CreateFontIndirect()
> with the specified height for lfHeight.

Maybe I could create font using CreateFontIndirect (with lfHeight) first and
then iterate in loop to check if bigger font also fits requested height. And
return the biggest font finally. What do you think?

> 
> (In fact, given that you're starting with the base font, your code can be
> further simplified to not do the
> GetObject()/SetLogFontStyle()/CreateFontIndirect()/CreateHFontRef()/etc calls
> above and just init best_font with *this. However, as I say above, I'm not
sure
> it's the right solution.)

[Alexei Svitkine (slow), 2014-05-09 21:17:50]

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:124: if (font_height > height) {
On 2014/05/08 14:08:17, Tomasz Moniuszko wrote:
> The reason for this fix is that GetTextMetrics returns the same font height
> (calculated in PlatformFontWin::CreateHFontRef) for fonts of different sizes.
> For instance:
> 
> Arial:
> font size=12 height=15
> font size=13 height=16
> font size=14 height=16
> font size=15 height=17
> 
> Segoe UI:
> font size=17 height=23
> font size=18 height=25
> font size=19 height=25
> font size=20 height=28
> 
> The result of PlatformFontWin::DeriveFontWithHeight call is dependent on base
> font's height. For instance:
> 
> gfx::Font arial_12("Arial", 12);
> gfx::Font arial_13 =
>
static_cast<PlatformFontWin*>(arial_12.platform_font())->DeriveFontWithHeight(16,
> 0);
> gfx::Font arial_15("Arial", 15);
> gfx::Font arial_14 =
>
static_cast<PlatformFontWin*>(arial_15.platform_font())->DeriveFontWithHeight(16,
> 0);
> 
> Both arial_13 and arial_14 fonts are the same height but they are different in
> size so EXPECT_EQ(arial_13.GetHeight(), arial_14.GetHeight()); will pass but
> EXPECT_EQ(arial_13.GetFontSize(), arial_14.GetFontSize()); will fail. This is
> the reason why RenderTextTest.ElidedText failed in my other review:
> https://codereview.chromium.org/230793003/
> 
> After applying this fix we will get Arial 14 font in both cases (if font is
> derived from Arial 12 or Arial 15).

I see, that makes sense since we iterate down in one case, but don't in the
other case.

Can you codify the above into a new unit test?

> Maybe I could create font using CreateFontIndirect (with lfHeight) first and
> then iterate in loop to check if bigger font also fits requested height. And
> return the biggest font finally. What do you think?

That sounds better than the current logic here. Could you also measure the
performance impact of the new code vs. the old one?

To get a good measurement, do it e.g. 100k times in a loop for both cases and
post the results. Thanks!

[Alexei Svitkine (slow), 2014-05-09 21:21:00]

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:71: const Font
bigger_font(base_font.GetFontName(), base_font.GetFontSize() + 10);
Oops, I see you already added a test.

Can you make it a separate test case?

A good convention to use is to add a _ and suffix after the name of the method,
so it could be something like:

DeriveFontWithHeight_Consistency

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:75: int requested_height =
(base_font.GetHeight() + bigger_font.GetHeight()) / 2;
Is it possible to do something more straight forward like your email example?
(i.e. with hardcoded heights), rather than this kind of more complicated
calculation?

[Tomasz Moniuszko, 2014-05-29 10:46:40]

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:124: if (font_height > height) {
> That sounds better than the current logic here. Could you also measure the
> performance impact of the new code vs. the old one?
> 
> To get a good measurement, do it e.g. 100k times in a loop for both cases and
> post the results. Thanks!

Done.

Test results for 100k repeats with different font heights (times in ms):
height=8 old_time=1155 new_time=3432
height=9 old_time=1154 new_time=3432
height=10 old_time=1155 new_time=3416
height=11 old_time=1139 new_time=3432
height=12 old_time=1185 new_time=3432
height=13 old_time=1139 new_time=3432
height=14 old_time=1139 new_time=3432
height=15 old_time=1155 new_time=3432
height=16 old_time=1170 new_time=3447
height=17 old_time=1155 new_time=3479
height=18 old_time=1154 new_time=3463
height=19 old_time=1155 new_time=3463
height=20 old_time=1170 new_time=3463
height=21 old_time=1155 new_time=3463
height=22 old_time=1154 new_time=3463
height=23 old_time=1155 new_time=3463

Average time is about 3 times greater. Some of this time is consumed to get
screen DC and create HFontRef in loop. This can be partially avoided by getting
TEXTMETRIC directly in DeriveFontWithHeight method and then creating HFontRef
only for the best font. The code is something like:

  LOGFONT font_info;
  GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
  font_info.lfHeight = height;
  SetLogFontStyle(style, &font_info);

  base::win::ScopedGetDC screen_dc(NULL);
  gfx::ScopedSetMapMode mode(screen_dc, MM_TEXT);

  base::win::ScopedGDIObject<HFONT> best_font(CreateFontIndirect(&font_info));
  TEXTMETRIC best_font_metrics;
  {
    base::win::ScopedSelectObject scoped_font(screen_dc, best_font);
    GetTextMetrics(screen_dc, &best_font_metrics);
  }

  while (best_font_metrics.tmHeight <= height) {
    font_info.lfHeight =
        -(best_font_metrics.tmHeight - best_font_metrics.tmInternalLeading + 1);
    base::win::ScopedGDIObject<HFONT> font(CreateFontIndirect(&font_info));
    TEXTMETRIC font_metrics;
    {
      base::win::ScopedSelectObject scoped_font(screen_dc, font);
      GetTextMetrics(screen_dc, &font_metrics);
    }
    if (font_metrics.tmHeight > height) {
      break;
    }
    best_font.Set(font.release());
    best_font_metrics = font_metrics;
  }
 
  return Font(new PlatformFontWin(
      CreateHFontRef(best_font.release(), best_font_metrics)));

With these additional changes I got test times about 2570 ms on my computer. I'm
not sure if it's worth to change this logic though because DeriveFontWithHeight
method is currently used only from DeriveFontIfNecessary function and only on
Windows XP. Also CreateHFontRef has to be slightly modified. What do you think?

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
File ui/gfx/platform_font_win_unittest.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:71: const Font
bigger_font(base_font.GetFontName(), base_font.GetFontSize() + 10);
On 2014/05/09 21:21:00, Alexei Svitkine wrote:
> Oops, I see you already added a test.
> 
> Can you make it a separate test case?
> 
> A good convention to use is to add a _ and suffix after the name of the
method,
> so it could be something like:
> 
> DeriveFontWithHeight_Consistency

Done.

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win_uni...
ui/gfx/platform_font_win_unittest.cc:75: int requested_height =
(base_font.GetHeight() + bigger_font.GetHeight()) / 2;
On 2014/05/09 21:21:00, Alexei Svitkine wrote:
> Is it possible to do something more straight forward like your email example?
> (i.e. with hardcoded heights), rather than this kind of more complicated
> calculation?

Done.

[Alexei Svitkine (slow), 2014-05-29 21:13:34]

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
ui/gfx/platform_font_win.cc:124: if (font_height > height) {
On 2014/05/29 10:46:40, Tomasz Moniuszko wrote:
> > That sounds better than the current logic here. Could you also measure the
> > performance impact of the new code vs. the old one?
> > 
> > To get a good measurement, do it e.g. 100k times in a loop for both cases
and
> > post the results. Thanks!
> 
> Done.
> 
> Test results for 100k repeats with different font heights (times in ms):
> height=8 old_time=1155 new_time=3432
> height=9 old_time=1154 new_time=3432
> height=10 old_time=1155 new_time=3416
> height=11 old_time=1139 new_time=3432
> height=12 old_time=1185 new_time=3432
> height=13 old_time=1139 new_time=3432
> height=14 old_time=1139 new_time=3432
> height=15 old_time=1155 new_time=3432
> height=16 old_time=1170 new_time=3447
> height=17 old_time=1155 new_time=3479
> height=18 old_time=1154 new_time=3463
> height=19 old_time=1155 new_time=3463
> height=20 old_time=1170 new_time=3463
> height=21 old_time=1155 new_time=3463
> height=22 old_time=1154 new_time=3463
> height=23 old_time=1155 new_time=3463
> 
> Average time is about 3 times greater. Some of this time is consumed to get
> screen DC and create HFontRef in loop. This can be partially avoided by
getting
> TEXTMETRIC directly in DeriveFontWithHeight method and then creating HFontRef
> only for the best font. The code is something like:
> 
>   LOGFONT font_info;
>   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
>   font_info.lfHeight = height;
>   SetLogFontStyle(style, &font_info);
> 
>   base::win::ScopedGetDC screen_dc(NULL);
>   gfx::ScopedSetMapMode mode(screen_dc, MM_TEXT);
> 
>   base::win::ScopedGDIObject<HFONT> best_font(CreateFontIndirect(&font_info));
>   TEXTMETRIC best_font_metrics;
>   {
>     base::win::ScopedSelectObject scoped_font(screen_dc, best_font);
>     GetTextMetrics(screen_dc, &best_font_metrics);
>   }
> 
>   while (best_font_metrics.tmHeight <= height) {
>     font_info.lfHeight =
>         -(best_font_metrics.tmHeight - best_font_metrics.tmInternalLeading +
1);
>     base::win::ScopedGDIObject<HFONT> font(CreateFontIndirect(&font_info));
>     TEXTMETRIC font_metrics;
>     {
>       base::win::ScopedSelectObject scoped_font(screen_dc, font);
>       GetTextMetrics(screen_dc, &font_metrics);
>     }
>     if (font_metrics.tmHeight > height) {
>       break;
>     }
>     best_font.Set(font.release());
>     best_font_metrics = font_metrics;
>   }
>  
>   return Font(new PlatformFontWin(
>       CreateHFontRef(best_font.release(), best_font_metrics)));
> 
> With these additional changes I got test times about 2570 ms on my computer.
I'm
> not sure if it's worth to change this logic though because
DeriveFontWithHeight
> method is currently used only from DeriveFontIfNecessary function and only on
> Windows XP. Also CreateHFontRef has to be slightly modified. What do you
think?

Seems like a sizeable-enough win to do the optimized path (especially since XP
machines are usually slower so it's a big difference). However, instead of
blowing up the code like above, could we make an internal version of Derive()
that simply takes more parameters? (e.g. taking the screen_dc and whatever other
things we need to cache between runs).

[msw, 2014-05-29 21:34:40]

https://codereview.chromium.org/251773002/diff/10001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/10001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:122: while (best_font_height <= height) {
Is it possible (and more efficient) to first guess a size change value to more
quickly approach the desired height? Something like:
best_font.Derive(best_font_height - height, style) (though I realize that size
and height aren't directly comparable like that...)

[Tomasz Moniuszko, 2014-06-02 12:38:31]

On 2014/05/29 21:13:34, Alexei Svitkine wrote:
> https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc
> File ui/gfx/platform_font_win.cc (right):
> 
>
https://codereview.chromium.org/251773002/diff/1/ui/gfx/platform_font_win.cc#...
> ui/gfx/platform_font_win.cc:124: if (font_height > height) {
> On 2014/05/29 10:46:40, Tomasz Moniuszko wrote:
> > > That sounds better than the current logic here. Could you also measure the
> > > performance impact of the new code vs. the old one?
> > > 
> > > To get a good measurement, do it e.g. 100k times in a loop for both cases
> and
> > > post the results. Thanks!
> > 
> > Done.
> > 
> > Test results for 100k repeats with different font heights (times in ms):
> > height=8 old_time=1155 new_time=3432
> > height=9 old_time=1154 new_time=3432
> > height=10 old_time=1155 new_time=3416
> > height=11 old_time=1139 new_time=3432
> > height=12 old_time=1185 new_time=3432
> > height=13 old_time=1139 new_time=3432
> > height=14 old_time=1139 new_time=3432
> > height=15 old_time=1155 new_time=3432
> > height=16 old_time=1170 new_time=3447
> > height=17 old_time=1155 new_time=3479
> > height=18 old_time=1154 new_time=3463
> > height=19 old_time=1155 new_time=3463
> > height=20 old_time=1170 new_time=3463
> > height=21 old_time=1155 new_time=3463
> > height=22 old_time=1154 new_time=3463
> > height=23 old_time=1155 new_time=3463
> > 
> > Average time is about 3 times greater. Some of this time is consumed to get
> > screen DC and create HFontRef in loop. This can be partially avoided by
> getting
> > TEXTMETRIC directly in DeriveFontWithHeight method and then creating
HFontRef
> > only for the best font. The code is something like:
> > 
> >   LOGFONT font_info;
> >   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
> >   font_info.lfHeight = height;
> >   SetLogFontStyle(style, &font_info);
> > 
> >   base::win::ScopedGetDC screen_dc(NULL);
> >   gfx::ScopedSetMapMode mode(screen_dc, MM_TEXT);
> > 
> >   base::win::ScopedGDIObject<HFONT>
best_font(CreateFontIndirect(&font_info));
> >   TEXTMETRIC best_font_metrics;
> >   {
> >     base::win::ScopedSelectObject scoped_font(screen_dc, best_font);
> >     GetTextMetrics(screen_dc, &best_font_metrics);
> >   }
> > 
> >   while (best_font_metrics.tmHeight <= height) {
> >     font_info.lfHeight =
> >         -(best_font_metrics.tmHeight - best_font_metrics.tmInternalLeading +
> 1);
> >     base::win::ScopedGDIObject<HFONT> font(CreateFontIndirect(&font_info));
> >     TEXTMETRIC font_metrics;
> >     {
> >       base::win::ScopedSelectObject scoped_font(screen_dc, font);
> >       GetTextMetrics(screen_dc, &font_metrics);
> >     }
> >     if (font_metrics.tmHeight > height) {
> >       break;
> >     }
> >     best_font.Set(font.release());
> >     best_font_metrics = font_metrics;
> >   }
> >  
> >   return Font(new PlatformFontWin(
> >       CreateHFontRef(best_font.release(), best_font_metrics)));
> > 
> > With these additional changes I got test times about 2570 ms on my computer.
> I'm
> > not sure if it's worth to change this logic though because
> DeriveFontWithHeight
> > method is currently used only from DeriveFontIfNecessary function and only
on
> > Windows XP. Also CreateHFontRef has to be slightly modified. What do you
> think?
> 
> Seems like a sizeable-enough win to do the optimized path (especially since XP
> machines are usually slower so it's a big difference). However, instead of
> blowing up the code like above, could we make an internal version of Derive()
> that simply takes more parameters? (e.g. taking the screen_dc and whatever
other
> things we need to cache between runs).

100k repeats test time after introducing DeriveWithCorrectedSize method which
reuses screen DC and doesn't create HFontRef until best font is calculated:
3040ms.

The same test time after introducing new version of CreateHFontRef which reuses
already calculated TEXTMETRIC: 2580ms.

[Tomasz Moniuszko, 2014-06-02 12:56:48]

On 2014/05/29 21:34:40, msw wrote:
>
https://codereview.chromium.org/251773002/diff/10001/ui/gfx/platform_font_win.cc
> File ui/gfx/platform_font_win.cc (right):
> 
>
https://codereview.chromium.org/251773002/diff/10001/ui/gfx/platform_font_win...
> ui/gfx/platform_font_win.cc:122: while (best_font_height <= height) {
> Is it possible (and more efficient) to first guess a size change value to more
> quickly approach the desired height? Something like:
> best_font.Derive(best_font_height - height, style) (though I realize that size
> and height aren't directly comparable like that...)

It seems that for given font's height there are not many font's sizes. For most
cases there's only one font's size for which font's height matches. And there
are some cases when fonts of two subsequent sizes have the same height values:
for example both Arial 13 and Arial 14 have height equal to 16.

With the current solution the best font is created with the first try (before
entering the loop) or in the first iteration of the loop. We cannot do better
first guess because we have font of desired height in the first try but we
cannot avoid examining next bigger font (bigger in terms of size).

[Alexei Svitkine (slow), 2014-06-02 15:42:55]

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:264: Font
PlatformFontWin::DeriveWithCorrectedSize(HFONT base_font) {
Make this a free-standing function in the anon namespace at the top of the file
instead.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:272: GetTextMetrics(screen_dc, &best_font_metrics);
Nit: Make a helper function in the anon namespace for getting the metrics given
a font and a dc, so these blocks can be much shorter, i.e.:

TEXTMETRIC best_font_metrics;
GetTextMetricsForFont(screen_dc, best_font, &best_font_metrics);

You can use it both here and in CreateHFontRef() above.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:280: -(best_font_metrics.tmHeight -
best_font_metrics.tmInternalLeading + 1);
Nit: Add a comment explaining this logic.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:287: if (font_metrics.tmHeight >
best_font_metrics.tmHeight) {
Nit: No {}'s.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:295: CreateHFontRef(best_font.release())));
Nit: Does this fit on the previous line?

[Tomasz Moniuszko, 2014-06-11 13:11:56]

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:264: Font
PlatformFontWin::DeriveWithCorrectedSize(HFONT base_font) {
On 2014/06/02 15:42:55, Alexei Svitkine wrote:
> Make this a free-standing function in the anon namespace at the top of the
file
> instead.

This method calls PlatformFontWin::CreateHFontRef which is a private method so
it cannot be called from anonymous namespace.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:272: GetTextMetrics(screen_dc, &best_font_metrics);
On 2014/06/02 15:42:55, Alexei Svitkine wrote:
> Nit: Make a helper function in the anon namespace for getting the metrics
given
> a font and a dc, so these blocks can be much shorter, i.e.:
> 
> TEXTMETRIC best_font_metrics;
> GetTextMetricsForFont(screen_dc, best_font, &best_font_metrics);
> 
> You can use it both here and in CreateHFontRef() above.

Done.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:280: -(best_font_metrics.tmHeight -
best_font_metrics.tmInternalLeading + 1);
On 2014/06/02 15:42:55, Alexei Svitkine wrote:
> Nit: Add a comment explaining this logic.

Done.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:287: if (font_metrics.tmHeight >
best_font_metrics.tmHeight) {
On 2014/06/02 15:42:55, Alexei Svitkine wrote:
> Nit: No {}'s.

Done.

https://codereview.chromium.org/251773002/diff/30001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:295: CreateHFontRef(best_font.release())));
On 2014/06/02 15:42:55, Alexei Svitkine wrote:
> Nit: Does this fit on the previous line?

Done.

[Alexei Svitkine (slow), 2014-06-11 16:58:48]

Looks good, almost there. Thanks!

https://codereview.chromium.org/251773002/diff/50001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/50001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:283: -(best_font_metrics.tmHeight -
best_font_metrics.tmInternalLeading + 1);
Question: Is it necessary to do this calculation or can you just
increment/decrement the previous lfHeight value (since it has been filled in by
GetObject()). Maybe via AdjustFontSize()?

[Tomasz Moniuszko, 2014-06-12 08:04:24]

https://codereview.chromium.org/251773002/diff/50001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/50001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:283: -(best_font_metrics.tmHeight -
best_font_metrics.tmInternalLeading + 1);
On 2014/06/11 16:58:48, Alexei Svitkine wrote:
> Question: Is it necessary to do this calculation or can you just
> increment/decrement the previous lfHeight value (since it has been filled in
by
> GetObject()). Maybe via AdjustFontSize()?

Good catch! In the worst case size might be not increased at all and we would
enter endless loop (I don't know if such font exists but I can imagine such
case).

lfHeight returned from GetObject is the same value that was passed to
CreateFontIndirect. It may be >= 0 (which means that we requested font by its
height). We need to ensure that lfHeight is < 0 (font's size) before entering
the loop. Then we can use AdjustFontSize inside the loop.

I made necessary changes.

[Alexei Svitkine (slow), 2014-06-12 17:58:00]

https://codereview.chromium.org/251773002/diff/70001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/70001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:280: if (font_info.lfHeight >= 0) {
The only caller of DeriveWithCorrectedSize() is DeriveFontWithHeight() which
sets lfHeight to |height| which is positive.

Instead, I suggest removing this if and just doing a DCHECK on it and making it
unconditional outside the loop. (With a comment explaining that we set it to a
negative value to indicate it's the size, not the height.)

[Tomasz Moniuszko, 2014-06-13 08:40:38]

https://codereview.chromium.org/251773002/diff/70001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/251773002/diff/70001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:280: if (font_info.lfHeight >= 0) {
On 2014/06/12 17:58:00, Alexei Svitkine wrote:
> The only caller of DeriveWithCorrectedSize() is DeriveFontWithHeight() which
> sets lfHeight to |height| which is positive.
> 
> Instead, I suggest removing this if and just doing a DCHECK on it and making
it
> unconditional outside the loop. (With a comment explaining that we set it to a
> negative value to indicate it's the size, not the height.)

I've removed the 'if'. I don't think DCHECK is really needed here as we set the
lfHeight to font size regardless of its previous value.

[Tomasz Moniuszko, 2014-06-23 09:38:17]

Hello Alexei,

Does the patch looks good to you now or it still needs some corrections?

Thanks,
Tomasz

[Alexei Svitkine (slow), 2014-06-23 14:30:36]

lgtm, sorry I had missed your previous reply

[Tomasz Moniuszko, 2014-06-24 06:59:43]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2014-06-24 07:00:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/251773002/90001

[commit-bot: I haz the power, 2014-06-24 09:28:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 10:04:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 10:04:55]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Tomasz Moniuszko, 2014-06-24 13:50:48]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2014-06-24 13:52:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/251773002/90001

[commit-bot: I haz the power, 2014-06-24 14:37:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 15:26:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 15:26:25]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Tomasz Moniuszko, 2014-06-25 07:33:47]

The CQ bit was checked by tmoniuszko@opera.com

[commit-bot: I haz the power, 2014-06-25 07:34:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/251773002/90001

[commit-bot: I haz the power, 2014-06-25 10:06:44]

Change committed as 279657