Remove views::Label::SetDisabledColor(). Replace with typography colors.

Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev.com/2913933002

BUG=691891
Review-Url: https://codereview.chromium.org/2910153002
Cr-Commit-Position: refs/heads/master@{#476897}
Committed: https://chromium.googlesource.com/chromium/src/+/30fe63c7bb9623a59ca6346ae956f5290f0864be

[tapted, 2017-05-30 07:41:14]

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

[commit-bot: I haz the power, 2017-05-30 07:41:30]

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-05-30 07:51:07]

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

[commit-bot: I haz the power, 2017-05-30 07:51:08]

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-05-31 00:53:39]

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

[commit-bot: I haz the power, 2017-05-31 00:54:05]

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-05-31 01:09:56]

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

[commit-bot: I haz the power, 2017-05-31 01:09:57]

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

[tapted, 2017-05-31 02:48:23]

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

[commit-bot: I haz the power, 2017-05-31 02:48: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-05-31 03:53:18]

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

[commit-bot: I haz the power, 2017-05-31 03:53:19]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-31 05:10:11]

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

[commit-bot: I haz the power, 2017-05-31 05:10:23]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-31 05:29:15]

Description was changed from

==========
Remove views::Label::SetDisabledColor

Replace it with views::style::STYLE_DISABLED.

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. Alternatively, a
consumer calls it, but the Label is never actually disabled.

In Harmony, the typography spec distinguishes between disabled grey and
"hint" grey which is what consumers will actually want in Harmony.

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED when creating a Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

[tapted, 2017-05-31 05:29:15]

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

[commit-bot: I haz the power, 2017-05-31 06:11:01]

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

[commit-bot: I haz the power, 2017-05-31 06:11:02]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-31 06:24:42]

Description was changed from

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. Alternatively, a
consumer calls it, but the Label is never actually disabled.

In Harmony, the typography spec distinguishes between disabled grey and
"hint" grey which is what consumers will actually want in Harmony.

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED when creating a Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. Alternatively, a
consumer calls it, but the Label is never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers will actually want in Harmony.

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED when creating a Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

[tapted, 2017-05-31 06:24:56]

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

[commit-bot: I haz the power, 2017-05-31 06:25:06]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-31 06:34:44]

Description was changed from

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. Alternatively, a
consumer calls it, but the Label is never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers will actually want in Harmony.

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED when creating a Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

[tapted, 2017-05-31 06:39:15]

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

[commit-bot: I haz the power, 2017-05-31 06:39:24]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-31 06:42:20]

Patchset #5 (id:80001) has been deleted

[tapted, 2017-05-31 06:42:38]

Description was changed from

==========
Remove views::Label::SetDisabledColor(). Replace with typography.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

[tapted, 2017-05-31 06:43:11]

Description was changed from

==========
Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev/2913933002

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev.com/2913933002

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

[tapted, 2017-05-31 06:50:23]

Color \o/

Getting it bootstrapped turned into a bit of chicken-and-egg because of this
disabled weirdness..

In the follow-up, I'm running jobs with a NOTREACHED() in
Label::OnEnabledChanged(), so I'm pretty sure all the consumers are covered.

[commit-bot: I haz the power, 2017-05-31 08:00:32]

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

