I started looking at this. Basically, native menus on Mac (NSMenu) do not support asynchronous operation. So the changes going in to make menus asynchronous for Android are cutting out the code Mac needs to invoke the native APIs.

There's a very strong desire to use native menus on Mac. I think there's a neat way to get them back by changing the MenuModel is wrapped. And simulating an asynchronous API using synchronous internals is a lot easier than the other way around.

That is, the current approach to invoke an "OnMenuClosed" callback is to use a views::MenuModelAdapter which allows a views::MenuItemView to communicate back via views::MenuDelegate::OnMenuClosed(). But a views::MenuDelegate (which only works for MenuItemView, not NSMenu) isn't necessary to get an OnMenuClosed -- MenuControllerDelegate::OnMenuClosed() also exists, and is used for both NSMenu and MenuItemView.

So I think we can move the `base::Closure& on_menu_closed_callback` into viewS::MenuRunner, and remove the extra views::MenuModelAdapters that were added to make the menus asynchronous for android. I've started toying with this, but I have a bit more to nut out.

We didn't originally edit the views::MenuRunner API to have callback mechanisms so that we could preserve one style of RunMenuAt during the transition. Though if for Mac there is no callback path if could be a good target to modify.


I'd be interested in knowing what synchronous paths are required for launching the NSMenu on Mac. As we were planning to delete the synchronous code path.

https://codereview.chromium.org/2394123002/ is out for review. The callback is essentially just plumbed through to the MenuModelAdapter (for views menus) or the MenuRunnerImplCocoa (for native menus). So there's no change to the RunMenuAt calls.

But I guess the weird part that getting a native NSMenu on Mac makes the call to RunMenuAt blocking, even if you pass MenuRunner::ASYNC.

Maybe there's a way to simulate ASYNC if it's needed. There's no way to make +[NSMenu popUpContextMenu:..] or -[NSMenu popUpMenuPositioningItem:] return until the menu is closed. But posting that task and another to spin a nested run loop in NSEventTrackingRunLoopMode might allow "things" to happen while the menu is still showing.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a7c6831b11536332403881c6619986b55f62e05

commit 7a7c6831b11536332403881c6619986b55f62e05
Author: tapted <tapted@chromium.org>
Date: Thu Oct 13 22:18:35 2016

Views: Expose an on_closed callback via the MenuRunner constructor.

Clients often store a MenuRunner instance as a member to ensure menus
are dismissed when the client is destroyed. If a client doesn't also
observe the menu closing, it effectively leaks the MenuRunner. So, it's
a common pattern to need an on_closed callback.

Previously, this callback was only provided via MenuModelAdapter APIs.
MenuModelAdapter isn't used for Native NSMenus on MacViews, so clients
wanting on_closed were not able to show native menus.

To fix, allow the MenuRunner constructor to pass an on_closed callback
through; either to the MenuModelAdapter, or the MenuRunnerImplCocoa
which gains support for invoking the callback.

Tests in menu_runner_cocoa_unittest.mm are paramaterized to test both
native (NSMenu, synchronous) and views (MenuItemView, asyncrhonous) to
ensure the behaviour is consistent and any sync/async divergence is
intentional.

4 clients no longer need a MenuModelAdapter - remove it from them.

BUG= 653334 

Review-Url: https://codereview.chromium.org/2394123002
Cr-Commit-Position: refs/heads/master@{#425186}

[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/download/download_shelf_context_menu_view.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/download/download_shelf_context_menu_view.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/combobox/combobox.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/combobox/combobox.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_cocoa_unittest.mm
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl_adapter.cc
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl_adapter.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl_cocoa.h
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl_cocoa.mm
[modify] https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05/ui/views/controls/menu/menu_runner_impl_interface.h

Native menus are back in bookmark bubble and OIB