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

lib/filterComposer.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
 
 const {defaultMatcher} = require("../adblockpluscore/lib/matcher");
 const {RegExpFilter} = require("../adblockpluscore/lib/filterClasses");
 const {filterNotifier} = require("../adblockpluscore/lib/filterNotifier");
 const {Prefs} = require("./prefs");
 const {extractHostFromFrame, isThirdParty} = require("./url");
 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;
 }
 
 function escapeChar(chr)
 {
   let code = chr.charCodeAt(0);
 
   // Control characters and leading digits must be escaped based on
@@ skipping 103 matching lines @@
 
       // Add an element hiding filter for each generated CSS selector
       for (let selector of selectors)
         filters.push(docDomain.replace(/^www\./, "") + "##" + selector);
     }
   }
 
   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({
     url: browser.extension.getURL("composer.html"),
     left: 50,
     top: 50,
     width: 420,
     height: 200,
     type: "popup"
   }).then(window =>
@@ skipping 107 matching lines @@
   // When tabs start loading we send them a message to ensure that the state
   // of the "block element" tool is reset. This is necessary since Firefox will
   // sometimes cache the state of a tab when the user navigates back / forward,
   // which includes the state of the "block element" tool.
   // Since sending this message will often fail (e.g. for new tabs which have
   // just been opened) we catch and ignore any exception thrown.
   browser.tabs.sendMessage(
     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);
+});