[commit-bot: I haz the power, 2017-05-31 08:00:33]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-06-01 04:55:58]

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:72: switch
(text_style) {
Seems like it might be OK to return the "red" and "green" colors as-is in this
case, since those are more physical than logical descriptors, so there's no real
"mapping"?

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:84: switch
(text_style) {
Did you consider trying to implement these colors using alpha blends of the
foreground and background colors with white and black?  For example, compute the
primary color initially by blending the foreground color with the default
background color, or blending the foreground color toward the opposite luma with
BlendTowardOppositeLuma().  Construct the disabled, hint, and secondary styles
by further blending the foreground or primary colors with the background color. 
Use GetReadableColor() over the background color to ensure the link color reads
properly.  Use GetReadableColor() with the foreground and
default-button's-background colors to compute the default button color.  Etc.

These sorts of routes might eliminate the need to bail out for all custom theme
cases above, plus make it clearer where these colors come from.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:207: //
"Cards accepted" label is "disabled" grey.
Nit: disabled -> hint?

This also applies in other files.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:217: :
base::MakeUnique<views::Label>(currency));
Nit: Nesting ?:s makes me unhappy.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:267: :
ui::NativeTheme::kColorId_LabelDisabledColor));
Is there a TODO here to use the typography provider to set the colors?  Seems
like this isn't really "disabled" any more than the other cases you're fixing.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:40: views::style::STYLE_DISABLED);
It doesn't seem like the context of this should be "disabled" either, it also
seems like it's just a grey label.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:85: void SetDisabledColorForLabelButton(SkColor
color);
Nit: I kinda wish we had a better way of doing this.

Have you ever tried token-based access control?  It's basically a way to give
specific classes access to individual methods:

class Label {
 public:
  // Most callers should not be setting an actual disabled color, but should be
  // doing ____ instead.
  class AllowedToUseSetDisabledColorToken {
   private:
    friend class LabelButton;

    AllowedToUseSetDisabledColorToken() = default;
  };

  void SetDisabledColorForLabelButton(
      SkColor color,
      const AllowedToUseSetDisabledColorToken& access_token);
  ...

Then only LabelButton can construct the token, so only it can call this method.

Maybe it's overkill here.

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:328: // Where the label appears in the UI. Passed in
from the constructor.
Nit: Maybe say this is a value defined in typography.h:TextContext or one of the
enums that extends it.

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/link...
ui/views/controls/link.cc:272: DCHECK(theme);
The old code conditionalized this.  Is it guaranteed true even in tests?

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography.cc:34: DCHECK(native_theme);
Why not just pass in by const ref then, as typography_provider.h does?

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.cc:60: return
theme.GetSystemColor(ui::NativeTheme::kColorId_LabelEnabledColor);
Nit: Simpler:

  return theme.GetSystemColor(
      (style == style::STYLE_DISABLED)
          ? ui::NativeTheme::kColorId_LabelDisabledColor
          : ui::NativeTheme::kColorId_LabelEnabledColor);

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.h:33: virtual SkColor GetColor(int context,
Nit: Be consistent everywhere about whether these have "text_" in front or not.

Currently, this class and subclasses don't have it here, but some subclasses do
have it on the args for GetFont(); and the very similar method in typography.h
has it as well.

[tapted, 2017-06-01 07:57:54]

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

[commit-bot: I haz the power, 2017-06-01 07:58:10]

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-06-01 09:03:39]

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

[commit-bot: I haz the power, 2017-06-01 09:03:40]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-01 11:22:19]

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:72: switch
(text_style) {
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Seems like it might be OK to return the "red" and "green" colors as-is in this
> case, since those are more physical than logical descriptors, so there's no
real
> "mapping"?

Done - Although I special-cased a white foreground to match the approach in
NativeThemeDarkAura (this will matter for the "Secure" text displayed in the
omnibox for incognito windows.. when we get around to migrating those labels to
typography styles).

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:84: switch
(text_style) {
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Did you consider trying to implement these colors using alpha blends of the
> foreground and background colors with white and black?  For example, compute
the
> primary color initially by blending the foreground color with the default
> background color, or blending the foreground color toward the opposite luma
with
> BlendTowardOppositeLuma().  Construct the disabled, hint, and secondary styles
> by further blending the foreground or primary colors with the background
color. 
> Use GetReadableColor() over the background color to ensure the link color
reads
> properly.  Use GetReadableColor() with the foreground and
> default-button's-background colors to compute the default button color.  Etc.
> 
> These sorts of routes might eliminate the need to bail out for all custom
theme
> cases above, plus make it clearer where these colors come from.

I didn't consider this.. But it might be overkill. There isn't much themed UI
that will pass in requests to this.

But it needs to start with something basic, otherwise the MD buttons in the
incognito downloads bar will attempt to draw black-on-black when we use
--secondary-ui-md..

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:207: //
"Cards accepted" label is "disabled" grey.
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: disabled -> hint?
> 
> This also applies in other files.

Done.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:217: :
base::MakeUnique<views::Label>(currency));
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: Nesting ?:s makes me unhappy.

Done.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:267: :
ui::NativeTheme::kColorId_LabelDisabledColor));
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Is there a TODO here to use the typography provider to set the colors?  Seems
> like this isn't really "disabled" any more than the other cases you're fixing.

