Merge pull request #11546 from electron/menu-callback

Add callback parameter for Menu.popup

atom/browser/api/atom_api_menu.cc

@@ -40,7 +40,6 @@ void Menu::AfterInit(v8::Isolate* isolate) {
   delegate.Get("getAcceleratorForCommandId", &get_accelerator_);
   delegate.Get("executeCommand", &execute_command_);
   delegate.Get("menuWillShow", &menu_will_show_);
-  delegate.Get("menuClosed", &menu_closed_);
 }
 
 bool Menu::IsCommandIdChecked(int command_id) const {
@@ -76,10 +75,6 @@ void Menu::MenuWillShow(ui::SimpleMenuModel* source) {
   menu_will_show_.Run();
 }
 
-void Menu::MenuClosed(ui::SimpleMenuModel* source) {
-  menu_closed_.Run();
-}
-
 void Menu::InsertItemAt(
     int index, int command_id, const base::string16& label) {
   model_->InsertItemAt(index, command_id, label);

atom/browser/api/atom_api_menu.h

@@ -52,9 +52,9 @@ class Menu : public mate::TrackableObject<Menu>,
       ui::Accelerator* accelerator) const override;
   void ExecuteCommand(int command_id, int event_flags) override;
   void MenuWillShow(ui::SimpleMenuModel* source) override;
-  void MenuClosed(ui::SimpleMenuModel* source) override;
 
-  virtual void PopupAt(Window* window, int x, int y, int positioning_item) = 0;
+  virtual void PopupAt(Window* window, int x, int y, int positioning_item,
+                       const base::Closure& callback) = 0;
   virtual void ClosePopupAt(int32_t window_id) = 0;
 
   std::unique_ptr<AtomMenuModel> model_;
@@ -94,7 +94,6 @@ class Menu : public mate::TrackableObject<Menu>,
   base::Callback<v8::Local<v8::Value>(int, bool)> get_accelerator_;
   base::Callback<void(v8::Local<v8::Value>, int)> execute_command_;
   base::Callback<void()> menu_will_show_;
-  base::Callback<void()> menu_closed_;
 
   DISALLOW_COPY_AND_ASSIGN(Menu);
 };

atom/browser/api/atom_api_menu_mac.h

@@ -22,18 +22,20 @@ class MenuMac : public Menu {
  protected:
   MenuMac(v8::Isolate* isolate, v8::Local<v8::Object> wrapper);
 
-  void PopupAt(Window* window, int x, int y, int positioning_item) override;
+  void PopupAt(Window* window, int x, int y, int positioning_item,
+               const base::Closure& callback) override;
   void PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
                  int32_t window_id,
                  int x,
                  int y,
-                 int positioning_item);
+                 int positioning_item,
+                 base::Closure callback);
   void ClosePopupAt(int32_t window_id) override;
 
  private:
   friend class Menu;
 
-  static void SendActionToFirstResponder(const std::string& action);
+  void OnClosed(int32_t window_id, base::Closure callback);
 
   scoped_nsobject<AtomMenuController> menu_controller_;
 

atom/browser/api/atom_api_menu_mac.mm

@@ -27,14 +27,15 @@ MenuMac::MenuMac(v8::Isolate* isolate, v8::Local<v8::Object> wrapper)
       weak_factory_(this) {
 }
 
-void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) {
+void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item,
+                      const base::Closure& callback) {
   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);
+                          positioning_item, callback);
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, popup);
 }
 
@@ -42,7 +43,8 @@ void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
                         int32_t window_id,
                         int x,
                         int y,
