Bug 1215376 - Fire menus.onShown for every menu draft
authorRob Wu <rob@robwu.nl>
Thu, 14 Sep 2017 18:28:36 +0200
changeset 716037 1eeb9c7654f852d1bbf476fef7174d802281969a
parent 716036 94ec7d0b50e338850b397a0674ae6adcc6e70d95
child 716038 ff47c55c1865e7c400d18ae3b3980f2599130a6b
push id94307
push userbmo:rob@robwu.nl
push dateFri, 05 Jan 2018 00:24:53 +0000
bugs1215376
milestone59.0a1
Bug 1215376 - Fire menus.onShown for every menu This commit lifts the restriction that onShown is only fired for extensions with a visible menu item. MozReview-Commit-ID: Ao4MgBoRWLR
browser/components/extensions/ext-menus.js
browser/components/extensions/schemas/menus.json
browser/components/extensions/test/browser/browser_ext_menus_events.js
browser/components/extensions/test/browser/browser_ext_menus_refresh.js
--- a/browser/components/extensions/ext-menus.js
+++ b/browser/components/extensions/ext-menus.js
@@ -27,16 +27,19 @@ var gMenuMap = new Map();
 
 // Map[Extension -> MenuItem]
 var gRootItems = new Map();
 
 // Map[Extension -> ID[]]
 // Menu IDs that were eligible for being shown in the current menu.
 var gShownMenuItems = new DefaultMap(() => []);
 
+// Set of extensions that are listening to onShown.
+var gOnShownSubscribers = new Set();
+
 // If id is not specified for an item we use an integer.
 var gNextMenuItemID = 0;
 
 // Used to assign unique names to radio groups.
 var gNextRadioGroupID = 0;
 
 // The max length of a menu item's label.
 var gMaxLabelLength = 64;
