Views: Expose an on_closed callback via the MenuRunner constructor.

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
Committed: https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05
Cr-Commit-Position: refs/heads/master@{#425186}

[tapted, 2016-10-12 00:48:30]

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

[commit-bot: I haz the power, 2016-10-12 00:48:50]

Dry run: 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-10-12 01:01:02]

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

[commit-bot: I haz the power, 2016-10-12 01:01:03]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2016-10-12 01:01:48]

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

[commit-bot: I haz the power, 2016-10-12 01:02:23]

Dry run: 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-10-12 01:13:50]

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

[commit-bot: I haz the power, 2016-10-12 01:13:51]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2016-10-12 02:17:13]

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

[commit-bot: I haz the power, 2016-10-12 02:17:34]

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

[tapted, 2016-10-12 03:12:38]

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

[commit-bot: I haz the power, 2016-10-12 03:12:54]

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

[tapted, 2016-10-12 03:15:12]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-10-12 03:15:21]

Patchset #2 (id:20001) has been deleted

[tapted, 2016-10-12 03:15:27]

Patchset #1 (id:1) has been deleted

[tapted, 2016-10-12 03:31:17]

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

[commit-bot: I haz the power, 2016-10-12 03:31:40]

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

[tapted, 2016-10-12 03:35:40]

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

[commit-bot: I haz the power, 2016-10-12 03:35:55]

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

[tapted, 2016-10-12 03:41:55]

Patchset #5 (id:140001) has been deleted

[tapted, 2016-10-12 03:47:35]

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

[commit-bot: I haz the power, 2016-10-12 03:47:57]

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

[tapted, 2016-10-12 04:26:30]

Description was changed from

==========
Views: wrap the OnMenuClosed() callback in MenuRunner


WIP

BUG=653334
==========

to

==========
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 consistence and any sync/async divergence is
intentional.

BUG=653334
==========

[tapted, 2016-10-12 04:26:30]

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

[tapted, 2016-10-12 04:26:41]

Patchset #5 (id:160001) has been deleted

[tapted, 2016-10-12 04:27:15]

Description was changed from

==========
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 consistence and any sync/async divergence is
intentional.

BUG=653334
==========

to

==========
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 consistence and any sync/async divergence is
intentional.

4 clients no longer need a MenuModelAdapter, so remove it.

BUG=653334
==========

[tapted, 2016-10-12 04:27:40]

Description was changed from

==========
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 consistence and any sync/async divergence is
intentional.

4 clients no longer need a MenuModelAdapter, so remove it.

BUG=653334
==========

to

==========
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 consistence and any sync/async divergence is
intentional.

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

BUG=653334
==========

[tapted, 2016-10-12 04:28:21]

Description was changed from

==========
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 consistence and any sync/async divergence is
intentional.

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

BUG=653334
==========

to

==========
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 consistence and any sync/async divergence is
intentional.

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

BUG=653334
==========

[tapted, 2016-10-12 04:28:40]

Description was changed from

==========
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 consistence and any sync/async divergence is
intentional.

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

BUG=653334
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-10-12 05:44:49]

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

[commit-bot: I haz the power, 2016-10-12 05:44:50]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tapted, 2016-10-12 07:40:28]

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

[commit-bot: I haz the power, 2016-10-12 07:40:41]

Dry run: 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-10-12 09:47:17]

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

[commit-bot: I haz the power, 2016-10-12 09:47:18]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-12 10:05:21]

Hi sky, please take a look

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:115: MenuRunner(ui::MenuModel* menu_model,
int32_t run_types);
Patchset 2 didn't keep this overload - there's a bit of fallout (and more if I
fixed those compile errors). But in a follow-up it might be neater to remove
this - to encourage providing a callback in _all_ cases, since clients who do
not reset() their |menu_runner_| instance effectively leak the menu until the
client itself is destroyed.

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:118: MenuRunner(MenuItemView* menu, int32_t
run_types);
I looked around for other things that might miss out on native menus. But there
aren't many callers of this overload left. Only 2 in the current Mac build:
 - scroll_bar (for a context menu that will probably get nerfed on Mac once
someone notices it exists at all).
 - RenderViewContextMenuBase's RenderViewContextMenuBase (since it does "live"
