Turn Bookmark Menus Async

Turn Bookmark Menus Async

Update BookmarkEditorView, and BookmarkMenuControllerViews to launch menus asynchronously.

Their context menus, drop down menus, and drag and drop menus have all been converted. Shutdown has been updated as previous actions were being performed on partially destructed menus, which can cause crashes.

A crash in MenuHost was found based on destruction order. This has bee fixed.

TEST=MenuControllerTest.AsynchronousDragHostDeleted, ran unit_tests to capture current bookmark tests, manually tested
BUG=557139, 557130
Committed: https://crrev.com/3b7f52e862d774f939ecbf7cdce4dfae047dbb12
Cr-Commit-Position: refs/heads/master@{#409316}

[jonross, 2016-07-20 14:26:29]

Patchset #1 (id:1) has been deleted

[jonross, 2016-07-20 14:27:10]

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

[jonross, 2016-07-20 14:27:11]

Sky@ could you provide a review of this change? It turns bookmark menus to
async, and fixes a crash in MenuHost that I found.

[sky, 2016-07-20 16:47:10]

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
(right):

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc:194: if
(!menu_runner_ || !menu_runner_->IsRunning() ||
Why do you need the !IsRunning() check? Shouldn't that be a no-op if !running?

[jonross, 2016-07-20 17:43:19]

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
(right):

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc:194: if
(!menu_runner_ || !menu_runner_->IsRunning() ||
On 2016/07/20 16:47:10, sky wrote:
> Why do you need the !IsRunning() check? Shouldn't that be a no-op if !running?

It is a no opt as long as there has not been any destruction of the classes.

I found that this method can be called after its associated menu/menucontroller
have shut. Menu holds onto the MenuController*, which can lead to calls on
destructed objects.

I'd considered updating MenuController to have other silent exits, but thought
it might be better to detect where calling code was causing crashes, and to fix
there.

[sky, 2016-07-20 19:41:53]

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
(right):

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc:194: if
(!menu_runner_ || !menu_runner_->IsRunning() ||
On 2016/07/20 17:43:19, jonross wrote:
> On 2016/07/20 16:47:10, sky wrote:
> > Why do you need the !IsRunning() check? Shouldn't that be a no-op if
!running?
> 
> It is a no opt as long as there has not been any destruction of the classes.
> 
> I found that this method can be called after its associated
menu/menucontroller
> have shut. Menu holds onto the MenuController*, which can lead to calls on
> destructed objects.
> 
> I'd considered updating MenuController to have other silent exits, but thought
> it might be better to detect where calling code was causing crashes, and to
fix
> there.

I would expect a function named Cancel() to be callable at any time, and it's
easy to miss that you should only call it under certain circumstances. I think
it better to have MenuRunner cleanup internal state so that Cancel() can be
called at anytime.

[jonross, 2016-07-21 14:02:58]

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
(right):

https://codereview.chromium.org/2155243007/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc:194: if
(!menu_runner_ || !menu_runner_->IsRunning() ||
On 2016/07/20 19:41:53, sky wrote:
> On 2016/07/20 17:43:19, jonross wrote:
> > On 2016/07/20 16:47:10, sky wrote:
> > > Why do you need the !IsRunning() check? Shouldn't that be a no-op if
> !running?
> > 
> > It is a no opt as long as there has not been any destruction of the classes.
> > 
> > I found that this method can be called after its associated
> menu/menucontroller
> > have shut. Menu holds onto the MenuController*, which can lead to calls on
> > destructed objects.
> > 
> > I'd considered updating MenuController to have other silent exits, but
thought
> > it might be better to detect where calling code was causing crashes, and to
> fix
> > there.
> 
> I would expect a function named Cancel() to be callable at any time, and it's
> easy to miss that you should only call it under certain circumstances. I think
> it better to have MenuRunner cleanup internal state so that Cancel() can be
> called at anytime.

Done.

[sky, 2016-07-21 16:07:54]

https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:507: if (GetActiveInstance() != this)
Your comment sort of implies this may be called on a deleted object. Is that the
case?

[jonross, 2016-07-21 18:30:25]

https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:507: if (GetActiveInstance() != this)
On 2016/07/21 16:07:54, sky wrote:
> Your comment sort of implies this may be called on a deleted object. Is that
the
> case?

Correct, MenuItemViews hold onto a raw pointer to their MenuController.
Currently they are unaware of the MC being destroyed.

[jonross, 2016-07-22 19:28:43]

On 2016/07/21 18:30:25, jonross wrote:
>
https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
> File ui/views/controls/menu/menu_controller.cc (right):
> 
>
https://codereview.chromium.org/2155243007/diff/40001/ui/views/controls/menu/...
> ui/views/controls/menu/menu_controller.cc:507: if (GetActiveInstance() !=
this)
> On 2016/07/21 16:07:54, sky wrote:
> > Your comment sort of implies this may be called on a deleted object. Is that
> the
> > case?
> 
> Correct, MenuItemViews hold onto a raw pointer to their MenuController.
> Currently they are unaware of the MC being destroyed.

The crash been seen in MenuController is gone after a rebase. Removing those
changes as unneccessary.

[sky, 2016-07-22 23:33:42]

LGTM

[jonross, 2016-07-25 13:27:48]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2016-07-25 13:28:15]

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-07-25 14:55:08]

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

[commit-bot: I haz the power, 2016-07-25 14:55:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jonross, 2016-07-28 13:37:11]

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

[commit-bot: I haz the power, 2016-07-28 13:37:30]

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

[jonross, 2016-07-28 14:15:00]

On 2016/07/25 14:55:09, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Sky@ the Windows bots found a bug in menu code. We were not listening for cancel
events in asynchronous menus.

I've refactored ActivationChangeObserverImpl out of MenuMessageLoopAura, and
moved this responsibility to MenuController, similar to when KeyEventHandler was
refactored.

I've added unittests for this as well.

Could you PTAL?

[sky, 2016-08-01 21:39:45]

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/activation_change_observer_impl.h (right):

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/activation_change_observer_impl.h:24: class
ActivationChangeObserverImpl
This name makes sense as an inner class, but for a top-level class it's a bit
unclear. How about a more focused name, perhaps MenuActivationChangeObserver?

Can we also combine the MenuKeyEventHandler would this class? It seems silly to
have to separate EventHandlers. Note that the order they are installed is
slightly different, but I'm not sure that matters.

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:449: // Only create a
MenuKeyEventHandler and ActivationChnageObserver for
'Chnage'->Change

[jonross, 2016-08-02 15:03:25]

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/activation_change_observer_impl.h (right):

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/activation_change_observer_impl.h:24: class
ActivationChangeObserverImpl
On 2016/08/01 21:39:45, sky wrote:
> This name makes sense as an inner class, but for a top-level class it's a bit
> unclear. How about a more focused name, perhaps MenuActivationChangeObserver?
> 
> Can we also combine the MenuKeyEventHandler would this class? It seems silly
to
> have to separate EventHandlers. Note that the order they are installed is
> slightly different, but I'm not sure that matters.

I've merged them.

The differences in how they were installed didn't matter. I've switched to the
format of MenuKeyEventHandler, as we only need to be installed once as a
pre-target handler.

For the merged class I went with MenuPreTargetHandler as a name. Doesn't quite
capture the activation change portion, but represents that it eats events. I'm
open to alternative names.

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/2155243007/diff/180001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:449: // Only create a
MenuKeyEventHandler and ActivationChnageObserver for
On 2016/08/01 21:39:45, sky wrote:
> 'Chnage'->Change

Done.

[sky, 2016-08-02 19:35:59]

I probably would have went with MenuEventHandler or MenuEventFilter, but what
you have is fine. LGTM

[jonross, 2016-08-02 19:46:58]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2016-08-02 19:47:55]

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-08-02 20:47:41]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-08-02 20:49:20]

Description was changed from

==========
Turn Bookmark Menus Async

Update BookmarkEditorView, and BookmarkMenuControllerViews to launch menus
asynchronously.

Their context menus, drop down menus, and drag and drop menus have all been
converted. Shutdown has been updated as previous actions were being performed on
partially destructed menus, which can cause crashes.

A crash in MenuHost was found based on destruction order. This has bee fixed.

TEST=MenuControllerTest.AsynchronousDragHostDeleted, ran unit_tests to capture
current bookmark tests, manually tested
BUG=557139, 557130
==========

to

==========
Turn Bookmark Menus Async

Update BookmarkEditorView, and BookmarkMenuControllerViews to launch menus
asynchronously.

Their context menus, drop down menus, and drag and drop menus have all been
converted. Shutdown has been updated as previous actions were being performed on
partially destructed menus, which can cause crashes.

A crash in MenuHost was found based on destruction order. This has bee fixed.

TEST=MenuControllerTest.AsynchronousDragHostDeleted, ran unit_tests to capture
current bookmark tests, manually tested
BUG=557139, 557130

Committed: https://crrev.com/3b7f52e862d774f939ecbf7cdce4dfae047dbb12
Cr-Commit-Position: refs/heads/master@{#409316}
==========

[commit-bot: I haz the power, 2016-08-02 20:49:21]

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