"Bootstrap" a toolkit-views Typography spec.

"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891, 701241
Review-Url: https://codereview.chromium.org/2734113006
Cr-Commit-Position: refs/heads/master@{#458261}
Committed: https://chromium.googlesource.com/chromium/src/+/9577332120682b5400df78b16edcc0c759bb5881

[tapted, 2017-03-08 08:48:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 08:48:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 09:37:06]

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

[commit-bot: I haz the power, 2017-03-08 09:37:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-08 10:08:13]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 10:08:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 11:07:14]

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

[commit-bot: I haz the power, 2017-03-08 11:07:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tapted, 2017-03-09 02:54:04]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 02:54:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 04:37:35]

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

[commit-bot: I haz the power, 2017-03-09 04:37:36]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-03-10 01:30:22]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 01:31:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 01:38:17]

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

[commit-bot: I haz the power, 2017-03-10 01:38:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-14 03:02:09]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 03:02:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 03:10:51]

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

[commit-bot: I haz the power, 2017-03-14 03:10:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-14 04:56:59]

Patchset #1 (id:1) has been deleted

[tapted, 2017-03-14 05:38:27]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 05:38:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 05:59:17]

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

[commit-bot: I haz the power, 2017-03-14 05:59:18]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-03-14 06:26:24]

Description was changed from

==========
Implement Harmony typography spec.

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts instead.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
 - Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

[tapted, 2017-03-14 06:42:56]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 06:43:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-14 06:44:05]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts instead.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
 - Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
 - Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

[tapted, 2017-03-14 06:45:35]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
 - Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

[tapted, 2017-03-14 06:45:59]

Patchset #1 (id:20001) has been deleted

[tapted, 2017-03-14 06:46:21]

Patchset #2 (id:60001) has been deleted

[tapted, 2017-03-14 06:46:32]

Patchset #2 (id:80001) has been deleted

[tapted, 2017-03-14 06:46:49]

Patchset #3 (id:120001) has been deleted

[tapted, 2017-03-14 06:46:59]

Patchset #3 (id:140001) has been deleted

[tapted, 2017-03-14 06:47:25]

Patchset #4 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-03-14 07:03:59]

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

[commit-bot: I haz the power, 2017-03-14 07:04:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tapted, 2017-03-14 09:01:37]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 09:01:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-14 09:02:09]

Patchset #5 (id:220001) has been deleted

[tapted, 2017-03-14 09:56:17]

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

[tapted, 2017-03-14 09:56:18]

Hi Peter and sky, please take a look.

I'm calling this a "Bootstrap" since it's the beginning of a large refactor.. I
tried to pick a meaningful/landable first "chunk" to give an idea of how the
callsites are going to change and survey what use-cases we need to support.
There are lots of TODOs in the code and a roadmap of planned follow-ups in the
CL description.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:29: BUTTON_TEXT,        // Text that appears on a
button control. Usually 12pt.
BUTTON_TEXT is different in the spec, but DIALOG_TEXT_SMALL, CONTROL_LABEL,
FIELD are all referred to as "Body 2" in the spec. I.e. they are the same thing.
 I just don't really like the name "BODY2" as a constant here, which means
there's some repetition. The advantage is that (maybe) someone reading the code
at the callsite doesn't have to look up what "Body2" means, and for dialogs that
aren't fully specced out a dev can make a judgement call without having a
designer say "that should be body1" or "body2".

The more detailed context information may be useful for other things too - e.g.
views::Label inherits views::View::SetEnabled(bool): something in context
"FIELD" or "BUTTON_TEXT" could use this to switch between PRIMARY/DISABLED
styles, but that might not make sense in other TextContexts.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:211: }
The tests above were a means to explore how "leading"/LineSpacing should be
calculated in the TypographyProvider. The default provider can just use 0 to
indicate that it should use Font::GetHeight().

But the harmony provider needs to account for how a leading of "20" should
change when the size of the base font changes. There's a discussion about this
at https://bugs.chromium.org/p/chromium/issues/detail?id=691891#c5  (i.e. how
HarmonyProvider will implement GetLineSpacing()).

Anyway, this test probably doesn't belong here - it should go with
HarmonyProvider. Not sure about LegacyFontSizeConstants - maybe we don't need it
- or maybe it's useful having "reality" documented somewhere - currently the
mapping between things like ResourceBundle::BaseFont to a font size and other
metrics is very unclear.

[commit-bot: I haz the power, 2017-03-14 10:13:23]

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

[commit-bot: I haz the power, 2017-03-14 10:13:24]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-14 17:31:47]

Maybe the embedder expanding the set of styles/contexts should happen like
contents NotificationType?

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:22: enum class TextContext {
It seems fair to enumerate the places in views that contain text, but I would
not want this to grow to include contexts outside of views. How would that be
prevented? For example, in this patch DEPRECATED_SMALL is only used outside of
views. I do not want to see that happen. Same thing with DIALOG_TEXT_SMALL.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:37: enum class TextStyle {
Similar comment as to TextContext. I'm ok with enumerating styles used in views,
nothing else.

[Peter Kasting, 2017-03-14 19:35:49]

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:22: enum class TextContext {
On 2017/03/14 17:31:47, sky wrote:
> It seems fair to enumerate the places in views that contain text, but I would
> not want this to grow to include contexts outside of views. How would that be
> prevented? For example, in this patch DEPRECATED_SMALL is only used outside of
> views. I do not want to see that happen. Same thing with DIALOG_TEXT_SMALL.

I chatted briefly with Scott about this, since it was sounding like some of the
split class stuff we've been talking about with ViewsDelegate, LayoutDelegate,
etc.

One way to implement this might be to copy how content/ and chrome/ split the
NotificationType enum.  Basically, content/ declares a NotificationType enum
inside the content:: namespace, and puts a signal NOTIFICATION_CONTENT_END value
at the end.  Then chrome/ declares a separate enum in a separate naming scope,
which sets the value of the first value to NOTIFICATION_CONTENT_END.

Then functions which want to take a notification type pass ints around instead,
so callers can provide something from either enum, and it works.  You don't get
compile-time typechecking from this, and the signature is uninformative compared
to if it actually said "NotificationType".

We could do something similar here.  A small improvement on the above might be
to create a type alias instead of using "int" in function signatures, except
that that alias would have to not conflict with the actual enum names (ugh). 
Another possibility, I suppose, would be to declare the core class as an enum
class and give it a specified underlying type known to be large enough to hold
all desired values, then do something similar on the chrome/ side and cast (or
write a wrapper that casts) from the "derived" enum to the "base" enum.  This
allows signatures to use the actual enum type, at the cost of making
chrome/-side users use the coercion mechanism everywhere they need to refer to a
real value.

Finally, you could make a counterargument that all this stuff should stay here
if it's conceptually correct for it to be in views/, e.g. because these styles
are implemented in terms of constants or concepts that are internal to
typography.cc and shouldn't be exposed outside views/.

[Peter Kasting, 2017-03-14 23:53:34]

I didn't look at everything, just a scattering of consumers + the typography and
label files.

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc
(right):

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:506:
views::Label::CustomFont{rb.GetFontListWithDelta(
Is the explicit "views::Label::CustomFont" necessary here?  I'd think the braces
alone might do the trick.  (Many files)

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: :
views::TextContext::DIALOG_TEXT_SMALL;
Nit: It would be nice to just use DIALOG_TITLE unconditionally if it didn't look
too bad in pre-Harmony.  (Same comment applies anywhere else we're doing a
conditional like this.  However, for the sake of sanity, it would be better to
make such a change by first landing a separate behavior-change-only CL, then
landing a CL here that changes the calling conventions without changing
behavior.)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/controls/labe...
ui/views/controls/label.h:50: // Create Labels with TextContext::CONTROL_LABEL
and TextStyle::PRIMARY.
Nit: Function comments should be descriptive ("Creates"), not imperative; see
http://google.github.io/styleguide/cppguide.html#Function_Comments .
 (multiple places)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:15: constexpr int kDialogMessageDelta = 1;
Nit: I tend to find it easier to understand when constants appear locally in the
scope that uses them, so I'm not scanning back up through the file to figure
them out.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:34: gfx::Font::Weight font_weight =
gfx::Font::Weight::NORMAL;
Nit: Declare this as close to first use as possible

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean up on
exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support SetProvider()
below to allow this to be reset while running, in which case we do want to clean
up.

Can we avoid SetProvider() by having this create the correct type of provider
(harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming there that
we may want one or the other, but it won't change at runtime.

Then we could return a raw pointer instead of a non-const-ref-to-unique-ptr,
which raised my eyebrows.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:53: // Legacy styles start here.
Are these intended to be "deprecated, don't add uses" and we'll have a TODO
about how to convert them to something else?

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:73: // Gets the line spacing, or 0 if it should be
provided by gfx::FontList.
I admit it's not totally clear to me how this works.

From what I know, real-life line spacing in printing is typically "baseline to
baseline".  In views, we're typically laying out a single line of text, with
other things around it spaced some distance away from the top or bottom. 
Occasionally we actually use a multi-line label, which could use this value
internally, but in the other (common) cases, does this value influence
surrounding layout at all?  Should it?  Maybe that's beyond the scope of the
typography provider to deal with?

(I guess I'm still wrestling with how "the dialog title has X pt leading, the
body text below it has Y pt leading" means the body text should be positioned
relative to the title, and who should be responsible for that positioning. 
Maybe everything goes in a single RenderText?)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:76: virtual ~TypographyProvider() {}
Nit: I tend to prefer "= default" to "{}" as the way of saying "I didn't forget
this, but it doesn't need to do anything special".  (2 places)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:85: // Helper class to query (or change) the current
TypographyProvider.
Hmm.  Is this really just a way to group some related functions together and not
an actual class?  If so, use a namespace instead; see
http://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_an...
.  Or make these static on another class, e.g. TypographyProvider (might require
some name changes? e.g. making the non-static methods there private and
suffixing them "Internal").

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:25: // On Windows, some bots say 11 and
some bots say 12. This is a bad thing.
OS versions differ?  Might check OSInfo::version().

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:96: EXPECT_TRUE(title_font_width == 6 ||
title_font_width == 8);  // Weird.
What.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:97: EXPECT_EQ(8,
title_font.GetExpectedTextWidth(1));
This line kinda contradicts the one just above

[tapted, 2017-03-15 00:30:05]

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
Still working on the rest, but I came up with this `ExtendableEnum` to solve the
layering while keeping the type safety and enum syntax. I think it will work...
(clang likes it :).

As far as "client" code goes, I think the only thing lost is the compiler
warning "value Foo not handled in switch", but that's unavoidable. Otherwise,
the switch syntax works just like an enum.

For declaring the `ExtendableEnum` the syntax is a bit more verbose (almost
Java-esque) and needs explicit initializers, but that's not too bad.

I guess questions at this stage would be
 - Should I proceed with it for TextContext/TextStyle?
 - Is where I've "extended" it in chrome/../layout_delegate.h the right place?
(chrome_typography.h maybe?)
 - Should I move ExtendableEnum to /src/base?


(no worries if you don't see this - I'll figure out something sensible)

[tapted, 2017-03-15 02:49:09]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:49:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 03:52:20]

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

[commit-bot: I haz the power, 2017-03-15 03:52:22]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-15 05:08:56]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 05:09:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 05:11:22]

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

[commit-bot: I haz the power, 2017-03-15 05:11:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tapted, 2017-03-15 05:49:50]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    TextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> TextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> TextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    ChromeTextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> ChromeTextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> ChromeTextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

[Peter Kasting, 2017-03-15 06:38:48]

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 00:30:05, tapted wrote:
> Still working on the rest, but I came up with this `ExtendableEnum` to solve
the
> layering while keeping the type safety and enum syntax. I think it will
work...
> (clang likes it :).
> 
> As far as "client" code goes, I think the only thing lost is the compiler
> warning "value Foo not handled in switch", but that's unavoidable. Otherwise,
> the switch syntax works just like an enum.
> 
> For declaring the `ExtendableEnum` the syntax is a bit more verbose (almost
> Java-esque) and needs explicit initializers, but that's not too bad.

Hmm.  My take on this is that overall this feels more verbose to me than the
content/ method (of two enums), by such a large degree that it's hard to read
and not worth doing, but that's also affected by my suspicion that the
compile-time typechecking from this will never catch any real bugs (and
therefore the benefits are low).

It's clever though :)

[tapted, 2017-03-15 07:57:42]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 07:57:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 08:02:06]

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

[commit-bot: I haz the power, 2017-03-15 08:02:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-15 08:09:06]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 08:09:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 08:16:49]

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

[commit-bot: I haz the power, 2017-03-15 08:16:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-15 08:21:34]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 08:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-15 08:26:25]