@@ -66,20 +69,20 @@ var gMenuBuilder = {
     const root = gRootItems.get(contextData.extension);
     if (!root) {
       return;
     }
 
     const children = this.buildChildren(root, contextData);
     const visible = children.slice(0, ACTION_MENU_TOP_LEVEL_LIMIT);
 
+    this.xulMenu = menu;
+    menu.addEventListener("popuphidden", this);
+
     if (visible.length) {
-      this.xulMenu = menu;
-      menu.addEventListener("popuphidden", this);
-
       const separator = menu.ownerDocument.createElement("menuseparator");
       menu.insertBefore(separator, menu.firstChild);
       this.itemsToCleanUp.add(separator);
 
       for (const child of visible) {
         this.itemsToCleanUp.add(child);
         menu.insertBefore(child, separator);
       }
@@ -326,17 +329,16 @@ var gMenuBuilder = {
 
     element.setAttribute("image", resolvedURL);
   },
 
   rebuildMenu(extension) {
     let {contextData} = this;
     if (!contextData) {
       // This happens if the menu is not visible.
-      // This also happens if there are no visible extension menu items.
       return;
     }
 
     if (!gShownMenuItems.has(extension)) {
       // The onShown event was not fired for the extension, so the extension
       // does not know that a menu is being shown, and therefore they should
       // not care whether the extension menu is updated.
       return;
@@ -375,38 +377,48 @@ var gMenuBuilder = {
       }
     } else if (oldRoot) {
       oldRoot.remove();
       this.removeSeparatorIfNoTopLevelItems();
     }
   },
 
   afterBuildingMenu(contextData) {
-    // Left-hand side is an optimization: No need to construct event details if
-    // nobody is subscribing to the event.
-    // Right-hand side is to prevent onShown from being fired when a menu is
-    // updated.
-    if (gShownMenuItems.size === 0 || this.contextData) {
+    if (this.contextData) {
+      // rebuildMenu can trigger us again, but the logic below should run only
+      // once per open menu.
       return;
     }
 
-    for (let [extension, menuIds] of gShownMenuItems.entries()) {
+    function dispatchOnShownEvent(extension) {
+      // Note: gShownMenuItems is a DefaultMap, so .get(extension) causes the
+      // extension to be stored in the map even if there are currently no
+      // shown menu items. This ensures that the onHidden event can be fired
+      // when the menu is closed.
+      let menuIds = gShownMenuItems.get(extension);
       extension.emit("webext-menu-shown", menuIds, contextData);
     }
 
+    if (contextData.onBrowserAction || contextData.onPageAction) {
+      dispatchOnShownEvent(contextData.extension);
+    } else {
+      gOnShownSubscribers.forEach(dispatchOnShownEvent);
+    }
+
     this.contextData = contextData;
   },
 
   handleEvent(event) {
     if (this.xulMenu != event.target || event.type != "popuphidden") {
       return;
     }
 
     delete this.xulMenu;
     delete this.contextData;
+
     let target = event.target;
     target.removeEventListener("popuphidden", this);
     for (let item of this.itemsToCleanUp) {
       item.remove();
     }
     this.itemsToCleanUp.clear();
     for (let extension of gShownMenuItems.keys()) {
       extension.emit("webext-menu-hidden");
@@ -783,16 +795,17 @@ this.menusInternal = class extends Exten
 
   onShutdown(reason) {
     let {extension} = this;
 
     if (gMenuMap.has(extension)) {
       gMenuMap.delete(extension);
       gRootItems.delete(extension);
       gShownMenuItems.delete(extension);
+      gOnShownSubscribers.delete(extension);
       if (!gMenuMap.size) {
         menuTracker.unregister();
       }
     }
   }
 
   getAPI(context) {
     let {extension} = context;
@@ -816,18 +829,20 @@ this.menusInternal = class extends Exten
             extension.tabManager.hasActiveTabPermission(contextData.tab) ||
             extension.whiteListedHosts.matches(contextData.inFrame ? contextData.frameUrl : contextData.pageUrl);
 
           addMenuEventInfo(info, contextData, includeSensitiveData);
 
           let tab = extension.tabManager.convert(contextData.tab);
           fire.sync(info, tab);
         };
+        gOnShownSubscribers.add(extension);
         extension.on("webext-menu-shown", listener);
         return () => {
+          gOnShownSubscribers.delete(extension);
           extension.off("webext-menu-shown", listener);
         };
       }).api(),
       onHidden: new EventManager(context, "menus.onHidden", fire => {
         let listener = () => {
           fire.sync();
         };
         extension.on("webext-menu-hidden", listener);
--- a/browser/components/extensions/schemas/menus.json
+++ b/browser/components/extensions/schemas/menus.json
@@ -400,17 +400,17 @@
             "description": "The details of the tab where the click took place. If the click did not take place in a tab, this parameter will be missing.",
             "optional": true
           }
         ]
       },
       {
         "name": "onShown",
         "type": "function",
-        "description": "Fired when a menu is shown that contains a menu item that was created by the extension.",
+        "description": "Fired when a menu is shown. The extension can add, modify or remove menu items and call menus.refresh() to update the menu.",
         "parameters": [
           {
             "name": "info",
             "type": "object",
             "description": "Information about the context of the menu action and the created menu items. For more information about each property, see contextMenusInternal.OnClickData. The following properties are only set if the extension has host permissions for the given context: linkUrl, linkText, srcUrl, pageUrl, frameUrl, selectionText.",
             "properties": {
               "menuIds": {
                 "description": "A list of IDs of the menu items that were shown.",
@@ -465,14 +465,14 @@
             "$ref": "tabs.Tab",
             "description": "The details of the tab where the menu was opened."
           }
         ]
       },
       {
         "name": "onHidden",
         "type": "function",
-        "description": "Fired when a menu is hidden. This event is only fired if the menu contained a menu item that was created by the extension.",
+        "description": "Fired when a menu is hidden. This event is only fired if onShown has fired before.",
         "parameters": []
       }
     ]
   }
 ]
--- a/browser/components/extensions/test/browser/browser_ext_menus_events.js
+++ b/browser/components/extensions/test/browser/browser_ext_menus_events.js
@@ -112,36 +112,63 @@ async function testShowHideEvent({menuCr
       "expected onShown info when host permissions are enabled");
     await doCloseMenu();
   }
 
   await extension.unload();
   await BrowserTestUtils.removeTab(tab);
 }
 
-add_task(async function test_no_show_hide_without_menu_item() {
+// Make sure that we won't trigger onShown when extensions cannot add menus.
+add_task(async function test_no_show_hide_for_unsupported_menu() {
   let extension = ExtensionTestUtils.loadExtension({
     background() {
       let events = [];
       browser.menus.onShown.addListener(data => events.push(data));
       browser.menus.onHidden.addListener(() => events.push("onHidden"));
       browser.test.onMessage.addListener(() => {
         browser.test.assertEq("[]", JSON.stringify(events),
-          "Should not have any events when the context menu does not match");
+          "Should not have any events when the context is unsupported.");
         browser.test.notifyPass("done listening to menu events");
       });
+    },
+    manifest: {
+      permissions: ["menus"],
+    },
+  });
+
+  await extension.startup();
+  // Open and close a menu for which the extension cannot add menu items.
+  await openChromeContextMenu("toolbar-context-menu", "#stop-reload-button");
+  await closeChromeContextMenu("toolbar-context-menu");
+
+  extension.sendMessage("check menu events");
+  await extension.awaitFinish("done listening to menu events");
+
+  await extension.unload();
+});
+
+add_task(async function test_show_hide_without_menu_item() {
+  let extension = ExtensionTestUtils.loadExtension({
+    background() {
+      let events = [];
+      browser.menus.onShown.addListener(data => events.push(data));
+      browser.menus.onHidden.addListener(() => events.push("onHidden"));
+      browser.test.onMessage.addListener(() => {
+        browser.test.sendMessage("events from menuless extension", events);
+      });
 
       browser.menus.create({
         title: "never shown",
         documentUrlPatterns: ["*://url-pattern-that-never-matches/*"],
         contexts: ["all"],
       });
     },
     manifest: {
-      permissions: ["menus"],
+      permissions: ["menus", PAGE_HOST_PATTERN],
     },
   });
 
   await extension.startup();
 
   // Run another context menu test where onShown/onHidden will fire.
   await testShowHideEvent({
     menuCreateParams: {
@@ -158,20 +185,27 @@ add_task(async function test_no_show_hid
     },
     async doCloseMenu() {
       await closeExtensionContextMenu();
     },
   });
 
   // Now the menu has been shown and hidden, and in another extension the
   // onShown/onHidden events have been dispatched.
-  // If the the first extension has not received any events by now, we can be
-  // confident that the onShown/onHidden events are not unexpectedly triggered.
   extension.sendMessage("check menu events");
-  await extension.awaitFinish("done listening to menu events");
+  let events = await extension.awaitMessage("events from menuless extension");
+  is(events.length, 2, "expect two events");
+  is(events[1], "onHidden", "last event should be onHidden");
+  Assert.deepEqual(events[0], {
+    menuIds: [],
+    contexts: ["page", "all"],
+    editable: false,
+    pageUrl: PAGE,
+    frameId: 0,
+  }, "expected onShown info from menuless extension");
   await extension.unload();
 });
 
 add_task(async function test_show_hide_pageAction() {
   await testShowHideEvent({
     menuCreateParams: {
       title: "pageAction item",
       contexts: ["page_action"],
--- a/browser/components/extensions/test/browser/browser_ext_menus_refresh.js
+++ b/browser/components/extensions/test/browser/browser_ext_menus_refresh.js
@@ -204,40 +204,39 @@ add_task(async function refresh_menus_wi
     },
     async doCloseMenu() {
       await closeExtensionContextMenu();
     },
   });
   await BrowserTestUtils.removeTab(tab);
 });
 
-// refresh() only works for extensions with at least one visible menu item.
-// This test will fail if we ever add support for adding new menu items even if
-// the extension has no existing menu item in the shown menu.
 add_task(async function refresh_without_menus_at_onShown() {
   const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
   let extension = loadExtensionWithMenusApi();
   await extension.startup();
 
   const doOpenMenu = () => openContextMenu("body");
   const doCloseMenu = () => closeExtensionContextMenu();
 
   await doOpenMenu();
+  await extension.awaitMessage("onShown fired");
   await extension.callMenuApi("create", {
     id: "too late",
     title: "created after shown",
   });
   await extension.callMenuApi("refresh");
   let elem = extension.getXULElementByMenuId("too late");
-  is(elem, null, "extension without visible menu items cannot add new items");
+  is(elem.getAttribute("label"), "created after shown",
+    "extension without visible menu items can add new items");
 
   await extension.callMenuApi("update", "too late", {title: "the menu item"});
   await extension.callMenuApi("refresh");
   elem = extension.getXULElementByMenuId("too late");
-  is(elem, null, "updated menu item should still be invisible");
+  is(elem.getAttribute("label"), "the menu item", "label should change");
 
   // The previously created menu item should be visible if the menu is closed
   // and re-opened.
   await doCloseMenu();
   await doOpenMenu();
   await extension.awaitMessage("onShown fired");
   elem = extension.getXULElementByMenuId("too late");
   is(elem.getAttribute("label"), "the menu item", "previously registered item");