Make bookmark context menu asynchronous

Make bookmark context menu asynchronous

BUG=569051
Committed: https://crrev.com/9e9b82bf205386de83909abd78fccc877f949ad1
Cr-Commit-Position: refs/heads/master@{#373756}

[mohsen, 2016-02-03 04:02:50]

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

[mohsen, 2016-02-03 04:02:50]

Please take a look...

[sky, 2016-02-03 16:51:57]

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc:81:
ignore_result(menu_runner_->RunMenuAt(parent_widget_, nullptr,
As the return value is no longer important I say we remove WARN_UNUSED_RESULT.

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.h (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.h:70: void
OnMenuClosed(views::MenuItemView* menu,
Remind me, is this called if the menu is closed because the runner is destroyed?

[mohsen, 2016-02-03 22:14:13]

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc:81:
ignore_result(menu_runner_->RunMenuAt(parent_widget_, nullptr,
On 2016/02/03 at 16:51:56, sky wrote:
> As the return value is no longer important I say we remove WARN_UNUSED_RESULT.

Done.

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.h (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.h:70: void
OnMenuClosed(views::MenuItemView* menu,
On 2016/02/03 at 16:51:56, sky wrote:
> Remind me, is this called if the menu is closed because the runner is
destroyed?

If the runner is destroyed as a result of an ExecuteCommand() this will not be
called. I believe, the runner is not destroyed for bookmark context menu on
ExecuteCommand(), so this will always be called whenever the menu is closed.

[sky, 2016-02-04 03:32:59]

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.h (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.h:70: void
OnMenuClosed(views::MenuItemView* menu,
On 2016/02/03 22:14:12, mohsen wrote:
> On 2016/02/03 at 16:51:56, sky wrote:
> > Remind me, is this called if the menu is closed because the runner is
> destroyed?
> 
> If the runner is destroyed as a result of an ExecuteCommand() this will not be
> called. I believe, the runner is not destroyed for bookmark context menu on
> ExecuteCommand(), so this will always be called whenever the menu is closed.

The case I'm worried about is if this object is destroyed while the menu is up.
Does that result in OnMenuClosed?

[mohsen, 2016-02-04 18:59:53]

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.h (right):

https://codereview.chromium.org/1647003002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.h:70: void
OnMenuClosed(views::MenuItemView* menu,
On 2016/02/04 at 03:32:59, sky wrote:
> On 2016/02/03 22:14:12, mohsen wrote:
> > On 2016/02/03 at 16:51:56, sky wrote:
> > > Remind me, is this called if the menu is closed because the runner is
> > destroyed?
> > 
> > If the runner is destroyed as a result of an ExecuteCommand() this will not
be
> > called. I believe, the runner is not destroyed for bookmark context menu on
> > ExecuteCommand(), so this will always be called whenever the menu is closed.
> 
> The case I'm worried about is if this object is destroyed while the menu is
up. Does that result in OnMenuClosed?

No, if MenuRunner is deleted, OnMenuClosed is not called on the delegate. For
bookmark context menu, the MenuRunner is only deleted if its owner
(BookmarkBarView or BookmarkMenuDelegate) is deleted which will also delete
context menu and we should have no problem. Right?

[sky, 2016-02-04 22:37:53]

Yes, LGTM

[mohsen, 2016-02-05 05:54:36]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2016-02-05 05:54:54]

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

[commit-bot: I haz the power, 2016-02-05 06:03:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-05 06:03:59]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-02-05 06:04:50]

Description was changed from

==========
Make bookmark context menu asynchronous

BUG=569051
==========

to

==========
Make bookmark context menu asynchronous

BUG=569051

Committed: https://crrev.com/9e9b82bf205386de83909abd78fccc877f949ad1
Cr-Commit-Position: refs/heads/master@{#373756}
==========

[commit-bot: I haz the power, 2016-02-05 06:04:50]

Patchset 5 (id:??) landed as
https://crrev.com/9e9b82bf205386de83909abd78fccc877f949ad1
Cr-Commit-Position: refs/heads/master@{#373756}