Issue 5347 - Check for contextMenus API support

ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 57 matching lines @@
   };
 
   /* 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);
-
-    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);
+    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;
 
       // but sometimes we only have the tab id when we create a Page object.
@@ skipping 322 matching lines @@
 
         if ("color" in badge)
           this._addChange("badgeColor", badge.color);
       }
     }
   };
 
 
   /* Context menus */
 
-  let ContextMenus = null;
-
-  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

-  {
-    let contextMenuItems = new ext.PageMap();
-    let contextMenuUpdating = false;
-
-    let updateContextMenu = () =>
-    {
-      if (contextMenuUpdating)
-        return;
-
-      contextMenuUpdating = true;
-
-      chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
-      {
-        chrome.contextMenus.removeAll(() =>
-        {
-          contextMenuUpdating = false;
-
-          if (tabs.length == 0)
-            return;
-
-          let items = contextMenuItems.get({id: tabs[0].id});
-
-          if (!items)
-            return;
-
-          items.forEach(item =>
-          {
-            chrome.contextMenus.create({
-              title: item.title,
-              contexts: item.contexts,
-              onclick(info, tab)
-              {
-                item.onclick(new Page(tab));
-              }
-            });
+  let contextMenuItems = new ext.PageMap();
+  let contextMenuUpdating = false;
+
+  let updateContextMenu = () =>
+  {
+    // Firefox for Android does not support context menus.
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1269062
+    if (!("contextMenus" in chrome) || contextMenuUpdating)
+      return;
+
+    contextMenuUpdating = true;
+
+    chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
+    {
+      chrome.contextMenus.removeAll(() =>
+      {
+        contextMenuUpdating = false;
+
+        if (tabs.length == 0)
+          return;
+
+        let items = contextMenuItems.get({id: tabs[0].id});
+
+        if (!items)
+          return;
+
+        items.forEach(item =>
+        {
+          chrome.contextMenus.create({
+            title: item.title,
+            contexts: item.contexts,
+            onclick(info, tab)
+            {
+              item.onclick(new Page(tab));
+            }
           });
         });
       });
-    };
-
-    ContextMenus = function(page)
-    {
-      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)
-        {
-          let index = items.indexOf(item);
-          if (index != -1)
-          {
-            items.splice(index, 1);
-            updateContextMenu();
-          }
+    });
+  };
+
+  let ContextMenus = function(page)
+  {
+    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)
+      {
+        let index = items.indexOf(item);
+        if (index != -1)
+        {
+          items.splice(index, 1);
+          updateContextMenu();
         }
       }
-    };
-
-    chrome.tabs.onActivated.addListener(updateContextMenu);
-
-    chrome.windows.onFocusChanged.addListener(windowId =>
-    {
-      if (windowId != chrome.windows.WINDOW_ID_NONE)
-        updateContextMenu();
-    });
-  }
+    }
+  };
+
+  chrome.tabs.onActivated.addListener(updateContextMenu);
+
+  chrome.windows.onFocusChanged.addListener(windowId =>
+  {
+    if (windowId != chrome.windows.WINDOW_ID_NONE)
+      updateContextMenu();
+  });
 
 
   /* Web requests */
 
   let framesOfTabs = new Map();
 
   ext.getFrame = (tabId, frameId) =>
   {
     let frames = framesOfTabs.get(tabId);
     return frames && frames.get(frameId);
@@ skipping 233 matching lines @@
   ext.windows = {
     create(createData, callback)
     {
       chrome.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }());