Make MdTextButton a LabelButton, use it in the DL shelf.

Make MdTextButton a LabelButton, use it in the DL shelf.

BUG=591582, 571500
Committed: https://crrev.com/9c64d516f6f9c9629391f940dc2d04608f770e06
Cr-Commit-Position: refs/heads/master@{#379998}

[Evan Stade, 2016-03-08 01:55:13]

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

[commit-bot: I haz the power, 2016-03-08 01:55:58]

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

[Evan Stade, 2016-03-08 02:04:15]

https://codereview.chromium.org/1773893002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/1773893002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:193: DCHECK_EQ(STYLE_BUTTON, style_);
We don't call SetIsDefault for any STYLE_TEXTBUTTON button (I think --- trybots
are confirming). I'm hoping that when we switch everything to MD we can get rid
of LabelButton::SetStyle(). Then we'll have:

button = views::MdTextButton::Create();

instead of

button = new views::LabelButton();
button->SetStyle();

with a tall stack of DCHECKs in SetStyle. SetIsDefault would move to
MdTextButton also.

https://codereview.chromium.org/1773893002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/md_text_button.h (right):

https://codereview.chromium.org/1773893002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.h:19: static LabelButton*
CreateStandardButton(ButtonListener* listener,
This factory function is useful even in an all-md world because it allows us to
construct and then init in one line (at the callsite).

[Evan Stade, 2016-03-08 02:04:32]

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

[Evan Stade, 2016-03-08 02:04:32]

+sky, see previous comments

[Evan Stade, 2016-03-08 02:04:45]

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

[commit-bot: I haz the power, 2016-03-08 02:05:31]

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

[Evan Stade, 2016-03-08 02:13:40]

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

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

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

[commit-bot: I haz the power, 2016-03-08 02:56:58]

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

[commit-bot: I haz the power, 2016-03-08 02:56:59]

Dry run: This issue passed the CQ dry run.

[sky, 2016-03-08 16:37:35]

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

https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.h:15: class VIEWS_EXPORT MdTextButton :
public LabelButton {
Once md is the default, what is the plan for this class? How does one know when
they want to use MdTextButton::CreateStandardButton vs creating a LabelButton
directly?

[Evan Stade, 2016-03-08 21:59:16]

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

https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.h:15: class VIEWS_EXPORT MdTextButton :
public LabelButton {
On 2016/03/08 16:37:34, sky wrote:
> Once md is the default, what is the plan for this class? How does one know
when
> they want to use MdTextButton::CreateStandardButton vs creating a LabelButton
> directly?

I'm not 100% sure this will be possible, but ideally LabelButton::SetStyle goes
away and the pattern of

button = new LabelButton
button->SetStyle(BUTTONBUTTON);

becomes

button = MdTextButton::Create();

LabelButton will still exist separately from MdTextButton because other stuff
like MenuButton extends it, but LabelButton itself will hopefully be an abstract
class.

[sky, 2016-03-08 23:03:21]

How will one know when to create a LabelButton vs an MdTextButton?

On Tue, Mar 8, 2016 at 1:59 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> File ui/views/controls/button/md_text_button.h (right):
>
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> ui/views/controls/button/md_text_button.h:15: class VIEWS_EXPORT
> MdTextButton : public LabelButton {
> On 2016/03/08 16:37:34, sky wrote:
>> Once md is the default, what is the plan for this class? How does one
> know when
>> they want to use MdTextButton::CreateStandardButton vs creating a
> LabelButton
>> directly?
>
> I'm not 100% sure this will be possible, but ideally
> LabelButton::SetStyle goes away and the pattern of
>
> button = new LabelButton
> button->SetStyle(BUTTONBUTTON);
>
> becomes
>
> button = MdTextButton::Create();
>
> LabelButton will still exist separately from MdTextButton because other
> stuff like MenuButton extends it, but LabelButton itself will hopefully
> be an abstract class.
>
> https://codereview.chromium.org/1773893002/

-- 
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-03-08 23:04:02]

On 2016/03/08 23:03:21, sky wrote:
> How will one know when to create a LabelButton vs an MdTextButton?

As an abstract class, LabelButton will not be something you can create directly.

> 
> On Tue, Mar 8, 2016 at 1:59 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> > File ui/views/controls/button/md_text_button.h (right):
> >
> >
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> > ui/views/controls/button/md_text_button.h:15: class VIEWS_EXPORT
> > MdTextButton : public LabelButton {
> > On 2016/03/08 16:37:34, sky wrote:
> >> Once md is the default, what is the plan for this class? How does one
> > know when
> >> they want to use MdTextButton::CreateStandardButton vs creating a
> > LabelButton
> >> directly?
> >
> > I'm not 100% sure this will be possible, but ideally
> > LabelButton::SetStyle goes away and the pattern of
> >
> > button = new LabelButton
> > button->SetStyle(BUTTONBUTTON);
> >
> > becomes
> >
> > button = MdTextButton::Create();
> >
> > LabelButton will still exist separately from MdTextButton because other
> > stuff like MenuButton extends it, but LabelButton itself will hopefully
> > be an abstract class.
> >
> > https://codereview.chromium.org/1773893002/
> 
> -- 
> 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.
>

[Evan Stade, 2016-03-08 23:05:47]

On 2016/03/08 23:04:02, Evan Stade wrote:
> On 2016/03/08 23:03:21, sky wrote:
> > How will one know when to create a LabelButton vs an MdTextButton?
> 
> As an abstract class, LabelButton will not be something you can create
directly.

or, if it's not abstract, the ctor is at least protected

> 
> > 
> > On Tue, Mar 8, 2016 at 1:59 PM,  <mailto:estade@chromium.org> wrote:
> > >
> > >
> >
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> > > File ui/views/controls/button/md_text_button.h (right):
> > >
> > >
> >
>
https://codereview.chromium.org/1773893002/diff/60001/ui/views/controls/butto...
> > > ui/views/controls/button/md_text_button.h:15: class VIEWS_EXPORT
> > > MdTextButton : public LabelButton {
> > > On 2016/03/08 16:37:34, sky wrote:
> > >> Once md is the default, what is the plan for this class? How does one
> > > know when
> > >> they want to use MdTextButton::CreateStandardButton vs creating a
> > > LabelButton
> > >> directly?
> > >
> > > I'm not 100% sure this will be possible, but ideally
> > > LabelButton::SetStyle goes away and the pattern of
> > >
> > > button = new LabelButton
> > > button->SetStyle(BUTTONBUTTON);
> > >
> > > becomes
> > >
> > > button = MdTextButton::Create();
> > >
> > > LabelButton will still exist separately from MdTextButton because other
> > > stuff like MenuButton extends it, but LabelButton itself will hopefully
> > > be an abstract class.
> > >
> > > https://codereview.chromium.org/1773893002/
> > 
> > -- 
> > 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.
> >

[sky, 2016-03-08 23:29:37]

LGTM

[Evan Stade, 2016-03-08 23:44:53]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-03-08 23:45:18]

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

[commit-bot: I haz the power, 2016-03-09 00:41:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-09 00:43:10]

Description was changed from

==========
Make MdTextButton a LabelButton, use it in the DL shelf.

BUG=591582, 571500
==========

to

==========
Make MdTextButton a LabelButton, use it in the DL shelf.

BUG=591582, 571500

Committed: https://crrev.com/9c64d516f6f9c9629391f940dc2d04608f770e06
Cr-Commit-Position: refs/heads/master@{#379998}
==========

[commit-bot: I haz the power, 2016-03-09 00:43:11]

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