Issue 5587, 5748 - Use mobile options page on Firefox for Android

lib/options.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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/** @module options */
+
+"use strict";
+
+const {port} = require("messaging");
+const info = require("info");
+
+const optionsUrl = "options.html";
+
+function findOptionsTab(callback)
+{
+  chrome.tabs.query({}, tabs =>
+  {
+    // We find a tab ourselves because Edge has a bug when quering tabs with
+    // extension URL protocol:
+    // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/8094141/
+    // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/8604703/
+    // Firefox won't let us query for moz-extension:// pages either, though
+    // starting with Firefox 56 an extension can query for its own URLs:
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1271354
+    let fullOptionsUrl = ext.getURL(optionsUrl);
+    callback(tabs.find(element => element.url == fullOptionsUrl));
+  });
+}
+
+function returnShowOptionsCall(optionsTab, callback)
+{
+  if (!callback)
+    return;
+
+  if (optionsTab)
+  {
+    callback(new ext.Page(optionsTab));
+  }
+  else
+  {
+    // If we don't already have an options page, it means we've just opened
+    // one, in which case we must find the tab, wait for it to be ready, and
+    // then return the call.
+    findOptionsTab(tab =>
+    {
+      function onMessage(message, sender)
+      {
+        if (message.type == "app.listen" &&
+            sender.page && sender.page.id == page.id)
+        {
+          port.off("app.listen");

[Sebastian Noack, 2017/09/30 02:38:44]

It seems you forgot to specify the listener function as second argument here.

[Manish Jethani, 2017/09/30 13:49:52]

Done.

+          callback(page);
+        }
+      }
+
+      let page = new ext.Page(tab);

[Sebastian Noack, 2017/09/30 02:38:45]

You could get rid of this temporary variable (without adding any duplication if
you just inline it above:

  callback(new ext.Path(tab))

And then just use tab.id instead of page.id in the check above that.

[Manish Jethani, 2017/09/30 13:49:53]

Done.

+      port.on("app.listen", onMessage);
+    });
+  }
+}
+
+/**
+ * Opens the options page.
+ *
+ * @param {function} callback
+ * @static

[Sebastian Noack, 2017/09/30 02:38:45]

Is marking this function as @static necessary? It seems we don't for any other
exported function in any other module. Except there is a reason we should remain
consistent, I guess.

[Manish Jethani, 2017/09/30 13:49:53]

Done.

+ */
+exports.showOptions = callback =>
+{
+  findOptionsTab(optionsTab =>
+  {
+    // Edge does not yet support runtime.openOptionsPage (tested version 38)
+    if ("openOptionsPage" in chrome.runtime &&
+        // Some versions of Firefox for Android before version 57 do have a
+        // runtime.openOptionsPage but it doesn't do anything.
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1364945
+        (info.application != "fennec" ||
+         parseInt(info.applicationVersion, 10) >= 57))
+    {
+      chrome.runtime.openOptionsPage(() =>
+      {
+        if (chrome.runtime.lastError)
+          return;
+
+        returnShowOptionsCall(optionsTab, callback);
+      });
+    }
+    else if (optionsTab)

[Sebastian Noack, 2017/09/30 02:38:44]

Shouldn't this branch go first?

[Manish Jethani, 2017/09/30 13:49:53]

I guess that depends. Do we want to skip calling runtime.openOptionsPage if
we've already found an options page open in a tab? Maybe runtime.openOptionsPage
does other things (like animating the tab or flashing it to draw the user's
attention), either now or in the future, so I'd say if the API is available then
maybe we should just use it instead of trying to do it ourselves, this way we'd
get the best user experience.

Right now though, on the platforms I've tested this on, it seems to make no
difference either way.

[Sebastian Noack, 2017/10/02 01:47:06]

I think you are right, it is better to rely on runitm.openOptionPage () when
available, in order to bring up the existing options tab if any.

+    {
+      // Firefox for Android before version 57 does not support
+      // runtime.openOptionsPage, nor does it support the windows API.
+      // Since there is effectively only one window on the mobile browser,
+      // there's no need to bring it into focus.
+      if ("windows" in chrome)
+        chrome.windows.update(optionsTab.windowId, {focused: true});
+
+      chrome.tabs.update(optionsTab.id, {active: true});
+
+      returnShowOptionsCall(optionsTab, callback);
+    }
+    else
+    {
+      // We use a relative URL here because of this Edge issue:
+      // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332
+      chrome.tabs.create({url: optionsUrl}, () =>
+      {
+        returnShowOptionsCall(optionsTab, callback);
+      });
+    }
+  });
+};