Issue 5347 - Check for contextMenus API support

ext/background.js

Index: ext/background.js
===================================================================
--- a/ext/background.js
+++ b/ext/background.js
@@ -70,17 +70,19 @@
   /* Pages */
 
   let Page = ext.Page = function(tab)
   {
     this.id = tab.id;
     this._url = tab.url && new URL(tab.url);
 
     this.browserAction = new BrowserAction(tab.id);
-    this.contextMenus = new ContextMenus(this);
+
+    if ("contextMenus" in chrome)

[Wladimir Palant, 2017/08/16 11:04:00]

Please add a comment explaining which platform this check is needed for, linking
to https://bugzilla.mozilla.org/show_bug.cgi?id=1269062. This will help knowing
when the check can be removed in future.

[Manish Jethani, 2017/08/16 12:09:45]

Done.

+      this.contextMenus = new ContextMenus(this);
   };
   Page.prototype = {
     get url()
     {
       // usually our Page objects are created from Chrome's Tab objects, which
       // provide the url. So we can return the url given in the constructor.
       if (this._url)
         return this._url;
@@ -413,91 +415,96 @@
           this._addChange("badgeColor", badge.color);
       }
     }
   };
 
 
   /* Context menus */
 
-  let contextMenuItems = new ext.PageMap();
-  let contextMenuUpdating = false;
+  let ContextMenus = null;
 
-  let updateContextMenu = () =>
+  if ("contextMenus" in chrome)

[Wladimir Palant, 2017/08/16 11:04:00]

If I see it correctly, the point of this check is merely avoiding to run too
much code? Doesn't seem worth it, particularly given that it will only affect
Firefox on Android (and even that probably not for long). I'd rather go for a
smaller change here, only prevent errors but nothing beyond that. It seems that
updateContextMenu is the only place where chrome.contextMenus is actually used -
we can have that function return early if chrome.contextMenus doesn't exist.
Other changes seem unnecessary.

[Manish Jethani, 2017/08/16 12:09:45]

Right, I was going for optimizing for mobile, but I agree that that should
probably be a separate exercise.

I think there's a good case for optimizing for mobile because of limited
resources (battery, network, and so on).

Done.

[Wladimir Palant, 2017/08/16 13:21:56]

Respectfully, I disagree. Limited resources or not, this isn't a hot path, with
it only being triggered when the user switches tabs. Considering that Mozilla is
likely to add context menu soon, we should do as little work and keep the
changes as localized as possible.

[Manish Jethani, 2017/08/16 19:39:09]

Yeah, I meant optimizing for mobile in general, not this specific piece of code.
It's not just performance in terms of CPU/memory, it's also storage, network
usage, the user experience, etc.

For example Firefox for Android has the navigator.connection API [1] which tells
you if you're on cellular or on Wi-Fi, so maybe use that to avoid updating the
filter lists on cellular. Again, may not be the best example (EasyList is really
like ~2.8 MB, even smaller compressed), but my point is that there may be things
to do specifically for mobile besides just checking for API support.

[1]: http://wicg.github.io/netinfo/#-dfn-connectiontype-dfn-enum

   {
-    if (contextMenuUpdating)
-      return;
+    let contextMenuItems = new ext.PageMap();
+    let contextMenuUpdating = false;
 
-    contextMenuUpdating = true;
-
-    chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
+    let updateContextMenu = () =>
     {
-      chrome.contextMenus.removeAll(() =>
-      {
-        contextMenuUpdating = false;
+      if (contextMenuUpdating)
+        return;
+
+      contextMenuUpdating = true;
 
-        if (tabs.length == 0)
-          return;
+      chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
+      {
+        chrome.contextMenus.removeAll(() =>
+        {
+          contextMenuUpdating = false;
 
-        let items = contextMenuItems.get({id: tabs[0].id});
+          if (tabs.length == 0)
+            return;
 
-        if (!items)
-          return;
+          let items = contextMenuItems.get({id: tabs[0].id});
 
-        items.forEach(item =>
-        {
-          chrome.contextMenus.create({
-            title: item.title,
-            contexts: item.contexts,
-            onclick(info, tab)
-            {
-              item.onclick(new Page(tab));
-            }
+          if (!items)
+            return;
+
+          items.forEach(item =>
+          {
+            chrome.contextMenus.create({
+              title: item.title,
+              contexts: item.contexts,
+              onclick(info, tab)
+              {
+                item.onclick(new Page(tab));
+              }
+            });
           });
         });
       });
-    });
-  };
+    };
 
-  let ContextMenus = function(page)
-  {
-    this._page = page;
-  };
-  ContextMenus.prototype = {
-    create(item)
+    ContextMenus = function(page)
     {
-      let items = contextMenuItems.get(this._page);
-      if (!items)
-        contextMenuItems.set(this._page, items = []);
+      this._page = page;
+    };
+    ContextMenus.prototype = {
+      create(item)
+      {
+        let items = contextMenuItems.get(this._page);
+        if (!items)
+          contextMenuItems.set(this._page, items = []);
 
-      items.push(item);
-      updateContextMenu();
-    },
-    remove(item)
-    {
-      let items = contextMenuItems.get(this._page);
-      if (items)
+        items.push(item);
+        updateContextMenu();
+      },
+      remove(item)
       {
-        let index = items.indexOf(item);
-        if (index != -1)
+        let items = contextMenuItems.get(this._page);
+        if (items)
         {
-          items.splice(index, 1);
-          updateContextMenu();
+          let index = items.indexOf(item);
+          if (index != -1)
+          {
+            items.splice(index, 1);
+            updateContextMenu();
+          }
         }
       }
-    }
-  };
+    };
 
-  chrome.tabs.onActivated.addListener(updateContextMenu);
+    chrome.tabs.onActivated.addListener(updateContextMenu);
 
-  chrome.windows.onFocusChanged.addListener(windowId =>
-  {
-    if (windowId != chrome.windows.WINDOW_ID_NONE)
-      updateContextMenu();
-  });
+    chrome.windows.onFocusChanged.addListener(windowId =>
+    {
+      if (windowId != chrome.windows.WINDOW_ID_NONE)
+        updateContextMenu();
+    });
+  }
 
 
   /* Web requests */
 
   let framesOfTabs = new Map();
 
   ext.getFrame = (tabId, frameId) =>
   {