Pushed InkDropHost inheritence up to CustomButton.

Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual

Committed: https://crrev.com/f8882433a6e62219aeecdd7654d3bcba2409f768
Cr-Commit-Position: refs/heads/master@{#368867}

[bruthig, 2015-12-23 20:05:12]

bruthig@chromium.org changed reviewers:
+ pkasting@chromium.org, sadrul@chromium.org

[bruthig, 2015-12-23 20:05:12]

varkha@, Can you review all changes for correctness?

sadrul@, Can you provide an OWNERs review for:
 - ui/views/*

pkasting@, Can you provide an OWNERs review for:
 - chrome/browser/*

[bruthig, 2015-12-23 20:08:17]

Description was changed from

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual
==========

to

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual
==========

[bruthig, 2015-12-23 20:08:17]

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

[Peter Kasting, 2015-12-28 13:44:33]

LGTM

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.h:23: class BarControlButton : public
views::ImageButton {
Does it still make sense to have this subclass of ImageButton?  I've never been
thrilled with it (especially its name) and I'm wondering if we can simplify
anything.

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
ui/views/controls/button/custom_button.cc:338: void
CustomButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
Having to override these just to give empty implementations somewhat suggests to
me that the InkDropHost base class should just define these as the default
implementations (instead of declaring these two functions pure virtual).

[varkha, 2015-12-28 14:54:55]

lgtm

[bruthig, 2015-12-28 17:36:16]

estade@, what are your thoughts regarding the BarControlButton class?

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.h:23: class BarControlButton : public
views::ImageButton {
On 2015/12/28 13:44:33, Peter Kasting wrote:
> Does it still make sense to have this subclass of ImageButton?  I've never
been
> thrilled with it (especially its name) and I'm wondering if we can simplify
> anything.

Adding estade@ for additional input.

I agree it would be good to do away with this class and generalize the necessary
behavior in LabelButton if it doesn't exist already.

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
ui/views/controls/button/custom_button.cc:338: void
CustomButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
On 2015/12/28 13:44:33, Peter Kasting wrote:
> Having to override these just to give empty implementations somewhat suggests
to
> me that the InkDropHost base class should just define these as the default
> implementations (instead of declaring these two functions pure virtual).

I agree that would simplify things but I thought the style guidelines didn't
allow multiple inheritance?

[Peter Kasting, 2015-12-28 17:38:41]

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
ui/views/controls/button/custom_button.cc:338: void
CustomButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
On 2015/12/28 17:36:16, bruthig wrote:
> On 2015/12/28 13:44:33, Peter Kasting wrote:
> > Having to override these just to give empty implementations somewhat
suggests
> to
> > me that the InkDropHost base class should just define these as the default
> > implementations (instead of declaring these two functions pure virtual).
> 
> I agree that would simplify things but I thought the style guidelines didn't
> allow multiple inheritance?

I don't consider this to be qualitatively different than what you're doing (it's
not as if you're pulling in multiple complicated base implementations that you
should have used composition for instead), and Chromium does this sort of thing
in a zillion places already.  I say go for it.

[bruthig, 2015-12-28 18:35:40]

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.h:23: class BarControlButton : public
views::ImageButton {
On 2015/12/28 17:36:16, bruthig wrote:
> On 2015/12/28 13:44:33, Peter Kasting wrote:
> > Does it still make sense to have this subclass of ImageButton?  I've never
> been
> > thrilled with it (especially its name) and I'm wondering if we can simplify
> > anything.
> 
> Adding estade@ for additional input.
> 
> I agree it would be good to do away with this class and generalize the
necessary
> behavior in LabelButton if it doesn't exist already.

pkasting@, is this something we can at least defer to after this review?

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1550443002/diff/1/ui/views/controls/button/cu...
ui/views/controls/button/custom_button.cc:338: void
CustomButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
On 2015/12/28 17:38:41, Peter Kasting wrote:
> On 2015/12/28 17:36:16, bruthig wrote:
> > On 2015/12/28 13:44:33, Peter Kasting wrote:
> > > Having to override these just to give empty implementations somewhat
> suggests
> > to
> > > me that the InkDropHost base class should just define these as the default
> > > implementations (instead of declaring these two functions pure virtual).
> > 
> > I agree that would simplify things but I thought the style guidelines didn't
> > allow multiple inheritance?
> 
> I don't consider this to be qualitatively different than what you're doing
(it's
> not as if you're pulling in multiple complicated base implementations that you
> should have used composition for instead), and Chromium does this sort of
thing
> in a zillion places already.  I say go for it.

Done.

[Peter Kasting, 2015-12-28 18:38:02]

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.h:23: class BarControlButton : public
views::ImageButton {
On 2015/12/28 18:35:40, bruthig wrote:
> On 2015/12/28 17:36:16, bruthig wrote:
> > On 2015/12/28 13:44:33, Peter Kasting wrote:
> > > Does it still make sense to have this subclass of ImageButton?  I've never
> > been
> > > thrilled with it (especially its name) and I'm wondering if we can
simplify
> > > anything.
> > 
> > Adding estade@ for additional input.
> > 
> > I agree it would be good to do away with this class and generalize the
> necessary
> > behavior in LabelButton if it doesn't exist already.
> 
> pkasting@, is this something we can at least defer to after this review?

Yes, in fact I would prefer any changes along these lines to be done separately.

[varkha, 2015-12-28 19:06:30]

A small nit while you are at it.

https://codereview.chromium.org/1550443002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.h (right):

https://codereview.chromium.org/1550443002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.h:30: // Add the |ink_drop_layer| in to a
visible layer tree.
nit: s/Add/Adds.

[bruthig, 2015-12-29 00:09:33]

https://codereview.chromium.org/1550443002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.h (right):

https://codereview.chromium.org/1550443002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.h:30: // Add the |ink_drop_layer| in to a
visible layer tree.
On 2015/12/28 19:06:30, varkha wrote:
> nit: s/Add/Adds.

Done.

[Evan Stade, 2015-12-29 19:20:35]

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

[Evan Stade, 2015-12-29 19:20:36]

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1550443002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.h:23: class BarControlButton : public
views::ImageButton {
On 2015/12/28 17:36:16, bruthig wrote:
> On 2015/12/28 13:44:33, Peter Kasting wrote:
> > Does it still make sense to have this subclass of ImageButton?  I've never
> been
> > thrilled with it (especially its name) and I'm wondering if we can simplify
> > anything.
> 
> Adding estade@ for additional input.
> 
> I agree it would be good to do away with this class and generalize the
necessary
> behavior in LabelButton if it doesn't exist already.

This class was introduced before it was an inkdrophost, and it still makes sense
after it isn't one any more. I don't think the added functionality can easily be
moved to ImageButton and I don't think making base views classes monolothic in
size and functionality is a good design anyway.

[Evan Stade, 2015-12-29 23:03:37]

https://codereview.chromium.org/1550443002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.cc (right):

https://codereview.chromium.org/1550443002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.cc:15: void
InkDropHost::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
p.s. why did you add this default impl instead of making CustomButton have a
default do-nothing impl? pure virtual is generally preferable

[Evan Stade, 2015-12-29 23:06:56]

https://codereview.chromium.org/1550443002/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host.cc (right):

https://codereview.chromium.org/1550443002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host.cc:15: void
InkDropHost::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {}
On 2015/12/29 23:03:37, Evan Stade wrote:
> p.s. why did you add this default impl instead of making CustomButton have a
> default do-nothing impl? pure virtual is generally preferable

ah, I see this was suggested by Peter. I prefer pure virtual in this situation,
but perhaps it's a policy decision that has been made at a broader level. It
seems like the Google C++ style guide forbids multiple inheritance (excluding
pure virtual interfaces), but admittedly that's not followed everywhere in
Chrome.

[sadrul, 2016-01-04 19:24:32]

I don't think we should do this. CustomButton shouldn't be an InkDropHost if it
doesn't provide a proper base implementation that works. You can create a
separate InkDropHost implementation that only overrides CalculateInkDropCenter()
to return the center-point of the View's bounds, and avoid some code duplication
that way if you want.

[sadrul, 2016-01-04 19:37:07]

On 2016/01/04 19:24:32, sadrul wrote:
> I don't think we should do this. CustomButton shouldn't be an InkDropHost if
it
> doesn't provide a proper base implementation that works. You can create a
> separate InkDropHost implementation that only overrides
CalculateInkDropCenter()
> to return the center-point of the View's bounds, and avoid some code
duplication
> that way if you want.

Looking at https://codereview.chromium.org/1390113006/, perhaps it would make
sense to have a ButtonInkDropHost (like there's a ButtonInkDropDelegate)?

[bruthig, 2016-01-06 16:36:20]

I've added a default implementation of Add/RemoveInkDropLayer
to CustomButton.

sadrul@, varkha@, can you take a look at the
Add/RemoveInkDropLayer methods across all of the buttons?

[sadrul, 2016-01-06 19:18:45]

https://codereview.chromium.org/1550443002/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1550443002/diff/160001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:338: SetFillsBoundsOpaquely(false);
It shouldn't happen here. At most, this should DCHECK() that layer() exists.
Even then, I don't think it's entirely right.

Is there a reason a ButtonInkDropHost wouldn't work?

[bruthig, 2016-01-11 15:34:14]

Ping, sadrul@

[sadrul, 2016-01-12 02:04:38]

lgtm

[bruthig, 2016-01-12 13:26:14]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-01-12 13:26:14]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1550443002/#ps180001
(title: "Merge with master")

[commit-bot: I haz the power, 2016-01-12 13:26:36]

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

[commit-bot: I haz the power, 2016-01-12 14:08:01]

Description was changed from

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual
==========

to

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual
==========

[commit-bot: I haz the power, 2016-01-12 14:08:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-12 14:08:23]

Failed to apply patch for chrome/browser/ui/views/toolbar/toolbar_button.cc:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/ui/views/toolbar/toolbar_button.cc:174
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/ui/views/toolbar/toolbar_button.cc' with
conflicts.
  U chrome/browser/ui/views/toolbar/toolbar_button.cc

Patch:       chrome/browser/ui/views/toolbar/toolbar_button.cc
Index: chrome/browser/ui/views/toolbar/toolbar_button.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_button.cc
b/chrome/browser/ui/views/toolbar/toolbar_button.cc
index
06c08fe1b019921e237fa507fa95cb1fb75640f6..8c16229931f58819dfb64f5cc313b7dfc02f835d
100644
--- a/chrome/browser/ui/views/toolbar/toolbar_button.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_button.cc
@@ -174,32 +174,26 @@ ToolbarButton::CreateDefaultBorder() const {
   return border;
 }
 
-void ToolbarButton::ShowContextMenuForView(View* source,
-                                           const gfx::Point& point,
-                                           ui::MenuSourceType source_type) {
-  if (!enabled())
-    return;
-
-  show_menu_factory_.InvalidateWeakPtrs();
-  ShowDropDownMenu(source_type);
-}
-
 void ToolbarButton::AddInkDropLayer(ui::Layer* ink_drop_layer) {
-  SetPaintToLayer(true);
-  SetFillsBoundsOpaquely(false);
   image()->SetPaintToLayer(true);
   image()->SetFillsBoundsOpaquely(false);
-
-  layer()->Add(ink_drop_layer);
-  layer()->StackAtBottom(ink_drop_layer);
+  views::LabelButton::AddInkDropLayer(ink_drop_layer);
 }
 
 void ToolbarButton::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
-  layer()->Remove(ink_drop_layer);
-
+  views::LabelButton::RemoveInkDropLayer(ink_drop_layer);
   image()->SetFillsBoundsOpaquely(true);
   image()->SetPaintToLayer(false);
-  SetPaintToLayer(false);
+}
+
+void ToolbarButton::ShowContextMenuForView(View* source,
+                                           const gfx::Point& point,
+                                           ui::MenuSourceType source_type) {
+  if (!enabled())
+    return;
+
+  show_menu_factory_.InvalidateWeakPtrs();
+  ShowDropDownMenu(source_type);
 }
 
 bool ToolbarButton::ShouldEnterPushedState(const ui::Event& event) {
@@ -298,7 +292,3 @@ void ToolbarButton::ShowDropDownMenu(ui::MenuSourceType
source_type) {
 const char* ToolbarButton::GetClassName() const {
   return "ToolbarButton";
 }
-
-gfx::Point ToolbarButton::CalculateInkDropCenter() const {
-  return GetLocalBounds().CenterPoint();
-}

[commit-bot: I haz the power, 2016-01-12 14:09:28]

Description was changed from

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual
==========

to

==========
Pushed InkDropHost inheritence up to CustomButton.

A handful of CustomButton descendants inherited the InkDropHost
interface and they all had the same function implementations. This
changelist pushes the common functionality up in to CustomButton.

TEST=Manual

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

[commit-bot: I haz the power, 2016-01-12 14:09:29]

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