Heh, yeah I'd considered this. But if the samples on the typography spec at
https://bugs.chromium.org/p/chromium/issues/attachment?aid=286827&inline=1 can
be trusted then it's likely that the email should be primary, and the text above
it should be the "hint". (In which case the TODO might belong closer to
`email_label`). So it may be more involved than just switching these to use
GetFont, but we won't know until this UI has a mock.

Left a note.

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/sad_tab_view.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/sad_tab_view.cc:40: views::style::STYLE_DISABLED);
On 2017/06/01 04:55:58, Peter Kasting wrote:
> It doesn't seem like the context of this should be "disabled" either, it also
> seems like it's just a grey label.

Done.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:85: void SetDisabledColorForLabelButton(SkColor
color);
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: I kinda wish we had a better way of doing this.
> 
> Have you ever tried token-based access control?  It's basically a way to give
> specific classes access to individual methods:
> 
> class Label {
>  public:
>   // Most callers should not be setting an actual disabled color, but should
be
>   // doing ____ instead.
>   class AllowedToUseSetDisabledColorToken {
>    private:
>     friend class LabelButton;
> 
>     AllowedToUseSetDisabledColorToken() = default;
>   };
> 
>   void SetDisabledColorForLabelButton(
>       SkColor color,
>       const AllowedToUseSetDisabledColorToken& access_token);
>   ...
> 
> Then only LabelButton can construct the token, so only it can call this
method.
> 
> Maybe it's overkill here.

The follow-up CL that removes this is basically ready for review in
https://codereview.chromium.org/2913933002 ..

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:328: // Where the label appears in the UI. Passed in
from the constructor.
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: Maybe say this is a value defined in typography.h:TextContext or one of
the
> enums that extends it.

Done.

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/ui/views/controls/link...
ui/views/controls/link.cc:272: DCHECK(theme);
On 2017/06/01 04:55:58, Peter Kasting wrote:
> The old code conditionalized this.  Is it guaranteed true even in tests?

Yup.

View::GetNativeTheme() and Widget::GetNativeTheme() always return non-null,
falling back to ui::NativeTheme::GetInstanceForNativeUi() (e.g., if there is no
GetWidget()).

(and GetInstanceForNativeUi() returns static locals.. except on Android where it
is NOTREACHED()/nullptr;)

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography.cc:34: DCHECK(native_theme);
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Why not just pass in by const ref then, as typography_provider.h does?

Then all the callsites need an extra character in front of GetNativeTheme() to
dereference it :).. 

I also considered that in future we could reasonably pass in nullptr and have
GetColor(), here, fall back to ui::NativeTheme::GetInstanceForNativeUi().
Currently those null checks are in view.cc or widget.cc, but it's possible
something will want a color when neither of those are around, like a Painter or
a Background.

(But I wanted it to be clear to the providers that they never have to consider a
null nativetheme, hence const-ref for those)

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.cc:60: return
theme.GetSystemColor(ui::NativeTheme::kColorId_LabelEnabledColor);
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return theme.GetSystemColor(
>       (style == style::STYLE_DISABLED)
>           ? ui::NativeTheme::kColorId_LabelDisabledColor
>           : ui::NativeTheme::kColorId_LabelEnabledColor);

