Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

InkDropHostView installs an InkDropGestureHandler which owns a
ScopedTargetHandler. Multiple ScopedTargetHandler's should be nested
properly, so an InkDropGestureHandler cannot be deleted just after
another one is created.

BUG=622905
Committed: https://crrev.com/533291a92875833fd30652a5c937137aa1119b49
Cr-Commit-Position: refs/heads/master@{#402518}

[mohsen, 2016-06-24 17:24:34]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2016-06-24 17:24:35]

sadrul@: Can you please take a look? (bruthig@ is OOO for a few days)

[sadrul, 2016-06-26 03:51:31]

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_host_view.cc:43: // delete the old one, if any,
*before* instantiating a new one.
You mean only one instance can exist *for a particular host_view*, right?

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_host_view.cc:229: gesture_handler_.reset(new
InkDropGestureHandler(this, ink_drop_.get()));
Add a comment here that any existing InkDropGestureHandler instance needs to be
destroyed first, before a new instance is created.

But, instead of recreating gesture_handler_, if it's non-null, can you just
change its |ink_drop_|? e.g. InkDropGestureHandler::SetInkDrop(...)

[mohsen, 2016-06-27 18:59:10]

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_host_view.cc:43: // delete the old one, if any,
*before* instantiating a new one.
On 2016/06/26 at 03:51:31, sadrul wrote:
> You mean only one instance can exist *for a particular host_view*, right?

Right. Updated the comment.

https://codereview.chromium.org/2096003002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_host_view.cc:229: gesture_handler_.reset(new
InkDropGestureHandler(this, ink_drop_.get()));
On 2016/06/26 at 03:51:31, sadrul wrote:
> Add a comment here that any existing InkDropGestureHandler instance needs to
be destroyed first, before a new instance is created.
> 
> But, instead of recreating gesture_handler_, if it's non-null, can you just
change its |ink_drop_|? e.g. InkDropGestureHandler::SetInkDrop(...)

Yes, Added SetInkDrop().

[mohsen, 2016-06-27 22:07:56]

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

[commit-bot: I haz the power, 2016-06-27 22:09:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-06-27 22:10:24]

lgtm (can this have a test?)

[commit-bot: I haz the power, 2016-06-28 01:07:41]

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

[commit-bot: I haz the power, 2016-06-28 01:07:41]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-06-28 02:39:26]

Description was changed from

==========
Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

InkDropHostView installs an InkDropGestureHandler which owns a
ScopedTargetHandler. Multiple ScopedTargetHandler's cannot exist at the
same time, so multiple InkDropGestureHandler's cannot exist at the same
time. Existing InkDropGestureHandler should be deleted before a new one
is created.

BUG=622905
==========

to

==========
Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

InkDropHostView installs an InkDropGestureHandler which owns a
ScopedTargetHandler. Multiple ScopedTargetHandler's should be nested
properly, so an InkDropGestureHandler cannot be deleted just after
another one is created.

BUG=622905
==========

[mohsen, 2016-06-28 03:08:38]

Added a test

[mohsen, 2016-06-28 17:02:17]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-06-28 17:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2096003002/#ps40001
(title: "Added test")

[commit-bot: I haz the power, 2016-06-28 17:02:45]

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-06-28 20:18:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-28 20:19:44]

Description was changed from

==========
Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

InkDropHostView installs an InkDropGestureHandler which owns a
ScopedTargetHandler. Multiple ScopedTargetHandler's should be nested
properly, so an InkDropGestureHandler cannot be deleted just after
another one is created.

BUG=622905
==========

to

==========
Fix crash when InkDropHostView::SetHasInkDrop() is called multiple times

InkDropHostView installs an InkDropGestureHandler which owns a
ScopedTargetHandler. Multiple ScopedTargetHandler's should be nested
properly, so an InkDropGestureHandler cannot be deleted just after
another one is created.

BUG=622905

Committed: https://crrev.com/533291a92875833fd30652a5c937137aa1119b49
Cr-Commit-Position: refs/heads/master@{#402518}
==========

[commit-bot: I haz the power, 2016-06-28 20:19:45]

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