Create utility function for configuring an ImageButton to use a vector

Create utility function for configuring an ImageButton to use a vector
icon.

Intended to supplant VectorIconButton.

BUG=691895

[bruthig, 2017-03-16 14:49:55]

bruthig@chromium.org changed reviewers:
+ bruthig@chromium.org

[bruthig, 2017-03-16 14:49:56]

I do like this more for the simple cases like FIP.

I think there may be use cases in the future that would benefit from a
standalone class but we can tackle them if they arise.  The best thing is these
two patterns aren't actually mutually exclusive.

[Evan Stade, 2017-03-16 17:39:45]

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

[Evan Stade, 2017-03-16 17:39:45]

bsep, what do you think?
all, where should the function go?

[bruthig, 2017-03-16 18:26:07]

My first reaction would be a image_button_util.(h|cc) file, but part of me
wonders if it would be worth it to make this function work for ImageView's as
well....maybe not worth thinking about at the present time tho...

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/image_button.cc (right):

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/image_button.cc:305:
button->SetInkDropMode(CustomButton::InkDropMode::ON);
I'm not sure I would couple the ink drop behavior in this function.

[Evan Stade, 2017-03-16 18:49:13]

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/image_button.cc (right):

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/image_button.cc:305:
button->SetInkDropMode(CustomButton::InkDropMode::ON);
On 2017/03/16 18:26:07, bruthig wrote:
> I'm not sure I would couple the ink drop behavior in this function.

for now, all call sites want it. To me that suggests that all future callsites
will want it to. If you don't want the ink drop behavior, you probably want an
ImageView instead of an ImageButton. If this assumption eventually proves to be
wrong, we can change it.

[Bret, 2017-03-16 18:50:10]

> bsep, what do you think?

Hmm, I like it. My first reaction was "oh no this is going to lead to lots of
repeated code" and then I remembered how much boilerplate it takes to make a
subclass... My second reaction was "owner classes might forget to update the
icon color" but they might forget to do that anyway, because we don't force them
to override it. So actually I have no trouble with it.

Would you like me to run with this?

[bruthig, 2017-03-16 19:11:23]

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/image_button.cc (right):

https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/image_button.cc:305:
button->SetInkDropMode(CustomButton::InkDropMode::ON);
On 2017/03/16 18:49:13, Evan Stade wrote:
> On 2017/03/16 18:26:07, bruthig wrote:
> > I'm not sure I would couple the ink drop behavior in this function.
> 
> for now, all call sites want it. To me that suggests that all future callsites
> will want it to. If you don't want the ink drop behavior, you probably want an
> ImageView instead of an ImageButton. If this assumption eventually proves to
be
> wrong, we can change it.

I'm not sure I would agree that all call sites want it all the time.  There
really isn't a need to set the ink drop mode every time the theme changes.

If you do keep it, can you update InkDropHostView::SetInkDropMode() to not
destroy the |ink_drop_| if the mode isn't actually changing.

[Evan Stade, 2017-03-16 19:16:28]

On 2017/03/16 19:11:23, bruthig wrote:
>
https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
> File ui/views/controls/button/image_button.cc (right):
> 
>
https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
> ui/views/controls/button/image_button.cc:305:
> button->SetInkDropMode(CustomButton::InkDropMode::ON);
> On 2017/03/16 18:49:13, Evan Stade wrote:
> > On 2017/03/16 18:26:07, bruthig wrote:
> > > I'm not sure I would couple the ink drop behavior in this function.
> > 
> > for now, all call sites want it. To me that suggests that all future
callsites
> > will want it to. If you don't want the ink drop behavior, you probably want
an
> > ImageView instead of an ImageButton. If this assumption eventually proves to
> be
> > wrong, we can change it.
> 
> I'm not sure I would agree that all call sites want it all the time.  There
> really isn't a need to set the ink drop mode every time the theme changes.

right, I was hoping without really looking into it that all these ink drop calls
were no-ops if no change was being made.

> 
> If you do keep it, can you update InkDropHostView::SetInkDropMode() to not
> destroy the |ink_drop_| if the mode isn't actually changing.

agreed.

> Would you like me to run with this?

I'm for it, sounds like Scott is for it as well, however I would prefer to keep
it separate from the harmony-related changes that are in your other CL.

[Evan Stade, 2017-03-16 21:47:14]

On 2017/03/16 19:16:28, Evan Stade wrote:
> On 2017/03/16 19:11:23, bruthig wrote:
> >
>
https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
> > File ui/views/controls/button/image_button.cc (right):
> > 
> >
>
https://codereview.chromium.org/2753833003/diff/20001/ui/views/controls/butto...
> > ui/views/controls/button/image_button.cc:305:
> > button->SetInkDropMode(CustomButton::InkDropMode::ON);
> > On 2017/03/16 18:49:13, Evan Stade wrote:
> > > On 2017/03/16 18:26:07, bruthig wrote:
> > > > I'm not sure I would couple the ink drop behavior in this function.
> > > 
> > > for now, all call sites want it. To me that suggests that all future
> callsites
> > > will want it to. If you don't want the ink drop behavior, you probably
want
> an
> > > ImageView instead of an ImageButton. If this assumption eventually proves
to
> > be
> > > wrong, we can change it.
> > 
> > I'm not sure I would agree that all call sites want it all the time.  There
> > really isn't a need to set the ink drop mode every time the theme changes.
> 
> right, I was hoping without really looking into it that all these ink drop
calls
> were no-ops if no change was being made.
> 
> > 
> > If you do keep it, can you update InkDropHostView::SetInkDropMode() to not
> > destroy the |ink_drop_| if the mode isn't actually changing.
> 
> agreed.
> 
> > Would you like me to run with this?
> 
> I'm for it, sounds like Scott is for it as well, however I would prefer to
keep
> it separate from the harmony-related changes that are in your other CL.

To be clear, I assume you are going to take over this change Bret?

[Bret, 2017-03-16 21:54:45]

> To be clear, I assume you are going to take over this change Bret?

Yes, I'm working on it now. I'll split out the harmony-related changes too.

[Evan Stade, 2017-03-18 01:04:21]

On 2017/03/16 21:54:45, Bret Sepulveda wrote:
> > To be clear, I assume you are going to take over this change Bret?
> 
> Yes, I'm working on it now. I'll split out the harmony-related changes too.

ok, closing.