menu item updates) (but I think this is dead code on Mac right now)

Then, a few more in a mac_views_browser_build
 - toolbar_button.cc (since they allow middle-clicks on menu items)
 - The "App" menu (wrench/hotdog - since it has crazy customizations)
 - bookmarks (we would not use native menus on mac either, since we need drag &
drop)
 - toolbar_action_view.cc (it exposes a MenuItemView* for testing)

(but then a few more that are only used in ash)

But none of these scare me off wanting to use native menus more widely :). I.e.
I think there's a neat solution for things we want using native menus on Mac.

[jonross, 2016-10-12 14:35:17]

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

[jonross, 2016-10-12 14:35:18]

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:115: MenuRunner(ui::MenuModel* menu_model,
int32_t run_types);
On 2016/10/12 10:05:21, tapted wrote:
> Patchset 2 didn't keep this overload - there's a bit of fallout (and more if I
> fixed those compile errors). But in a follow-up it might be neater to remove
> this - to encourage providing a callback in _all_ cases, since clients who do
> not reset() their |menu_runner_| instance effectively leak the menu until the
> client itself is destroyed.

There are some clients which are transient. The views triggering the menus don't
last long. (Such as the Notifications bubble on Chromium OS.) So the closed
MenuRunner is not always around long.

However I wouldn't be against a forced closing step

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:164:
on_menu_closed_callback_.Run();
I just want to confirm, the beginning of this method perform the blocking calls.
Removing the calls into ui/views/controls/menu/menu_controller?

If so, that doesn't block the cleanup that we want to do there (remove the
synchronous code path)

One far off concern with having a set of menus blocking is related to the mus
window server, and how we ack events. As nested message loops remove guarantees
on receiving acks for a given event. However if we know the an isolated set of
cases, like this, we could post the acks for those.

[sky, 2016-10-12 16:22:32]

We already have MenuWillClose in MenuModel. Did you consider adding MenuClosed
in MenuModel?

[tapted, 2016-10-13 01:22:07]

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

[commit-bot: I haz the power, 2016-10-13 01:22:16]

Dry run: 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-10-13 01:32:55]

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

[commit-bot: I haz the power, 2016-10-13 01:32:56]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2016-10-13 02:06:43]

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

[commit-bot: I haz the power, 2016-10-13 02:07:11]

Dry run: 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-10-13 02:18:34]

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

[commit-bot: I haz the power, 2016-10-13 02:18:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2016-10-13 02:46:14]

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

[commit-bot: I haz the power, 2016-10-13 02:46:34]

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

[tapted, 2016-10-13 02:47:06]

Patchset #7 (id:220001) has been deleted

[tapted, 2016-10-13 02:47:25]

Patchset #8 (id:260001) has been deleted

[tapted, 2016-10-13 02:47:44]

Patchset #8 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-10-13 02:58:04]

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

[commit-bot: I haz the power, 2016-10-13 02:58:05]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2016-10-13 05:18:12]

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

[tapted, 2016-10-13 05:18:15]

Patchset #8 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-10-13 05:18:38]

Dry run: 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-10-13 06:17:07]

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

[commit-bot: I haz the power, 2016-10-13 06:17:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2016-10-13 08:02:47]

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

[commit-bot: I haz the power, 2016-10-13 08:02:57]

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

[tapted, 2016-10-13 08:03:18]

Patchset #8 (id:320001) has been deleted

[tapted, 2016-10-13 08:21:45]

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

[commit-bot: I haz the power, 2016-10-13 08:22:03]

Dry run: 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-10-13 08:57:45]

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

[commit-bot: I haz the power, 2016-10-13 08:57:46]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-13 11:43:38]

On 2016/10/12 16:22:32, sky wrote:
> We already have MenuWillClose in MenuModel. Did you consider adding MenuClosed
> in MenuModel?

