Converted all Views to use an InkDropDelegate instead of a InkDropAnimationController.

Converted all Views to use an InkDropDelegate instead of a InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.

Committed: https://crrev.com/502a5c9660bfd1c2ac949e4388bc8697d06b5507
Cr-Commit-Position: refs/heads/master@{#366694}

[bruthig, 2015-11-27 20:49:30]

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

[bruthig, 2015-11-27 20:49:30]

varkha@, can you please take a first look?

[varkha, 2015-11-27 22:07:50]

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

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:91:
ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN);
Just heads up. I think with https://codereview.chromium.org/1411523009/ this
would need to be done in OnClickCanceled() override.

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:107:
ink_drop_delegate()->OnAction(views::InkDropState::ACTION_PENDING);
Is there enough information in CustomButton to drive this transition given that
QUICK_ACTION is invoked from there?

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:139:
ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN);
I think this will move OnClickCanceled with
https://codereview.chromium.org/1411523009/.

https://codereview.chromium.org/1478303003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1478303003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:409:
ink_drop_delegate()->OnAction(views::InkDropState::QUICK_ACTION);
If we are doing this in CustomButton then we should match that with a transition
to hidden in OnClickCanceled from https://codereview.chromium.org/1411523009/ in
this class as well.

[bruthig, 2015-12-07 18:44:46]

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

[bruthig, 2015-12-07 18:44:47]

varkha@, can you PTAL for correctness?

