[MD] Share button code between find bar and download shelf

[MD] Share button code between find bar and download shelf

- improved theme support in both find bar and download shelf
- inkdrop effect in download shelf
- less code duplication

Looks better on inverted color schemes on Windows or Linux (with GTK
theming on). Should have no effect on CrOS because the browser theme
doesn't change these colors and there's only one native theme.

BUG=512443
Committed: https://crrev.com/1028d4b5822ef0db26d626d00e85b6b8233fb41f
Cr-Commit-Position: refs/heads/master@{#357696}

[Evan Stade, 2015-11-03 20:25:01]

Description was changed from

==========
wip - share button code between find bar and download shelf

- improved theme support
- inkdrop effect in download shelf
- less code duplication

BUG=
==========

to

==========
[MD] Share button code between find bar and download shelf

- improved theme support in both find bar and download shelf
- inkdrop effect in download shelf
- less code duplication

Looks better on inverted color schemes on Windows or Linux (with GTK
theming on). Should have no effect on CrOS because the browser theme
doesn't change these colors and there's only one native theme.

BUG=512443
==========

[Evan Stade, 2015-11-03 20:25:01]

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

[Evan Stade, 2015-11-03 21:48:20]

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

[commit-bot: I haz the power, 2015-11-03 21:49:26]

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

[commit-bot: I haz the power, 2015-11-03 23:19:24]

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

[commit-bot: I haz the power, 2015-11-03 23:19:25]

Dry run: This issue passed the CQ dry run.

[sky, 2015-11-03 23:46:43]

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:30: const int
kInkDropSmallCornerRadius = 2;
Are these constants really specific to buttons and not defined in some common
place?

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:58: image =
gfx::CreateVectorIcon(id_, 16, SkColorSetA(icon_color, 0xff / 2));
Do we not have 16 defined some where?

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:77:
layer()->StackAtBottom(ink_drop_layer);
Is this really necessary?

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:80: void
BarControlButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
I'm not familiar with the ink drop layer. What is the expectation if the view is
deleted with the layer? Also, why do you need to explicitly remove it?

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.h:31: const
base::Callback<SkColor(void)>& get_text_color_callback);
It seems like all owners of this class are also themselves views that could just
as well override OnThemeChanged and call some function when the theme changes
with the color. Seems simpler than having the callback.

[Evan Stade, 2015-11-03 23:51:17]

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

[Evan Stade, 2015-11-03 23:51:17]

+moshayedi for inkdrop related questions

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:30: const int
kInkDropSmallCornerRadius = 2;
On 2015/11/03 23:46:43, sky wrote:
> Are these constants really specific to buttons and not defined in some common
> place?

I don't know, this code is just moved from FindBarView::FindBarButton

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:58: image =
gfx::CreateVectorIcon(id_, 16, SkColorSetA(icon_color, 0xff / 2));
On 2015/11/03 23:46:43, sky wrote:
> Do we not have 16 defined some where?

If we landed https://codereview.chromium.org/1406613002/ this 16 would not be
necessary

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:77:
layer()->StackAtBottom(ink_drop_layer);
On 2015/11/03 23:46:43, sky wrote:
> Is this really necessary?

I don't know, this code is just moved from FindBarView::FindBarButton

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:80: void
BarControlButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
On 2015/11/03 23:46:43, sky wrote:
> I'm not familiar with the ink drop layer. What is the expectation if the view
is
> deleted with the layer? Also, why do you need to explicitly remove it?

I don't know, this code is just moved from FindBarView::FindBarButton

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1408223008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.h:31: const
base::Callback<SkColor(void)>& get_text_color_callback);
On 2015/11/03 23:46:43, sky wrote:
> It seems like all owners of this class are also themselves views that could
just
> as well override OnThemeChanged and call some function when the theme changes
> with the color. Seems simpler than having the callback.

The reason I went with the callback is that it's impossible to forget. This way,
BarControlButtons are always compelled to be theme-aware.

[sky, 2015-11-04 00:36:46]

Ok, LGTM

[Evan Stade, 2015-11-04 00:46:35]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-11-04 00:48:38]

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

[commit-bot: I haz the power, 2015-11-04 01:12:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-04 01:13:36]

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