Done.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.h:33: virtual SkColor GetColor(int context,
On 2017/06/01 04:55:58, Peter Kasting wrote:
> Nit: Be consistent everywhere about whether these have "text_" in front or
not.
> 
> Currently, this class and subclasses don't have it here, but some subclasses
do
> have it on the args for GetFont(); and the very similar method in typography.h
> has it as well.

Done. Updated the TypographyProvider hierarchy to use shorter names.
typography.h/cc are consistent with each other, but with long names. It was a
semi-concsious decision to use verbose names for the "consumer" API, while using
shorter ones for the "provider" API since they occur often there. But I'd be
comfortable with short everywhere too...

[Peter Kasting, 2017-06-01 19:08:01]

LGTM

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2910153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:84: switch
(text_style) {
On 2017/06/01 11:22:18, tapted wrote:
> On 2017/06/01 04:55:58, Peter Kasting wrote:
> > Did you consider trying to implement these colors using alpha blends of the
> > foreground and background colors with white and black?  For example, compute
> the
> > primary color initially by blending the foreground color with the default
> > background color, or blending the foreground color toward the opposite luma
> with
> > BlendTowardOppositeLuma().  Construct the disabled, hint, and secondary
styles
> > by further blending the foreground or primary colors with the background
> color. 
> > Use GetReadableColor() over the background color to ensure the link color
> reads
> > properly.  Use GetReadableColor() with the foreground and
> > default-button's-background colors to compute the default button color. 
Etc.
> > 
> > These sorts of routes might eliminate the need to bail out for all custom
> theme
> > cases above, plus make it clearer where these colors come from.
> 
> I didn't consider this.. But it might be overkill. There isn't much themed UI
> that will pass in requests to this.

Long-term, I'd like all the colors in all Chrome UI to respond to system
settings changes.  It doesn't happen in too many places currently, but I try to
push for code that accommodates it where I can.

> But it needs to start with something basic, otherwise the MD buttons in the
> incognito downloads bar will attempt to draw black-on-black when we use
> --secondary-ui-md..

FWIW, I'm totally fine with checking in something like what you have and having
a TODO for a more "generic" system, if any of my ideas for said system make
sense.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography.cc:34: DCHECK(native_theme);
On 2017/06/01 11:22:18, tapted wrote:
> On 2017/06/01 04:55:58, Peter Kasting wrote:
> > Why not just pass in by const ref then, as typography_provider.h does?
> 
> Then all the callsites need an extra character in front of GetNativeTheme() to
> dereference it :).. 
> 
> I also considered that in future we could reasonably pass in nullptr and have
> GetColor(), here, fall back to ui::NativeTheme::GetInstanceForNativeUi().
> Currently those null checks are in view.cc or widget.cc, but it's possible
> something will want a color when neither of those are around, like a Painter
or
> a Background.

Hmm.  That sounds like a "maybe it would be useful to pass null someday" rather
than an "in the future I'll be changing these to pass null".

In keeping with the last sentence in
http://google.github.io/styleguide/cppguide.html#Reference_Arguments , I'd
probably pass const ref and add the '*' at all callsites.  But I will leave the
final decision to you.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.h:33: virtual SkColor GetColor(int context,
On 2017/06/01 11:22:18, tapted wrote:
> On 2017/06/01 04:55:58, Peter Kasting wrote:
> > Nit: Be consistent everywhere about whether these have "text_" in front or
> not.
> > 
> > Currently, this class and subclasses don't have it here, but some subclasses
> do
> > have it on the args for GetFont(); and the very similar method in
typography.h
> > has it as well.
> 
> Done. Updated the TypographyProvider hierarchy to use shorter names.
> typography.h/cc are consistent with each other, but with long names. It was a
> semi-concsious decision to use verbose names for the "consumer" API, while
using
> shorter ones for the "provider" API since they occur often there. But I'd be
> comfortable with short everywhere too...

I have a weak preference for "short everywhere" or "long everywhere" since it
feels more consistent (these feel like passthroughs to each other and should
thus share names).

The main question in short-vs.-long would be whether the longer name makes
anything clearer to readers, and whether a comment on the function (about these
args being defined by the TextContext enum) is warranted if so.

https://codereview.chromium.org/2910153002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2910153002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:81: :
gfx::kGoogleRed700;
Can we handle this automatically by doing something like

  return GetReadableColor(gfx::kGoogleRed700,
some_expr_that_gets_the_background_color);

?

If so we could factor the red/green handling out of both these switches into a
separate block above.

[tapted, 2017-06-02 00:14:16]

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

[commit-bot: I haz the power, 2017-06-02 00:14:58]

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-06-02 00:18:09]

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

[commit-bot: I haz the power, 2017-06-02 00:18:10]

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-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...)

