Merge pull request #4198 from atom/default-popup-x-y

Cleanup the Menu::popup code

atom/browser/api/atom_api_menu.cc

@@ -169,8 +169,7 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("isItemCheckedAt", &Menu::IsItemCheckedAt)
       .SetMethod("isEnabledAt", &Menu::IsEnabledAt)
       .SetMethod("isVisibleAt", &Menu::IsVisibleAt)
-      .SetMethod("_popup", &Menu::Popup)
-      .SetMethod("_popupAt", &Menu::PopupAt);
+      .SetMethod("popupAt", &Menu::PopupAt);
 }
 
 }  // namespace api

atom/browser/api/atom_api_menu.h

@@ -51,9 +51,9 @@ class Menu : public mate::TrackableObject<Menu>,
   void ExecuteCommand(int command_id, int event_flags) override;
   void MenuWillShow(ui::SimpleMenuModel* source) override;
 
-  virtual void Popup(Window* window) = 0;
-  virtual void PopupAt(Window* window, int x, int y,
-      int positioningItem = 0) = 0;
+  virtual void PopupAt(Window* window,
+                       int x = -1, int y = -1,
+                       int positioning_item = 0) = 0;
 
   scoped_ptr<AtomMenuModel> model_;
   Menu* parent_;

atom/browser/api/atom_api_menu_mac.h