PTAL

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc
(right):

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:506:
views::Label::CustomFont{rb.GetFontListWithDelta(
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Is the explicit "views::Label::CustomFont" necessary here?  I'd think the
braces
> alone might do the trick.  (Many files)

Hunh - you're right. It's not needed. Removed.

I kinda liked the idea of making callers that rely on the CustomFont constructor
having to use some "unwieldy" syntax to use it ;). We could always make a
regular class+constructor rather than rely on the uniform initialization syntax,
but maybe trimming down the syntax will help avoid some churn until more of
those TODOs in Label are done (e.g. making CustomFont a {delta+weight} rather
than a {FontList}).

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: :
views::TextContext::DIALOG_TEXT_SMALL;
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Nit: It would be nice to just use DIALOG_TITLE unconditionally if it didn't
look
> too bad in pre-Harmony.  (Same comment applies anywhere else we're doing a
> conditional like this.  However, for the sake of sanity, it would be better to
> make such a change by first landing a separate behavior-change-only CL, then
> landing a CL here that changes the calling conventions without changing
> behavior.)

The difference between these two fonts is significant -- added screengrabs to
http://crbug.com/700196#c1

(here I'm just moving an existing conditional up :).

Also, (for other cases) if we can land the "bootstrap" first it will give us a
way to go directly to a views::TextContext rather than doing more verbose things
with ui::ResourceBundle::GetSharedInstance()->GetFontListWithDelta(..) to get to
harmony-mode values.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:15: constexpr int kDialogMessageDelta = 1;
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Nit: I tend to find it easier to understand when constants appear locally in
the
> scope that uses them, so I'm not scanning back up through the file to figure
> them out.

Done.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:34: gfx::Font::Weight font_weight =
gfx::Font::Weight::NORMAL;
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Nit: Declare this as close to first use as possible

Done.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean up on
> exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support SetProvider()
> below to allow this to be reset while running, in which case we do want to
clean
> up.

Why do we need cleanup in that case? Threads on chromium-dev suggest we want to
move away from exit-time destructors in a more general way. If it's needed, I
was thinking views::Typography could provide a Reset() that just sets the
provider back to DefaultProvider.

> 
> Can we avoid SetProvider() by having this create the correct type of provider
> (harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming there
that
> we may want one or the other, but it won't change at runtime.

I don't think we can do that -- views::Typography can't know about the other
providers the way that LayoutDelegate::Get() is able to do it for
HarmonyLayoutDelegate.

> 
> Then we could return a raw pointer instead of a non-const-ref-to-unique-ptr,
> which raised my eyebrows.

The NCRTUP was to avoid using special concepts that base::Singleton/LazyInstance
introduce, while still avoiding raw pointers so that the ownership transfer is
clear.

We could switch to LazyInstance, but base/lazy_instance.h says "This class is
useful for places you would normally use a function-level static, but you need
to have guaranteed thread-safety." - Chrome now compiles with thread-safe
statics (and this use doesn't really need to be thread-safe in any case), so I
think we can do the same thing without introducing new concepts (just
CR_DEFINE_STATIC_LOCAL I guess, but it's pretty simple).

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:22: enum class TextContext {
On 2017/03/14 17:31:47, sky wrote:
> It seems fair to enumerate the places in views that contain text, but I would
> not want this to grow to include contexts outside of views. How would that be
> prevented? For example, in this patch DEPRECATED_SMALL is only used outside of
> views. I do not want to see that happen. Same thing with DIALOG_TEXT_SMALL.

Done.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:37: enum class TextStyle {
On 2017/03/14 17:31:47, sky wrote:
> Similar comment as to TextContext. I'm ok with enumerating styles used in
views,
> nothing else. 

Done.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:53: // Legacy styles start here.
On 2017/03/14 23:53:33, Peter Kasting wrote:
> Are these intended to be "deprecated, don't add uses" and we'll have a TODO
> about how to convert them to something else?

It's more about a set of styles that just don't exist in the Harmony Typography
spec :/ (which I've tried to avoid focusing on under ui/views).

this is less important to call out now things are split anyway.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:73: // Gets the line spacing, or 0 if it should be
provided by gfx::FontList.
On 2017/03/14 23:53:33, Peter Kasting wrote:
> I admit it's not totally clear to me how this works.
> 
> From what I know, real-life line spacing in printing is typically "baseline to
> baseline".  In views, we're typically laying out a single line of text, with
> other things around it spaced some distance away from the top or bottom. 
> Occasionally we actually use a multi-line label, which could use this value
> internally, but in the other (common) cases, does this value influence
> surrounding layout at all?  Should it?

I think it does and should. views::Label has a line_height() property, which is
fed through to gfx::RenderText::SetMinLineHeight(). Label::GetHeightForWidth and
GetTextSize() cater for line_height() in both the single-line and multi-line
cases. And RenderText always tries to center the text its rendering in the
display rectangle its given.

>  Maybe that's beyond the scope of the
> typography provider to deal with?

Yeah, the plan would just be to let Label::SetLineHeight() deal with how it
should be interpreted.

> 
> (I guess I'm still wrestling with how "the dialog title has X pt leading, the
> body text below it has Y pt leading" means the body text should be positioned
> relative to the title, and who should be responsible for that positioning. 
> Maybe everything goes in a single RenderText?)

RenderText will pick an *actual* height for each line that depends on the
GetHeight() of every gfx::Font, the MinLineHeight(), as well as the tallest font
glyph that actually got rendered on that line. But then it just gives bounding
boxes to the things using it and centers text within those - I think it will all
work out..

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:76: virtual ~TypographyProvider() {}
On 2017/03/14 23:53:34, Peter Kasting wrote:
> Nit: I tend to prefer "= default" to "{}" as the way of saying "I didn't
forget
> this, but it doesn't need to do anything special".  (2 places)

Done.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:85: // Helper class to query (or change) the current
TypographyProvider.
On 2017/03/14 23:53:34, Peter Kasting wrote:
> Hmm.  Is this really just a way to group some related functions together and
not
> an actual class?  If so, use a namespace instead; see
>
http://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_an...
> .  Or make these static on another class, e.g. TypographyProvider (might
require
> some name changes? e.g. making the non-static methods there private and
> suffixing them "Internal").

Hm - conceptually, `Typography` also encapsulates the "Storage" defined in the
anonymous namespace in the .cc. The methods are tightly bound to that storage,
it just happens that the actual data storage doesn't need to be declared in the
header.

A class also bundles linkage (VIEWS_EXPORT) and allows declaring `friends`
(although the latter doesn't apply here).

But this is just a convenience to explore ways to avoid the syntactic mess we
get around things like ResourceBundle (`FooProvider::Get().GetBar` feels really
awkward to me). However, this doesn't scale well with more methods. Barely
anything actually needs to get the provider anyway, so I've split things up and
gone for the more traditional approach.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:25: // On Windows, some bots say 11 and
some bots say 12. This is a bad thing.
On 2017/03/14 23:53:34, Peter Kasting wrote:
> OS versions differ?  Might check OSInfo::version().

I'll need to smoke out the bot - might have been a one-off (or possibly me
getting lost in error output)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:96: EXPECT_TRUE(title_font_width == 6 ||
title_font_width == 8);  // Weird.
On 2017/03/14 23:53:34, Peter Kasting wrote:
> What.

Yeah - there was one bot run that was different to all the rest :/. I want to
smoke it out. Simplified these for now (plan is to land these change-detectors
DISABLED_ and smoke them out -- http://crbug.com/701241

Also moved these to layout_delegate_unittest.cc - there's a lot of stuff from
the spec, and we want to test the "extended" enums for the stuff below.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography_unittest.cc:97: EXPECT_EQ(8,
title_font.GetExpectedTextWidth(1));
On 2017/03/14 23:53:34, Peter Kasting wrote:
> This line kinda contradicts the one just above

Acknowledged.

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 06:38:48, Peter Kasting wrote:
> On 2017/03/15 00:30:05, tapted wrote:
> > Still working on the rest, but I came up with this `ExtendableEnum` to solve
> the
> > layering while keeping the type safety and enum syntax. I think it will
> work...
> > (clang likes it :).
> > 
> > As far as "client" code goes, I think the only thing lost is the compiler
> > warning "value Foo not handled in switch", but that's unavoidable.
Otherwise,
> > the switch syntax works just like an enum.
> > 
> > For declaring the `ExtendableEnum` the syntax is a bit more verbose (almost
> > Java-esque) and needs explicit initializers, but that's not too bad.
> 
> Hmm.  My take on this is that overall this feels more verbose to me than the
> content/ method (of two enums), by such a large degree that it's hard to read
> and not worth doing, but that's also affected by my suspicion that the
> compile-time typechecking from this will never catch any real bugs (and
> therefore the benefits are low).
> 
> It's clever though :)


The problem I see is that TextContext and TextStyle are (mostly)orthogonal
concepts, but it they are both typedef to `int` it will be really easy to get
them swapped around. On top of this, there are a lot of `default:` cases, and
subtle differences, so it will be easy to get something that even looks correct
when run, but may cause subtle and hard to diagnose bugs if we later decide to
tweak the typographyprovider (it may even depend on things from ui::NativeTheme,
so not even show up on test computers).

[commit-bot: I haz the power, 2017-03-15 08:30:43]

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

[commit-bot: I haz the power, 2017-03-15 08:30:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-15 08:34:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 08:34:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 08:44:39]

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

[commit-bot: I haz the power, 2017-03-15 08:44:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-15 10:01:49]

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: :
views::TextContext::DIALOG_TEXT_SMALL;
On 2017/03/15 08:26:24, tapted wrote:
> On 2017/03/14 23:53:33, Peter Kasting wrote:
> > Nit: It would be nice to just use DIALOG_TITLE unconditionally if it didn't
> look
> > too bad in pre-Harmony.  (Same comment applies anywhere else we're doing a
> > conditional like this.  However, for the sake of sanity, it would be better
to
> > make such a change by first landing a separate behavior-change-only CL, then
> > landing a CL here that changes the calling conventions without changing
> > behavior.)
> 
> The difference between these two fonts is significant -- added screengrabs to
> http://crbug.com/700196#c1
> 
> (here I'm just moving an existing conditional up :).

Honestly I'd be fine with this if we shortened the text string, which I imagine
we'll end up wanting for Harmony anyways.

> Also, (for other cases) if we can land the "bootstrap" first it will give us a
> way to go directly to a views::TextContext rather than doing more verbose
things
> with ui::ResourceBundle::GetSharedInstance()->GetFontListWithDelta(..) to get
to
> harmony-mode values.

I'm fine landing in whatever order, I mostly just want to kill as many
pre-Harmony conditionals as I can as fast as I can :P

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
On 2017/03/15 08:26:24, tapted wrote:
> On 2017/03/14 23:53:33, Peter Kasting wrote:
> > Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean up
on
> > exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support
SetProvider()
> > below to allow this to be reset while running, in which case we do want to
> clean
> > up.
> 
> Why do we need cleanup in that case? Threads on chromium-dev suggest we want
to
> move away from exit-time destructors in a more general way. If it's needed, I
> was thinking views::Typography could provide a Reset() that just sets the
> provider back to DefaultProvider.

We need to avoid leaking the thing we're replacing, since the program isn't
shutting down.

> > Can we avoid SetProvider() by having this create the correct type of
provider
> > (harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming there
> that
> > we may want one or the other, but it won't change at runtime.
> 
> I don't think we can do that -- views::Typography can't know about the other
> providers the way that LayoutDelegate::Get() is able to do it for
> HarmonyLayoutDelegate.

Unless we provide a delegate that does this, or split Typography across the
views/chrome boundary a la ViewsDelegate (in which case we address the enum that
way too).

Swapping providers at runtime feels error-prone (what if we race the swap and
query this thing ahead of time?), which would be another nice thing to avoid.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:73: // Gets the line spacing, or 0 if it should be
provided by gfx::FontList.
On 2017/03/15 08:26:24, tapted wrote:
> On 2017/03/14 23:53:33, Peter Kasting wrote:
> > I admit it's not totally clear to me how this works.
> > 
> > From what I know, real-life line spacing in printing is typically "baseline
to
> > baseline".  In views, we're typically laying out a single line of text, with
> > other things around it spaced some distance away from the top or bottom. 
> > Occasionally we actually use a multi-line label, which could use this value
> > internally, but in the other (common) cases, does this value influence
> > surrounding layout at all?  Should it?
> 
> I think it does and should. views::Label has a line_height() property, which
is
> fed through to gfx::RenderText::SetMinLineHeight(). Label::GetHeightForWidth
and
> GetTextSize() cater for line_height() in both the single-line and multi-line
> cases. And RenderText always tries to center the text its rendering in the
> display rectangle its given.

My worry here is that that means this line spacing is "the box height, within
which we should vertically center single lines of text".  Which is quite
different from "the spacing to place between the baseline of this font and the
font on the next line".

For example, a font of height 10, with line spacing of 20, placed atop a dialog,
will have 5 px of padding atop it in the first case, and 0 in the second.

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 08:26:25, tapted wrote:
> On 2017/03/15 06:38:48, Peter Kasting wrote:
> > On 2017/03/15 00:30:05, tapted wrote:
> > > Still working on the rest, but I came up with this `ExtendableEnum` to
solve
> > the
> > > layering while keeping the type safety and enum syntax. I think it will
> > work...
> > > (clang likes it :).
> > > 
> > > As far as "client" code goes, I think the only thing lost is the compiler
> > > warning "value Foo not handled in switch", but that's unavoidable.
> Otherwise,
> > > the switch syntax works just like an enum.
> > > 
> > > For declaring the `ExtendableEnum` the syntax is a bit more verbose
(almost
> > > Java-esque) and needs explicit initializers, but that's not too bad.
> > 
> > Hmm.  My take on this is that overall this feels more verbose to me than the
> > content/ method (of two enums), by such a large degree that it's hard to
read
> > and not worth doing, but that's also affected by my suspicion that the
> > compile-time typechecking from this will never catch any real bugs (and
> > therefore the benefits are low).
> > 
> > It's clever though :)
> 
> 
> The problem I see is that TextContext and TextStyle are (mostly)orthogonal
> concepts, but it they are both typedef to `int` it will be really easy to get
> them swapped around. On top of this, there are a lot of `default:` cases, and
> subtle differences, so it will be easy to get something that even looks
correct
> when run, but may cause subtle and hard to diagnose bugs if we later decide to
> tweak the typographyprovider (it may even depend on things from
ui::NativeTheme,
> so not even show up on test computers).

Hmm.  The confusion between those two is a fair thing to avoid.

When coupled with the issues with getting the correct provider (and my desire
not to provide a setter), this would push me to split the class into two halves
instead.  It just feels so gross :(.  Maybe I'm overreacting, I'm tired so my
judgment is questionable.

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:49: static constexpr const Value DIALOG_TITLE = 1;
I don't think you need both constexpr and const on a variable declaration, just
on a method (in C++14)?  I think on variables, constexpr still implies const
post-C++11?

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:52: static constexpr const Value CONTROL_LABEL = 3;
Skipped 2

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:67: enum { VIEWS_MAX = 15 };
15?  (2 places)

[tapted, 2017-03-15 11:04:07]

(didn't get everything, and I have some compilers to poke still)

Tomorrow, I'll try out replacing Storage() with a null-by-default pointer and
have TestViewsDelegate install the DefaultProvider rather than
CR_DEFINE_STATIC_LOCAL

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
On 2017/03/15 10:01:48, Peter Kasting wrote:
> On 2017/03/15 08:26:24, tapted wrote:
> > On 2017/03/14 23:53:33, Peter Kasting wrote:
> > > Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean
up
> on
> > > exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support
> SetProvider()
> > > below to allow this to be reset while running, in which case we do want to
> > clean
> > > up.
> > 
> > Why do we need cleanup in that case? Threads on chromium-dev suggest we want
> to
> > move away from exit-time destructors in a more general way. If it's needed,
I
> > was thinking views::Typography could provide a Reset() that just sets the
> > provider back to DefaultProvider.
> 
> We need to avoid leaking the thing we're replacing, since the program isn't
> shutting down.

I think that's taken care of - Storage() exposes its local member by reference,
so assigning to it will free whatever is held in the unique_ptr (it's just the
unique_ptr that's leaked, not the thing inside it).

> 
> > > Can we avoid SetProvider() by having this create the correct type of
> provider
> > > (harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming
there
> > that
> > > we may want one or the other, but it won't change at runtime.
> > 
> > I don't think we can do that -- views::Typography can't know about the other
> > providers the way that LayoutDelegate::Get() is able to do it for
> > HarmonyLayoutDelegate.
> 
> Unless we provide a delegate that does this, or split Typography across the
> views/chrome boundary a la ViewsDelegate (in which case we address the enum
that
> way too).
> 
> Swapping providers at runtime feels error-prone (what if we race the swap and
> query this thing ahead of time?), which would be another nice thing to avoid.

Hm. I wanted to explore something that provides a stronger "not going to be
null" guarantee than, e.g., ViewsDelegate. ui::NativeTheme does something like
this, but that's just per-platform (doesn't have to be delegated really). But
here we will always need a runtime delegate, since it's behind a chrome://flag

We could start with the provider being null, and assert() if it's queried before
being Set(), or Set() again when non-null. This would allow us to guarantee that
its set exactly once.

Then the 'DefaultTypographyProvider' would be installed perhaps by
TestViewsDelegate rather than being available by default. I think this will
work.

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:73: // Gets the line spacing, or 0 if it should be
provided by gfx::FontList.
On 2017/03/15 10:01:48, Peter Kasting wrote:
> On 2017/03/15 08:26:24, tapted wrote:
> > On 2017/03/14 23:53:33, Peter Kasting wrote:
> > > I admit it's not totally clear to me how this works.
> > > 
> > > From what I know, real-life line spacing in printing is typically
"baseline
> to
> > > baseline".  In views, we're typically laying out a single line of text,
with
> > > other things around it spaced some distance away from the top or bottom. 
> > > Occasionally we actually use a multi-line label, which could use this
value
> > > internally, but in the other (common) cases, does this value influence
> > > surrounding layout at all?  Should it?
> > 
> > I think it does and should. views::Label has a line_height() property, which
> is
> > fed through to gfx::RenderText::SetMinLineHeight(). Label::GetHeightForWidth
> and
> > GetTextSize() cater for line_height() in both the single-line and multi-line
> > cases. And RenderText always tries to center the text its rendering in the
> > display rectangle its given.
> 
> My worry here is that that means this line spacing is "the box height, within
> which we should vertically center single lines of text".  Which is quite
> different from "the spacing to place between the baseline of this font and the
> font on the next line".
> 
> For example, a font of height 10, with line spacing of 20, placed atop a
dialog,
> will have 5 px of padding atop it in the first case, and 0 in the second.

(disclaimer: I'm not a typesetting expert) I think RenderText tries to do
something smarter in the second case - there's a method with lots of arithmetic
- DetermineBaselineCenteringText -
https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?q=DetermineBaselin...

it doesn't seem to take into account any line_height "override" that we'd be
doing by making the line spacing 20px rather than the default (i.e.
Font::GetHeight()) -- the line_height override just affects the bounding box
within which the text is centered.

(of course multiline makes things more complicated)

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 10:01:48, Peter Kasting wrote:
> On 2017/03/15 08:26:25, tapted wrote:
> > On 2017/03/15 06:38:48, Peter Kasting wrote:
> > > On 2017/03/15 00:30:05, tapted wrote:
> > > > Still working on the rest, but I came up with this `ExtendableEnum` to
> solve
> > > the
> > > > layering while keeping the type safety and enum syntax. I think it will
> > > work...
> > > > (clang likes it :).
> > > > 
> > > > As far as "client" code goes, I think the only thing lost is the
compiler
> > > > warning "value Foo not handled in switch", but that's unavoidable.
> > Otherwise,
> > > > the switch syntax works just like an enum.
> > > > 
> > > > For declaring the `ExtendableEnum` the syntax is a bit more verbose
> (almost
> > > > Java-esque) and needs explicit initializers, but that's not too bad.
> > > 
> > > Hmm.  My take on this is that overall this feels more verbose to me than
the
> > > content/ method (of two enums), by such a large degree that it's hard to
> read
> > > and not worth doing, but that's also affected by my suspicion that the
> > > compile-time typechecking from this will never catch any real bugs (and
> > > therefore the benefits are low).
> > > 
> > > It's clever though :)
> > 
> > 
> > The problem I see is that TextContext and TextStyle are (mostly)orthogonal
> > concepts, but it they are both typedef to `int` it will be really easy to
get
> > them swapped around. On top of this, there are a lot of `default:` cases,
and
> > subtle differences, so it will be easy to get something that even looks
> correct
> > when run, but may cause subtle and hard to diagnose bugs if we later decide
to
> > tweak the typographyprovider (it may even depend on things from
> ui::NativeTheme,
> > so not even show up on test computers).
> 
> Hmm.  The confusion between those two is a fair thing to avoid.
> 
> When coupled with the issues with getting the correct provider (and my desire
> not to provide a setter), this would push me to split the class into two
halves
> instead.  It just feels so gross :(.  Maybe I'm overreacting, I'm tired so my
> judgment is questionable.

I think if we split the class like that we never be able to pass enum values
from the chrome "half" to a views::Label constructor and have it ask the
delegate on our behalf -- we'd go back to requiring manual and separate
configuration of font size, font weight, color, line height every time we create
a new views::Label.

(we could inherit from views::Label, or have ViewsDelegate provide a
"LabelFactory"..  but that's a whole world of other problems :/)

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:67: enum { VIEWS_MAX = 15 };
On 2017/03/15 10:01:48, Peter Kasting wrote:
> 15?  (2 places)

I guess I was going for
 - a buffer
 - not too big
 - 2^N - 1

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: virtual int GetLineSpacing(TextContext
context, TextStyle style) const = 0;
(note to self: rename to GetLineHeight to match views::Label?)

[Peter Kasting, 2017-03-15 17:52:47]

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
On 2017/03/15 11:04:06, tapted wrote:
> On 2017/03/15 10:01:48, Peter Kasting wrote:
> > On 2017/03/15 08:26:24, tapted wrote:
> > > On 2017/03/14 23:53:33, Peter Kasting wrote:
> > > > Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to clean
> up
> > on
> > > > exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support
> > SetProvider()
> > > > below to allow this to be reset while running, in which case we do want
to
> > > clean
> > > > up.
> > > 
> > > Why do we need cleanup in that case? Threads on chromium-dev suggest we
want
> > to
> > > move away from exit-time destructors in a more general way. If it's
needed,
> I
> > > was thinking views::Typography could provide a Reset() that just sets the
> > > provider back to DefaultProvider.
> > 
> > We need to avoid leaking the thing we're replacing, since the program isn't
> > shutting down.
> 
> I think that's taken care of - Storage() exposes its local member by
reference,
> so assigning to it will free whatever is held in the unique_ptr (it's just the
> unique_ptr that's leaked, not the thing inside it).

That was my point -- the unique_ptr is the thing that makes this possible.  I
was trying to get rid of the unique_ptr itself, which means disallowing changing
this at runtime.

> > > > Can we avoid SetProvider() by having this create the correct type of
> > provider
> > > > (harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming
> there
> > > that
> > > > we may want one or the other, but it won't change at runtime.
> > > 
> > > I don't think we can do that -- views::Typography can't know about the
other
> > > providers the way that LayoutDelegate::Get() is able to do it for
> > > HarmonyLayoutDelegate.
> > 
> > Unless we provide a delegate that does this, or split Typography across the
> > views/chrome boundary a la ViewsDelegate (in which case we address the enum
> that
> > way too).
> > 
> > Swapping providers at runtime feels error-prone (what if we race the swap
and
> > query this thing ahead of time?), which would be another nice thing to
avoid.
> 
> Hm. I wanted to explore something that provides a stronger "not going to be
> null" guarantee than, e.g., ViewsDelegate.

ViewsDelegate should be guaranteed to be non-null.  All null checks in the code
should be removed.  Any test that then breaks because of not having a
ViewsDelegate must instantiate one.

Sorry if this didn't get communicated more broadly :)

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:67: enum { VIEWS_MAX = 15 };
On 2017/03/15 11:04:07, tapted wrote:
> On 2017/03/15 10:01:48, Peter Kasting wrote:
> > 15?  (2 places)
> 
> I guess I was going for
>  - a buffer
>  - not too big
>  - 2^N - 1

Do we need a buffer?  That's only important if these values can't be changed. 
But the values aren't persisted on disk/network anywhere, so it seems OK to have
the last value just be the max, and if we add future values, everything gets
renumbered at recompile.

[tapted, 2017-03-15 22:43:35]

(just following up on 3 threads here with some "next steps")

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.cc:80: (new DefaultProvider));
On 2017/03/15 17:52:47, Peter Kasting wrote:
> On 2017/03/15 11:04:06, tapted wrote:
> > On 2017/03/15 10:01:48, Peter Kasting wrote:
> > > On 2017/03/15 08:26:24, tapted wrote:
> > > > On 2017/03/14 23:53:33, Peter Kasting wrote:
> > > > > Hmm.  Using a unique_ptr here is kind of a mess.  We don't want to
clean
> > up
> > > on
> > > > > exit (hence CR_DEFINE_STATIC_LOCAL), but we're trying to support
> > > SetProvider()
> > > > > below to allow this to be reset while running, in which case we do
want
> to
> > > > clean
> > > > > up.
> > > > 
> > > > Why do we need cleanup in that case? Threads on chromium-dev suggest we
> want
> > > to
> > > > move away from exit-time destructors in a more general way. If it's
> needed,
> > I
> > > > was thinking views::Typography could provide a Reset() that just sets
the
> > > > provider back to DefaultProvider.
> > > 
> > > We need to avoid leaking the thing we're replacing, since the program
isn't
> > > shutting down.
> > 
> > I think that's taken care of - Storage() exposes its local member by
> reference,
> > so assigning to it will free whatever is held in the unique_ptr (it's just
the
> > unique_ptr that's leaked, not the thing inside it).
> 
> That was my point -- the unique_ptr is the thing that makes this possible.  I
> was trying to get rid of the unique_ptr itself, which means disallowing
changing
> this at runtime.
> 
> > > > > Can we avoid SetProvider() by having this create the correct type of
> > > provider
> > > > > (harmony or not) as needed, a la LayoutDelegate::Get()?  I'm assuming
> > there
> > > > that
> > > > > we may want one or the other, but it won't change at runtime.
> > > > 
> > > > I don't think we can do that -- views::Typography can't know about the
> other
> > > > providers the way that LayoutDelegate::Get() is able to do it for
> > > > HarmonyLayoutDelegate.
> > > 
> > > Unless we provide a delegate that does this, or split Typography across
the
> > > views/chrome boundary a la ViewsDelegate (in which case we address the
enum
> > that
> > > way too).
> > > 
> > > Swapping providers at runtime feels error-prone (what if we race the swap
> and
> > > query this thing ahead of time?), which would be another nice thing to
> avoid.
> > 
> > Hm. I wanted to explore something that provides a stronger "not going to be
> > null" guarantee than, e.g., ViewsDelegate.
> 
> ViewsDelegate should be guaranteed to be non-null.  All null checks in the
code
> should be removed.  Any test that then breaks because of not having a
> ViewsDelegate must instantiate one.
> 
> Sorry if this didn't get communicated more broadly :)

I'm going to try piggy-backing off the ViewsDelegate some more.

and probably add (namespace) views::typography::GetFont(.., ..) to make the call
sites bearable :)

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
ui/views/style/typography.h:73: // Gets the line spacing, or 0 if it should be
provided by gfx::FontList.
On 2017/03/15 11:04:06, tapted wrote:
> On 2017/03/15 10:01:48, Peter Kasting wrote:
> > On 2017/03/15 08:26:24, tapted wrote:
> > > On 2017/03/14 23:53:33, Peter Kasting wrote:
> > > > I admit it's not totally clear to me how this works.
> > > > 
> > > > From what I know, real-life line spacing in printing is typically
> "baseline
> > to
> > > > baseline".  In views, we're typically laying out a single line of text,
> with
> > > > other things around it spaced some distance away from the top or bottom.

> > > > Occasionally we actually use a multi-line label, which could use this
> value
> > > > internally, but in the other (common) cases, does this value influence
> > > > surrounding layout at all?  Should it?
> > > 
> > > I think it does and should. views::Label has a line_height() property,
which
> > is
> > > fed through to gfx::RenderText::SetMinLineHeight().
Label::GetHeightForWidth
> > and
> > > GetTextSize() cater for line_height() in both the single-line and
multi-line
> > > cases. And RenderText always tries to center the text its rendering in the
> > > display rectangle its given.
> > 
> > My worry here is that that means this line spacing is "the box height,
within
> > which we should vertically center single lines of text".  Which is quite
> > different from "the spacing to place between the baseline of this font and
the
> > font on the next line".
> > 
> > For example, a font of height 10, with line spacing of 20, placed atop a
> dialog,
> > will have 5 px of padding atop it in the first case, and 0 in the second.
> 
> (disclaimer: I'm not a typesetting expert) I think RenderText tries to do
> something smarter in the second case - there's a method with lots of
arithmetic
> - DetermineBaselineCenteringText -
>
https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?q=DetermineBaselin...
> 
> it doesn't seem to take into account any line_height "override" that we'd be
> doing by making the line spacing 20px rather than the default (i.e.
> Font::GetHeight()) -- the line_height override just affects the bounding box
> within which the text is centered.
> 
> (of course multiline makes things more complicated)

bettes@ recently pointed me to https://goto.google.com/ukysc which talks about
line spacing in the MD spec and may have some guidance here

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 11:04:07, tapted wrote:
> On 2017/03/15 10:01:48, Peter Kasting wrote:
> > On 2017/03/15 08:26:25, tapted wrote:
> > > On 2017/03/15 06:38:48, Peter Kasting wrote:
> > > > On 2017/03/15 00:30:05, tapted wrote:
> > > > > Still working on the rest, but I came up with this `ExtendableEnum` to
> > solve
> > > > the
> > > > > layering while keeping the type safety and enum syntax. I think it
will
> > > > work...
> > > > > (clang likes it :).
> > > > > 
> > > > > As far as "client" code goes, I think the only thing lost is the
> compiler
> > > > > warning "value Foo not handled in switch", but that's unavoidable.
> > > Otherwise,
> > > > > the switch syntax works just like an enum.
> > > > > 
> > > > > For declaring the `ExtendableEnum` the syntax is a bit more verbose
> > (almost
> > > > > Java-esque) and needs explicit initializers, but that's not too bad.
> > > > 
> > > > Hmm.  My take on this is that overall this feels more verbose to me than
> the
> > > > content/ method (of two enums), by such a large degree that it's hard to
> > read
> > > > and not worth doing, but that's also affected by my suspicion that the
> > > > compile-time typechecking from this will never catch any real bugs (and
> > > > therefore the benefits are low).
> > > > 
> > > > It's clever though :)
> > > 
> > > 
> > > The problem I see is that TextContext and TextStyle are (mostly)orthogonal
> > > concepts, but it they are both typedef to `int` it will be really easy to
> get
> > > them swapped around. On top of this, there are a lot of `default:` cases,
> and
> > > subtle differences, so it will be easy to get something that even looks
> > correct
> > > when run, but may cause subtle and hard to diagnose bugs if we later
decide
> to
> > > tweak the typographyprovider (it may even depend on things from
> > ui::NativeTheme,
> > > so not even show up on test computers).
> > 
> > Hmm.  The confusion between those two is a fair thing to avoid.
> > 
> > When coupled with the issues with getting the correct provider (and my
desire
> > not to provide a setter), this would push me to split the class into two
> halves
> > instead.  It just feels so gross :(.  Maybe I'm overreacting, I'm tired so
my
> > judgment is questionable.
> 
> I think if we split the class like that we never be able to pass enum values
> from the chrome "half" to a views::Label constructor and have it ask the
> delegate on our behalf -- we'd go back to requiring manual and separate
> configuration of font size, font weight, color, line height every time we
create
> a new views::Label.
> 
> (we could inherit from views::Label, or have ViewsDelegate provide a
> "LabelFactory"..  but that's a whole world of other problems :/)

I dreamed up a way that would let us have an enum and let us `DCHECK` that they
haven't been swapped around. I think that will satisfy me enough to drop the
type safety (and I get the impression nobody's really excited about
ExtendableEnum).

We can merely require that all TextStyle enum values are greater than, say,
10,000, and require TypographyProviders to check.

[tapted, 2017-03-15 23:03:09]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 23:03:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 00:05:03]

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

[commit-bot: I haz the power, 2017-03-16 00:05:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Peter Kasting, 2017-03-16 00:17:47]

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/260001/ui/views/style/typogra...
ui/views/style/typography.h:43: class VIEWS_EXPORT TextContext : public
ExtendableEnum<TextContext> {
On 2017/03/15 22:43:35, tapted wrote:
> On 2017/03/15 11:04:07, tapted wrote:
> > On 2017/03/15 10:01:48, Peter Kasting wrote:
> > > On 2017/03/15 08:26:25, tapted wrote:
> > > > On 2017/03/15 06:38:48, Peter Kasting wrote:
> > > > > On 2017/03/15 00:30:05, tapted wrote:
> > > > > > Still working on the rest, but I came up with this `ExtendableEnum`
to
> > > solve
> > > > > the
> > > > > > layering while keeping the type safety and enum syntax. I think it
> will
> > > > > work...
> > > > > > (clang likes it :).
> > > > > > 
> > > > > > As far as "client" code goes, I think the only thing lost is the
> > compiler
> > > > > > warning "value Foo not handled in switch", but that's unavoidable.
> > > > Otherwise,
> > > > > > the switch syntax works just like an enum.
> > > > > > 
> > > > > > For declaring the `ExtendableEnum` the syntax is a bit more verbose
> > > (almost
> > > > > > Java-esque) and needs explicit initializers, but that's not too bad.
> > > > > 
> > > > > Hmm.  My take on this is that overall this feels more verbose to me
than
> > the
> > > > > content/ method (of two enums), by such a large degree that it's hard
to
> > > read
> > > > > and not worth doing, but that's also affected by my suspicion that the
> > > > > compile-time typechecking from this will never catch any real bugs
(and
> > > > > therefore the benefits are low).
> > > > > 
> > > > > It's clever though :)
> > > > 
> > > > 
> > > > The problem I see is that TextContext and TextStyle are
(mostly)orthogonal
> > > > concepts, but it they are both typedef to `int` it will be really easy
to
> > get
> > > > them swapped around. On top of this, there are a lot of `default:`
cases,
> > and
> > > > subtle differences, so it will be easy to get something that even looks
> > > correct
> > > > when run, but may cause subtle and hard to diagnose bugs if we later
> decide
> > to
> > > > tweak the typographyprovider (it may even depend on things from
> > > ui::NativeTheme,
> > > > so not even show up on test computers).
> > > 
> > > Hmm.  The confusion between those two is a fair thing to avoid.
> > > 
> > > When coupled with the issues with getting the correct provider (and my
> desire
> > > not to provide a setter), this would push me to split the class into two
> > halves
> > > instead.  It just feels so gross :(.  Maybe I'm overreacting, I'm tired so
> my
> > > judgment is questionable.
> > 
> > I think if we split the class like that we never be able to pass enum values
> > from the chrome "half" to a views::Label constructor and have it ask the
> > delegate on our behalf -- we'd go back to requiring manual and separate
> > configuration of font size, font weight, color, line height every time we
> create
> > a new views::Label.
> > 
> > (we could inherit from views::Label, or have ViewsDelegate provide a
> > "LabelFactory"..  but that's a whole world of other problems :/)
> 
> I dreamed up a way that would let us have an enum and let us `DCHECK` that
they
> haven't been swapped around. I think that will satisfy me enough to drop the
> type safety (and I get the impression nobody's really excited about
> ExtendableEnum).
> 
> We can merely require that all TextStyle enum values are greater than, say,
> 10,000, and require TypographyProviders to check.

Yeah, that'd work.

Heck, make it a bitfield and just OR them all together into one number.  :D

[tapted, 2017-03-16 05:25:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 05:25:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 05:40:57]

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

[commit-bot: I haz the power, 2017-03-16 05:40:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-16 06:14:18]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 06:14:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-16 06:15:27]

Patchset #14 (id:420001) has been deleted

[tapted, 2017-03-16 06:15:52]

Patchset #10 (id:340001) has been deleted

[tapted, 2017-03-16 06:16:12]

Patchset #10 (id:360001) has been deleted

[commit-bot: I haz the power, 2017-03-16 06:49:28]

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

[commit-bot: I haz the power, 2017-03-16 06:49:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tapted, 2017-03-16 07:49:40]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 07:49:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-16 07:51:33]

Patchset #13 (id:460001) has been deleted

[commit-bot: I haz the power, 2017-03-16 07:52:20]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-03-16 08:08:05]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 08:08:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-16 08:10:44]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 08:11:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 09:04:54]

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

[commit-bot: I haz the power, 2017-03-16 09:04:55]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-16 11:13:40]

PTAL - I think I've addressed everything

 - TypographyProvider now provided by the ViewsDelegate
 - vanilla enums
 - I'm was too wary of making a bitmask - I think it would add a layer of
complexity that's not needed.
 - I tried to find the right balance in the naming to avoid repetition (e.g.
typography::TYPE_FOO)
 - A few tests failed because they had no ViewsDelegate - fixed 'em

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:49: static constexpr const Value DIALOG_TITLE = 1;
On 2017/03/15 10:01:48, Peter Kasting wrote:
> I don't think you need both constexpr and const on a variable declaration,
just
> on a method (in C++14)?  I think on variables, constexpr still implies const
> post-C++11?

You are correct! (but, no longer using this)

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:52: static constexpr const Value CONTROL_LABEL = 3;
On 2017/03/15 10:01:48, Peter Kasting wrote:
> Skipped 2

no longer using this (it is perilous, clearly ;)

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography.h:67: enum { VIEWS_MAX = 15 };
On 2017/03/15 17:52:47, Peter Kasting wrote:
> On 2017/03/15 11:04:07, tapted wrote:
> > On 2017/03/15 10:01:48, Peter Kasting wrote:
> > > 15?  (2 places)
> > 
> > I guess I was going for
> >  - a buffer
> >  - not too big
> >  - 2^N - 1
> 
> Do we need a buffer?  That's only important if these values can't be changed. 
> But the values aren't persisted on disk/network anywhere, so it seems OK to
have
> the last value just be the max, and if we add future values, everything gets
> renumbered at recompile.

buffer thinking was that there would be less chance of someone forgetting to
update VIEWS_MAX when adding something and "15 constants is all that views would
ever need".

We now need a buffer of a different kind (between context and style, I picked
0x1000)

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2734113006/diff/380001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: virtual int GetLineSpacing(TextContext
context, TextStyle style) const = 0;
On 2017/03/15 11:04:07, tapted wrote:
> (note to self: rename to GetLineHeight to match views::Label?)

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: title_context =
CONTEXT_DIALOG_TEXT_SMALL;
gcc refused to do a ternary if-else with two different enum types :/

[sky, 2017-03-16 16:23:51]

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.cc:54: Init(text, typography::GetFont(text_context,
text_style));
Is the plan to change other controls to call to GetFont()? For example, in
Textfield?

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:16: namespace typography {
How about putting this in the style namespace? Two reason for that. 'style'
matches the name of the directory, which is one of the agreed upon ways to use
namespaces, and 'style' is much shorter, and most importantly I don't have to
think about how to spell 'style':)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:21: VIEWS_TEXT_CONTEXT_START = 0,
Can all these enums be named consistently? e.g. CONTEXT_START, CONTEXT_END,
CONTEXT_MAX? Similar with TextStyle enums should also start with STYLE_, e.g.
STYLE_START, STYLE_END...

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:26: // Text to label a control, usually next to it.
"Body 2". Usually 12pt.
This description implies it's only for labels near other controls. Why not just
CONTEXT_LABEL?

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:29: // An editable text field. Usually matches
CONTROL_LABEL.
That you say 'editable' implies we may have different fonts for editable vs
uneditable. Is that true? I also favor naming this CONTEXT_TEXT_FIELD.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:30: CONTEXT_FIELD,
CONTEXT_TEXT_FIELD.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:33: CONTEXT_BUTTON_TEXT,
CONTEXT_BUTTON (for consistency with other names).

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:45: VIEWS_TEXT_STYLE_START = TEXT_CONTEXT_MAX,
Why does this need to start at TEXT_CONTEXT_MAX? Aren't the style and context
separate values?

[Peter Kasting, 2017-03-16 19:33:13]

> I'm was too wary of making a bitmask - I think it would add a layer of
complexity that's not needed.

I was not serious :).  (Bitmask would be a bad idea.)

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: title_context =
CONTEXT_DIALOG_TEXT_SMALL;
On 2017/03/16 11:13:39, tapted wrote:
> gcc refused to do a ternary if-else with two different enum types :/

You can do it if you cast one of them to int (and then the compiler will coerce
the other), but that might be ugly.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:45: VIEWS_TEXT_STYLE_START = TEXT_CONTEXT_MAX,
On 2017/03/16 16:23:51, sky wrote:
> Why does this need to start at TEXT_CONTEXT_MAX? Aren't the style and context
> separate values?

This was explained in previous CL comments, but it should be explained in code
comments here.  Basically, using distinct value ranges lets us get runtime
safety via DCHECKs that people have not swapped the context and style values.

[sky, 2017-03-16 20:48:47]

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:45: VIEWS_TEXT_STYLE_START = TEXT_CONTEXT_MAX,
On 2017/03/16 19:33:13, Peter Kasting wrote:
> On 2017/03/16 16:23:51, sky wrote:
> > Why does this need to start at TEXT_CONTEXT_MAX? Aren't the style and
context
> > separate values?
> 
> This was explained in previous CL comments, but it should be explained in code
> comments here.  Basically, using distinct value ranges lets us get runtime
> safety via DCHECKs that people have not swapped the context and style values.

Ok, got it.

[Peter Kasting, 2017-03-17 02:26:01]

I like how this is working in general.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:235: int
rating_font, user_count_font;
Nit: Naming wise I'd prefer "context" or "text_context" or something to "font".

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:14: CONTEXT_HEADLINE =
views::typography::VIEWS_TEXT_CONTEXT_END + 1,
Nit: No need for + 1, and it's confusing (2 places)

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE,
The one discomfort I have with putting "DIALOG" in these names is that it
implies they should not be used outside dialogs, and I don't know whether that's
true.  Something generic like "Body 1" might actually make sense if we think
these values should be applied to all UI in views, dialog or no.  It also makes
it easier to map code directly to mocks, though it makes it harder to read the
code in the absence of those mocks and get as much semantic meaning out of it.

There's no real need to get this perfect now as we can change it later, but I
wanted to raise this so you could consider the tradeoffs.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:20:
CONTEXT_DIALOG_TEXT_SMALL,
Nit: "MESSAGE" above and "TEXT" here seem like they should be the same word? 
Maybe change to use TEXT for both?

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:22: //
ResourceBundle::SmallFont (11 pt).
Nit: If this is "DEPRECATED", talk about what that means, why it's here, and
what code that uses this should generally do instead (if there is any guiding
principle).

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:39: // Used to draw
attention to a web domain.
Without reading the code, I'd have asked: Does this mean a site/link/URL? 
"Domain" is a very specific part of a URL, so its use here makes me uncertain
about the use of this.  I imagine future readers might also have this question.

Also, "draw attention to" could mean just about anything.  Does that mean people
shouldn't use this if they're not trying to "draw attention"?  Same question on
extensions below.

For both of these reasons, I wonder if we should just say these two are
STYLE_EMPHASIZED or something.  The other values seem very specific and hard to
use or generalize.

Are these expected to be long-lived contexts, or are they effectively
deprecated?

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.cc:91: //
LegacyTypographyProvider.
I'm confused by this comment.  HarmonyLayoutDelegate isn't overriding this
method in this patch.  What does "shouldn't inherit" mean here, then?

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.h:103: virtual const
views::TypographyProvider& GetTypographyProvider() const;
Nit: Add comment.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:18: using
views::typography::GetFont;
Nit: Consider moving this to the one function below that actually uses it.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:79: gfx::FontList
small_font = rb.GetFontList(ResourceBundle::SmallFont);
BTW, are we trying to phase out these various ResourceBundle font specifiers
over time?  Does that come later?  Do we need a note in ResourceBundle marking
these as deprecated and saying what to do instead?

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:104: // Check that
asking for fonts of a given size match the Harmony spec.
Nit: Might want to write in the comment what action someone should take if this
test starts failing in the future.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:182: // Test that
the default TypographyProivder correctly maps TextContexts relative
Nit: Provider

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_item_view.cc:100: constexpr int
kUpperLabelStyle = views::typography::STYLE_PRIMARY;
Nit: Might just inline these two below, seems just as clear as the temps

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/subtle_notification_view.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/subtle_notification_view.cc:40: // really a dialog, but
a dialog title is a good fit.
This is an example of why DIALOG in the context names may not be the best
choice.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.cc:26: #include "ui/base/resource/resource_bundle.h"
I think this #include can be removed now.

Also check if it can be removed from any other files where you've eliminated
references to ResourceBundle.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.cc:54: Init(text, typography::GetFont(text_context,
text_style));
On 2017/03/16 16:23:51, sky wrote:
> Is the plan to change other controls to call to GetFont()? For example, in
> Textfield?

We probably should eventually... this is connected to my question elsewhere
about killing the ResourceBundle font enum.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:40: // TOOD(tapted): Audit users of this class when MD
is default. Then add
Nit: TOOD?  :)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:43: struct CustomFont {
Nit: Just for the sake of keeping the diff minimal I might have given this a
non-explicit one-arg constructor (I know it's banned by the style guide, but I'm
OK with it as a transitionary step) to avoid the need for {} in various places
until the TODOs here are addressed.

But now that you've already done the work, I guess might as well not bother.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:57: // Construct a Label in the given |text_context|.
The text_style can change
Nit: Use || consistently

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.cc:16: DCHECK(ViewsDelegate::GetInstance());
Nit: I kinda don't think we should have this, or at least not here, but maybe it
was helping you track down bugs.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.cc:17: DCHECK(text_context > VIEWS_TEXT_CONTEXT_START
&&
Nit: Split to two DCHECKs, then change all three DCHECKs to use _LT, _GE, etc.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:21: VIEWS_TEXT_CONTEXT_START = 0,
On 2017/03/16 16:23:51, sky wrote:
> Can all these enums be named consistently? e.g. CONTEXT_START, CONTEXT_END,
> CONTEXT_MAX? Similar with TextStyle enums should also start with STYLE_, e.g.
> STYLE_START, STYLE_END...

I like having the "marker" values, which should never be used directly, have
different names.  CONTEXT_ are the real values, while TEXT_CONTEXT_ are markers.
 VIEWS_ prefixes marker values that are specific to the views side of the enum,
which is why MAX is different.

That said, this naming scheme could be one of the things a comment could talk
about, which explained why we're doing these markers.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:24: CONTEXT_DIALOG_TITLE,
Nit: Should be "= VIEWS_TEXT_CONTEXT_START" (then use >= in the DCHECK)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:58: STYLE_ACTIVE_TAB,
Nit: I tend to prefer STYLE_TAB_ACTIVE, which reads more poorly in English, but
groups better when alphabetizing.

Speaking of which, consider alphabetizing the CONTEXT_ and STYLE_ values.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography_provider.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.cc:19: constexpr gfx::Font::FontStyle
kFontStyle = gfx::Font::NORMAL;
Nit: Not sure deinlining this really buys much readability

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:32: virtual ~TypographyProvider() =
default;
Nit: Destructor before the other methods; see
http://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:51: // Describes the font that the default
GetFont() implementation uses. Always
Nit: Maybe "Describes" -> "Sets the |size_delta| and |font_weight| for"?

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:52: // writes a value, even for styles it
doesn't know about.
Nit: "writes a value" -> "sets values"?

[tapted, 2017-03-17 05:54:34]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 05:54:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 06:06:27]

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

[commit-bot: I haz the power, 2017-03-17 06:06:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-03-17 10:21:00]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 10:21:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-03-17 10:27:32]

Patchset #15 (id:520001) has been deleted

[tapted, 2017-03-17 10:30:09]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::Typography, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
Typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> TextContext::DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    TextContext::DIALOG_TITLE
-  GetFontListWithDelta(1) -> TextContext::DIALOG_MESSAGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    ChromeTextContext::DIALOG_TEXT_SMALL or TextContext::CONTROL_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> ChromeTextContext::DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> ChromeTextStyle::{WEB_DOMAIN,EXTENSION_NAME}
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

[tapted, 2017-03-17 10:33:11]

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:236: title_context =
CONTEXT_DIALOG_TEXT_SMALL;
On 2017/03/16 19:33:13, Peter Kasting wrote:
> On 2017/03/16 11:13:39, tapted wrote:
> > gcc refused to do a ternary if-else with two different enum types :/
> 
> You can do it if you cast one of them to int (and then the compiler will
coerce
> the other), but that might be ugly.

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:235: int
rating_font, user_count_font;
On 2017/03/17 02:25:59, Peter Kasting wrote:
> Nit: Naming wise I'd prefer "context" or "text_context" or something to
"font".

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:14: CONTEXT_HEADLINE =
views::typography::VIEWS_TEXT_CONTEXT_END + 1,
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: No need for + 1, and it's confusing (2 places)

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE,
On 2017/03/17 02:26:00, Peter Kasting wrote:
> The one discomfort I have with putting "DIALOG" in these names is that it
> implies they should not be used outside dialogs, and I don't know whether
that's
> true.  Something generic like "Body 1" might actually make sense if we think
> these values should be applied to all UI in views, dialog or no.  It also
makes
> it easier to map code directly to mocks, though it makes it harder to read the
> code in the absence of those mocks and get as much semantic meaning out of it.
> 
> There's no real need to get this perfect now as we can change it later, but I
> wanted to raise this so you could consider the tradeoffs.

How about BODY_TEXT_LARGE, BODY_TEXT_SMALL? (done)

(I think that gets me over the lack of intrinsic documentation in Body1/Body2,
although LARGE/SMALL feels somewhat reminiscent of the ResourceBundle constants
that get abused..)

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:20:
CONTEXT_DIALOG_TEXT_SMALL,
On 2017/03/17 02:25:59, Peter Kasting wrote:
> Nit: "MESSAGE" above and "TEXT" here seem like they should be the same word? 
> Maybe change to use TEXT for both?

(see previous comment)

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:22: //
ResourceBundle::SmallFont (11 pt).
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: If this is "DEPRECATED", talk about what that means, why it's here, and
> what code that uses this should generally do instead (if there is any guiding
> principle).

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:39: // Used to draw
attention to a web domain.
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Without reading the code, I'd have asked: Does this mean a site/link/URL? 
> "Domain" is a very specific part of a URL, so its use here makes me uncertain
> about the use of this.  I imagine future readers might also have this
question.
> 
> Also, "draw attention to" could mean just about anything.  Does that mean
people
> shouldn't use this if they're not trying to "draw attention"?  Same question
on
> extensions below.
> 
> For both of these reasons, I wonder if we should just say these two are
> STYLE_EMPHASIZED or something.  The other values seem very specific and hard
to
> use or generalize.
> 
> Are these expected to be long-lived contexts, or are they effectively
> deprecated?

I like STYLE_EMPHASIZED for now - it's a little too much like "this is bold",
but I guess that's what it is :/. There isn't a bold font in the spec (except
for button titles on Windows), so this is _probably_ going away. One of the
places where this is used doesn't have a mock - (hostname - first one at
http://crbug.com/700196 ). The other doesn't have a mock either - and I couldn't
figure out how to invoke it... and I just realised why! You can't. It's dead
code.

IDC_CREATE_SHORTCUTS is #ifdefed out on Mac in browser_command_controller.cc --
it's always disabled.

On other platforms, it's only added to app_menu_model.cc when
`extensions::util::IsNewBookmarkAppsEnabled()` is false, but the definition of
that is

bool IsNewBookmarkAppsEnabled() {
#if defined(OS_MACOSX)
  return base::CommandLine::ForCurrentProcess()->HasSwitch(
      switches::kEnableNewBookmarkApps);
#else
  return !base::CommandLine::ForCurrentProcess()->HasSwitch(
      switches::kDisableNewBookmarkApps);
#endif
}

I think we have fully launched this on non-mac, so that flag can be made
mac-only. Filed http://crbug.com/702551



Anyway, I wanted to steer clear of "STYLE_EMPHASIZED" since we might want to
highlight a hostname in some other manner under harmony - i.e. not necessarily
by making it bold, and not necessarily the same way other things are emphasized.
But we can worry about that later.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.cc:91: //
LegacyTypographyProvider.
On 2017/03/17 02:26:00, Peter Kasting wrote:
> I'm confused by this comment.  HarmonyLayoutDelegate isn't overriding this
> method in this patch.  What does "shouldn't inherit" mean here, then?

Sorry - yes, this was pretty unclear (static here feels weird, so I wanted to
comment). To make it a data member, HarmonyLayoutDelegate would inherit it,
which isn't right. But, with a static, when HarmonyLayoutDelegate *does*
override this method then this will never get created. Updated the comment.

  // This is not a data member because then HarmonyLayoutDelegate would inherit
  // it, even when it provides its own.

(but I'm open to other suggestions too :)

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.h:103: virtual const
views::TypographyProvider& GetTypographyProvider() const;
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Add comment.

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:18: using
views::typography::GetFont;
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Consider moving this to the one function below that actually uses it.

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:79: gfx::FontList
small_font = rb.GetFontList(ResourceBundle::SmallFont);
On 2017/03/17 02:26:00, Peter Kasting wrote:
> BTW, are we trying to phase out these various ResourceBundle font specifiers
> over time?  Does that come later?  Do we need a note in ResourceBundle marking
> these as deprecated and saying what to do instead?

Yes - I want to get rid of them. This will be easiest when a lot of Cocoa UI
that uses them is no longer needed. And I'm hoping that by gradually phasing out
views interfaces that accept a gfx::FontList them, they will be pointed to the
typography stuff.

But code that doesn't have access to views::style::* is going to need them, and
the current advice in resource_bundle.h , i.e., "Phase these out in favour of
client code providing their own constant with the desired font size delta." is
the correct hint for that code (i.e. to use GetFontListWithDelta() -- the
methods that take e.g. ResourceBundle::SmallFont already say "Deprecated":

  // Deprecated. Returns fonts using hard-coded size deltas implied by |style|.
  const gfx::FontList& GetFontList(FontStyle style);
  const gfx::Font& GetFont(FontStyle style);
.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:104: // Check that
asking for fonts of a given size match the Harmony spec.
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Might want to write in the comment what action someone should take if
this
> test starts failing in the future.

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:182: // Test that
the default TypographyProivder correctly maps TextContexts relative
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Provider

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/credentials_item_view.cc:100: constexpr int
kUpperLabelStyle = views::typography::STYLE_PRIMARY;
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Might just inline these two below, seems just as clear as the temps

Done.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/subtle_notification_view.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/subtle_notification_view.cc:40: // really a dialog, but
a dialog title is a good fit.
On 2017/03/17 02:26:00, Peter Kasting wrote:
> This is an example of why DIALOG in the context names may not be the best
> choice.

(removed from MESSAGE/BODY, but I think we still need it for TITLE)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.cc:26: #include "ui/base/resource/resource_bundle.h"
On 2017/03/17 02:26:00, Peter Kasting wrote:
> I think this #include can be removed now.
> 
> Also check if it can be removed from any other files where you've eliminated
> references to ResourceBundle.

Done. Yeah - most of the client code files still use them for icons and things,
but I missed this one.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.cc:54: Init(text, typography::GetFont(text_context,
text_style));
On 2017/03/17 02:26:00, Peter Kasting wrote:
> On 2017/03/16 16:23:51, sky wrote:
> > Is the plan to change other controls to call to GetFont()? For example, in
> > Textfield?
> 
> We probably should eventually... this is connected to my question elsewhere
> about killing the ResourceBundle font enum.

Yes! Everything should be using style::GetFont().

ResourceBundle's enums have haunted me for ages and I want to delete them
(largely because 'BaseFont' is usually 13pt on Mac and 12pt everywhere else,
then there's arithmetic piled on top of that in a whole lot of Cocoa and other
code, so they're impossible to change)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:40: // TOOD(tapted): Audit users of this class when MD
is default. Then add
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: TOOD?  :)

whoops. fixed. (I have spell checking for comments, but it ignores all caps and
CamelCase - doh)

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:43: struct CustomFont {
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Just for the sake of keeping the diff minimal I might have given this a
> non-explicit one-arg constructor (I know it's banned by the style guide, but
I'm
> OK with it as a transitionary step) to avoid the need for {} in various places
> until the TODOs here are addressed.
> 
> But now that you've already done the work, I guess might as well not bother.

Yeah, I also want any new code to be considering a TextContext off the bat when
they can, rather than have a FontList slip through unnoticed..

https://codereview.chromium.org/2734113006/diff/500001/ui/views/controls/labe...
ui/views/controls/label.h:57: // Construct a Label in the given |text_context|.
The text_style can change
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Use || consistently

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.cc:16: DCHECK(ViewsDelegate::GetInstance());
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: I kinda don't think we should have this, or at least not here, but maybe
it
> was helping you track down bugs.

removed. I didn't originally have it - the test crashes were a bit obtuse, but
it was simple enough to debug without this I guess. (I put this in to sweep for
more, but nothing turned up).

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.cc:17: DCHECK(text_context > VIEWS_TEXT_CONTEXT_START
&&
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Split to two DCHECKs, then change all three DCHECKs to use _LT, _GE, etc.

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:16: namespace typography {
On 2017/03/16 16:23:51, sky wrote:
> How about putting this in the style namespace? Two reason for that. 'style'
> matches the name of the directory, which is one of the agreed upon ways to use
> namespaces, and 'style' is much shorter, and most importantly I don't have to
> think about how to spell 'style':)

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:21: VIEWS_TEXT_CONTEXT_START = 0,
On 2017/03/17 02:26:00, Peter Kasting wrote:
> On 2017/03/16 16:23:51, sky wrote:
> > Can all these enums be named consistently? e.g. CONTEXT_START, CONTEXT_END,
> > CONTEXT_MAX? Similar with TextStyle enums should also start with STYLE_,
e.g.
> > STYLE_START, STYLE_END...
> 
> I like having the "marker" values, which should never be used directly, have
> different names.  CONTEXT_ are the real values, while TEXT_CONTEXT_ are
markers.
>  VIEWS_ prefixes marker values that are specific to the views side of the
enum,
> which is why MAX is different.
> 
> That said, this naming scheme could be one of the things a comment could talk
> about, which explained why we're doing these markers.

Yeah - that was the intent. I've added an explainer.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:24: CONTEXT_DIALOG_TITLE,
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: Should be "= VIEWS_TEXT_CONTEXT_START" (then use >= in the DCHECK)

Done (did the same for STYLE).

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:26: // Text to label a control, usually next to it.
"Body 2". Usually 12pt.
On 2017/03/16 16:23:51, sky wrote:
> This description implies it's only for labels near other controls. Why not
just
> CONTEXT_LABEL?

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:29: // An editable text field. Usually matches
CONTROL_LABEL.
On 2017/03/16 16:23:51, sky wrote:
> That you say 'editable' implies we may have different fonts for editable vs
> uneditable. Is that true? I also favor naming this CONTEXT_TEXT_FIELD.

There's no plan to make them different at this stage, but naming a 'context'
enum that encompasses both labels and fields. (I'm also considering the
possibility of using `context` to affect text alignment, or background colors in
a generic way, which might distinguish these two).

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:30: CONTEXT_FIELD,
On 2017/03/16 16:23:51, sky wrote:
> CONTEXT_TEXT_FIELD.

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:33: CONTEXT_BUTTON_TEXT,
On 2017/03/16 16:23:51, sky wrote:
> CONTEXT_BUTTON (for consistency with other names).

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography.h:58: STYLE_ACTIVE_TAB,
On 2017/03/17 02:26:00, Peter Kasting wrote:
> Nit: I tend to prefer STYLE_TAB_ACTIVE, which reads more poorly in English,
but
> groups better when alphabetizing.

Done.

> 
> Speaking of which, consider alphabetizing the CONTEXT_ and STYLE_ values.

I tried this out in a number of ways but it never felt right :/. Right now,
they're sorted roughly by size (context), or by frequency of use (for style).
Moving PRIMARY from the top, or making sizes non-monotonic felt weird.

Since typography.h and chrome_typography.h currently capture the entire spec I
don't think it will be common for values to be added or removed with regularity
(i.e. developers shouldn't be wondering "where do I put this"). Or if it's added
to the end, it's probably for an obscure case so satisfies the "frequency of
use" sorting.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography_provider.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.cc:19: constexpr gfx::Font::FontStyle
kFontStyle = gfx::Font::NORMAL;
On 2017/03/17 02:26:01, Peter Kasting wrote:
> Nit: Not sure deinlining this really buys much readability

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:32: virtual ~TypographyProvider() =
default;
On 2017/03/17 02:26:01, Peter Kasting wrote:
> Nit: Destructor before the other methods; see
> http://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:51: // Describes the font that the default
GetFont() implementation uses. Always
On 2017/03/17 02:26:01, Peter Kasting wrote:
> Nit: Maybe "Describes" -> "Sets the |size_delta| and |font_weight| for"?

Done.

https://codereview.chromium.org/2734113006/diff/500001/ui/views/style/typogra...
ui/views/style/typography_provider.h:52: // writes a value, even for styles it
doesn't know about.
On 2017/03/17 02:26:01, Peter Kasting wrote:
> Nit: "writes a value" -> "sets values"?

Done.

[commit-bot: I haz the power, 2017-03-17 11:05:47]

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

[commit-bot: I haz the power, 2017-03-17 11:05:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-17 20:59:39]

LGTM

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE,
On 2017/03/17 10:33:10, tapted wrote:
> On 2017/03/17 02:26:00, Peter Kasting wrote:
> > The one discomfort I have with putting "DIALOG" in these names is that it
> > implies they should not be used outside dialogs, and I don't know whether
> that's
> > true.  Something generic like "Body 1" might actually make sense if we think
> > these values should be applied to all UI in views, dialog or no.  It also
> makes
> > it easier to map code directly to mocks, though it makes it harder to read
the
> > code in the absence of those mocks and get as much semantic meaning out of
it.
> > 
> > There's no real need to get this perfect now as we can change it later, but
I
> > wanted to raise this so you could consider the tradeoffs.
> 
> How about BODY_TEXT_LARGE, BODY_TEXT_SMALL? (done)
> 
> (I think that gets me over the lack of intrinsic documentation in Body1/Body2,
> although LARGE/SMALL feels somewhat reminiscent of the ResourceBundle
constants
> that get abused..)

Yeah, I hear your concerns.  What if we called the first one just BODY_TEXT or
BODY_TEXT_NORMAL, to suggest that people should default to it if they don't know
what to do?

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.cc:91: //
LegacyTypographyProvider.
On 2017/03/17 10:33:10, tapted wrote:
> On 2017/03/17 02:26:00, Peter Kasting wrote:
> > I'm confused by this comment.  HarmonyLayoutDelegate isn't overriding this
> > method in this patch.  What does "shouldn't inherit" mean here, then?
> 
> Sorry - yes, this was pretty unclear (static here feels weird, so I wanted to
> comment). To make it a data member, HarmonyLayoutDelegate would inherit it,
> which isn't right. But, with a static, when HarmonyLayoutDelegate *does*
> override this method then this will never get created. Updated the comment.
> 
>   // This is not a data member because then HarmonyLayoutDelegate would
inherit
>   // it, even when it provides its own.
> 
> (but I'm open to other suggestions too :)

Ah.  That's fine if you want.  I think the comment assumes that the reader is
wondering "why wasn't this a nonstatic data member?".  Whereas I figured a
static local was the sane thing to do, so it was answering a question I didn't
have, so I was confused.

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:79: gfx::FontList
small_font = rb.GetFontList(ResourceBundle::SmallFont);
On 2017/03/17 10:33:10, tapted wrote:
> On 2017/03/17 02:26:00, Peter Kasting wrote:
> > BTW, are we trying to phase out these various ResourceBundle font specifiers
> > over time?  Does that come later?  Do we need a note in ResourceBundle
marking
> > these as deprecated and saying what to do instead?
> 
> Yes - I want to get rid of them. This will be easiest when a lot of Cocoa UI
> that uses them is no longer needed. And I'm hoping that by gradually phasing
out
> views interfaces that accept a gfx::FontList them, they will be pointed to the
> typography stuff.
> 
> But code that doesn't have access to views::style::* is going to need them,
and
> the current advice in resource_bundle.h , i.e., "Phase these out in favour of
> client code providing their own constant with the desired font size delta." is
> the correct hint for that code (i.e. to use GetFontListWithDelta() -- the
> methods that take e.g. ResourceBundle::SmallFont already say "Deprecated":
> 
>   // Deprecated. Returns fonts using hard-coded size deltas implied by
|style|.
>   const gfx::FontList& GetFontList(FontStyle style);
>   const gfx::Font& GetFont(FontStyle style);
> .

OK.  Would be nice to someday rename these ...Deprecated() or something to try
to push people off them, but it sounds like there should be no action taken in
this patch, or any immediate followup.

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:238: 
Nit: No need for blank line here now

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:41: // Used to draw
attention to section of body text such as an extension name or
Nit: section -> a section

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.h:103: // Returns the class that
maps views::style (TextContext and TextStyle) to
Nit: style -> style values (or style enums)

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:103: // tests fail,
the Harmony TypographyProvider needs to be updated to cater for
Nit: cater for -> handle the?

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
ui/views/style/typography.h:34: CONTEXT_TEXT_FIELD,
Nit: Is TEXTFIELD better than TEXT_FIELD?  The views class, for example, is
named Textfield, not TextField.

This is super minor, but I thought I'd ask.

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
ui/views/style/typography.h:50: // This allows them to be verified at runtime.
E.g. if they have been swapped.
Nit: Maybe "This allows the code to verify at runtime that arguments of the two
types have not been swapped."

[tapted, 2017-03-20 00:38:14]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 00:38:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 00:41:10]

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

[commit-bot: I haz the power, 2017-03-20 00:41:11]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tapted, 2017-03-20 04:18:44]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 04:18:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 06:31:53]

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

[commit-bot: I haz the power, 2017-03-20 06:31:55]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-20 07:33:36]

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/500001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:17: CONTEXT_DIALOG_MESSAGE,
On 2017/03/17 20:59:39, Peter Kasting wrote:
> On 2017/03/17 10:33:10, tapted wrote:
> > On 2017/03/17 02:26:00, Peter Kasting wrote:
> > > The one discomfort I have with putting "DIALOG" in these names is that it
> > > implies they should not be used outside dialogs, and I don't know whether
> > that's
> > > true.  Something generic like "Body 1" might actually make sense if we
think
> > > these values should be applied to all UI in views, dialog or no.  It also
> > makes
> > > it easier to map code directly to mocks, though it makes it harder to read
> the
> > > code in the absence of those mocks and get as much semantic meaning out of
> it.
> > > 
> > > There's no real need to get this perfect now as we can change it later,
but
> I
> > > wanted to raise this so you could consider the tradeoffs.
> > 
> > How about BODY_TEXT_LARGE, BODY_TEXT_SMALL? (done)
> > 
> > (I think that gets me over the lack of intrinsic documentation in
Body1/Body2,
> > although LARGE/SMALL feels somewhat reminiscent of the ResourceBundle
> constants
> > that get abused..)
> 
> Yeah, I hear your concerns.  What if we called the first one just BODY_TEXT or
> BODY_TEXT_NORMAL, to suggest that people should default to it if they don't
know
> what to do?

The specs don't really settle on one or the other as what's "normal". The way
I've interpreted it is that 13pt=Body1 tends to get used more often for the
"message" that appears at the top of a dialog. And 12pt=Body2 gets used more for
"instructions" or for text that needs to blend with control labels.

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/content_setting_bubble_contents.cc:238: 
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: No need for blank line here now

Done.

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_typography.h (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_typography.h:41: // Used to draw
attention to section of body text such as an extension name or
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: section -> a section

Done.

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate.h (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate.h:103: // Returns the class that
maps views::style (TextContext and TextStyle) to
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: style -> style values (or style enums)

Done.

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/540001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:103: // tests fail,
the Harmony TypographyProvider needs to be updated to cater for
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: cater for -> handle the?

Done.

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
ui/views/style/typography.h:34: CONTEXT_TEXT_FIELD,
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: Is TEXTFIELD better than TEXT_FIELD?  The views class, for example, is
> named Textfield, not TextField.
> 
> This is super minor, but I thought I'd ask.

Done (-> TEXTFIELD). Heh, niggles like this can be frustrating. I think
TEXTFIELD is more correct in this context for the reason you point out.
(although views::Textfield is a bit of an outlier in a "git grep" fight and,
e.g., the Cocoa class is NSTextField).

https://codereview.chromium.org/2734113006/diff/540001/ui/views/style/typogra...
ui/views/style/typography.h:50: // This allows them to be verified at runtime.
E.g. if they have been swapped.
On 2017/03/17 20:59:39, Peter Kasting wrote:
> Nit: Maybe "This allows the code to verify at runtime that arguments of the
two
> types have not been swapped."

Done.

[sky, 2017-03-20 16:33:12]

Two minor questions.

https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2346: // Disabled since this relies on machine
configuration. http://crbug.com/701241.
The bug points at the harmony bug, which isn't at all obvious how it relates to
this test. What's the plan to enable this test? I would expect the bots to be
configured in similar ways, but maybe you're referring to font differences
between OS version? If it's the later, it seems like fonts may always differ
between major OS version.

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
ui/views/style/typography.h:56: STYLE_PRIMARY = VIEWS_TEXT_STYLE_START,
Did you consider STYLE_ENABLED? I like that better as 'primary' implies there is
a secondary. 'enabled' also better aligns with 'disabled'.

[Peter Kasting, 2017-03-20 20:00:34]

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
ui/views/style/typography.h:56: STYLE_PRIMARY = VIEWS_TEXT_STYLE_START,
On 2017/03/20 16:33:11, sky wrote:
> Did you consider STYLE_ENABLED? I like that better as 'primary' implies there
is
> a secondary.

There is a "secondary"; it's over on the Chrome side.

[tapted, 2017-03-20 21:55:22]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891, 701241
==========

[tapted, 2017-03-20 22:31:43]

https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2734113006/diff/580001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:2346: // Disabled since this relies on machine
configuration. http://crbug.com/701241.
On 2017/03/20 16:33:11, sky wrote:
> The bug points at the harmony bug, which isn't at all obvious how it relates
to
> this test. What's the plan to enable this test? I would expect the bots to be
> configured in similar ways, but maybe you're referring to font differences
> between OS version? If it's the later, it seems like fonts may always differ
> between major OS version.

The plan would be to bring this up at the same time as the tests in
c/b/ui/views/harmony/layout_delegate_unittest.cc that also reference
http://crbug.com/701241

I hadn't referenced http://crbug.com/701241 in the CL description (added it
now). http://crbug.com/701241 is "Bring up views::Typography "change detector"
tests" and talks about the rationale for starting with these tests disabled, and
the plan to bring them up. 

Specifically for this RenderTextTest, it's referencing the 'standard' font sizes
from the Harmony spec with the goal to validate the assumption that it is safe
to rely on the result of gfx::Font::GetFontSize() on a "default OS
configuration" as a means to derive an appropriate line spacing.

For example, if typesetting that Latin string gives font glyphs that are "too
tall" then the assumption that we can add a constant to gfx::Font::GetFontSize()
to obtain the "in-spec" line spacing may be wrong (or may indicate a bug in how
GetFontSize() is calculated, which involves a bit of arithmetic and
platform-specific assumptions).

I could remove the EXPECT_EQ(<integer literal>, ..) checks below and this test
might not suffer from the flakiness I'm concerned about. But then it's harder to
identify whether the failure is due to a misconfigured bot. However, once the
tests in layout_delegate_unittest.cc are robust on all our bots, then it should
be safe just to switch this on regardless.



You can see in an earlier patchset -

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
- the lengths I was going to to get the tests green. There was one bot run on
Windows where the base font size was 11 rather than 12 - I need to smoke it out
and any like it.

The OS version also plays a role, especially on Mac - Apple changed the system
typeface from Lucida Grande to "Helvetica Neue" to "San Francisco" across 3
consecutive versions :|


So, yup, fonts are likely to differ between OS versions. But we need a way to
detect this change in any new OS release. The Harmony typography provider will
need platform, and perhaps OS-version-specific, logic to ensure that the
"default" configuration gives a line height of, e.g., 20. (But we can't just say
"line height is 20" because in a *non*-default configuration -- when a user has
configured larger fonts -- that number will be wrong).

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2734113006/diff/580001/ui/views/style/typogra...
ui/views/style/typography.h:56: STYLE_PRIMARY = VIEWS_TEXT_STYLE_START,
On 2017/03/20 20:00:34, Peter Kasting wrote:
> On 2017/03/20 16:33:11, sky wrote:
> > Did you consider STYLE_ENABLED? I like that better as 'primary' implies
there
> is
> > a secondary.
> 
> There is a "secondary"; it's over on the Chrome side.

There is that :). I also think ENABLED is too specific here -- DISABLED only
applies to text contexts where it makes sense for it to have a disabled state.
But PRIMARY applies to any text context -- it's not necessarily the inverse of
DISABLED.

E.g., in theory, a UI might toggle between PRIMARY/DISABLED or
SECONDARY/DISABLED or HINT/DISABLED -- and we can get rid of lot of Label:: data
members for "disabled" colors this way since the "disabled" state is orthogonal
to the "style". That is, calling View::SetEnabled(false) on a Label doesn't need
to change its *style* - we just override its current style with DISABLED until
something does SetEnabled(true).

[sky, 2017-03-20 23:55:04]

LGTM

[tapted, 2017-03-21 00:42:26]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-03-21 00:42:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2734113006/#ps580001
(title: "Rebase (conflict in layout_delegate.h due to r457774)")

[commit-bot: I haz the power, 2017-03-21 00:43:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 00:52:00]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1490056946715050,
"parent_rev": "33bc6b04dccef0bc2c1066dad202474e35e6dda7", "commit_rev":
"9577332120682b5400df78b16edcc0c759bb5881"}

[commit-bot: I haz the power, 2017-03-21 00:52:43]

Description was changed from

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891, 701241
==========

to

==========
"Bootstrap" a toolkit-views Typography spec.

Under Harmony, we want to abstract away decisions about font selection.
Harmony has a set of styles as part of the spec that encapsulate:
- Font (typeface)
- Font size, weight and color
- Line spacing ("leading")

gfx::Font[List] can represent Font, size and weight, but not color or
line spacing. Also callers typically _never_ care about typeface, but
APIs using gfx::Font are unable to encapsulate this. One side-effect is
that a lot of callers use gfx::Font::Derive(..) rather than
ResourceBundle::GetFontWithDelta(..) and so miss out on caching
benefits.

The typography concepts are split into "TextContext" and "TextStyle".
The former describes the location of the text, and the latter describes
its current state (e.g. Disabled) or other appearance (e.g. a Hyperlink)
in that context.

This CL exposes a set of abstract typography concepts at
views::style, and explores the impact on client code by refactoring
the views::Label constructor that takes a gfx::FontList to take a
Label::CustomFont instead. Where it makes sense, callers are migrated to
typography concepts rather than use Label::CustomFont.

The transformations typically look like:
- ui::ResourceBundle::MediumFont -> views::style::CONTEXT_DIALOG_TITLE "15pt"
- GetFontListWithDelta(ui::kTitleFontSizeDelta) ->
    views::style::CONTEXT_DIALOG_TITLE
-  GetFontListWithDelta(1) -> (chrome) CONTEXT_BODY_TEXT_LARGE "13pt"
- [default constructor] or ResourceBundle::BaseFont ->
    CONTEXT_BODY_TEXT_LARGE or views::style::CONTEXT_LABEL "12pt"
- ui::ResourceBundle::SmallFont -> (chrome) CONTEXT_DEPRECATED_SMALL
    (Harmony doesn't have an 11pt font in the spec)
- ui::ResourceBundle::BoldFont -> (chrome) STYLE_EMPHASIZED
    (Harmony doesn't have a "bold" font in the spec)

These transformations, and the default TypographyProvider, are chosen to
effectively be a "no-op" so that no existing dialogs change font sizes
or other properties.

Follow-ups will:
- Implement a Harmony TypographyProvider.
- Require all Label constructors to take a typography concept or
    Label::CustomFont.
- Remove Label::SetFontList, Label::Set*Color, etc. (use typography or
    merge into CustomFont).
- Remove gfx::FontList from the API of other toolkit-views classes.
- Audit remaining users of Label::CustomFont to see if they can be
    replaced by Typography concepts.
- Remove ResourceBundle::FontStyle (SmallFont, etc.)
- Remove ui/default_style.h (ui::kFooFontSizeDelta, etc.)

BUG=691891, 701241

Review-Url: https://codereview.chromium.org/2734113006
Cr-Commit-Position: refs/heads/master@{#458261}
Committed:
https://chromium.googlesource.com/chromium/src/+/9577332120682b5400df78b16edc...
==========

[commit-bot: I haz the power, 2017-03-21 00:52:46]

Committed patchset #17 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/9577332120682b5400df78b16edc...