[tapted, 2017-06-02 00:36:38]

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

[commit-bot: I haz the power, 2017-06-02 00:37:10]

Dry run: CQ is trying da patch.

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

[tapted, 2017-06-02 01:29:13]

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

[tapted, 2017-06-02 01:29:15]

+sky for owners (ash/*, c/b/chromeos, and ui/chromeos), or any other thoughts
about this. Thanks!

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography.cc:34: DCHECK(native_theme);
On 2017/06/01 19:08:01, Peter Kasting wrote:
> On 2017/06/01 11:22:18, tapted wrote:
> > On 2017/06/01 04:55:58, Peter Kasting wrote:
> > > Why not just pass in by const ref then, as typography_provider.h does?
> > 
> > Then all the callsites need an extra character in front of GetNativeTheme()
to
> > dereference it :).. 
> > 
> > I also considered that in future we could reasonably pass in nullptr and
have
> > GetColor(), here, fall back to ui::NativeTheme::GetInstanceForNativeUi().
> > Currently those null checks are in view.cc or widget.cc, but it's possible
> > something will want a color when neither of those are around, like a Painter
> or
> > a Background.
> 
> Hmm.  That sounds like a "maybe it would be useful to pass null someday"
rather
> than an "in the future I'll be changing these to pass null".
> 
> In keeping with the last sentence in
> http://google.github.io/styleguide/cppguide.html#Reference_Arguments , I'd
> probably pass const ref and add the '*' at all callsites.  But I will leave
the
> final decision to you.

I feel a '*' at callsites is more cognitive load for the callers than the issue
raised in that sentence. i.e. forcing callers to dereference requires them to
think whether they should DCHECK or check for null or provide a fallback. So..
really I think GetNativeTheme() should return const-ref rather than a pointer.
But that's a bigger refactor and raises questions on android where it currently
can return null.

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

https://codereview.chromium.org/2910153002/diff/100001/ui/views/style/typogra...
ui/views/style/typography_provider.h:33: virtual SkColor GetColor(int context,
On 2017/06/01 19:08:01, Peter Kasting wrote:
> On 2017/06/01 11:22:18, tapted wrote:
> > On 2017/06/01 04:55:58, Peter Kasting wrote:
> > > Nit: Be consistent everywhere about whether these have "text_" in front or
> > not.
> > > 
> > > Currently, this class and subclasses don't have it here, but some
subclasses
> > do
> > > have it on the args for GetFont(); and the very similar method in
> typography.h
> > > has it as well.
> > 
> > Done. Updated the TypographyProvider hierarchy to use shorter names.
> > typography.h/cc are consistent with each other, but with long names. It was
a
> > semi-concsious decision to use verbose names for the "consumer" API, while
> using
> > shorter ones for the "provider" API since they occur often there. But I'd be
> > comfortable with short everywhere too...
> 
> I have a weak preference for "short everywhere" or "long everywhere" since it
> feels more consistent (these feel like passthroughs to each other and should
> thus share names).
> 
> The main question in short-vs.-long would be whether the longer name makes
> anything clearer to readers, and whether a comment on the function (about
these
> args being defined by the TextContext enum) is warranted if so.

I made https://codereview.chromium.org/2918003002/ to rename the rest to short
(to keep the diff here smaller).

https://codereview.chromium.org/2910153002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2910153002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:81: :
gfx::kGoogleRed700;
On 2017/06/01 19:08:01, Peter Kasting wrote:
> Can we handle this automatically by doing something like
> 
>   return GetReadableColor(gfx::kGoogleRed700,
> some_expr_that_gets_the_background_color);
> 
> ?
> 
> If so we could factor the red/green handling out of both these switches into a
> separate block above.

Getting the background color in a generic way is really hard. Most often, it's a
transparent background, but views::Label has specific logic to annotate a
predicted background to pick out readable colours (Label never actually draws
the color given to Label::SetBackgroundColor, nor even does it pass that on to
RenderText).

We could use the same fallback as Label
(ui::NativeTheme::kColorId_DialogBackground), but that's making a lot of
assumptions.

Left a TODO and some notes here.

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

https://codereview.chromium.org/2910153002/diff/180001/ui/views/controls/labe...
ui/views/controls/label.h:85: void SetDisabledColorForLabelButton(SkColor
color);
(note I'm getting rid of this here: https://codereview.chromium.org/2913933002 )

[commit-bot: I haz the power, 2017-06-02 01:39:13]

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

[commit-bot: I haz the power, 2017-06-02 01:39:14]

Dry run: This issue passed the CQ dry run.

[sky, 2017-06-02 14:14:23]

I'm assuming none of the calls to set disabled colors here actually mattered, so
LGTM

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc
(left):

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:544:
tap_label_->SetDisabledColor(kTapHereLabelColor);
Was this never needed?

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/ui/idle_app_name_notification_view.cc (left):

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/ui/idle_app_name_notification_view.cc:194:
label->SetDisabledColor(text_color);
I'm going to assume this was never needed either.

[tapted, 2017-06-03 09:46:55]

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc
(left):

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:544:
tap_label_->SetDisabledColor(kTapHereLabelColor);
On 2017/06/02 14:14:22, sky wrote:
> Was this never needed?

Nope - not needed. These labels are never disabled. Checked these by scanning
the file, and also testing with

void Label::OnEnabledChanged() {
  NOTREACHED();
}

in trybots in the follow up - only LabelButton really uses the disabled state.

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/ui/idle_app_name_notification_view.cc (left):

https://codereview.chromium.org/2910153002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/ui/idle_app_name_notification_view.cc:194:
label->SetDisabledColor(text_color);
On 2017/06/02 14:14:22, sky wrote:
> I'm going to assume this was never needed either.

Acknowledged.

[tapted, 2017-06-03 09:47:07]

The CQ bit was checked by tapted@chromium.org

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

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/2910153002/#ps180001
(title: "rebase for r476345")

[commit-bot: I haz the power, 2017-06-03 09:47:19]

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-06-03 12:04:06]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496483227381870,
"parent_rev": "ebc5387c163515bfa5a5dfcfa211cc2698afca54", "commit_rev":
"30fe63c7bb9623a59ca6346ae956f5290f0864be"}

[commit-bot: I haz the power, 2017-06-03 12:04:18]

Description was changed from

==========
Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev.com/2913933002

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

to

==========
Remove views::Label::SetDisabledColor(). Replace with typography colors.

Everything calling Label::SetDisabledColor() is just using it as a
roundabout way to make the text grey instead of black. This is bad
because "disabled" is a property that is fed through to a11y clients.
Alternatively, a consumer calls SetDisabledColor(), but the Label is
never actually disabled.

In Harmony, the typography spec distinguishes between "secondary" grey
and "hint" grey which is what consumers should actually use to describe
their text (once there's a mock which says which should be used).

Bootstrap views::style::GetColor() to support consumers that want a grey
Label to specify style::STYLE_DISABLED, or STYLE_HINT when creating a
Label.

Only LabelButton was using SetDisabledColor() properly. It sometimes
gets a disabled color from the NativeTheme and sometimes overrides it.
Nothing else needs this, so move disabled-label functionality to a Label
subclass in a follow-up: crrev.com/2913933002

BUG=691891

Review-Url: https://codereview.chromium.org/2910153002
Cr-Commit-Position: refs/heads/master@{#476897}
Committed:
https://chromium.googlesource.com/chromium/src/+/30fe63c7bb9623a59ca6346ae956...
==========

[commit-bot: I haz the power, 2017-06-03 12:04:20]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/30fe63c7bb9623a59ca6346ae956...