That turns out to be hard. However, deleting both MenuDelegate::MenuWillClose
and SimpleMenuModel::Delegate::MenuClosed was surprisingly easy.... Only
RenderViewContextMenu is using this properly.

Details: I poked around trying to remove on_menu_closed_callback from the
MenuModelAdapter in PatchSet 7. The problem is that the model and the model
delegate are often in cross-platform code. However MenuModelAdapter and
MenuRunner are in views-specific code - it gets a bit curly trying to convince
the cross-platform code to call back into the views code, e.g., to delete the
MenuRunner - see comments for some specific cases in PatchSet 7. We'd need a
delegate-delegate or something. Some simple cases like the download shelf are
neat, and I figured out something for views::Combobox, but doing it for some
others makes a mess.

So, I think there's still a case for a closed callback via MenuRunner -- it's a
good fit for doing the platform-specific cleanup around running a menu, whereas
the model delegates can concern themselves only with cross-platform code.

Instead, I explored whether we could just delete MenuWillClose from the
cross-platform delegate interfaces (i.e. just use the callback passed into
MenuRunner). This is in PatchSet 8 - I'm pretty sure it works (trybots are green
:), and it's a bit of a code cleanup. But it doesn't feel right.. and it's also
orthogonal to this CL (i.e. there's no added churn if we still want to explore
that in a follow-up still). So, I went back to PatchSet 6 in the end.

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right):

https://codereview.chromium.org/2394123002/diff/200001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner_impl_cocoa.mm:164:
on_menu_closed_callback_.Run();
On 2016/10/12 14:35:18, jonross wrote:
> I just want to confirm, the beginning of this method perform the blocking
calls.
> Removing the calls into ui/views/controls/menu/menu_controller?
> 
> If so, that doesn't block the cleanup that we want to do there (remove the
> synchronous code path)

Yup - exactly.

> 
> One far off concern with having a set of menus blocking is related to the mus
> window server, and how we ack events. As nested message loops remove
guarantees
> on receiving acks for a given event. However if we know the an isolated set of
> cases, like this, we could post the acks for those.

It's also possible we can get the mac-themed views menus to a point where
they're almost indistinguishable, so we can use that on Mus. For now they don't
integrate well with Mac screenreaders, and there are other subtle UI and UX
differences that that were making toolkit-views on mac less palatable :/

[sky, 2016-10-13 17:44:07]

LGTM with default arg

https://codereview.chromium.org/2394123002/diff/360001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2394123002/diff/360001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:114: const base::Closure&
on_menu_closed_callback);
Combine this with the next constructor and use a default value? (style guide now
allows default values in cases like this).

[tapted, 2016-10-13 21:29:15]

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

[commit-bot: I haz the power, 2016-10-13 21:29:37]

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

[tapted, 2016-10-13 21:31:40]

https://codereview.chromium.org/2394123002/diff/360001/ui/views/controls/menu...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/2394123002/diff/360001/ui/views/controls/menu...
ui/views/controls/menu/menu_runner.h:114: const base::Closure&
on_menu_closed_callback);
On 2016/10/13 17:44:07, sky wrote:
> Combine this with the next constructor and use a default value? (style guide
now
> allows default values in cases like this).

Done.

[tapted, 2016-10-13 21:31:54]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2016-10-13 21:32:03]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-10-13 21:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2394123002/#ps370001
(title: "default arg")

[commit-bot: I haz the power, 2016-10-13 21:32:31]

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-10-13 22:19:04]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-10-13 22:19:05]

Committed patchset #10 (id:370001)

[commit-bot: I haz the power, 2016-10-13 22:21:47]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05
Cr-Commit-Position: refs/heads/master@{#425186}
==========

[commit-bot: I haz the power, 2016-10-13 22:21:48]

Patchset 10 (id:??) landed as
https://crrev.com/7a7c6831b11536332403881c6619986b55f62e05
Cr-Commit-Position: refs/heads/master@{#425186}