From 4e859b47185cb441b210e4bd85c10644088e4106 Mon Sep 17 00:00:00 2001 From: Birunthan Mohanathas Date: Thu, 14 Sep 2017 00:13:45 +0300 Subject: [PATCH] Remove "async" option from `menu.popup()` All menus are async now. See "Cleanup MenuRunner API" https://codereview.chromium.org/2790773002 --- atom/browser/api/atom_api_menu.h | 3 +- atom/browser/api/atom_api_menu_mac.h | 9 +++-- atom/browser/api/atom_api_menu_mac.mm | 50 ++++++++++--------------- atom/browser/api/atom_api_menu_views.cc | 5 +-- atom/browser/api/atom_api_menu_views.h | 3 +- lib/browser/api/menu.js | 5 +-- spec/api-menu-spec.js | 8 ++-- 7 files changed, 32 insertions(+), 51 deletions(-) diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index df50640b85ae..f2316fa1893a 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -53,8 +53,7 @@ class Menu : public mate::TrackableObject, void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; - virtual void PopupAt( - Window* window, int x, int y, int positioning_item, bool async) = 0; + virtual void PopupAt(Window* window, int x, int y, int positioning_item) = 0; virtual void ClosePopupAt(int32_t window_id) = 0; std::unique_ptr model_; diff --git a/atom/browser/api/atom_api_menu_mac.h b/atom/browser/api/atom_api_menu_mac.h index bc70e5b5ae59..ed897a7907a1 100644 --- a/atom/browser/api/atom_api_menu_mac.h +++ b/atom/browser/api/atom_api_menu_mac.h @@ -22,11 +22,12 @@ class MenuMac : public Menu { protected: MenuMac(v8::Isolate* isolate, v8::Local wrapper); - void PopupAt( - Window* window, int x, int y, int positioning_item, bool async) override; + void PopupAt(Window* window, int x, int y, int positioning_item) override; void PopupOnUI(const base::WeakPtr& native_window, - int32_t window_id, int x, int y, int positioning_item, - bool async); + int32_t window_id, + int x, + int y, + int positioning_item); void ClosePopupAt(int32_t window_id) override; private: diff --git a/atom/browser/api/atom_api_menu_mac.mm b/atom/browser/api/atom_api_menu_mac.mm index 9741202dd727..e9ed004938cd 100644 --- a/atom/browser/api/atom_api_menu_mac.mm +++ b/atom/browser/api/atom_api_menu_mac.mm @@ -27,24 +27,22 @@ MenuMac::MenuMac(v8::Isolate* isolate, v8::Local wrapper) weak_factory_(this) { } -void MenuMac::PopupAt( - Window* window, int x, int y, int positioning_item, bool async) { +void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) { NativeWindow* native_window = window->window(); if (!native_window) return; auto popup = base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(), native_window->GetWeakPtr(), window->ID(), x, y, - positioning_item, async); - if (async) - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup); - else - popup.Run(); + positioning_item); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup); } void MenuMac::PopupOnUI(const base::WeakPtr& native_window, - int32_t window_id, int x, int y, int positioning_item, - bool async) { + int32_t window_id, + int x, + int y, + int positioning_item) { if (!native_window) return; brightray::InspectableWebContents* web_contents = @@ -92,29 +90,21 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, if (rightmostMenuPoint > screenRight) position.x = position.x - [menu size].width; + [popup_controllers_[window_id] setCloseCallback:close_callback]; + // Make sure events can be pumped while the menu is up. + base::MessageLoop::ScopedNestableTaskAllower allow( + base::MessageLoop::current()); - if (async) { - [popup_controllers_[window_id] setCloseCallback:close_callback]; - // Make sure events can be pumped while the menu is up. - base::MessageLoop::ScopedNestableTaskAllower allow( - base::MessageLoop::current()); + // One of the events that could be pumped is |window.close()|. + // User-initiated event-tracking loops protect against this by + // setting flags in -[CrApplication sendEvent:], but since + // web-content menus are initiated by IPC message the setup has to + // be done manually. + base::mac::ScopedSendingEvent sendingEventScoper; - // One of the events that could be pumped is |window.close()|. - // User-initiated event-tracking loops protect against this by - // setting flags in -[CrApplication sendEvent:], but since - // web-content menus are initiated by IPC message the setup has to - // be done manually. - base::mac::ScopedSendingEvent sendingEventScoper; - - // Don't emit unresponsive event when showing menu. - atom::UnresponsiveSuppressor suppressor; - [menu popUpMenuPositioningItem:item atLocation:position inView:view]; - } else { - // Don't emit unresponsive event when showing menu. - atom::UnresponsiveSuppressor suppressor; - [menu popUpMenuPositioningItem:item atLocation:position inView:view]; - close_callback.Run(); - } + // Don't emit unresponsive event when showing menu. + atom::UnresponsiveSuppressor suppressor; + [menu popUpMenuPositioningItem:item atLocation:position inView:view]; } void MenuMac::ClosePopupAt(int32_t window_id) { diff --git a/atom/browser/api/atom_api_menu_views.cc b/atom/browser/api/atom_api_menu_views.cc index 24d017c3366a..7a53d5d55307 100644 --- a/atom/browser/api/atom_api_menu_views.cc +++ b/atom/browser/api/atom_api_menu_views.cc @@ -20,8 +20,7 @@ MenuViews::MenuViews(v8::Isolate* isolate, v8::Local wrapper) weak_factory_(this) { } -void MenuViews::PopupAt( - Window* window, int x, int y, int positioning_item, bool async) { +void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) { NativeWindow* native_window = static_cast(window->window()); if (!native_window) return; @@ -42,8 +41,6 @@ void MenuViews::PopupAt( } int flags = MenuRunner::CONTEXT_MENU | MenuRunner::HAS_MNEMONICS; - if (async) - flags |= MenuRunner::ASYNC; // Don't emit unresponsive event when showing menu. atom::UnresponsiveSuppressor suppressor; diff --git a/atom/browser/api/atom_api_menu_views.h b/atom/browser/api/atom_api_menu_views.h index a974f75bc76d..d0c33b3960e2 100644 --- a/atom/browser/api/atom_api_menu_views.h +++ b/atom/browser/api/atom_api_menu_views.h @@ -21,8 +21,7 @@ class MenuViews : public Menu { MenuViews(v8::Isolate* isolate, v8::Local wrapper); protected: - void PopupAt( - Window* window, int x, int y, int positioning_item, bool async) override; + void PopupAt(Window* window, int x, int y, int positioning_item) override; void ClosePopupAt(int32_t window_id) override; private: diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 33c324d4fa8c..a340f74abcf0 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -44,7 +44,6 @@ Menu.prototype._init = function () { } Menu.prototype.popup = function (window, x, y, positioningItem) { - let asyncPopup let [newX, newY, newPosition, newWindow] = [x, y, positioningItem, window] // menu.popup(x, y, positioningItem) @@ -61,7 +60,6 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { newX = opts.x newY = opts.y newPosition = opts.positioningItem - asyncPopup = opts.async } // set defaults @@ -69,9 +67,8 @@ Menu.prototype.popup = function (window, x, y, positioningItem) { if (typeof y !== 'number') newY = -1 if (typeof positioningItem !== 'number') newPosition = -1 if (!window) newWindow = BrowserWindow.getFocusedWindow() - if (typeof asyncPopup !== 'boolean') asyncPopup = false - this.popupAt(newWindow, newX, newY, newPosition, asyncPopup) + this.popupAt(newWindow, newX, newY, newPosition) } Menu.prototype.closePopup = function (window) { diff --git a/spec/api-menu-spec.js b/spec/api-menu-spec.js index 2a74fc7297af..538c640e7121 100644 --- a/spec/api-menu-spec.js +++ b/spec/api-menu-spec.js @@ -282,11 +282,9 @@ describe('Menu module', () => { return closeWindow(w).then(() => { w = null }) }) - describe('when called with async: true', () => { - it('returns immediately', () => { - menu.popup(w, {x: 100, y: 100, async: true}) - menu.closePopup(w) - }) + it('returns immediately', () => { + menu.popup(w, {x: 100, y: 100, async: true}) + menu.closePopup(w) }) })