@@ -19,8 +19,7 @@ class MenuMac : public Menu {
  protected:
   MenuMac();
 
-  void Popup(Window* window) override;
-  void PopupAt(Window* window, int x, int y, int positioningItem = 0) override;
+  void PopupAt(Window* window, int x, int y, int positioning_item = 0) override;
 
   base::scoped_nsobject<AtomMenuController> menu_controller_;
 

atom/browser/api/atom_api_menu_mac.mm

@@ -18,39 +18,7 @@ namespace api {
 MenuMac::MenuMac() {
 }
 
-void MenuMac::Popup(Window* window) {
-  NativeWindow* native_window = window->window();
-  if (!native_window)
-    return;
-  content::WebContents* web_contents = native_window->web_contents();
-  if (!web_contents)
-    return;
-
-  NSWindow* nswindow = native_window->GetNativeWindow();
-  base::scoped_nsobject<AtomMenuController> menu_controller(
-      [[AtomMenuController alloc] initWithModel:model_.get()]);
-
-  // Fake out a context menu event.
-  NSEvent* currentEvent = [NSApp currentEvent];
-  NSPoint position = [nswindow mouseLocationOutsideOfEventStream];
-  NSTimeInterval eventTime = [currentEvent timestamp];
-  NSEvent* clickEvent = [NSEvent mouseEventWithType:NSRightMouseDown
-                                           location:position
-                                      modifierFlags:NSRightMouseDownMask
-                                          timestamp:eventTime
-                                       windowNumber:[nswindow windowNumber]
-                                            context:nil
-                                        eventNumber:0
-                                         clickCount:1
-                                           pressure:1.0];
-
-  // Show the menu.
-  [NSMenu popUpContextMenu:[menu_controller menu]
-                 withEvent:clickEvent
-                   forView:web_contents->GetContentNativeView()];
-}
-
-void MenuMac::PopupAt(Window* window, int x, int y, int positioningItem) {
+void MenuMac::PopupAt(Window* window, int x, int y, int positioning_item) {
   NativeWindow* native_window = window->window();
   if (!native_window)
     return;
@@ -63,16 +31,23 @@ void MenuMac::PopupAt(Window* window, int x, int y, int positioningItem) {
   NSMenu* menu = [menu_controller menu];
   NSView* view = web_contents->GetContentNativeView();
 
+  // Which menu item to show.
+  NSMenuItem* item = nil;
+  if (positioning_item < [menu numberOfItems] && positioning_item >= 0)
+    item = [menu itemAtIndex:positioning_item];
+
+  // (-1, -1) means showing on mouse location.
+  NSPoint position;
+  if (x == -1 || y == -1) {
+    NSWindow* nswindow = native_window->GetNativeWindow();
+    position = [view convertPoint:[nswindow mouseLocationOutsideOfEventStream]
+                         fromView:nil];
+  } else {
+    position = NSMakePoint(x, [view frame].size.height - y);
+  }
+
   // Show the menu.
-  if (positioningItem >= [menu numberOfItems]) {
-    positioningItem = [menu numberOfItems] - 1;
-  }
-  if (positioningItem < 0) {
-    positioningItem = 0;
-  }
-  [menu popUpMenuPositioningItem:[menu itemAtIndex:positioningItem]
-                      atLocation:NSMakePoint(x, [view frame].size.height - y)
-                          inView:view];
+  [menu popUpMenuPositioningItem:item atLocation:position inView:view];
 }
 
 // static

atom/browser/api/atom_api_menu_views.cc

@@ -16,11 +16,7 @@ namespace api {
 MenuViews::MenuViews() {
 }
 
-void MenuViews::Popup(Window* window) {
-  PopupAtPoint(window, gfx::Screen::GetNativeScreen()->GetCursorScreenPoint());
-}
-
-void MenuViews::PopupAt(Window* window, int x, int y, int positioningItem) {
+void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
   NativeWindow* native_window = static_cast<NativeWindow*>(window->window());
   if (!native_window)
     return;
@@ -31,18 +27,23 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioningItem) {
   if (!view)
     return;
 
-  gfx::Point origin = view->GetViewBounds().origin();
-  PopupAtPoint(window, gfx::Point(origin.x() + x, origin.y() + y));
-}
+  // (-1, -1) means showing on mouse location.
+  gfx::Point location;
+  if (x == -1 || y == -1) {
+    location = gfx::Screen::GetNativeScreen()->GetCursorScreenPoint();
+  } else {
+    gfx::Point origin = view->GetViewBounds().origin();
+    location = gfx::Point(origin.x() + x, origin.y() + y);
+  }
 
-void MenuViews::PopupAtPoint(Window* window, const gfx::Point& point) {
+  // Show the menu.
   views::MenuRunner menu_runner(
       model(),
       views::MenuRunner::CONTEXT_MENU | views::MenuRunner::HAS_MNEMONICS);
   ignore_result(menu_runner.RunMenuAt(
       static_cast<NativeWindowViews*>(window->window())->widget(),
       NULL,
-      gfx::Rect(point, gfx::Size()),
+      gfx::Rect(location, gfx::Size()),
       views::MENU_ANCHOR_TOPLEFT,
       ui::MENU_SOURCE_MOUSE));
 }

atom/browser/api/atom_api_menu_views.h

@@ -17,12 +17,9 @@ class MenuViews : public Menu {
   MenuViews();
 
  protected:
-  void Popup(Window* window) override;
-  void PopupAt(Window* window, int x, int y, int positioningItem = 0) override;
+  void PopupAt(Window* window, int x, int y, int positioning_item = 0) override;
 
  private:
-  void PopupAtPoint(Window* window, const gfx::Point& point);
-
   DISALLOW_COPY_AND_ASSIGN(MenuViews);
 };
 

atom/browser/api/lib/menu.js

@@ -159,17 +159,20 @@ Menu.prototype._init = function() {
 };
 
 Menu.prototype.popup = function(window, x, y, positioningItem) {
-  if ((window != null ? window.constructor : void 0) !== BrowserWindow) {
+  if (typeof window != 'object' || window.constructor !== BrowserWindow) {
     // Shift.
+    positioningItem = y;
     y = x;
     x = window;
     window = BrowserWindow.getFocusedWindow();
   }
-  if ((x != null) && (y != null)) {
-    return this._popupAt(window, x, y, positioningItem || 0);
-  } else {
-    return this._popup(window);
-  }
+
+  // Default parameters.
+  if (typeof x !== 'number') x = -1;
+  if (typeof y !== 'number') y = -1;
+  if (typeof positioningItem !== 'number') positioningItem = 0;
+
+  this.popupAt(window, x, y, positioningItem);
 };
 
 Menu.prototype.append = function(item) {

docs/api/menu.md

@@ -239,16 +239,16 @@ will become properties of the constructed menu items.
 
 ### `Menu.popup([browserWindow, x, y, positioningItem])`
 
-* `browserWindow` BrowserWindow (optional)
-* `x` Number (optional)
-* `y` Number (**required** if `x` is used)
-* `positioningItem` Number (optional) _OS X_
+* `browserWindow` BrowserWindow (optional) - Default is `null`.
+* `x` Number (optional) - Default is -1.
+* `y` Number (**required** if `x` is used) - Default is -1.
+* `positioningItem` Number (optional) _OS X_ - The index of the menu item to
+  be positioned under the mouse cursor at the specified coordinates. Default is
+  -1.
 
-Pops up this menu as a context menu in the `browserWindow`. You
-can optionally provide a `x,y` coordinate to place the menu at, otherwise it
-will be placed at the current mouse cursor position. `positioningItem` is the
-index of the menu item to be positioned under the mouse cursor at the specified
-coordinates (only supported on OS X).
+Pops up this menu as a context menu in the `browserWindow`. You can optionally
+provide a `x, y` coordinate to place the menu at, otherwise it will be placed
+at the current mouse cursor position.
 
 ### `Menu.append(menuItem)`