Bug 1469705: allow menus.refresh() without an onShown listener r=mixedpuppy
authorPeter Simonyi <pts@petersimonyi.ca>
Thu, 05 Jul 2018 22:06:23 -0400
changeset 486334 9a65efe88395ef3d8e79f652a317bafbd9bbe962
parent 486333 ff3f57736492ae3884047c9e27521db1ce4f1138
child 486335 114f05d1f4a4bea799aa0f7ca548cef7d0bae950
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1469705
milestone63.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 1469705: allow menus.refresh() without an onShown listener r=mixedpuppy MozReview-Commit-ID: 3GbQEoSFTW8
browser/components/extensions/parent/ext-menus.js
browser/components/extensions/test/browser/browser_ext_menus_refresh.js
--- a/browser/components/extensions/parent/ext-menus.js
+++ b/browser/components/extensions/parent/ext-menus.js
@@ -348,24 +348,23 @@ var gMenuBuilder = {
 
   rebuildMenu(extension) {
     let {contextData} = this;
     if (!contextData) {
       // This happens if the menu is not visible.
       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;
-    }
-
     if (contextData.onBrowserAction || contextData.onPageAction) {
+      if (contextData.extension.id !== extension.id) {
+        // The extension that just called refresh() is not the owner of the
+        // action whose context menu is showing, so it can't have any items in
+        // the menu anyway and nothing will change.
+        return;
+      }
       // The action menu can only have items from one extension, so remove all
       // items (including the separator) and rebuild the action menu (if any).
       for (let item of this.itemsToCleanUp) {
         item.remove();
       }
       this.itemsToCleanUp.clear();
       this.buildActionContextMenu(contextData);
       return;
--- a/browser/components/extensions/test/browser/browser_ext_menus_refresh.js
+++ b/browser/components/extensions/test/browser/browser_ext_menus_refresh.js
@@ -8,22 +8,27 @@ const PAGE = "http://mochi.test:8888/bro
 // Load an extension that has the "menus" permission. The returned Extension
 // instance has a `callMenuApi` method to easily call a browser.menus method
 // and wait for its result. It also emits the "onShown fired" message whenever
 // the menus.onShown event is fired.
 // The `getXULElementByMenuId` method returns the XUL element that corresponds
 // to the menu item ID from the browser.menus API (if existent, null otherwise).
 function loadExtensionWithMenusApi() {
   async function background() {
-    browser.menus.onShown.addListener(() => {
+    function shownHandler() {
       browser.test.sendMessage("onShown fired");
-    });
+    }
+
+    browser.menus.onShown.addListener(shownHandler);
     browser.test.onMessage.addListener((method, ...params) => {
       let result;
-      if (method === "create") {
+      if (method === "* remove onShown listener") {
+        browser.menus.onShown.removeListener(shownHandler);
+        result = Promise.resolve();
+      } else if (method === "create") {
         result = new Promise(resolve => {
           browser.menus.create(params[0], resolve);
         });
       } else {
         result = browser.menus[method](...params);
       }
       result.then(() => {
         browser.test.sendMessage(`${method}-result`);
@@ -40,16 +45,20 @@ function loadExtensionWithMenusApi() {
   });
 
   extension.callMenuApi = async function(method, ...params) {
     info(`Calling ${method}(${JSON.stringify(params)})`);
     extension.sendMessage(method, ...params);
     return extension.awaitMessage(`${method}-result`);
   };
 
+  extension.removeOnShownListener = async function() {
+    extension.callMenuApi("* remove onShown listener");
+  };
+
   extension.getXULElementByMenuId = id => {
     // Same implementation as elementId getter in ext-menus.js
     if (typeof id != "number") {
       id = `_${id}`;
     }
     let xulId = `${makeWidgetId(extension.id)}-menuitem-${id}`;
     return document.getElementById(xulId);
   };
@@ -152,65 +161,135 @@ async function testRefreshMenusWhileVisi
   // Refresh after menu was hidden - should be noop.
   await extension.callMenuApi("refresh");
   elem = extension.getXULElementByMenuId("abc");
   is(elem, null, "menu item must still be gone");
 
   await extension.unload();
 }
 
+// Check that one extension calling refresh() doesn't interfere with others.
+// When expectOtherItems == false, the other extension's menu items should not
+// show at all (e.g. for browserAction).
+async function testRefreshOther({contexts, doOpenMenu, doCloseMenu,
+                                 expectOtherItems}) {
+  let extension = loadExtensionWithMenusApi();
+  let other_extension = loadExtensionWithMenusApi();
+  await extension.startup();
+  await other_extension.startup();
+
+  await extension.callMenuApi("create", {
+    id: "action_item",
+    title: "visible menu item",
+    contexts: contexts,
+  });
+
+  await other_extension.callMenuApi("create", {
+    id: "action_item",
+    title: "other menu item",
+    contexts: contexts,
+  });
+
+  await doOpenMenu(extension);
+  await extension.awaitMessage("onShown fired");
+  if (expectOtherItems) {
+    await other_extension.awaitMessage("onShown fired");
+  }
+
+  let elem = extension.getXULElementByMenuId("action_item");
+  is(elem.getAttribute("label"), "visible menu item", "extension menu shown");
+  elem = other_extension.getXULElementByMenuId("action_item");
+  if (expectOtherItems) {
+    is(elem.getAttribute("label"), "other menu item",
+       "other extension's menu is also shown");
+  } else {
+    is(elem, null, "other extension's menu should be hidden");
+  }
+
+  await extension.callMenuApi("update", "action_item", {title: "changed"});
+  await other_extension.callMenuApi("update", "action_item", {title: "foo"});
+  await other_extension.callMenuApi("refresh");
+
+  // refreshing the menu of an unrelated extension should not affect the menu
+  // of another extension.
+  elem = extension.getXULElementByMenuId("action_item");
+  is(elem.getAttribute("label"), "visible menu item", "extension menu shown");
+  elem = other_extension.getXULElementByMenuId("action_item");
+  if (expectOtherItems) {
+    is(elem.getAttribute("label"), "foo", "other extension's item is updated");
+  } else {
+    is(elem, null, "other extension's menu should still be hidden");
+  }
+
+  await doCloseMenu();
+  await extension.unload();
+  await other_extension.unload();
+}
+
 add_task(async function refresh_menus_with_browser_action() {
-  await testRefreshMenusWhileVisible({
+  const args = {
     contexts: ["browser_action"],
     async doOpenMenu(extension) {
       await openActionContextMenu(extension, "browser");
     },
     async doCloseMenu() {
       await closeActionContextMenu();
     },
-  });
+  };
+  await testRefreshMenusWhileVisible(args);
+  args.expectOtherItems = false;
+  await testRefreshOther(args);
 });
 
 add_task(async function refresh_menus_with_tab() {
   const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
-  await testRefreshMenusWhileVisible({
+  const args = {
     contexts: ["tab"],
     async doOpenMenu() {
       await openTabContextMenu();
     },
     async doCloseMenu() {
       await closeTabContextMenu();
     },
-  });
+  };
+  await testRefreshMenusWhileVisible(args);
+  args.expectOtherItems = true;
+  await testRefreshOther(args);
   BrowserTestUtils.removeTab(tab);
 });
 
 add_task(async function refresh_menus_with_tools_menu() {
-  await testRefreshMenusWhileVisible({
+  const args = {
     contexts: ["tools_menu"],
     async doOpenMenu() {
       await openToolsMenu();
     },
     async doCloseMenu() {
       await closeToolsMenu();
     },
-  });
+  };
+  await testRefreshMenusWhileVisible(args);
+  args.expectOtherItems = true;
+  await testRefreshOther(args);
 });
 
 add_task(async function refresh_menus_with_page() {
   const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
-  await testRefreshMenusWhileVisible({
+  const args = {
     contexts: ["page"],
     async doOpenMenu() {
       await openContextMenu("body");
     },
     async doCloseMenu() {
       await closeExtensionContextMenu();
     },
-  });
+  };
+  await testRefreshMenusWhileVisible(args);
+  args.expectOtherItems = true;
+  await testRefreshOther(args);
   BrowserTestUtils.removeTab(tab);
 });
 
 add_task(async function refresh_without_menus_at_onShown() {
   const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
   let extension = loadExtensionWithMenusApi();
   await extension.startup();
 
@@ -241,56 +320,42 @@ add_task(async function refresh_without_
   elem = extension.getXULElementByMenuId("too late");
   is(elem.getAttribute("label"), "the menu item", "previously registered item");
   await doCloseMenu();
 
   await extension.unload();
   BrowserTestUtils.removeTab(tab);
 });
 
-add_task(async function refresh_menus_with_browser_action() {
+add_task(async function refresh_without_onShown() {
+  const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);
   let extension = loadExtensionWithMenusApi();
-  let other_extension = loadExtensionWithMenusApi();
   await extension.startup();
-  await other_extension.startup();
+  await extension.removeOnShownListener();
 
+  const doOpenMenu = () => openContextMenu("body");
+  const doCloseMenu = () => closeExtensionContextMenu();
+
+  await doOpenMenu();
   await extension.callMenuApi("create", {
-    id: "action_item",
-    title: "visible menu item",
-    contexts: ["browser_action"],
-  });
-
-  await other_extension.callMenuApi("create", {
-    id: "action_item",
-    title: "other menu item",
-    contexts: ["browser_action"],
+    id: "too late",
+    title: "created after shown",
   });
 
-  await openActionContextMenu(extension, "browser");
-  await extension.awaitMessage("onShown fired");
-
-  let elem = extension.getXULElementByMenuId("action_item");
-  is(elem.getAttribute("label"), "visible menu item", "extension menu shown");
-  elem = other_extension.getXULElementByMenuId("action_item");
-  is(elem, null, "other extension's menu should be hidden");
+  is(extension.getXULElementByMenuId("too late"), null,
+     "item created after shown is not visible before refresh");
 
-  await extension.callMenuApi("update", "action_item", {title: "changed"});
-  await other_extension.callMenuApi("update", "action_item", {title: "foo"});
-  await other_extension.callMenuApi("refresh");
+  await extension.callMenuApi("refresh");
+  let elem = extension.getXULElementByMenuId("too late");
+  is(elem.getAttribute("label"), "created after shown",
+     "refresh updates the menu even without onShown");
 
-  // refreshing the menu of an unrelated extension should not affect the menu
-  // of another extension.
-  elem = extension.getXULElementByMenuId("action_item");
-  is(elem.getAttribute("label"), "visible menu item", "extension menu shown");
-  elem = other_extension.getXULElementByMenuId("action_item");
-  is(elem, null, "other extension's menu should be hidden");
-
-  await closeActionContextMenu();
+  await doCloseMenu();
   await extension.unload();
-  await other_extension.unload();
+  BrowserTestUtils.removeTab(tab);
 });
 
 add_task(async function refresh_menus_during_navigation() {
   const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE + "?1");
   let extension = loadExtensionWithMenusApi();
   await extension.startup();
 
   await extension.callMenuApi("create", {