Harmony checkbox/radio tweaks

Harmony checkbox/radio tweaks

a) new focus ring is 2dp and somewhat transparent
b) adjustments to text and icon coloration

BUG=635156
Committed: https://crrev.com/ed836b02166a6f4ee65657137dc4577c99a2715b
Cr-Commit-Position: refs/heads/master@{#412015}

[Evan Stade, 2016-08-12 23:43:16]

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

[sky, 2016-08-13 16:03:32]

Assuming you want these changes all the time, LGTM . By that I mean some of the
changes aren't conditional, such as the checkbox one, but others are.

https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:109: static const SkColor kPrimaryTextColor =
SkColorSetRGB(0x33, 0x33, 0x33);
optional: Is there a reason you want this here, and not inside the conditional
where it is used? I would be tempted to combine the if statement into a single
one with the static inside it, but perhaps you have it like this because you
foresee more differences?

[Evan Stade, 2016-08-14 01:49:21]

It was my intent that all the changes to checkbox be conditional. Do you see one
that's not? I count 3 changes in checkbox.cc all guarded by UseMd().

https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
File ui/native_theme/common_theme.cc (right):

https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
ui/native_theme/common_theme.cc:109: static const SkColor kPrimaryTextColor =
SkColorSetRGB(0x33, 0x33, 0x33);
On 2016/08/13 16:03:32, sky wrote:
> optional: Is there a reason you want this here, and not inside the conditional
> where it is used?

I was following the convention in the rest of the file, where a bunch of color
constants are defined and then a switch below (or in this case, for now, a
conditional) uses the constants. I suspect more than one kColorId thing will
want kPrimaryTextColor because it's part of a three-tone palette that bettes@
came up with.

>  I would be tempted to combine the if statement into a single
> one with the static inside it, but perhaps you have it like this because you
> foresee more differences?

Indeed, I was assuming there would be more secondary-ui related color changes.
I'm not sure that will actually happen but it seems likely.

[Evan Stade, 2016-08-14 01:49:35]

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

[commit-bot: I haz the power, 2016-08-14 01:49:42]

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

[Evan Stade, 2016-08-14 01:51:32]

p.s. thanks a lot for these expeditious weekend reviews!

[commit-bot: I haz the power, 2016-08-14 02:17:01]

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

[commit-bot: I haz the power, 2016-08-14 02:17:02]

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

[sky, 2016-08-15 14:59:44]

On 2016/08/14 01:49:21, Evan Stade wrote:
> It was my intent that all the changes to checkbox be conditional. Do you see
one
> that's not? I count 3 changes in checkbox.cc all guarded by UseMd().

Sorry for not being clear. I was referring to the change in NativeTheme
considering ui::MaterialDesignController::IsSecondaryUiMaterial(), but the
changes in Checkbox, such as line 138 (new) doesn't. Yes, line 138 is behing an
md check, but not ui::MaterialDesignController::IsSecondaryUiMaterial().

> 
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
> File ui/native_theme/common_theme.cc (right):
> 
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
> ui/native_theme/common_theme.cc:109: static const SkColor kPrimaryTextColor =
> SkColorSetRGB(0x33, 0x33, 0x33);
> On 2016/08/13 16:03:32, sky wrote:
> > optional: Is there a reason you want this here, and not inside the
conditional
> > where it is used?
> 
> I was following the convention in the rest of the file, where a bunch of color
> constants are defined and then a switch below (or in this case, for now, a
> conditional) uses the constants. I suspect more than one kColorId thing will
> want kPrimaryTextColor because it's part of a three-tone palette that bettes@
> came up with.
> 
> >  I would be tempted to combine the if statement into a single
> > one with the static inside it, but perhaps you have it like this because you
> > foresee more differences?
> 
> Indeed, I was assuming there would be more secondary-ui related color changes.
> I'm not sure that will actually happen but it seems likely.

[Evan Stade, 2016-08-15 15:16:21]

On 2016/08/15 14:59:44, sky wrote:
> On 2016/08/14 01:49:21, Evan Stade wrote:
> > It was my intent that all the changes to checkbox be conditional. Do you see
> one
> > that's not? I count 3 changes in checkbox.cc all guarded by UseMd().
> 
> Sorry for not being clear. I was referring to the change in NativeTheme
> considering ui::MaterialDesignController::IsSecondaryUiMaterial(), but the
> changes in Checkbox, such as line 138 (new) doesn't. Yes, line 138 is behing
an
> md check, but not ui::MaterialDesignController::IsSecondaryUiMaterial().

UseMd() is shorthand for that check.

> 
> > 
> >
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
> > File ui/native_theme/common_theme.cc (right):
> > 
> >
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
> > ui/native_theme/common_theme.cc:109: static const SkColor kPrimaryTextColor
=
> > SkColorSetRGB(0x33, 0x33, 0x33);
> > On 2016/08/13 16:03:32, sky wrote:
> > > optional: Is there a reason you want this here, and not inside the
> conditional
> > > where it is used?
> > 
> > I was following the convention in the rest of the file, where a bunch of
color
> > constants are defined and then a switch below (or in this case, for now, a
> > conditional) uses the constants. I suspect more than one kColorId thing will
> > want kPrimaryTextColor because it's part of a three-tone palette that
bettes@
> > came up with.
> > 
> > >  I would be tempted to combine the if statement into a single
> > > one with the static inside it, but perhaps you have it like this because
you
> > > foresee more differences?
> > 
> > Indeed, I was assuming there would be more secondary-ui related color
changes.
> > I'm not sure that will actually happen but it seems likely.

[sky, 2016-08-15 15:53:48]

My mistake, SLGTM

On Mon, Aug 15, 2016 at 8:16 AM,  <estade@chromium.org> wrote:
> On 2016/08/15 14:59:44, sky wrote:
>> On 2016/08/14 01:49:21, Evan Stade wrote:
>> > It was my intent that all the changes to checkbox be conditional. Do you
>> > see
>> one
>> > that's not? I count 3 changes in checkbox.cc all guarded by UseMd().
>>
>> Sorry for not being clear. I was referring to the change in NativeTheme
>> considering ui::MaterialDesignController::IsSecondaryUiMaterial(), but the
>> changes in Checkbox, such as line 138 (new) doesn't. Yes, line 138 is
>> behing
> an
>> md check, but not ui::MaterialDesignController::IsSecondaryUiMaterial().
>
> UseMd() is shorthand for that check.
>
>
>>
>> >
>> >
>>
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
>> > File ui/native_theme/common_theme.cc (right):
>> >
>> >
>>
>
https://codereview.chromium.org/2240603002/diff/20001/ui/native_theme/common_...
>> > ui/native_theme/common_theme.cc:109: static const SkColor
>> > kPrimaryTextColor
> =
>> > SkColorSetRGB(0x33, 0x33, 0x33);
>> > On 2016/08/13 16:03:32, sky wrote:
>> > > optional: Is there a reason you want this here, and not inside the
>> conditional
>> > > where it is used?
>> >
>> > I was following the convention in the rest of the file, where a bunch of
> color
>> > constants are defined and then a switch below (or in this case, for now,
>> > a
>> > conditional) uses the constants. I suspect more than one kColorId thing
>> > will
>> > want kPrimaryTextColor because it's part of a three-tone palette that
> bettes@
>> > came up with.
>> >
>> > > I would be tempted to combine the if statement into a single
>> > > one with the static inside it, but perhaps you have it like this
>> > > because
> you
>> > > foresee more differences?
>> >
>> > Indeed, I was assuming there would be more secondary-ui related color
> changes.
>> > I'm not sure that will actually happen but it seems likely.
>
>
>
> https://codereview.chromium.org/2240603002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-08-15 17:19:43]

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

[commit-bot: I haz the power, 2016-08-15 17:20:02]

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

[Evan Stade, 2016-08-15 18:41:43]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-08-15 18:41:46]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-08-15 18:41:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2240603002/#ps40001
(title: "fix tests")

[commit-bot: I haz the power, 2016-08-15 18:42:15]

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, 2016-08-15 19:25:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-15 19:27:22]

Description was changed from

==========
Harmony checkbox/radio tweaks

a) new focus ring is 2dp and somewhat transparent
b) adjustments to text and icon coloration

BUG=635156
==========

to

==========
Harmony checkbox/radio tweaks

a) new focus ring is 2dp and somewhat transparent
b) adjustments to text and icon coloration

BUG=635156

Committed: https://crrev.com/ed836b02166a6f4ee65657137dc4577c99a2715b
Cr-Commit-Position: refs/heads/master@{#412015}
==========

[commit-bot: I haz the power, 2016-08-15 19:27:25]

Patchset 3 (id:??) landed as
https://crrev.com/ed836b02166a6f4ee65657137dc4577c99a2715b
Cr-Commit-Position: refs/heads/master@{#412015}