Bug 1543940 - menu.popup() should take a document argument instead of toolbox r=ochameau
authorJulian Descottes <jdescottes@mozilla.com>
Wed, 08 May 2019 21:35:36 +0000
changeset 535017 388f61b1b134177011dcfe55f52faff072a50052
parent 535016 0484e2e8f7fd170c14218dcf60c9c3983e1a8267
child 535018 a0768b78ff32581c457122430db68e11a0a5c479
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersochameau
bugs1543940
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1543940 - menu.popup() should take a document argument instead of toolbox r=ochameau Depends on D27693 Menu::popup and popupAtZoom are expecting a toolbox argument as last argument. However, half of the callsites do not have access to the toolbox and just pass a { doc } object. This is misleading when trying to work on menu.js because you cannot rely on toolbox APIs. Differential Revision: https://phabricator.services.mozilla.com/D28036
devtools/client/accessibility/components/AccessibilityRow.js
devtools/client/framework/menu.js
devtools/client/framework/test/browser_menu_api.js
devtools/client/framework/toolbox.js
devtools/client/inspector/changes/ChangesContextMenu.js
devtools/client/inspector/markup/markup-context-menu.js
devtools/client/inspector/shared/style-inspector-menu.js
devtools/client/shared/components/menu/utils.js
devtools/client/shared/components/tabs/TabBar.js
devtools/client/styleeditor/StyleEditorUI.jsm
devtools/client/webconsole/webconsole-wrapper.js
--- a/devtools/client/accessibility/components/AccessibilityRow.js
+++ b/devtools/client/accessibility/components/AccessibilityRow.js
@@ -198,17 +198,17 @@ class AccessibilityRow extends Component
     if (supports.snapshot) {
       menu.append(new MenuItem({
         id: "menu-printtojson",
         label: L10N.getStr("accessibility.tree.menu.printToJSON"),
         click: () => this.printToJSON(),
       }));
     }
 
-    menu.popup(e.screenX, e.screenY, gToolbox);
+    menu.popup(e.screenX, e.screenY, gToolbox.doc);
 
     if (gTelemetry) {
       gTelemetry.scalarAdd(TELEMETRY_ACCESSIBLE_CONTEXT_MENU_OPENED, 1);
     }
   }
 
   /**
    * Render accessible row component.
--- a/devtools/client/framework/menu.js
+++ b/devtools/client/framework/menu.js
@@ -54,38 +54,36 @@ Menu.prototype.insert = function(pos, me
 /**
  * Show the Menu with anchor element's coordinate.
  * For example, In the case of zoom in/out the devtool panel, we should multiply
  * element's position to zoom value.
  * If you know the screen coodinate of display position, you should use Menu.pop().
  *
  * @param {int} x
  * @param {int} y
- * @param Toolbox toolbox
+ * @param {Document} doc
  */
