Cleanup MenuRunner API

Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TBR=ben@chromium.org
TEST=manual and test suites
BUG=557130
Review-Url: https://codereview.chromium.org/2790773002
Cr-Commit-Position: refs/heads/master@{#466469}
Committed: https://chromium.googlesource.com/chromium/src/+/5ab7c2d09320aa6f4fc6ba0118908ac7e378e77b

[jonross, 2017-03-30 21:31:40]

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

[commit-bot: I haz the power, 2017-03-30 21:33:15]

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, 2017-03-30 21:54:09]

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

[commit-bot: I haz the power, 2017-03-30 21:54:10]

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

[jonross, 2017-03-31 15:26:13]

Description was changed from

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TEST=manual and test suites
BUG=557130
==========

to

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TEST=manual and test suites
BUG=557130
==========

[jonross, 2017-03-31 15:53:16]

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

[commit-bot: I haz the power, 2017-03-31 15:53:42]

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, 2017-03-31 16:08:17]

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

[commit-bot: I haz the power, 2017-03-31 16:08:18]

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

[jonross, 2017-03-31 17:03:42]

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

[commit-bot: I haz the power, 2017-03-31 17:04:08]

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, 2017-03-31 17:25:03]

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

[commit-bot: I haz the power, 2017-03-31 17:25:03]

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

[jonross, 2017-03-31 20:26:02]

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

[commit-bot: I haz the power, 2017-03-31 20:26:32]

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, 2017-03-31 20:43:23]

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

[commit-bot: I haz the power, 2017-03-31 20:43:24]

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

[jonross, 2017-03-31 20:51:01]

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

[commit-bot: I haz the power, 2017-03-31 20:51:43]

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, 2017-03-31 22:51:57]

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

[commit-bot: I haz the power, 2017-03-31 22:51:58]

Dry run: This issue passed the CQ dry run.

[jonross, 2017-03-31 23:47:57]

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

[jonross, 2017-03-31 23:47:58]

Hey sky@

This is a followup to the last menu cleanup patch.

With the switch over to async, the previous MenuRunner::RunResult was no longer
needed anywhere. So this change removes that.

Additionally it clears out all of the ASYNC runtype, since that is the only
option.

PTAL.

Thanks,
Jon

[sky, 2017-04-01 17:15:09]

Don't we still need the return type for the mac side that may still run a
nested message loop?

On Fri, Mar 31, 2017 at 4:47 PM, <jonross@chromium.org> wrote:

> Reviewers: sky
> CL: https://codereview.chromium.org/2790773002/
>
> Message:
> Hey sky@
>
> This is a followup to the last menu cleanup patch.
>
> With the switch over to async, the previous MenuRunner::RunResult was no
> longer
> needed anywhere. So this change removes that.
>
> Additionally it clears out all of the ASYNC runtype, since that is the only
> option.
>
> PTAL.
>
> Thanks,
> Jon
>
> Description:
> Cleanup MenuRunner API
>
> All menus have switched over to MenuRunner::ASYNC. The non-async code path
> has
> also been removed. Due to this the flag is redundant. This change removes
> its
> usage throughout the code base.
>
> MenuRunner::RunMenuAt features a return type to declare if a menu was
> deleted
> or not during execution. This was used for cleanup relating to the nesting
> of
> message loops. With the nested message loop code path gone this is no
> longer
> necessary. No call site was actively performing different functions on
> this.
>
> Now all callsites are notified via MenuDelegate::OnMenuClosed, or a
> callback
> provided to MenuRunner, when a proper closure occurs.
>
> Deletion during a run is only triggered by either an entire system
> shutdown, or
> when callsites explicitly delete the menu when executing a menu item. All
> callsites have been updated to cleanup properly when deleting the menu.
>
> With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
> needed. So this change removes it.
>
> TEST=manual and test suites
> BUG=557130
>
> Affected files (+281, -473 lines):
> M ash/common/shelf/shelf_view.cc
> M ash/root_window_controller.cc
> M ash/shell/window_type_launcher.cc
> M ash/shell_unittest.cc
> M chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
> M chrome/browser/ui/views/bookmarks/bookmark_context_menu.h
> M chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc
> M chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
> M chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h
> M chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
> M chrome/browser/ui/views/download/download_shelf_context_menu_view.cc
> M chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
> M chrome/browser/ui/views/frame/browser_frame.cc
> M chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
> M chrome/browser/ui/views/menu_model_adapter_test.cc
> M chrome/browser/ui/views/menu_test_base.cc
> M chrome/browser/ui/views/menu_view_drag_and_drop_test.cc
> M chrome/browser/ui/views/page_info/permission_selector_row.cc
> M chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
> M chrome/browser/ui/views/status_icons/status_icon_win.cc
> M chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
> M chrome/browser/ui/views/task_manager_view.cc
> M chrome/browser/ui/views/toolbar/app_menu.h
> M chrome/browser/ui/views/toolbar/app_menu.cc
> M chrome/browser/ui/views/toolbar/toolbar_action_view.cc
> M chrome/browser/ui/views/toolbar/toolbar_button.cc
> M chrome/browser/ui/views/translate/translate_bubble_view.cc
> M components/renderer_context_menu/views/toolkit_delegate_views.cc
> M content/shell/browser/shell_web_contents_view_delegate_views.cc
> M mash/browser/browser.cc
> M mash/example/window_type_launcher/window_type_launcher.cc
> M ui/app_list/views/app_list_item_view.cc
> M ui/app_list/views/search_result_tile_item_view.cc
> M ui/app_list/views/search_result_view.cc
> M ui/message_center/views/message_view_context_menu_controller.cc
> M ui/views/controls/combobox/combobox.cc
> M ui/views/controls/label.cc
> M ui/views/controls/menu/menu_controller.h
> M ui/views/controls/menu/menu_controller.cc
> M ui/views/controls/menu/menu_controller_delegate.h
> M ui/views/controls/menu/menu_controller_unittest.cc
> M ui/views/controls/menu/menu_delegate.h
> M ui/views/controls/menu/menu_model_adapter.h
> M ui/views/controls/menu/menu_model_adapter.cc
> M ui/views/controls/menu/menu_runner.h
> M ui/views/controls/menu/menu_runner.cc
> M ui/views/controls/menu/menu_runner_cocoa_unittest.mm
> M ui/views/controls/menu/menu_runner_handler.h
> M ui/views/controls/menu/menu_runner_impl.h
> M ui/views/controls/menu/menu_runner_impl.cc
> M ui/views/controls/menu/menu_runner_impl_adapter.h
> M ui/views/controls/menu/menu_runner_impl_adapter.cc
> M ui/views/controls/menu/menu_runner_impl_cocoa.h
> M ui/views/controls/menu/menu_runner_impl_cocoa.mm
> M ui/views/controls/menu/menu_runner_impl_interface.h
> M ui/views/controls/menu/menu_runner_unittest.cc
> M ui/views/controls/scrollbar/base_scroll_bar.cc
> M ui/views/controls/textfield/textfield.cc
> M ui/views/examples/menu_example.cc
> M ui/views/examples/tree_view_example.cc
> M ui/views/test/combobox_test_api.cc
> M ui/views/test/menu_test_utils.h
> M ui/views/test/menu_test_utils.cc
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2017-04-07 17:47:56]

