Update dialog close buttons to use vector icons and ripples.

Update dialog close buttons to use vector icons and ripples.

BUG=635178
Committed: https://crrev.com/6047258bfb18b2fa84bcb1be85243bd48f3ed71b
Cr-Commit-Position: refs/heads/master@{#416979}

[Evan Stade, 2016-09-06 19:36:17]

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

[Evan Stade, 2016-09-06 19:36:22]

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

[commit-bot: I haz the power, 2016-09-06 19:37:56]

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, 2016-09-06 20:04:31]

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

[commit-bot: I haz the power, 2016-09-06 20:04:31]

Dry run: Exceeded global retry quota

[sky, 2016-09-06 21:22:47]

https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
File ui/views/controls/button/vector_icon_button.h (right):

https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
ui/views/controls/button/vector_icon_button.h:31: const
base::Callback<SkColor(void)>& get_text_color_callback);
I would prefer not to put this class in views because of the use of the callback
and using a constant called gfx::kChromeIconGrey. We generally don't use
callbacks for this sort of thing in views code, and it results in an awkward
api. I would prefer the subclass approach, or making VectorIconButton takes a
special listener for obtaining the color.

[Evan Stade, 2016-09-06 21:45:55]

https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
File ui/views/controls/button/vector_icon_button.h (right):

https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
ui/views/controls/button/vector_icon_button.h:31: const
base::Callback<SkColor(void)>& get_text_color_callback);
On 2016/09/06 21:22:47, sky wrote:
> I would prefer not to put this class in views because of the use of the
callback
> and using a constant called gfx::kChromeIconGrey. We generally don't use
> callbacks for this sort of thing in views code, and it results in an awkward
> api. I would prefer the subclass approach, or making VectorIconButton takes a
> special listener for obtaining the color.

I can get rid of the kChromeIconGrey pretty easily. As far as the callback, are
you suggesting an interface with a single GetTextColor interface? Would it be
sufficiently less awkward if this instead took a std::function and callers could
provide a lambda?

[sky, 2016-09-06 22:18:57]

On 2016/09/06 21:45:55, Evan Stade wrote:
>
https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
> File ui/views/controls/button/vector_icon_button.h (right):
> 
>
https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
> ui/views/controls/button/vector_icon_button.h:31: const
> base::Callback<SkColor(void)>& get_text_color_callback);
> On 2016/09/06 21:22:47, sky wrote:
> > I would prefer not to put this class in views because of the use of the
> callback
> > and using a constant called gfx::kChromeIconGrey. We generally don't use
> > callbacks for this sort of thing in views code, and it results in an awkward
> > api. I would prefer the subclass approach, or making VectorIconButton takes
a
> > special listener for obtaining the color.
> 
> I can get rid of the kChromeIconGrey pretty easily. As far as the callback,
are
> you suggesting an interface with a single GetTextColor interface? Would it be
> sufficiently less awkward if this instead took a std::function and callers
could
> provide a lambda?

What about VectorIconButtonListener that is a ButtonListener with the additional
function?

[Evan Stade, 2016-09-06 23:57:57]

On 2016/09/06 22:18:57, sky wrote:
> On 2016/09/06 21:45:55, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
> > File ui/views/controls/button/vector_icon_button.h (right):
> > 
> >
>
https://codereview.chromium.org/2305933002/diff/1/ui/views/controls/button/ve...
> > ui/views/controls/button/vector_icon_button.h:31: const
> > base::Callback<SkColor(void)>& get_text_color_callback);
> > On 2016/09/06 21:22:47, sky wrote:
> > > I would prefer not to put this class in views because of the use of the
> > callback
> > > and using a constant called gfx::kChromeIconGrey. We generally don't use
> > > callbacks for this sort of thing in views code, and it results in an
awkward
> > > api. I would prefer the subclass approach, or making VectorIconButton
takes
> a
> > > special listener for obtaining the color.
> > 
> > I can get rid of the kChromeIconGrey pretty easily. As far as the callback,
> are
> > you suggesting an interface with a single GetTextColor interface? Would it
be
> > sufficiently less awkward if this instead took a std::function and callers
> could
> > provide a lambda?
> 
> What about VectorIconButtonListener that is a ButtonListener with the
additional
> function?

OK, done. Named it VectorIconButtonDelegate since it doesn't just listen for
clicks.

[Evan Stade, 2016-09-06 23:58:02]

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

[commit-bot: I haz the power, 2016-09-06 23:58:34]

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, 2016-09-07 01:17:31]

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

[commit-bot: I haz the power, 2016-09-07 01:17:32]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-07 03:42:19]

https://codereview.chromium.org/2305933002/diff/20001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2305933002/diff/20001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:99: #if defined(OS_WIN)
Why do you prefer this here and not in the CreateCloseButton?

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/vector_icon_button.h (right):

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.h:20: class VIEWS_EXPORT
VectorIconButtonDelegate : public ButtonListener {
Move to own header as suggested in style guide.

[Evan Stade, 2016-09-07 16:17:33]

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

[Evan Stade, 2016-09-07 16:17:37]

https://codereview.chromium.org/2305933002/diff/20001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2305933002/diff/20001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:99: #if defined(OS_WIN)
On 2016/09/07 03:42:18, sky wrote:
> Why do you prefer this here and not in the CreateCloseButton?

because the HTCLOSE exception only makes sense in BubbleFrameView. External
users of this function will in all likelihood want a tooltip. (Probably we
shouldn't have to expose CreateCloseButton but that's how it currently stands.)

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/vector_icon_button.h (right):

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/09/07 03:42:18, sky wrote:
> 2016

in my mind this is a file move + rename

https://codereview.chromium.org/2305933002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/vector_icon_button.h:20: class VIEWS_EXPORT
VectorIconButtonDelegate : public ButtonListener {
On 2016/09/07 03:42:19, sky wrote:
> Move to own header as suggested in style guide.

Done.

[commit-bot: I haz the power, 2016-09-07 16:18:09]

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, 2016-09-07 17:22:34]

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

[sky, 2016-09-07 17:22:34]

LGTM

[commit-bot: I haz the power, 2016-09-07 17:22:35]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-09-07 17:25:10]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-07 17:25:34]

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-09-07 17:29:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-07 17:31:23]

Description was changed from

==========
Update dialog close buttons to use vector icons and ripples.

BUG=635178
==========

to

==========
Update dialog close buttons to use vector icons and ripples.

BUG=635178

Committed: https://crrev.com/6047258bfb18b2fa84bcb1be85243bd48f3ed71b
Cr-Commit-Position: refs/heads/master@{#416979}
==========

[commit-bot: I haz the power, 2016-09-07 17:31:25]

Patchset 4 (id:??) landed as
https://crrev.com/6047258bfb18b2fa84bcb1be85243bd48f3ed71b
Cr-Commit-Position: refs/heads/master@{#416979}