-Menu.prototype.popupWithZoom = function(x, y, toolbox) {
-  const zoom = getCurrentZoom(toolbox.doc);
-  this.popup(x * zoom, y * zoom, toolbox);
+Menu.prototype.popupWithZoom = function(x, y, doc) {
+  const zoom = getCurrentZoom(doc);
+  this.popup(x * zoom, y * zoom, doc);
 };
 
 /**
  * Show the Menu at a specified location on the screen
  *
  * Missing features:
  *   - browserWindow - BrowserWindow (optional) - Default is null.
  *   - positioningItem Number - (optional) OS X
  *
  * @param {int} screenX
  * @param {int} screenY
- * @param Toolbox toolbox (non standard)
- *        Needed so we in which window to inject XUL
+ * @param {Document} doc
+ *        The document that should own the context menu.
  */
-Menu.prototype.popup = function(screenX, screenY, toolbox) {
-  const doc = toolbox.doc;
-
+Menu.prototype.popup = function(screenX, screenY, doc) {
   let popupset = doc.querySelector("popupset");
   if (!popupset) {
     popupset = doc.createXULElement("popupset");
     doc.documentElement.appendChild(popupset);
   }
   // See bug 1285229, on Windows, opening the same popup multiple times in a
   // row ends up duplicating the popup. The newly inserted popup doesn't
   // dismiss the old one. So remove any previously displayed popup before
--- a/devtools/client/framework/test/browser_menu_api.js
+++ b/devtools/client/framework/test/browser_menu_api.js
@@ -75,17 +75,17 @@ async function testMenuPopup(toolbox) {
   }
 
   // Append an invisible MenuItem, which shouldn't show up in the DOM
   menu.append(new MenuItem({
     label: "Invisible",
     visible: false,
   }));
 
-  menu.popup(0, 0, toolbox);
+  menu.popup(0, 0, toolbox.doc);
 
   ok(toolbox.doc.querySelector("#menu-popup"), "A popup is in the DOM");
 
   const menuSeparators =
     toolbox.doc.querySelectorAll("#menu-popup > menuseparator");
   is(menuSeparators.length, 1, "A separator is in the menu");
 
   const menuItems = toolbox.doc.querySelectorAll("#menu-popup > menuitem");
@@ -138,17 +138,17 @@ async function testSubmenu(toolbox) {
   menu.append(new MenuItem({
     label: "Submenu parent with attributes",
     id: "submenu-parent-with-attrs",
     submenu: submenu,
     accesskey: "A",
     disabled: true,
   }));
 
-  menu.popup(0, 0, toolbox);
+  menu.popup(0, 0, toolbox.doc);
   ok(toolbox.doc.querySelector("#menu-popup"), "A popup is in the DOM");
   is(toolbox.doc.querySelectorAll("#menu-popup > menuitem").length, 0,
     "No menuitem children");
 
   const menus = toolbox.doc.querySelectorAll("#menu-popup > menu");
   is(menus.length, 2, "Correct number of menus");
   ok(!menus[0].hasAttribute("label"), "No label: should be set by localization");
   ok(!menus[0].hasAttribute("disabled"), "Correct disabled state");
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -3248,17 +3248,17 @@ Toolbox.prototype = {
    */
   openTextBoxContextMenu: function(x, y) {
     const menu = createEditContextMenu(this.win, "toolbox-menu");
 
     // Fire event for tests
     menu.once("open", () => this.emit("menu-open"));
     menu.once("close", () => this.emit("menu-close"));
 
-    menu.popup(x, y, { doc: this.doc });
+    menu.popup(x, y, this.doc);
   },
 
   /**
    * Connects to the Gecko Profiler when the developer tools are open. This is
    * necessary because of the WebConsole's `profile` and `profileEnd` methods.
    */
   async initPerformance() {
     // If:
--- a/devtools/client/inspector/changes/ChangesContextMenu.js
+++ b/devtools/client/inspector/changes/ChangesContextMenu.js
@@ -82,17 +82,17 @@ class ChangesContextMenu {
     // Select All option
     const menuitemSelectAll = new MenuItem({
       label: getStr("changes.contextmenu.selectAll"),
       accesskey: getStr("changes.contextmenu.selectAll.accessKey"),
       click: this._onSelectAll,
     });
     menu.append(menuitemSelectAll);
 
-    menu.popup(screenX, screenY, this.inspector.toolbox);
+    menu.popup(screenX, screenY, this.inspector.toolbox.doc);
     return menu;
   }
 
   _hasTextSelected() {
     const selection = this.window.getSelection();
     return selection.toString() && !selection.isCollapsed;
   }
 
--- a/devtools/client/inspector/markup/markup-context-menu.js
+++ b/devtools/client/inspector/markup/markup-context-menu.js
@@ -747,17 +747,17 @@ class MarkupContextMenu {
         type: "separator",
       }));
     }
 
     for (const menuitem of nodeLinkMenuItems) {
       menu.append(menuitem);
     }
 
-    menu.popup(screenX, screenY, this.toolbox);
+    menu.popup(screenX, screenY, this.toolbox.doc);
     return menu;
   }
 
   async _updateA11YMenuItem(menuItem) {
     const hasMethod = await this.target.actorHasMethod(
       "domwalker", "hasAccessibilityProperties").catch(
         // Connection to DOMWalker might have been already closed.
         error => console.warn(error));
--- a/devtools/client/inspector/shared/style-inspector-menu.js
+++ b/devtools/client/inspector/shared/style-inspector-menu.js
@@ -233,17 +233,17 @@ StyleInspectorMenu.prototype = {
       click: () => {
         this._onToggleOrigSources();
       },
       type: "checkbox",
       checked: Services.prefs.getBoolPref(PREF_ORIG_SOURCES),
     });
     menu.append(menuitemSources);
 
-    menu.popup(screenX, screenY, this.inspector._toolbox);
+    menu.popup(screenX, screenY, this.inspector.toolbox.doc);
     return menu;
   },
 
   _hasTextSelected: function() {
     let hasTextSelected;
     const selection = this.styleWindow.getSelection();
 
     const node = this._getClickedNode();
--- a/devtools/client/shared/components/menu/utils.js
+++ b/devtools/client/shared/components/menu/utils.js
@@ -66,14 +66,14 @@ function showMenu(items, options) {
 
   let doc;
   if (options.useTopLevelWindow) {
     doc = getTopLevelWindow(window).document;
   } else {
     doc = window.parent.document;
   }
 
-  menu.popup(screenX, screenY, { doc });
+  menu.popup(screenX, screenY, doc);
 }
 
 module.exports = {
   showMenu,
 };
--- a/devtools/client/shared/components/tabs/TabBar.js
+++ b/devtools/client/shared/components/tabs/TabBar.js
@@ -295,17 +295,17 @@ class Tabbar extends Component {
     // XXX Missing menu API for specifying target (anchor)
     // and relative position to it. See also:
     // https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/openPopup
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1274551
     const rect = target.getBoundingClientRect();
     const screenX = target.ownerDocument.defaultView.mozInnerScreenX;
     const screenY = target.ownerDocument.defaultView.mozInnerScreenY;
     menu.popupWithZoom(rect.left + screenX, rect.bottom + screenY,
-                       { doc: this.props.menuDocument });
+      this.props.menuDocument);
 
     return menu;
   }
 
   // Rendering
 
   renderTab(tab) {
     if (typeof tab.panel === "function") {
--- a/devtools/client/styleeditor/StyleEditorUI.jsm
+++ b/devtools/client/styleeditor/StyleEditorUI.jsm
@@ -196,17 +196,17 @@ StyleEditorUI.prototype = {
 
     this._optionsMenu.once("open", () => {
       this._optionsButton.setAttribute("open", true);
     });
     this._optionsMenu.once("close", () => {
       this._optionsButton.removeAttribute("open");
     });
 
-    this._optionsMenu.popup(screenX, screenY, this._toolbox);
+    this._optionsMenu.popup(screenX, screenY, this._toolbox.doc);
   },
 
   /**
    * Refresh editors to reflect the stylesheets in the document.
    *
    * @param {string} event
    *        Event name
    * @param {StyleSheet} styleSheet
--- a/devtools/client/webconsole/webconsole-wrapper.js
+++ b/devtools/client/webconsole/webconsole-wrapper.js
@@ -251,27 +251,27 @@ class WebConsoleWrapper {
           rootActorId,
           executionPoint,
           toolbox: this.toolbox,
           url,
         });
 
         // Emit the "menu-open" event for testing.
         menu.once("open", () => this.emit("menu-open"));
-        menu.popup(screenX, screenY, { doc: this.hud.chromeWindow.document });
+        menu.popup(screenX, screenY, this.hud.chromeWindow.document);
 
         return menu;
       };
 
       serviceContainer.openEditContextMenu = (e) => {
         const { screenX, screenY } = e;
         const menu = createEditContextMenu(window, "webconsole-menu");
         // Emit the "menu-open" event for testing.
         menu.once("open", () => this.emit("menu-open"));
-        menu.popup(screenX, screenY, { doc: this.hud.chromeWindow.document });
+        menu.popup(screenX, screenY, this.hud.chromeWindow.document);
 
         return menu;
       };
 
       if (this.toolbox) {
         this.toolbox.threadClient.addListener("paused", this.dispatchPaused.bind(this));
         this.toolbox.threadClient.addListener(
           "progress", this.dispatchProgress.bind(this));