sadrul@ Please review changes in:
 - ui/views/*

pkasting@ Please review changes in 
 - chrome/browser/*

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

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:91:
ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN);
On 2015/11/27 22:07:50, varkha wrote:
> Just heads up. I think with https://codereview.chromium.org/1411523009/ this
> would need to be done in OnClickCanceled() override.

Done.

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:107:
ink_drop_delegate()->OnAction(views::InkDropState::ACTION_PENDING);
On 2015/11/27 22:07:50, varkha wrote:
> Is there enough information in CustomButton to drive this transition given
that
> QUICK_ACTION is invoked from there?

Done.

https://codereview.chromium.org/1478303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:139:
ink_drop_delegate()->OnAction(views::InkDropState::HIDDEN);
On 2015/11/27 22:07:50, varkha wrote:
> I think this will move OnClickCanceled with
> https://codereview.chromium.org/1411523009/.

Done.

https://codereview.chromium.org/1478303003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1478303003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:409:
ink_drop_delegate()->OnAction(views::InkDropState::QUICK_ACTION);
On 2015/11/27 22:07:50, varkha wrote:
> If we are doing this in CustomButton then we should match that with a
transition
> to hidden in OnClickCanceled from https://codereview.chromium.org/1411523009/
in
> this class as well.

Done.

[bruthig, 2015-12-07 18:47:15]

Description was changed from

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.
==========

to

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate instead of the
InkDropAnimationController.  This review is the follow up that updates all the
Views to use the delegate instead of the controller.
==========

[bruthig, 2015-12-07 18:47:41]

Description was changed from

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate instead of the
InkDropAnimationController.  This review is the follow up that updates all the
Views to use the delegate instead of the controller.
==========

to

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.
==========

[varkha, 2015-12-07 19:24:47]

I'll pull it to try locally.

https://codereview.chromium.org/1478303003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1478303003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:89: if
(IsTriggerableEvent(event)) {
nit: Can roll all the conditions together now that AnimateToState is called in
the base class.

[bruthig, 2015-12-07 19:39:03]

https://codereview.chromium.org/1478303003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1478303003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:89: if
(IsTriggerableEvent(event)) {
On 2015/12/07 19:24:47, varkha wrote:
> nit: Can roll all the conditions together now that AnimateToState is called in
> the base class.

Done.

[varkha, 2015-12-07 19:50:13]

The change LG but I would probably wait for
https://codereview.chromium.org/1494433003/ to land in order to avoid
proliferating the crash on Windows.

[Peter Kasting, 2015-12-08 20:47:49]

LGTM

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:24:
SetInkDropDelegate(new_ink_drop_delegate.Pass());
Nit: Can this just be this?:

  SetInkDropDelegate(
      make_scoped_ptr(new views::ButtonInkDropDelegate(this, this)));

If not, I think we're converting .Pass() to std::move()?  (Not sure)

Same comment applies in other files.

[varkha, 2015-12-08 21:25:31]

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:24:
SetInkDropDelegate(new_ink_drop_delegate.Pass());
On 2015/12/08 20:47:49, Peter Kasting wrote:
> Nit: Can this just be this?:
> 
>   SetInkDropDelegate(
>       make_scoped_ptr(new views::ButtonInkDropDelegate(this, this)));
> 
> If not, I think we're converting .Pass() to std::move()?  (Not sure)
> 
> Same comment applies in other files.

This will need to change to conform to
https://codereview.chromium.org/1494433003/ anyway.

[bruthig, 2015-12-09 18:20:21]

varkha@ Please review merge with https://codereview.chromium.org/1494433003

sadrul@ Please review changes in:
 - ui/views/*

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:24:
SetInkDropDelegate(new_ink_drop_delegate.Pass());
On 2015/12/08 21:25:31, varkha wrote:
> On 2015/12/08 20:47:49, Peter Kasting wrote:
> > Nit: Can this just be this?:
> > 
> >   SetInkDropDelegate(
> >       make_scoped_ptr(new views::ButtonInkDropDelegate(this, this)));
> > 
> > If not, I think we're converting .Pass() to std::move()?  (Not sure)
> > 
> > Same comment applies in other files.
> 
> This will need to change to conform to
> https://codereview.chromium.org/1494433003/ anyway.

Done.

[varkha, 2015-12-09 21:23:36]

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.h:45:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
Do you need forward declaration for InkDropDelegate?

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/app_menu_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/app_menu_button.h:124:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
Do you need a forward declaration for InkDropDelegate?

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:93:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
Needs a forward declaration for InkDropDelegate.

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

https://codereview.chromium.org/1478303003/diff/140001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:414:
ink_drop_delegate()->OnAction(views::InkDropState::QUICK_ACTION);
Wouldn't it make sense to start animating before base class is called?

[bruthig, 2015-12-11 22:09:46]

varkha@, can you PTAL for correctness?

sadrul@ Please review changes in:
 - ui/views/*

pkasting@ Please review changes in: (minor changes since your lgtm) 
 - chrome/browser/*

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.h:45:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
On 2015/12/09 21:23:35, varkha wrote:
> Do you need forward declaration for InkDropDelegate?

Done.

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/app_menu_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/app_menu_button.h:124:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
On 2015/12/09 21:23:36, varkha wrote:
> Do you need a forward declaration for InkDropDelegate?

Done.

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1478303003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:93:
scoped_ptr<views::InkDropDelegate> ink_drop_delegate_;
On 2015/12/09 21:23:36, varkha wrote:
> Needs a forward declaration for InkDropDelegate.

Done.

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

https://codereview.chromium.org/1478303003/diff/140001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:414:
ink_drop_delegate()->OnAction(views::InkDropState::QUICK_ACTION);
On 2015/12/09 21:23:36, varkha wrote:
> Wouldn't it make sense to start animating before base class is called?

Done.

[Peter Kasting, 2015-12-12 01:56:19]

I reviewed the diff between patch set 7 (what I last reviewed) and the current
patch set.  I think this is stuff that's checked in, so my comments below are
more about that checked in code than what you're doing in _this_ CL, which
AFAICT hasn't really changed since I reviewed it, so doesn't need a new signoff
from me.  (Correct me if I'm wrong.)

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:38: ink_drop_delegate_.reset();
I don't think you need to explicitly do this.  It's about to be destroyed
anyway, and that destruction will happen before the base class destructor runs. 
I don't think this explicit destruction affects anything.

I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call too.  The
only purpose of this call is to make the DCHECK in the base class work.  But
that DCHECK only checks that this set call has been made, and importantly, it
_doesn't_ actually check that the ink drop delegate has been destroyed, which is
the critical bit.  So to me this is all theater, and we should remove both the
set call here and the DCHECK there.  It doesn't actually make the code more
correct or help other subclassers write safer code.

There _are_ ways we could guarantee the destruction of this object, but I think
they're more mess/work than they're worth; one is to give the base class the
address of the scoped_ptr instead of the contained raw pointer, and the other is
to hook up some delegate callbacks so when the delegate is destroyed it
notifiers an interested listener.  There's no benefit to these other than
sanity-checking the destruction, and given that that destruction should happen
correctly with no explicit code written in the subclass destructor at all, I
wouldn't do them.

[varkha, 2015-12-12 02:43:03]

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:38: ink_drop_delegate_.reset();
On 2015/12/12 01:56:18, Peter Kasting wrote:
> I don't think you need to explicitly do this.  It's about to be destroyed
> anyway, and that destruction will happen before the base class destructor
runs. 
> I don't think this explicit destruction affects anything.
> 
> I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call too. 
The
> only purpose of this call is to make the DCHECK in the base class work.  But
> that DCHECK only checks that this set call has been made, and importantly, it
> _doesn't_ actually check that the ink drop delegate has been destroyed, which
is
> the critical bit.  So to me this is all theater, and we should remove both the
> set call here and the DCHECK there.  It doesn't actually make the code more
> correct or help other subclassers write safer code.
> 
> There _are_ ways we could guarantee the destruction of this object, but I
think
> they're more mess/work than they're worth; one is to give the base class the
> address of the scoped_ptr instead of the contained raw pointer, and the other
is
> to hook up some delegate callbacks so when the delegate is destroyed it
> notifiers an interested listener.  There's no benefit to these other than
> sanity-checking the destruction, and given that that destruction should happen
> correctly with no explicit code written in the subclass destructor at all, I
> wouldn't do them.

I partially agree with this comment. Setting the delegate pointer in the base
class here is indeed to verify that nothing accesses it in the destructor of the
base class. Resetting the scoped_ptr here however is logical as this
BarControlButton is an InkDropHost and so its RemoveInkDropLayer will get called
when ink_drop_delegate_ will be destroyed. Doing it after dtor returns seemed a
bit wrong to me thus this sequence.
I agree with the rest - there are ways to enforce correct sequence but they all
seem like an overkill here. Some are mentioned in review for
https://codereview.chromium.org/1494433003/. My original approach there was to
own the delegate in the base class and make the DCHECK verify that it has been
disposed of before the base class destructor but I have changed that in the last
patch set following a comment.
If I may suggest here I would make the code consistent everywhere as a first
step and if we see that this destruction sequence can be consolidated refactor
all those paths in a separate CL.

[Peter Kasting, 2015-12-12 02:46:09]

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.cc:38: ink_drop_delegate_.reset();
On 2015/12/12 02:43:03, varkha wrote:
> On 2015/12/12 01:56:18, Peter Kasting wrote:
> > I don't think you need to explicitly do this.  It's about to be destroyed
> > anyway, and that destruction will happen before the base class destructor
> runs. 
> > I don't think this explicit destruction affects anything.
> > 
> > I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call too. 
> The
> > only purpose of this call is to make the DCHECK in the base class work.  But
> > that DCHECK only checks that this set call has been made, and importantly,
it
> > _doesn't_ actually check that the ink drop delegate has been destroyed,
which
> is
> > the critical bit.  So to me this is all theater, and we should remove both
the
> > set call here and the DCHECK there.  It doesn't actually make the code more
> > correct or help other subclassers write safer code.
> > 
> > There _are_ ways we could guarantee the destruction of this object, but I
> think
> > they're more mess/work than they're worth; one is to give the base class the
> > address of the scoped_ptr instead of the contained raw pointer, and the
other
> is
> > to hook up some delegate callbacks so when the delegate is destroyed it
> > notifiers an interested listener.  There's no benefit to these other than
> > sanity-checking the destruction, and given that that destruction should
happen
> > correctly with no explicit code written in the subclass destructor at all, I
> > wouldn't do them.
> 
> I partially agree with this comment. Setting the delegate pointer in the base
> class here is indeed to verify that nothing accesses it in the destructor of
the
> base class. Resetting the scoped_ptr here however is logical as this
> BarControlButton is an InkDropHost and so its RemoveInkDropLayer will get
called
> when ink_drop_delegate_ will be destroyed. Doing it after dtor returns seemed
a
> bit wrong to me thus this sequence.

Eh?  Members are destroyed before the dtor returns, not after.  Resetting the
scoped_ptr here explicitly has basically no observable effect at all.  (It just
affects the relative order of different members' destruction, it does not affect
when members are destroyed relative to when the subclass portion of the object
is destroyed or relative to when the destructor returns.)

[varkha, 2015-12-12 02:53:06]

On 2015/12/12 02:46:09, Peter Kasting wrote:
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> File chrome/browser/ui/views/bar_control_button.cc (right):
> 
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> chrome/browser/ui/views/bar_control_button.cc:38: ink_drop_delegate_.reset();
> On 2015/12/12 02:43:03, varkha wrote:
> > On 2015/12/12 01:56:18, Peter Kasting wrote:
> > > I don't think you need to explicitly do this.  It's about to be destroyed
> > > anyway, and that destruction will happen before the base class destructor
> > runs. 
> > > I don't think this explicit destruction affects anything.
> > > 
> > > I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call
too. 
> > The
> > > only purpose of this call is to make the DCHECK in the base class work. 
But
> > > that DCHECK only checks that this set call has been made, and importantly,
> it
> > > _doesn't_ actually check that the ink drop delegate has been destroyed,
> which
> > is
> > > the critical bit.  So to me this is all theater, and we should remove both
> the
> > > set call here and the DCHECK there.  It doesn't actually make the code
more
> > > correct or help other subclassers write safer code.
> > > 
> > > There _are_ ways we could guarantee the destruction of this object, but I
> > think
> > > they're more mess/work than they're worth; one is to give the base class
the
> > > address of the scoped_ptr instead of the contained raw pointer, and the
> other
> > is
> > > to hook up some delegate callbacks so when the delegate is destroyed it
> > > notifiers an interested listener.  There's no benefit to these other than
> > > sanity-checking the destruction, and given that that destruction should
> happen
> > > correctly with no explicit code written in the subclass destructor at all,
I
> > > wouldn't do them.
> > 
> > I partially agree with this comment. Setting the delegate pointer in the
base
> > class here is indeed to verify that nothing accesses it in the destructor of
> the
> > base class. Resetting the scoped_ptr here however is logical as this
> > BarControlButton is an InkDropHost and so its RemoveInkDropLayer will get
> called
> > when ink_drop_delegate_ will be destroyed. Doing it after dtor returns
seemed
> a
> > bit wrong to me thus this sequence.
> 
> Eh?  Members are destroyed before the dtor returns, not after.  Resetting the
> scoped_ptr here explicitly has basically no observable effect at all.  (It
just
> affects the relative order of different members' destruction, it does not
affect
> when members are destroyed relative to when the subclass portion of the object
> is destroyed or relative to when the destructor returns.)

I think the destruction order in any class is that only after the body of the
destructor is executed, the compiler calls the destructors for all non-static
non-variant members of the class, in reverse order of declaration (and then
proceeds to the base classes). So dtor completes and returns before scoped_ptr
is destroyed unless reset is called in dtor. As the code is written now the
RemoveInkDropLayer is called while inside dtor. If you remove reset() call it
will get called after dtor returns, that is easy to verify.

[Peter Kasting, 2015-12-12 02:58:25]

On 2015/12/12 02:53:06, varkha wrote:
> On 2015/12/12 02:46:09, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > File chrome/browser/ui/views/bar_control_button.cc (right):
> > 
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > chrome/browser/ui/views/bar_control_button.cc:38:
ink_drop_delegate_.reset();
> > On 2015/12/12 02:43:03, varkha wrote:
> > > On 2015/12/12 01:56:18, Peter Kasting wrote:
> > > > I don't think you need to explicitly do this.  It's about to be
destroyed
> > > > anyway, and that destruction will happen before the base class
destructor
> > > runs. 
> > > > I don't think this explicit destruction affects anything.
> > > > 
> > > > I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call
> too. 
> > > The
> > > > only purpose of this call is to make the DCHECK in the base class work. 
> But
> > > > that DCHECK only checks that this set call has been made, and
importantly,
> > it
> > > > _doesn't_ actually check that the ink drop delegate has been destroyed,
> > which
> > > is
> > > > the critical bit.  So to me this is all theater, and we should remove
both
> > the
> > > > set call here and the DCHECK there.  It doesn't actually make the code
> more
> > > > correct or help other subclassers write safer code.
> > > > 
> > > > There _are_ ways we could guarantee the destruction of this object, but
I
> > > think
> > > > they're more mess/work than they're worth; one is to give the base class
> the
> > > > address of the scoped_ptr instead of the contained raw pointer, and the
> > other
> > > is
> > > > to hook up some delegate callbacks so when the delegate is destroyed it
> > > > notifiers an interested listener.  There's no benefit to these other
than
> > > > sanity-checking the destruction, and given that that destruction should
> > happen
> > > > correctly with no explicit code written in the subclass destructor at
all,
> I
> > > > wouldn't do them.
> > > 
> > > I partially agree with this comment. Setting the delegate pointer in the
> base
> > > class here is indeed to verify that nothing accesses it in the destructor
of
> > the
> > > base class. Resetting the scoped_ptr here however is logical as this
> > > BarControlButton is an InkDropHost and so its RemoveInkDropLayer will get
> > called
> > > when ink_drop_delegate_ will be destroyed. Doing it after dtor returns
> seemed
> > a
> > > bit wrong to me thus this sequence.
> > 
> > Eh?  Members are destroyed before the dtor returns, not after.  Resetting
the
> > scoped_ptr here explicitly has basically no observable effect at all.  (It
> just
> > affects the relative order of different members' destruction, it does not
> affect
> > when members are destroyed relative to when the subclass portion of the
object
> > is destroyed or relative to when the destructor returns.)
> 
> I think the destruction order in any class is that only after the body of the
> destructor is executed, the compiler calls the destructors for all non-static
> non-variant members of the class, in reverse order of declaration (and then
> proceeds to the base classes).

That is correct --

> So dtor completes and returns before scoped_ptr
> is destroyed unless reset is called in dtor.

-- And that is incorrect.

The explicit body of the destructor is executed, and the members are then
destroyed after the body executes and before the destructor returns.

Think similarly to when a function-scope local is destroyed when the function
returns: after the body of the function, but before the function actually
returns.

> As the code is written now the
> RemoveInkDropLayer is called while inside dtor. If you remove reset() call it
> will get called after dtor returns, that is easy to verify.

It will get called after the body of the destructor but before the destructor
returns.  Depending on whether there are further subclass members declared after
this one, this will either be completely equivalent to what's happening now, or
different only in a way that should not actually matter.

[varkha, 2015-12-12 03:09:26]

On 2015/12/12 02:58:25, Peter Kasting wrote:
> On 2015/12/12 02:53:06, varkha wrote:
> > On 2015/12/12 02:46:09, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > > File chrome/browser/ui/views/bar_control_button.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > > chrome/browser/ui/views/bar_control_button.cc:38:
> ink_drop_delegate_.reset();
> > > On 2015/12/12 02:43:03, varkha wrote:
> > > > On 2015/12/12 01:56:18, Peter Kasting wrote:
> > > > > I don't think you need to explicitly do this.  It's about to be
> destroyed
> > > > > anyway, and that destruction will happen before the base class
> destructor
> > > > runs. 
> > > > > I don't think this explicit destruction affects anything.
> > > > > 
> > > > > I'm also a bit skeptical about the set_ink_drop_delegate(nullptr) call
> > too. 
> > > > The
> > > > > only purpose of this call is to make the DCHECK in the base class
work. 
> > But
> > > > > that DCHECK only checks that this set call has been made, and
> importantly,
> > > it
> > > > > _doesn't_ actually check that the ink drop delegate has been
destroyed,
> > > which
> > > > is
> > > > > the critical bit.  So to me this is all theater, and we should remove
> both
> > > the
> > > > > set call here and the DCHECK there.  It doesn't actually make the code
> > more
> > > > > correct or help other subclassers write safer code.
> > > > > 
> > > > > There _are_ ways we could guarantee the destruction of this object,
but
> I
> > > > think
> > > > > they're more mess/work than they're worth; one is to give the base
class
> > the
> > > > > address of the scoped_ptr instead of the contained raw pointer, and
the
> > > other
> > > > is
> > > > > to hook up some delegate callbacks so when the delegate is destroyed
it
> > > > > notifiers an interested listener.  There's no benefit to these other
> than
> > > > > sanity-checking the destruction, and given that that destruction
should
> > > happen
> > > > > correctly with no explicit code written in the subclass destructor at
> all,
> > I
> > > > > wouldn't do them.
> > > > 
> > > > I partially agree with this comment. Setting the delegate pointer in the
> > base
> > > > class here is indeed to verify that nothing accesses it in the
destructor
> of
> > > the
> > > > base class. Resetting the scoped_ptr here however is logical as this
> > > > BarControlButton is an InkDropHost and so its RemoveInkDropLayer will
get
> > > called
> > > > when ink_drop_delegate_ will be destroyed. Doing it after dtor returns
> > seemed
> > > a
> > > > bit wrong to me thus this sequence.
> > > 
> > > Eh?  Members are destroyed before the dtor returns, not after.  Resetting
> the
> > > scoped_ptr here explicitly has basically no observable effect at all.  (It
> > just
> > > affects the relative order of different members' destruction, it does not
> > affect
> > > when members are destroyed relative to when the subclass portion of the
> object
> > > is destroyed or relative to when the destructor returns.)
> > 
> > I think the destruction order in any class is that only after the body of
the
> > destructor is executed, the compiler calls the destructors for all
non-static
> > non-variant members of the class, in reverse order of declaration (and then
> > proceeds to the base classes).
> 
> That is correct --
> 
> > So dtor completes and returns before scoped_ptr
> > is destroyed unless reset is called in dtor.
> 
> -- And that is incorrect.
> 
> The explicit body of the destructor is executed, and the members are then
> destroyed after the body executes and before the destructor returns.
> 
> Think similarly to when a function-scope local is destroyed when the function
> returns: after the body of the function, but before the function actually
> returns.
> 
> > As the code is written now the
> > RemoveInkDropLayer is called while inside dtor. If you remove reset() call
it
> > will get called after dtor returns, that is easy to verify.
> 
> It will get called after the body of the destructor but before the destructor
> returns.  Depending on whether there are further subclass members declared
after
> this one, this will either be completely equivalent to what's happening now,
or
> different only in a way that should not actually matter.

Yes, I meant that dtor body will complete (and you are right, it will not
actually return) so it would not matter unless we are doing something in the
dtor body that would invalidate the state for a call to RemoveInkDropLayer. Do
you think this is safe to assume?

[Peter Kasting, 2015-12-12 05:55:38]

On 2015/12/12 03:09:26, varkha wrote:
> On 2015/12/12 02:58:25, Peter Kasting wrote:
> > On 2015/12/12 02:53:06, varkha wrote:
> > > On 2015/12/12 02:46:09, Peter Kasting wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > > > File chrome/browser/ui/views/bar_control_button.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1478303003/diff/200001/chrome/browser/ui/view...
> > > > chrome/browser/ui/views/bar_control_button.cc:38:
> > ink_drop_delegate_.reset();
> > > > On 2015/12/12 02:43:03, varkha wrote:
> > > > > On 2015/12/12 01:56:18, Peter Kasting wrote:
> > > > > > I don't think you need to explicitly do this.  It's about to be
> > destroyed
> > > > > > anyway, and that destruction will happen before the base class
> > destructor
> > > > > runs. 
> > > > > > I don't think this explicit destruction affects anything.
> > > > > > 
> > > > > > I'm also a bit skeptical about the set_ink_drop_delegate(nullptr)
call
> > > too. 
> > > > > The
> > > > > > only purpose of this call is to make the DCHECK in the base class
> work. 
> > > But
> > > > > > that DCHECK only checks that this set call has been made, and
> > importantly,
> > > > it
> > > > > > _doesn't_ actually check that the ink drop delegate has been
> destroyed,
> > > > which
> > > > > is
> > > > > > the critical bit.  So to me this is all theater, and we should
remove
> > both
> > > > the
> > > > > > set call here and the DCHECK there.  It doesn't actually make the
code
> > > more
> > > > > > correct or help other subclassers write safer code.
> > > > > > 
> > > > > > There _are_ ways we could guarantee the destruction of this object,
> but
> > I
> > > > > think
> > > > > > they're more mess/work than they're worth; one is to give the base
> class
> > > the
> > > > > > address of the scoped_ptr instead of the contained raw pointer, and
> the
> > > > other
> > > > > is
> > > > > > to hook up some delegate callbacks so when the delegate is destroyed
> it
> > > > > > notifiers an interested listener.  There's no benefit to these other
> > than
> > > > > > sanity-checking the destruction, and given that that destruction
> should
> > > > happen
> > > > > > correctly with no explicit code written in the subclass destructor
at
> > all,
> > > I
> > > > > > wouldn't do them.
> > > > > 
> > > > > I partially agree with this comment. Setting the delegate pointer in
the
> > > base
> > > > > class here is indeed to verify that nothing accesses it in the
> destructor
> > of
> > > > the
> > > > > base class. Resetting the scoped_ptr here however is logical as this
> > > > > BarControlButton is an InkDropHost and so its RemoveInkDropLayer will
> get
> > > > called
> > > > > when ink_drop_delegate_ will be destroyed. Doing it after dtor returns
> > > seemed
> > > > a
> > > > > bit wrong to me thus this sequence.
> > > > 
> > > > Eh?  Members are destroyed before the dtor returns, not after. 
Resetting
> > the
> > > > scoped_ptr here explicitly has basically no observable effect at all. 
(It
> > > just
> > > > affects the relative order of different members' destruction, it does
not
> > > affect
> > > > when members are destroyed relative to when the subclass portion of the
> > object
> > > > is destroyed or relative to when the destructor returns.)
> > > 
> > > I think the destruction order in any class is that only after the body of
> the
> > > destructor is executed, the compiler calls the destructors for all
> non-static
> > > non-variant members of the class, in reverse order of declaration (and
then
> > > proceeds to the base classes).
> > 
> > That is correct --
> > 
> > > So dtor completes and returns before scoped_ptr
> > > is destroyed unless reset is called in dtor.
> > 
> > -- And that is incorrect.
> > 
> > The explicit body of the destructor is executed, and the members are then
> > destroyed after the body executes and before the destructor returns.
> > 
> > Think similarly to when a function-scope local is destroyed when the
function
> > returns: after the body of the function, but before the function actually
> > returns.
> > 
> > > As the code is written now the
> > > RemoveInkDropLayer is called while inside dtor. If you remove reset() call
> it
> > > will get called after dtor returns, that is easy to verify.
> > 
> > It will get called after the body of the destructor but before the
destructor
> > returns.  Depending on whether there are further subclass members declared
> after
> > this one, this will either be completely equivalent to what's happening now,
> or
> > different only in a way that should not actually matter.
> 
> Yes, I meant that dtor body will complete (and you are right, it will not
> actually return) so it would not matter unless we are doing something in the
> dtor body that would invalidate the state for a call to RemoveInkDropLayer. Do
> you think this is safe to assume?

We don't need to assume it: we know it's OK in the cases in this CL, and if in
the future a class needs to do something where it's not OK, then in that
specific case the class can add an explicit reset() call.

[bruthig, 2015-12-15 22:55:21]

Patchset #12 (id:220001) has been deleted

[bruthig, 2015-12-15 22:55:28]

Patchset #12 (id:240001) has been deleted

[bruthig, 2015-12-18 18:36:05]

pkasting@, I've removed the explicit calls to destroy the
InkDropDelegates.  Review if you wish, but not required.

varkha@, I've removed ALL explicit calls to destroy the
InkDropDelegates and replaced the ink_drop_delegate_unittests
with VerifyAllInkDropLayersRemovedAfterDestruction in
ink_drop_animation_controller_factory_unittest.cc. PTAL.

sadrul@, PTAL at:
 - ui/views/*

[Peter Kasting, 2015-12-18 18:44:03]

LGTM

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
File ui/views/animation/test/test_ink_drop_host.h (right):

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:13: // A non-functional,
implementation of an InkDropHost that can be used as during
Nit: While here: Remove comma; remove "as"

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:14: // tests.  Tracks the current
number of ink drop layers being hosted.
Nit: "Tracks the number of hosted ink drop layers"?

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:20: int
current_number_of_ink_drop_layers() const {
Nit: I would name all these like "num_ink_drop_layers()".

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:30: // The number of ink drop
layers currently hosted by this.
Nit: Comment adds nothing to the code, remove.

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

https://codereview.chromium.org/1478303003/diff/260001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:60: // descendant classes.
I wouldn't add this comment, since there's no reason a reader would want to
access |ink_drop_delegate_| here anyway.

If you really want to keep it, remove the comma.

[bruthig, 2015-12-21 16:54:34]

I have addressed pkasting@'s nits from patch set 12.

varkha@, I've removed ALL explicit calls to destroy the
InkDropDelegates and replaced the ink_drop_delegate_unittests
with VerifyAllInkDropLayersRemovedAfterDestruction in
ink_drop_animation_controller_factory_unittest.cc. PTAL.

sadrul@, PTAL at:
 - ui/views/*

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
File ui/views/animation/test/test_ink_drop_host.h (right):

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:13: // A non-functional,
implementation of an InkDropHost that can be used as during
On 2015/12/18 18:44:02, Peter Kasting wrote:
> Nit: While here: Remove comma; remove "as"

Done.

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:14: // tests.  Tracks the current
number of ink drop layers being hosted.
On 2015/12/18 18:44:02, Peter Kasting wrote:
> Nit: "Tracks the number of hosted ink drop layers"?

Done.

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:20: int
current_number_of_ink_drop_layers() const {
On 2015/12/18 18:44:02, Peter Kasting wrote:
> Nit: I would name all these like "num_ink_drop_layers()".

Done.

https://codereview.chromium.org/1478303003/diff/260001/ui/views/animation/tes...
ui/views/animation/test/test_ink_drop_host.h:30: // The number of ink drop
layers currently hosted by this.
On 2015/12/18 18:44:02, Peter Kasting wrote:
> Nit: Comment adds nothing to the code, remove.

Done.

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

https://codereview.chromium.org/1478303003/diff/260001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:60: // descendant classes.
On 2015/12/18 18:44:03, Peter Kasting wrote:
> I wouldn't add this comment, since there's no reason a reader would want to
> access |ink_drop_delegate_| here anyway.
> 
> If you really want to keep it, remove the comma.

Done.

[varkha, 2015-12-21 18:37:31]

Still LGTM.

[sadrul, 2015-12-21 22:24:35]

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (left):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:62: }
This looks like a good check to have. Why remove?

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:130: }
It doesn't look like this is actually ever used?

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:191: InkDropState
ink_drop_action_on_click_;
Can there be an InkDropState::NONE, instead of having a separate bool?

[bruthig, 2015-12-22 16:49:18]

sadrul@, can you PTAL?

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (left):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:62: }
On 2015/12/21 22:24:35, sadrul (slow until new year) wrote:
> This looks like a good check to have. Why remove?

See comments #16 https://codereview.chromium.org/1478303003/#msg16
through #22 https://codereview.chromium.org/1478303003/#msg22
for context.

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:130: }
On 2015/12/21 22:24:35, sadrul (slow until new year) wrote:
> It doesn't look like this is actually ever used?

I expect it to be used by buttons that would like to use something other than
the QUICK_ACTION state for a button click. More concretely there is work going
on to make menu shows asynchronous.  I expect these buttons to use this function
to set the click action to ACTIVATED once the dust has settled.

Would you rather I omit it for now?

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:191: InkDropState
ink_drop_action_on_click_;
On 2015/12/21 22:24:35, sadrul (slow until new year) wrote:
> Can there be an InkDropState::NONE, instead of having a separate bool?

There are plans to add an InkDropDelegate::InkDropAction enum that will be used
here instead.  The InkDropAction enum will have a NONE value and we will be able
to drop this flag.  I've added TODO's to track this work.

[sadrul, 2015-12-22 22:34:35]

lgtm

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:130: }
On 2015/12/22 16:49:17, bruthig wrote:
> On 2015/12/21 22:24:35, sadrul (slow until new year) wrote:
> > It doesn't look like this is actually ever used?
> 
> I expect it to be used by buttons that would like to use something other than
> the QUICK_ACTION state for a button click. More concretely there is work going
> on to make menu shows asynchronous.  I expect these buttons to use this
function
> to set the click action to ACTIVATED once the dust has settled.
> 
> Would you rather I omit it for now?

Yeah, let's remove that until we need it.

[bruthig, 2015-12-22 23:10:10]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-12-22 23:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, varkha@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1478303003/#ps340001
(title: "Removed CustomButton::set_ink_drop_action_on_click() method.")

[bruthig, 2015-12-22 23:10:15]

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1478303003/diff/300001/ui/views/controls/butt...
ui/views/controls/button/custom_button.h:130: }
On 2015/12/22 22:34:35, sadrul (slow until new year) wrote:
> On 2015/12/22 16:49:17, bruthig wrote:
> > On 2015/12/21 22:24:35, sadrul (slow until new year) wrote:
> > > It doesn't look like this is actually ever used?
> > 
> > I expect it to be used by buttons that would like to use something other
than
> > the QUICK_ACTION state for a button click. More concretely there is work
going
> > on to make menu shows asynchronous.  I expect these buttons to use this
> function
> > to set the click action to ACTIVATED once the dust has settled.
> > 
> > Would you rather I omit it for now?
> 
> Yeah, let's remove that until we need it.

Done.

[commit-bot: I haz the power, 2015-12-22 23:11:03]

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

[commit-bot: I haz the power, 2015-12-22 23:57:51]

Description was changed from

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.
==========

to

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.
==========

[commit-bot: I haz the power, 2015-12-22 23:57:52]

Committed patchset #16 (id:340001)

[commit-bot: I haz the power, 2015-12-22 23:58:29]

Description was changed from

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.
==========

to

==========
Converted all Views to use an InkDropDelegate instead of a
InkDropAnimationController.

An InkDropDelegate was introduced in https://codereview.chromium.org/1411833006/
but not all the Views were updated to use the InkDropDelegate
instead of the InkDropAnimationController.  This review is the
follow up that updates all the Views to use the delegate instead
of the controller.

Committed: https://crrev.com/502a5c9660bfd1c2ac949e4388bc8697d06b5507
Cr-Commit-Position: refs/heads/master@{#366694}
==========

[commit-bot: I haz the power, 2015-12-22 23:58:30]

Patchset 16 (id:??) landed as
https://crrev.com/502a5c9660bfd1c2ac949e4388bc8697d06b5507
Cr-Commit-Position: refs/heads/master@{#366694}

[bruthig, 2016-01-06 15:46:50]

Patchset #17 (id:360001) has been deleted