-                        int positioning_item) {
+                        int positioning_item,
+                        base::Closure callback) {
   if (!native_window)
     return;
   brightray::InspectableWebContents* web_contents =
@@ -50,8 +52,8 @@ void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
   if (!web_contents)
     return;
 
-  auto close_callback = base::Bind(&MenuMac::ClosePopupAt,
+  auto close_callback = base::Bind(
-                                   weak_factory_.GetWeakPtr(), window_id);
+      &MenuMac::OnClosed, weak_factory_.GetWeakPtr(), window_id, callback);
   popup_controllers_[window_id] = base::scoped_nsobject<AtomMenuController>(
       [[AtomMenuController alloc] initWithModel:model()
                           useDefaultAccelerator:NO]);
@@ -108,13 +110,24 @@ void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
 }
 
 void MenuMac::ClosePopupAt(int32_t window_id) {
-  auto it = popup_controllers_.find(window_id);
+  auto controller = popup_controllers_.find(window_id);
-  if (it != popup_controllers_.end()) {
+  if (controller != popup_controllers_.end()) {
-    popup_controllers_.erase(it);
+    // Close the controller for the window.
+    [controller->second cancel];
   } else if (window_id == -1) {
-    popup_controllers_.clear();
+    // Or just close all opened controllers.
+    for (auto it = popup_controllers_.begin();
+         it != popup_controllers_.end();) {
+      // The iterator is invalidated after the call.
+      [(it++)->second cancel];
     }
   }
+}
+
+void MenuMac::OnClosed(int32_t window_id, base::Closure callback) {
+  popup_controllers_.erase(window_id);
+  callback.Run();
+}
 
 // static
 void Menu::SetApplicationMenu(Menu* base_menu) {

atom/browser/api/atom_api_menu_views.cc

@@ -6,7 +6,8 @@
 
 #include "atom/browser/native_window_views.h"
 #include "atom/browser/unresponsive_suppressor.h"
-#include "content/public/browser/render_widget_host_view.h"
+#include "brightray/browser/inspectable_web_contents.h"
+#include "brightray/browser/inspectable_web_contents_view.h"
 #include "ui/display/screen.h"
 
 using views::MenuRunner;
@@ -20,23 +21,22 @@ MenuViews::MenuViews(v8::Isolate* isolate, v8::Local<v8::Object> wrapper)
       weak_factory_(this) {
 }
 
-void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
+void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item,
+                        const base::Closure& callback) {
   NativeWindow* native_window = static_cast<NativeWindow*>(window->window());
   if (!native_window)
     return;
-  content::WebContents* web_contents = native_window->web_contents();
+  auto* web_contents = native_window->inspectable_web_contents();
   if (!web_contents)
     return;
-  content::RenderWidgetHostView* view = web_contents->GetRenderWidgetHostView();
-  if (!view)
-    return;
 
   // (-1, -1) means showing on mouse location.
   gfx::Point location;
   if (x == -1 || y == -1) {
     location = display::Screen::GetScreen()->GetCursorScreenPoint();
   } else {
-    gfx::Point origin = view->GetViewBounds().origin();
+    auto* view = web_contents->GetView()->GetWebView();
+    gfx::Point origin = view->bounds().origin();
     location = gfx::Point(origin.x() + x, origin.y() + y);
   }
 
@@ -48,7 +48,7 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
   // Show the menu.
   int32_t window_id = window->ID();
   auto close_callback = base::Bind(
-      &MenuViews::OnClosed, weak_factory_.GetWeakPtr(), window_id);
+      &MenuViews::OnClosed, weak_factory_.GetWeakPtr(), window_id, callback);
   menu_runners_[window_id] = std::unique_ptr<MenuRunner>(new MenuRunner(
       model(), flags, close_callback));
   menu_runners_[window_id]->RunMenuAt(
@@ -73,8 +73,9 @@ void MenuViews::ClosePopupAt(int32_t window_id) {
   }
 }
 
-void MenuViews::OnClosed(int32_t window_id) {
+void MenuViews::OnClosed(int32_t window_id, base::Closure callback) {
   menu_runners_.erase(window_id);
+  callback.Run();
 }
 
 // static

atom/browser/api/atom_api_menu_views.h

@@ -21,11 +21,12 @@ class MenuViews : public Menu {
   MenuViews(v8::Isolate* isolate, v8::Local<v8::Object> wrapper);
 
  protected:
-  void PopupAt(Window* window, int x, int y, int positioning_item) override;
+  void PopupAt(Window* window, int x, int y, int positioning_item,
+               const base::Closure& callback) override;
   void ClosePopupAt(int32_t window_id) override;
 
  private:
-  void OnClosed(int32_t window_id);
+  void OnClosed(int32_t window_id, base::Closure callback);
 
   // window ID -> open context menu
   std::map<int32_t, std::unique_ptr<views::MenuRunner>> menu_runners_;

atom/browser/ui/cocoa/atom_menu_controller.mm

@@ -12,12 +12,9 @@
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/accelerators/platform_accelerator_cocoa.h"
 #include "ui/base/l10n/l10n_util_mac.h"
-#include "content/public/browser/browser_thread.h"
 #include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/gfx/image/image.h"
 
-using content::BrowserThread;
-
 namespace {
 
 struct Role {
@@ -118,8 +115,9 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
 - (void)cancel {
   if (isMenuOpen_) {
     [menu_ cancelTracking];
-    model_->MenuWillClose();
     isMenuOpen_ = NO;
+    model_->MenuWillClose();
+    closeCallback.Run();
   }
 }
 
@@ -334,11 +332,7 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
   if (isMenuOpen_) {
     isMenuOpen_ = NO;
     model_->MenuWillClose();
-
+    closeCallback.Run();
-    // Post async task so that itemSelected runs before the close callback
-    // deletes the controller from the map which deallocates it
-    if (!closeCallback.is_null())
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, closeCallback);
   }
 }
 

docs/api/menu.md

@@ -59,7 +59,7 @@ will become properties of the constructed menu items.
 
 The `menu` object has the following instance methods:
 
-#### `menu.popup([browserWindow, options])`
+#### `menu.popup([browserWindow, options, callback])`
 
 * `browserWindow` [BrowserWindow](browser-window.md) (optional) - Default is the focused window.
 * `options` Object (optional)
@@ -70,6 +70,7 @@ The `menu` object has the following instance methods:
   * `positioningItem` Number (optional) _macOS_ - The index of the menu item to
     be positioned under the mouse cursor at the specified coordinates. Default
     is -1.
+* `callback` Function (optional) - Called when menu is closed.
 
 Pops up this menu as a context menu in the [`BrowserWindow`](browser-window.md).
 
@@ -114,10 +115,6 @@ can have a submenu.
 Objects created with `new Menu` or returned by `Menu.buildFromTemplate` emit
 the following events:
 
-#### Event: 'closed'
-
-Emitted when the menu is closed.
-
 ## Examples
 
 The `Menu` class is only available in the main process, but you can also use it

lib/browser/api/menu.js

@@ -39,9 +39,6 @@ Menu.prototype._init = function () {
         const found = this.groupsMap[id].find(item => item.checked) || null
         if (!found) v8Util.setHiddenValue(this.groupsMap[id][0], 'checked', true)
       }
-    },
-    menuClosed: () => {
-      this.emit('closed')
     }
   }
 }
