Adjust default font sizes for new hand hinted fonts.

[cros] Adjust default font sizes for new hand hinted fonts.

BUG=chromium-os:8658
TEST=Manual.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66999

[Nikita (slow), 2010-11-17 17:36:35]

See issue latest comments for screenshots how fixed font sizes looks.

[Daniel Erat, 2010-11-17 18:25:28]

On 2010/11/17 17:36:35, Nikita Kostylev wrote:
> See issue latest comments for screenshots how fixed font sizes looks.

Some of the text (particularly the time and input method in the status bar and
the text in the buttons on the login screen) looks too big compared to other
parts of the UI.

On the "Let's get started" screen, it's strange that the "Select your language:"
label is a larger size than the "Continue" button.  It's also weird that the
"Sign in" button on the next screen has text that's larger than the "Continue"
button.

Do you have mocks that these can be compared against?  I'll send the screenshots
to Ken so he can offer feedback too.

[Nikita (slow), 2010-11-17 18:34:55]

Clock button:
It has to use BaseFont now (previously Base+1).
I'll fix that.

Continue button:
hasn't been updated to look like others, I'll fix that too.

Basically it should not have a font size regression except the cases where we
manually changed font size.
I'll scan codebase to find those places.

[Nikita (slow), 2010-11-17 19:15:34]

Fixed cros specific controls.

Ben,
there are some non cros-specific places where we use DeriveFont(x, ..)
and I need to touch these places too.

Specifically:
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
chrome/browser/ui/views/sad_tab_view.cc
chrome/browser/chromeos/wm_overview_title.cc (although this one is cros
specific)
chrome/browser/chromeos/frame/bubble_frame_view.cc
chrome/browser/ui/views/location_bar/location_bar_view.cc
chrome/browser/ui/views/extensions/extension_install_prompt2.cc

I'm think about approach as in autocomplete_popup_contents_view.cc

// The size delta between the font used for the edit and the result rows.
// Passed to gfx::Font::DeriveFont.
#if !defined(OS_CHROMEOS)
const int kEditFontAdjust = -1;
#else
// Don't adjust font on chromeos as it becomes too small.
const int kEditFontAdjust = 0;
#endif

What

[Nikita (slow), 2010-11-18 14:50:58]

Please take a look, I've added a single define that'll help control font sizes
from single place.

[Ben Goodger (Google), 2010-11-18 16:27:58]

app/ and views/ portions LGTM.

[Daniel Erat, 2010-11-18 16:28:02]

Looks pretty good to me.  This doesn't result in any font sizing changes when
the HAND_HINTED_FONTS symbol is undefined, right?

http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc
File app/resource_bundle.cc (right):

http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc#newc...
app/resource_bundle.cc:19: #if defined(OS_CHROMEOS) &&
defined(CROS_HAND_HINTED_FONTS)
CROS_HAND_HINTED_FONTS is perhaps a bit inaccurate -- this only applies if we're
using hand-hinted fonts _and_ the BCI (rather than the auto-hinter), right? 
Could you use something like CROS_FONTS_USING_BCI or
CROS_FONTS_HAVE_ACCURATE_SIZES or something instead, along with a comment in the
place where it's #define-ed elaborating on what that means?

[whywhat, 2010-11-18 18:39:21]

LGTM

[Nikita (slow), 2010-11-18 19:11:04]

Added fixes in remaining places that use DeriveFont(...).

Yes, when CROS_HAND_HINTED_FONTS is not defined all deltas are as previously.

http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc
File app/resource_bundle.cc (right):

http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc#newc...
app/resource_bundle.cc:19: #if defined(OS_CHROMEOS) &&
defined(CROS_HAND_HINTED_FONTS)
On 2010/11/18 16:28:03, Daniel Erat wrote:
> CROS_HAND_HINTED_FONTS is perhaps a bit inaccurate -- this only applies if
we're
> using hand-hinted fonts _and_ the BCI (rather than the auto-hinter), right? 
> Could you use something like CROS_FONTS_USING_BCI or
> CROS_FONTS_HAVE_ACCURATE_SIZES or something instead, along with a comment in
the
> place where it's #define-ed elaborating on what that means?

As discussed, font size correction is needed for updated fonts regardless of BCI
on/off setting.
Added comment near define.

[Daniel Erat, 2010-11-18 19:28:06]

Thanks, LGTM

On Thu, Nov 18, 2010 at 11:11 AM,  <nkostylev@chromium.org> wrote:
> Added fixes in remaining places that use DeriveFont(...).
>
> Yes, when CROS_HAND_HINTED_FONTS is not defined all deltas are as
> previously.
>
>
> http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc
> File app/resource_bundle.cc (right):
>
>
http://codereview.chromium.org/5180002/diff/11001/app/resource_bundle.cc#newc...
> app/resource_bundle.cc:19: #if defined(OS_CHROMEOS) &&
> defined(CROS_HAND_HINTED_FONTS)
> On 2010/11/18 16:28:03, Daniel Erat wrote:
>>
>> CROS_HAND_HINTED_FONTS is perhaps a bit inaccurate -- this only
>
> applies if we're
>>
>> using hand-hinted fonts _and_ the BCI (rather than the auto-hinter),
>
> right?
>>
>> Could you use something like CROS_FONTS_USING_BCI or
>> CROS_FONTS_HAVE_ACCURATE_SIZES or something instead, along with a
>
> comment in the
>>
>> place where it's #define-ed elaborating on what that means?
>
> As discussed, font size correction is needed for updated fonts
> regardless of BCI on/off setting.
> Added comment near define.
>
> http://codereview.chromium.org/5180002/
>

[bstell, 2010-11-18 21:38:05]

The bytecode (BCI) hinted fonts have a different set of color fringing. Since
the color change is an unresolved discussion I am going to turn off BCI so these
changes should not go in (or be rolled back if necessary).

http://code.google.com/p/chromium-os/issues/detail?id=8963

[Nikita (slow), 2010-11-18 22:10:13]

I intend to check in these changes but with define disabled.

That'll make font sizes unchanged but provide an easy way to turn it on.
Also some cleanup is useful here.

[bstell, 2010-11-19 00:33:33]

On 2010/11/18 22:10:13, Nikita Kostylev wrote:
> I intend to check in these changes but with define disabled.
> 
> That'll make font sizes unchanged but provide an easy way to turn it on.
> Also some cleanup is useful here.

Ok, the LGTM