On 2017/04/01 17:15:09, sky wrote:
> Don't we still need the return type for the mac side that may still run a
> nested message loop?

I do not believe so. All sites launching a menu were not performing anything
differently on MENU_DELETED.

Over time we converted each menu to either
   - Tear down menu in OnMenuClosed
   - Explicitly destroy menu in ExecuteCommand, knowing that OnMenuClosed would
not follow

Originally MENU_DELETED was used to not perform followup cleanup on now deleted
menu objects. But all of that has been removed.

[sky, 2017-04-07 19:16:45]

LGTM

[jonross, 2017-04-07 19:26:20]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-04-07 19:26: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, 2017-04-07 19:38:59]

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

[commit-bot: I haz the power, 2017-04-07 19:39:00]

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...)

[jonross, 2017-04-07 20:55:20]

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

[jonross, 2017-04-07 20:55:21]

Hey Ben,

Could you provide an owners review to:

components/renderer_context_menu/views/toolkit_delegate_views.cc
content/shell/browser/shell_web_contents_view_delegate_views.cc

They have been updated to reflect MenuRunner->RunMenuAt no longer having a
return value, and for views::MenuRunner::ASYNC no longer existing, as it is the
default.

Thanks,
Jon

[jonross, 2017-04-21 21:10:34]

Description was changed from

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TEST=manual and test suites
BUG=557130
==========

to

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TBR=ben@chromium.org
TEST=manual and test suites
BUG=557130
==========

[jonross, 2017-04-21 21:37:52]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-04-21 21:37:53]

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/2790773002/#ps100001
(title: "Rebase")

[jonross, 2017-04-21 21:38:36]

I have TBRed ben@ as the changes to those files were mechanical based on the
refactor of MenuRunner.

[commit-bot: I haz the power, 2017-04-21 21:38:52]

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, 2017-04-21 22:34:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492810672627100,
"parent_rev": "4dc0ad690ed3a08c56b78b0f679580f74d3acf73", "commit_rev":
"5ab7c2d09320aa6f4fc6ba0118908ac7e378e77b"}

[commit-bot: I haz the power, 2017-04-21 22:34:49]

Description was changed from

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TBR=ben@chromium.org
TEST=manual and test suites
BUG=557130
==========

to

==========
Cleanup MenuRunner API

All menus have switched over to MenuRunner::ASYNC. The non-async code path has
also been removed. Due to this the flag is redundant. This change removes its
usage throughout the code base.

MenuRunner::RunMenuAt features a return type to declare if a menu was deleted
or not during execution. This was used for cleanup relating to the nesting of
message loops. With the nested message loop code path gone this is no longer
necessary. No call site was actively performing different functions on this.

Now all callsites are notified via MenuDelegate::OnMenuClosed, or a callback
provided to MenuRunner, when a proper closure occurs.

Deletion during a run is only triggered by either an entire system shutdown, or
when callsites explicitly delete the menu when executing a menu item. All
callsites have been updated to cleanup properly when deleting the menu.

With the closing logic cleaned up MenuRunner::RunMenuAt return is no longer
needed. So this change removes it.

TBR=ben@chromium.org
TEST=manual and test suites
BUG=557130

Review-Url: https://codereview.chromium.org/2790773002
Cr-Commit-Position: refs/heads/master@{#466469}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ab7c2d09320aa6f4fc6ba011890...
==========

[commit-bot: I haz the power, 2017-04-21 22:34:51]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/5ab7c2d09320aa6f4fc6ba011890...