@@ -49,18 +46,26 @@ Menu.prototype._init = function () {
 Menu.prototype.popup = function (window, x, y, positioningItem) {
   let [newX, newY, newPosition, newWindow] = [x, y, positioningItem, window]
   let opts
+  let callback
 
   // menu.popup(x, y, positioningItem)
   if (window != null && !(window instanceof BrowserWindow)) {
     [newPosition, newY, newX, newWindow] = [y, x, window, null]
   }
 
+  // menu.popup([w], x, y, callback)
+  if (typeof newPosition === 'function') {
+    callback = newPosition
+  }
+
   // menu.popup({})
   if (window != null && window.constructor === Object) {
     opts = window
+    callback = arguments[1]
   // menu.popup(window, {})
   } else if (x && typeof x === 'object') {
     opts = x
+    callback = arguments[2]
   }
 
   if (opts) {
@@ -68,6 +73,9 @@ Menu.prototype.popup = function (window, x, y, positioningItem) {
     newY = opts.y
     newPosition = opts.positioningItem
   }
+  if (typeof callback !== 'function') {
+    callback = () => {}
+  }
 
   // set defaults
   if (typeof newX !== 'number') newX = -1
@@ -88,7 +96,7 @@ Menu.prototype.popup = function (window, x, y, positioningItem) {
     }
   }
 
-  this.popupAt(newWindow, newX, newY, newPosition)
+  this.popupAt(newWindow, newX, newY, newPosition, callback)
 
   return { browserWindow: newWindow, x: newX, y: newY, position: newPosition }
 }

lib/browser/api/web-contents.js

@@ -280,8 +280,7 @@ WebContents.prototype._init = function () {
   this.on('pepper-context-menu', function (event, params, callback) {
     // Access Menu via electron.Menu to prevent circular require.
     const menu = electron.Menu.buildFromTemplate(params.menu)
-    menu.once('closed', callback)
+    menu.popup(event.sender.getOwnerBrowserWindow(), params.x, params.y, callback)
-    menu.popup(event.sender.getOwnerBrowserWindow(), params.x, params.y)
   })
 
   // The devtools requests the webContents to reload.

spec/api-menu-spec.js

@@ -323,36 +323,15 @@ describe('Menu module', () => {
       assert.equal(x, 100)
       assert.equal(y, 101)
     })
+
+    it('calls the callback', (done) => {
+      menu.popup({}, () => done())
+      menu.closePopup()
     })
 
-  describe('Menu.closePopup()', () => {
+    it('works with old style', (done) => {
-    let w = null
+      menu.popup(w, 100, 101, () => done())
-    let menu
+      menu.closePopup()
-
-    beforeEach((done) => {
-      w = new BrowserWindow({show: false, width: 200, height: 200})
-      menu = Menu.buildFromTemplate([
-        {
-          label: '1'
-        }
-      ])
-
-      w.loadURL('data:text/html,<html>teszt</html>')
-      w.webContents.on('dom-ready', () => {
-        done()
-      })
-    })
-
-    afterEach(() => {
-      return closeWindow(w).then(() => { w = null })
-    })
-
-    it('emits closed event', (done) => {
-      menu.popup(w, {x: 100, y: 100})
-      menu.on('closed', () => {
-        done()
-      })
-      menu.closePopup(w)
     })
   })
 

spec/static/index.html

@@ -68,10 +68,11 @@
   // npm run test -match=menu
   const moduleMatch = process.env.npm_config_match
     ? new RegExp(process.env.npm_config_match, 'g')
-    : /.*/gi
+    : null
 
   walker.on('file', (file) => {
-    if (/-spec\.js$/.test(file) && moduleMatch.test(file) && !file.includes(crashSpec)) {
+    if (/-spec\.js$/.test(file) && !file.includes(crashSpec) &&
+        (!moduleMatch || moduleMatch.test(file))) {
       mocha.addFile(file)
     }
   })