Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879
Committed: https://crrev.com/38da8029333feee3045ce7cd899c540683782980
Cr-Commit-Position: refs/heads/master@{#377477}

[tapted, 2016-02-12 00:18:19]

Description was changed from

==========
cl format


ui and views done


Caching code


gross version


pointers -> cache

BUG=
==========

to

==========
Remove enum ui::ResourceBundle::SmallFont and friends

These are actually no better than magic numbers, but client code tries
to attach some semantic meaning to them. We now find that uses of, e.g.,
`BaseFont` can appear too large on one platform, but we can't simply
change the size of `BaseFont` without breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that they're designed to offer.

This CL removes the SmallFont/BoldFont/BaseFont/etc. enum, and adds:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

A follow-up will provide platform-specific constants in
ui/base/default_style.h, e.g., for Mac where a smaller font is used for
dialog text.

BUG=564879
==========

[tapted, 2016-02-23 06:51:47]

Description was changed from

==========
Remove enum ui::ResourceBundle::SmallFont and friends

These are actually no better than magic numbers, but client code tries
to attach some semantic meaning to them. We now find that uses of, e.g.,
`BaseFont` can appear too large on one platform, but we can't simply
change the size of `BaseFont` without breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that they're designed to offer.

This CL removes the SmallFont/BoldFont/BaseFont/etc. enum, and adds:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

A follow-up will provide platform-specific constants in
ui/base/default_style.h, e.g., for Mac where a smaller font is used for
dialog text.

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::SmallFont and friends

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

BUG=564879
==========

[tapted, 2016-02-24 07:48:57]

Description was changed from

==========
Start removing enum ui::ResourceBundle::SmallFont and friends

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc: goo.gl/QBT62L

BUG=564879
==========

[tapted, 2016-02-24 07:53:25]

Description was changed from

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc: goo.gl/QBT62L

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design http://doc: goo.gl/QBT62L

BUG=564879
==========

[tapted, 2016-02-24 07:56:33]

Description was changed from

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design http://doc: goo.gl/QBT62L

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879
==========

[tapted, 2016-02-24 08:13:28]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2016-02-24 08:13:29]

Hi sky, please take a look

I actually put together a design doc for this ( http://goo.gl/QBT62L - it's
public). It has some of the other things I tried, and a more verbose description
of the problem, and why I think this is a good way to fix it. Once I got around
to implementing it, the CL feels a lot less scary than I thought it would be,
but please do tell me to approach chrome-eng-review or other
gfx::Font/ResourceBundle OWNERS/stakeholders if you think it needs that. Not
really sure who "owns" all this stuff though (maybe derat, msw, or asvitkine?)


The tl;dr is: MacViews fonts are too big. It's a combination of UI assuming a
default-constructed views::Label() is "correct", and a mismatch in the semantics
of things like ResourceBundle::SmallFont on different platforms.

The plan is to phase out ui::ResourceBundle::FontStyle completely, since they're
essentially magic numbers, but code uses them like they are not.

https://codereview.chromium.org/1689623004/diff/40001/ui/base/default_style.h
File ui/base/default_style.h (right):

https://codereview.chromium.org/1689623004/diff/40001/ui/base/default_style.h...
ui/base/default_style.h:40: #endif  // UI_BASE_DEFAULT_STYLE_H_
This file is actually similar to chrome/browser/ui/chrome_style.h which actually
has a TODO that the constants should live in src/ui. I haven't taken them out of
chrome_style.h yet though -- it would be a lot of churn, and it's not a
mechanical change since the constants there are FontStyle-style, and the idea
would be to assess cases where it would be more appropriate to use a constant
with a better semantic meaning.

[sky, 2016-02-24 18:33:26]

sky@chromium.org changed reviewers:
+ ben@chromium.org
- sky@chromium.org

[sky, 2016-02-24 18:33:27]

I'm a bit overwhelmed with other big changes of late, as well as taking over the
review load for Sadrul while he is out. I'm sending this to Ben.

[Ben Goodger (Google), 2016-02-24 18:42:24]

This seems like a good improvement for specificity.

lgtm

[msw, 2016-02-24 19:28:06]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-02-24 19:28:07]

Drive by q and nit; otherwise lgtm

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:546: gfx::FontList& base =
font_cache_[base_key];
q: where is this base font added to the cache?
(won't this fail the first time it's called?)

https://codereview.chromium.org/1689623004/diff/60001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane.cc:31: const gfx::Font::FontStyle
kHoverStyle = gfx::Font::BOLD;
nit: hover wasn't bold before, afaict. intentional change?

[tfarina, 2016-02-24 20:08:55]

https://codereview.chromium.org/1689623004/diff/60001/ui/base/default_style.h
File ui/base/default_style.h (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/default_style.h...
ui/base/default_style.h:10: // This file contains the constants that provide the
default style for UI
Thanks!

Can I move the things in
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/chr...
here after this lands?

[tfarina, 2016-02-24 20:39:24]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2016-02-24 20:39:24]

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.h:249: gfx::Font::FontStyle style =
gfx::Font::NORMAL);
is default parameters now allowed? I thought they weren't!

