Bug 1215376 - Fire menus.onShown for every menu draft
authorRob Wu <rob@robwu.nl>
Thu, 14 Sep 2017 18:28:36 +0200
changeset 670565 c5173f32aea2ef3a809f031c92a36107e6112bac
parent 670564 529890e6596990efbfc5f6023cc76481f571e016
child 670566 bd3e0a4f72a728bc786fb76f5e0768abb4a02b53
child 670697 37aecab2623c86c348b0437b9eb494528c0ee4de
child 671356 fca421ef5c7808c8c279a81abe3cdf6cbe1580ca
push id81676
push userbmo:rob@robwu.nl
push dateTue, 26 Sep 2017 17:26:42 +0000
bugs1215376
milestone57.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);
       }
@@ -308,17 +311,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;
@@ -357,38 +359,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");
@@ -745,16 +757,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;
@@ -778,18 +791,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
@@ -396,17 +396,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.",
@@ -461,14 +461,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");