Issue 6948 - Begun simplifying the "block element" context menu logic

lib/filterComposer.js

Index: lib/filterComposer.js
diff --git a/lib/filterComposer.js b/lib/filterComposer.js
index b12a56b6e9bff4c57875506d0e889f2b84ebe1e2..fe5dcdf3071e1762e31cfb4e4bd461a67bbcac23 100644
--- a/lib/filterComposer.js
+++ b/lib/filterComposer.js
@@ -28,19 +28,6 @@ const {getKey, checkWhitelisted} = require("./whitelisting");
 const {port} = require("./messaging");
 const info = require("info");
 
-let readyPages = new ext.PageMap();
-
-/**
- * Checks whether the given page is ready to use the filter composer
- *
- * @param {Page} page
- * @return {boolean}
- */
-exports.isPageReady = page =>

[kzar, 2018/09/28 15:37:34]

I coudln't see where `isPageReady` was being used in adblockpluschrome,
adblockpluscore or adblockplusui, so I just removed it.

-{
-  return readyPages.has(page);
-};
-
 function isValidString(s)
 {
   return s && s.indexOf("\0") == -1;
@@ -164,53 +151,6 @@ function composeFilters(details)
   return {filters, selectors};
 }
 
-let contextMenuItem = {
-  title: browser.i18n.getMessage("block_element"),
-  contexts: ["image", "video", "audio"],
-  onclick(page)
-  {
-    page.sendMessage({type: "composer.content.contextMenuClicked"});
-  }
-};
-
-function updateContextMenu(page, filter)
-{
-  page.contextMenus.remove(contextMenuItem);
-
-  if (typeof filter == "undefined")
-    filter = checkWhitelisted(page);
-
-  // We don't support the filter composer on Firefox for Android, because the
-  // user experience on mobile is quite different.
-  if (info.application != "fennec" &&
-      !filter && Prefs.shouldShowBlockElementMenu && readyPages.has(page))
-  {
-    page.contextMenus.create(contextMenuItem);
-  }
-}
-
-filterNotifier.on("page.WhitelistingStateRevalidate", updateContextMenu);
-
-Prefs.on("shouldShowBlockElementMenu", () =>
-{
-  browser.tabs.query({}, tabs =>
-  {
-    for (let tab of tabs)
-      updateContextMenu(new ext.Page(tab));
-  });
-});
-
-port.on("composer.isPageReady", (message, sender) =>
-{
-  return readyPages.has(new ext.Page({id: message.pageId}));
-});
-
-port.on("composer.ready", (message, sender) =>
-{
-  readyPages.set(sender.page, null);
-  updateContextMenu(sender.page);
-});
-
 port.on("composer.openDialog", (message, sender) =>
 {
   return browser.windows.create({
@@ -338,3 +278,90 @@ ext.pages.onLoading.addListener(page =>
     page.id, {type: "composer.content.finished"}
   ).catch(() => {});
 });
+
+
+/* Context menu and popup button */
+
+let readyActivePages = new ext.PageMap();
+let showingContextMenu = false;
+
+function showOrHideContextMenu(activePage)
+{
+  // Firefox for Android does not support browser.contextMenus.
+  // https://bugzilla.mozilla.org/show_bug.cgi?id=1269062
+  if (!("contextMenus" in browser))
+    return;
+
+  let shouldShowContextMenu = Prefs.shouldShowBlockElementMenu &&
+                                readyActivePages.get(activePage);
+
+  if (shouldShowContextMenu && !showingContextMenu)

[kzar, 2018/09/28 15:37:34]

The previous logic wasn't great, I noticed it was adding and removing the
context menu more often than was necessary. I think this new logic should work
more reliably.

+  {
+    browser.contextMenus.create({

[kzar, 2018/09/28 15:37:34]

I originally kept the `contextMenuItem` variable, then passed that through to
`browser.contextMenues.create`. Surprisingly however, I noticed that the API
mutates the passed in object, adding a generated ID property. Then, the API
throws an exception the next time it's called with that Object since the
generated ID property already exists...

+      id: "block_element",
+      title: browser.i18n.getMessage("block_element"),
+      contexts: ["image", "video", "audio"],
+      onclick(itemInfo, tab)
+      {
+        let page = new ext.Page(tab);
+        page.sendMessage({type: "composer.content.contextMenuClicked"});
+      }
+    });
+    showingContextMenu = true;
+  }
+  else if (!shouldShowContextMenu && showingContextMenu)
+  {
+    browser.contextMenus.remove("block_element");
+    showingContextMenu = false;
+  }
+}
+
+function updateContextMenu(updatedPage)
+{
+  browser.tabs.query({active: true, lastFocusedWindow: true}).then(tabs =>
+  {
+    if (tabs.length > 0 && (!updatedPage || updatedPage.id == tabs[0].id))
+      showOrHideContextMenu(updatedPage || new ext.Page(tabs[0]));
+  });
+}
+
+browser.tabs.onActivated.addListener(activeInfo =>
+{
+  showOrHideContextMenu(new ext.Page({id: activeInfo.tabId}));
+});
+
+// Firefox for Android does not support browser.windows.

[kzar, 2018/09/28 15:37:34]

Like we discussed, it's kind of pointless to worry about Firefox for Android in
this file at all... however since we're stuck doing that I've gone with feature
detection like you suggested.

+// https://issues.adblockplus.org/ticket/5347
+if ("windows" in browser)
+{
+  browser.windows.onFocusChanged.addListener(windowId =>
+  {
+    if (windowId != browser.windows.WINDOW_ID_NONE)
+      updateContextMenu();
+  });
+}
+
+filterNotifier.on("page.WhitelistingStateRevalidate", (page, filter) =>
+{
+  if (readyActivePages.has(page))
+  {
+    readyActivePages.set(page, !filter);
+    updateContextMenu(page);
+  }
+});
+
+Prefs.on("shouldShowBlockElementMenu", () =>
+{
+  updateContextMenu();
+});
+
+port.on("composer.isPageReady", (message, sender) =>
+{
+  return readyActivePages.has(new ext.Page({id: message.pageId}));
+});
+
+port.on("composer.ready", (message, sender) =>
+{
+  readyActivePages.set(sender.page, !checkWhitelisted(sender.page));
+  updateContextMenu(sender.page);
+});