[tapted, 2016-02-25 00:21:11]

https://codereview.chromium.org/1689623004/diff/60001/ui/base/default_style.h
File ui/base/default_style.h (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/default_style.h...
ui/base/default_style.h:10: // This file contains the constants that provide the
default style for UI
On 2016/02/24 20:08:55, tfarina wrote:
> Thanks!
> 
> Can I move the things in
>
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/chr...
> here after this lands?

An earlier patchset did this, but the methods need
chrome/browser/themes/theme_properties.h, the `int` constants aren't yet needed
outside of chrome/browser and the FrontStyle constants should just be deleted
and users migrated over to these (since it's the deprecated type).

So, deleting the FontStyle constants in chrome_style.h is definitely on the
roadmap, but the other stuff should probably stay in chrome_style.h until we
find that it's needed elsewhere (possibly it is needed elsewhere already, and
there's some duplicated constants that should converge).

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:546: gfx::FontList& base =
font_cache_[base_key];
On 2016/02/24 19:28:06, msw wrote:
> q: where is this base font added to the cache?
> (won't this fail the first time it's called?)

Yup - this is where it will get added. It won't fail -- map::operator[] will
automatically insert a default-constructed item if it's not present, and that's
exactly what the "base" font should be.

And (per the linked doc) while the gfx::Font default constructor is on my
hit-list, since it's expensive (and code uses it without realising), the
gfx::FontList default constructor is cheap as of r249976 so it's not as high
priority. I'd have to revise this if the default constructor goes away, but for
now it's easier to read than an insert/make_pair dealie.

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.h:249: gfx::Font::FontStyle style =
gfx::Font::NORMAL);
On 2016/02/24 20:39:24, tfarina wrote:
> is default parameters now allowed? I thought they weren't!

This has gone through all the approvals for the google C++ style guide (e.g.
https://google.github.io/styleguide/cppguide.html#Default_Arguments [and the
internal go/cppguide] both say "Default arguments are allowed on non-virtual
functions when the default is guaranteed to always have the same value.".
They're used extensively in Blink, and it has been discussed here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wgZ2wPHrD94/0mcVcA2x...

For this method I think it's a no-brainer.

https://codereview.chromium.org/1689623004/diff/60001/ui/views/controls/tabbe...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/1689623004/diff/60001/ui/views/controls/tabbe...
ui/views/controls/tabbed_pane/tabbed_pane.cc:31: const gfx::Font::FontStyle
kHoverStyle = gfx::Font::BOLD;
On 2016/02/24 19:28:06, msw wrote:
> nit: hover wasn't bold before, afaict. intentional change?

gosh thanks! I checked over these so much... clearly 'BoldFont' is not enough
edit distance away from 'BaseFont' :/. Fixed.

[tapted, 2016-02-25 02:34:54]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-02-25 02:34:54]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1689623004/#ps80001
(title: "fix transcription error")

[commit-bot: I haz the power, 2016-02-25 02:39:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1689623004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1689623004/80001

[commit-bot: I haz the power, 2016-02-25 02:51:06]

Description was changed from

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879
==========

[commit-bot: I haz the power, 2016-02-25 02:51:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-25 02:51:52]

Description was changed from

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879
==========

to

==========
Start removing enum ui::ResourceBundle::FontStyle, fix MacViews font sizes

SmallFont, MediumFont, etc., are actually no better than magic numbers,
but client code tries to attach some semantic meaning to them. We now
find that uses of, e.g., `BaseFont` can appear too large on one
platform, but we can't simply change the size of `BaseFont` without
breaking a lot of existing UI.

They're also limited, with clients often forced to derive fonts of a
different size to suit mocks, then missing out on all the caching
benefits that ResourceBundle::FontStyle is designed to offer.

This CL adds a more flexible API, with:

- GetFont[List]WithDelta(int, Font::FontStyle)
- An as-needed cache that will cache any combination requested by client
code
- ui/base/default_style.h, with the constants from c/b/chrome_style.h
for recommended sizes for dialog text and dialog titles.

Users of ResourceBundle::FontStyle under ui/ are removed to give an idea
of how this would be used.

Rationale: this fixes font sizes for MacViews (which are too large) without
font sizes unexpectedly changing elsewhere in Cocoa or Views code.

Design doc http://goo.gl/QBT62L

BUG=564879

Committed: https://crrev.com/38da8029333feee3045ce7cd899c540683782980
Cr-Commit-Position: refs/heads/master@{#377477}
==========

[commit-bot: I haz the power, 2016-02-25 02:51:53]

Patchset 5 (id:??) landed as
https://crrev.com/38da8029333feee3045ce7cd899c540683782980
Cr-Commit-Position: refs/heads/master@{#377477}