Introduce new flag to control usage of MD in secondary (not top-chrome)

Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the buttons in the
bookmark bubble.

BUG=512440
Committed: https://crrev.com/b3ca344bc7073dd65b0df4f9add903a61fc818b6
Cr-Commit-Position: refs/heads/master@{#391882}

[Evan Stade, 2016-04-27 22:35:25]

Description was changed from

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Use this flag to control dialog buttons. Also update the buttons in the
bookmark bubble.

BUG=512440
==========

to

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is
orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the
buttons in the
bookmark bubble.

BUG=512440
==========

[Evan Stade, 2016-04-27 22:35:26]

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

[Evan Stade, 2016-04-27 22:36:13]

+varkha for the MaterialDesignController change
+sky for *

[Evan Stade, 2016-04-27 22:36:20]

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

[commit-bot: I haz the power, 2016-04-27 22:36:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926943002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926943002/1

[varkha, 2016-04-27 23:06:06]

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
ui/base/material_design/material_design_controller.h:66: static Extent extent_;
You would probably want to save / restore the mode here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/mater...

This is used to ensure that the tests do not change global state when a shard
runs multiple tests.

https://codereview.chromium.org/1926943002/diff/1/ui/views/controls/button/md...
File ui/views/controls/button/md_text_button.h (right):

https://codereview.chromium.org/1926943002/diff/1/ui/views/controls/button/md...
ui/views/controls/button/md_text_button.h:30: // As above, but onl creates an
MdTextButton if MD is enabled in the secondary
nit: only

[Evan Stade, 2016-04-27 23:20:40]

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
ui/base/material_design/material_design_controller.h:66: static Extent extent_;
On 2016/04/27 23:06:05, varkha wrote:
> You would probably want to save / restore the mode here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/mater...
> 
> This is used to ensure that the tests do not change global state when a shard
> runs multiple tests.

I think this would make sense as soon as we decide we need a test that changes
|extent_|.

https://codereview.chromium.org/1926943002/diff/1/ui/views/controls/button/md...
File ui/views/controls/button/md_text_button.h (right):

https://codereview.chromium.org/1926943002/diff/1/ui/views/controls/button/md...
ui/views/controls/button/md_text_button.h:30: // As above, but onl creates an
MdTextButton if MD is enabled in the secondary
On 2016/04/27 23:06:06, varkha wrote:
> nit: only

Done.

[varkha, 2016-04-27 23:30:58]

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
ui/base/material_design/material_design_controller.h:66: static Extent extent_;
On 2016/04/27 23:20:40, Evan Stade wrote:
> On 2016/04/27 23:06:05, varkha wrote:
> > You would probably want to save / restore the mode here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/mater...
> > 
> > This is used to ensure that the tests do not change global state when a
shard
> > runs multiple tests.
> 
> I think this would make sense as soon as we decide we need a test that changes
> |extent_|.

True but easy to forget and the results may be flaky rather than just failures.
I think it is easier to keep this atomic.

[Evan Stade, 2016-04-27 23:54:39]

On 2016/04/27 23:30:58, varkha wrote:
>
https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
> File ui/base/material_design/material_design_controller.h (right):
> 
>
https://codereview.chromium.org/1926943002/diff/1/ui/base/material_design/mat...
> ui/base/material_design/material_design_controller.h:66: static Extent
extent_;
> On 2016/04/27 23:20:40, Evan Stade wrote:
> > On 2016/04/27 23:06:05, varkha wrote:
> > > You would probably want to save / restore the mode here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/mater...
> > > 
> > > This is used to ensure that the tests do not change global state when a
> shard
> > > runs multiple tests.
> > 
> > I think this would make sense as soon as we decide we need a test that
changes
> > |extent_|.
> 
> True but easy to forget and the results may be flaky rather than just
failures.
> I think it is easier to keep this atomic.

To elaborate on my stance, MaterialDesignControllerTestAPI needs to either take
an extent in the ctor or have an extent setter method for the resetting to be
anything but a confusing no-op. But since it's not used anywhere yet, I don't
want to add a setter. When we do need to add a setter, it will be obvious that
we also need to save/restore the extent.

[varkha, 2016-04-27 23:57:23]

> > I think it is easier to keep this atomic.
> 
> To elaborate on my stance, MaterialDesignControllerTestAPI needs to either
take
> an extent in the ctor or have an extent setter method for the resetting to be
> anything but a confusing no-op. But since it's not used anywhere yet, I don't
> want to add a setter. When we do need to add a setter, it will be obvious that
> we also need to save/restore the extent.

Sure, thanks for considering it, you are right, it is probably simpler for now.

[Evan Stade, 2016-05-03 16:08:29]

On 2016/04/27 23:57:23, varkha wrote:
> > > I think it is easier to keep this atomic.
> > 
> > To elaborate on my stance, MaterialDesignControllerTestAPI needs to either
> take
> > an extent in the ctor or have an extent setter method for the resetting to
be
> > anything but a confusing no-op. But since it's not used anywhere yet, I
don't
> > want to add a setter. When we do need to add a setter, it will be obvious
that
> > we also need to save/restore the extent.
> 
> Sure, thanks for considering it, you are right, it is probably simpler for
now.

ping

[Evan Stade, 2016-05-04 15:26:12]

On 2016/05/03 16:08:29, Evan Stade wrote:
> On 2016/04/27 23:57:23, varkha wrote:
> > > > I think it is easier to keep this atomic.
> > > 
> > > To elaborate on my stance, MaterialDesignControllerTestAPI needs to either
> > take
> > > an extent in the ctor or have an extent setter method for the resetting to
> be
> > > anything but a confusing no-op. But since it's not used anywhere yet, I
> don't
> > > want to add a setter. When we do need to add a setter, it will be obvious
> that
> > > we also need to save/restore the extent.
> > 
> > Sure, thanks for considering it, you are right, it is probably simpler for
> now.
> 
> ping

ping both varkha@ and sky@ --- note this is also blocking further work in the
area like https://codereview.chromium.org/1931013002/

[sky, 2016-05-04 17:35:39]

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:32: enum Extent {
nit: enum class

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:32: enum Extent {
Can this be private? Also, if secondary implicitly includes primary than an enum
doesn't seem like a good fit.

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:34: PRIMARY = 0,
nit: it would be clearer if this included UI in it, eg PRIMARY_UI and
SECONDARY_UI.

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:35: // Apply the Material
Design modes to secondary UI (bubble contents, etc.).
I assume this includes primary as well?

[varkha, 2016-05-04 17:46:02]

Sorry, didn't realize this was blocked on me, I didn't have any more comments.
Added a suggestion for sky's comment, otherwise lgtm.

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:66: static Extent extent_;
Maybe just a boolean extend_to_secondary_ui_ or something.

[Evan Stade, 2016-05-04 22:51:04]

switched from enum to bool

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1926943002/diff/20001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:32: enum Extent {
On 2016/05/04 17:35:39, sky wrote:
> Can this be private? Also, if secondary implicitly includes primary than an
enum
> doesn't seem like a good fit.

Done.

[varkha, 2016-05-05 01:42:49]

Still lgtm.

[sky, 2016-05-05 15:40:12]

LGTM

https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:136: virtual void
UpdateStyleForDefaultness();
defaultness? How about just UpdateStyleForDefault?

[Evan Stade, 2016-05-05 15:54:07]

https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:136: virtual void
UpdateStyleForDefaultness();
On 2016/05/05 15:40:12, sky wrote:
> defaultness? How about just UpdateStyleForDefault?

if I don't put the -ness it seems easier to misinterpret, e.g. "update the style
if this button is default". I think the problem is that "default" is the wrong
part of speech (an adjective) whereas defaultness, or defaulthood, or whatever
word we want to make up, is more noun-y.

[sky, 2016-05-05 16:36:16]

Would passing boolean default parameter clarify this?

On Thu, May 5, 2016 at 8:54 AM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
> File ui/views/controls/button/label_button.h (right):
>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
> ui/views/controls/button/label_button.h:136: virtual void
> UpdateStyleForDefaultness();
> On 2016/05/05 15:40:12, sky wrote:
>> defaultness? How about just UpdateStyleForDefault?
>
> if I don't put the -ness it seems easier to misinterpret, e.g. "update
> the style if this button is default". I think the problem is that
> "default" is the wrong part of speech (an adjective) whereas
> defaultness, or defaulthood, or whatever word we want to make up, is
> more noun-y.
>
> https://codereview.chromium.org/1926943002/

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

[sky, 2016-05-05 16:36:29]

Or perhaps UpdateStyleForDefaultState.

On Thu, May 5, 2016 at 9:36 AM, Scott Violet <sky@chromium.org> wrote:
> Would passing boolean default parameter clarify this?
>
> On Thu, May 5, 2016 at 8:54 AM,  <estade@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
>> File ui/views/controls/button/label_button.h (right):
>>
>>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
>> ui/views/controls/button/label_button.h:136: virtual void
>> UpdateStyleForDefaultness();
>> On 2016/05/05 15:40:12, sky wrote:
>>> defaultness? How about just UpdateStyleForDefault?
>>
>> if I don't put the -ness it seems easier to misinterpret, e.g. "update
>> the style if this button is default". I think the problem is that
>> "default" is the wrong part of speech (an adjective) whereas
>> defaultness, or defaulthood, or whatever word we want to make up, is
>> more noun-y.
>>
>> https://codereview.chromium.org/1926943002/

-- 
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-05-05 17:56:54]

On 2016/05/05 16:36:29, sky wrote:
> Or perhaps UpdateStyleForDefaultState.
> 
> On Thu, May 5, 2016 at 9:36 AM, Scott Violet <mailto:sky@chromium.org> wrote:
> > Would passing boolean default parameter clarify this?
> >
> > On Thu, May 5, 2016 at 8:54 AM,  <mailto:estade@chromium.org> wrote:
> >>
> >>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
> >> File ui/views/controls/button/label_button.h (right):
> >>
> >>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
> >> ui/views/controls/button/label_button.h:136: virtual void
> >> UpdateStyleForDefaultness();
> >> On 2016/05/05 15:40:12, sky wrote:
> >>> defaultness? How about just UpdateStyleForDefault?
> >>
> >> if I don't put the -ness it seems easier to misinterpret, e.g. "update
> >> the style if this button is default". I think the problem is that
> >> "default" is the wrong part of speech (an adjective) whereas
> >> defaultness, or defaulthood, or whatever word we want to make up, is
> >> more noun-y.
> >>
> >> https://codereview.chromium.org/1926943002/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 

UpdateStyleForDefaultState still sounds to me like you're updating the style for
a certain state (the default one)... UpdateStyleToIndicateDefaultStatus?

[sky, 2016-05-05 18:07:20]

SGTM

On Thu, May 5, 2016 at 10:56 AM,  <estade@chromium.org> wrote:
> On 2016/05/05 16:36:29, sky wrote:
>> Or perhaps UpdateStyleForDefaultState.
>>
>> On Thu, May 5, 2016 at 9:36 AM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>> > Would passing boolean default parameter clarify this?
>> >
>> > On Thu, May 5, 2016 at 8:54 AM, <mailto:estade@chromium.org> wrote:
>> >>
>> >>
>>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
>> >> File ui/views/controls/button/label_button.h (right):
>> >>
>> >>
>>
>
https://codereview.chromium.org/1926943002/diff/60001/ui/views/controls/butto...
>> >> ui/views/controls/button/label_button.h:136: virtual void
>> >> UpdateStyleForDefaultness();
>> >> On 2016/05/05 15:40:12, sky wrote:
>> >>> defaultness? How about just UpdateStyleForDefault?
>> >>
>> >> if I don't put the -ness it seems easier to misinterpret, e.g. "update
>> >> the style if this button is default". I think the problem is that
>> >> "default" is the wrong part of speech (an adjective) whereas
>> >> defaultness, or defaulthood, or whatever word we want to make up, is
>> >> more noun-y.
>> >>
>> >> https://codereview.chromium.org/1926943002/
>>
>> --
>> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> UpdateStyleForDefaultState still sounds to me like you're updating the style
> for
> a certain state (the default one)... UpdateStyleToIndicateDefaultStatus?
>
> https://codereview.chromium.org/1926943002/

-- 
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-05-05 18:07:58]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-05-05 18:07:58]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1926943002/#ps80001
(title: "new fn name")

[commit-bot: I haz the power, 2016-05-05 18:08:29]

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

[commit-bot: I haz the power, 2016-05-05 20:00:28]

Description was changed from

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is
orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the
buttons in the
bookmark bubble.

BUG=512440
==========

to

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is
orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the
buttons in the
bookmark bubble.

BUG=512440
==========

[commit-bot: I haz the power, 2016-05-05 20:00:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-05 20:02:05]

Description was changed from

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is
orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the
buttons in the
bookmark bubble.

BUG=512440
==========

to

==========
Introduce new flag to control usage of MD in secondary (not top-chrome)
UI.

Example usage would be:

--top-chrome-md="material-hybrid" --secondary-ui-md

This is a separate flag instead of a new state for --top-chrome-md because it is
orthogonal to the material/hybrid mode.

For now, use this flag to control standard dialog/bubble buttons as well as the
buttons in the
bookmark bubble.

BUG=512440

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

[commit-bot: I haz the power, 2016-05-05 20:02:07]

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