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

ext/background.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 299 matching lines @@
   chrome.tabs.onRemoved.addListener(forgetTab);
 
   chrome.tabs.onActivated.addListener(details =>
   {
     ext.pages.onActivated._dispatch(new Page({id: details.tabId}));
   });
 
 
   /* Browser actions */
 
-  // On Firefox for Android, open the options page directly when the browser
-  // action is clicked.
-  if (!("getPopup" in chrome.browserAction))
-  {
-    chrome.browserAction.onClicked.addListener(() =>
-    {
-      ext.pages.query({active: true, lastFocusedWindow: true}, pages =>
-      {
-        let currentPage = pages[0];
-
-        ext.showOptions(optionsPage =>
-        {
-          if (!currentPage.url ||
-              currentPage.url.toString() == optionsPage.url.toString())

[Manish Jethani, 2017/09/06 01:12:12]

Do nothing if the options page is opened from the options page itself.

[Sebastian Noack, 2017/09/06 03:17:12]

I wonder whether this should rather be handled by the options page itself, than
by the code here? Also there are other scenarios like pages with protocols
adblocking isn't supported anyway (e.g. ftp://, moz-extension://, about:), where
therefore the option to whitelist this page should not show up either.

[Manish Jethani, 2017/09/06 10:32:22]

Perhaps that's a better idea.
Right, I'll add the check for http://*/* and https://*/*

[Manish Jethani, 2017/09/07 11:04:16]

Done.

-          {
-            return;
-          }
-
-          optionsPage.sendMessage({
-            type: "app.respond",
-            action: "showPageOptions",
-            args: [
-              {
-                host: currentPage.url.hostname,
-                whitelisted: require("whitelisting")

[Manish Jethani, 2017/09/06 01:12:12]

require is not available at the time of initialization of this file so we have
to put it here.

[Sebastian Noack, 2017/09/06 03:17:12]

Which raises the question, why is this code inside ext, and not somewhere in
lib/*, in the first place. Latest after these changes, this doesn't seem to be
abstraction code anymore, but application logic.

[Manish Jethani, 2017/09/06 10:32:22]

Makes sense.

Do you think this should be a new module like lib/mobileOptions.js?

[Sebastian Noack, 2017/09/07 02:53:38]

No strong opinion. Though, another option would be lib/browserAction.js.

[Manish Jethani, 2017/09/07 11:04:16]

Done.

I've moved it to lib/browserAction.js now.

-                             .checkWhitelisted(currentPage)

[Thomas Greiner, 2017/09/06 12:40:03]

Looking at the desktop equivalent of this feature at
https://hg.adblockplus.org/adblockpluschrome/file/a401d9555223/popup.js#l96 I
noticed that the UI's behavior needs to change because it differs from the
existing implementation in that it only cares about whether the page's domain is
whitelisted. So to make this compatible with the way it is currently
implemented, we'd instead need to check whether the filter
"@@||example.com^$document" is active.

So either we make it compatible now and change it to use `checkWhitelisted()` in
a future dependency update or we accept this inconsistency until the UI's
behavior is corrected. I'm fine with either of them so feel free to choose
whichever you prefer.

Sorry about the trouble.

[Thomas Greiner, 2017/09/06 13:06:34]

FYI: I've created a spec ticket to get the Product team's take on this as well
(see
https://bitbucket.org/adblockplus/spec/issues/72/enable-toggle-behavior-is-in...).

[Manish Jethani, 2017/09/07 11:12:55]

I'm not sure I understand.

The difference between spec and popup UI behavior is only in the third point,
i.e. switching it to "on"?

Shouldn't checkWhitelisted("example.com") effectively check for
"@@||example.com^$document", or does it do something different?

[Thomas Greiner, 2017/09/13 14:56:29]

It checks whether the page is whitelisted, not the domain. The page might be
whitelisted even if the domain isn't.

We've discussed it in today's UI meeting as well and agreed on being consistent
with the way it's implemented in the popup for now. That means that we can go
with `checkWhitelisted()` in this review.

Later on (meaning: sometime after the webext launch) we'll have to revisit this
topic though to resolve this inconsistency.

-              }
-            ]
-          });
-        });
-      });
-    });
-  }
-
   let BrowserAction = function(tabId)
   {
     this._tabId = tabId;
     this._changes = null;
   };
   BrowserAction.prototype = {
     _applyChanges()
     {
       if ("iconPath" in this._changes)
       {
         // Firefox for Android displays the browser action not as an icon but
         // as a menu item. There is no icon, but such an option may be added in
         // the future.
         // https://bugzilla.mozilla.org/show_bug.cgi?id=1331746
         if ("setIcon" in chrome.browserAction)
         {
-          chrome.browserAction.setIcon({
-            tabId: this._tabId,
-            path: {
-              16: this._changes.iconPath.replace("$size", "16"),
-              19: this._changes.iconPath.replace("$size", "19"),
-              20: this._changes.iconPath.replace("$size", "20"),
-              32: this._changes.iconPath.replace("$size", "32"),
-              38: this._changes.iconPath.replace("$size", "38"),
-              40: this._changes.iconPath.replace("$size", "40")
-            }
-          });
+          let path = {
+            16: this._changes.iconPath.replace("$size", "16"),
+            19: this._changes.iconPath.replace("$size", "19"),
+            20: this._changes.iconPath.replace("$size", "20"),
+            32: this._changes.iconPath.replace("$size", "32"),
+            38: this._changes.iconPath.replace("$size", "38"),
+            40: this._changes.iconPath.replace("$size", "40")
+          };
+          try
+          {
+            chrome.browserAction.setIcon({tabId: this._tabId, path});
+          }
+          catch (e)
+          {
+            // Edge throws if passed icon sizes different than 19,20,38,40px.
+            delete path[16];
+            delete path[32];
+            chrome.browserAction.setIcon({tabId: this._tabId, path});
+          }
         }
       }
 
       if ("badgeText" in this._changes)
       {
         // There is no badge on Firefox for Android; the browser action is
         // simply a menu item.
         if ("setBadgeText" in chrome.browserAction)
         {
           chrome.browserAction.setBadgeText({
@@ skipping 321 matching lines @@
       items[key] = value;
       chrome.storage.local.set(items, callback);
     },
     remove(key, callback)
     {
       chrome.storage.local.remove(key, callback);
     },
     onChanged: chrome.storage.onChanged
   };
 
-  /* Options */
-
-  ext.showOptions = callback =>
-  {
-    let info = require("info");
-
-    if ("openOptionsPage" in chrome.runtime &&
-        // We don't use runtime.openOptionsPage on Firefox for Android because
-        // we have a different options page for mobile.
-        info.application != "fennec")
-    {
-      if (!callback)
-      {
-        chrome.runtime.openOptionsPage();
-      }
-      else
-      {
-        chrome.runtime.openOptionsPage(() =>
-        {
-          if (chrome.runtime.lastError)
-            return;
-
-          chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
-          {
-            if (tabs.length > 0)
-            {
-              if (tabs[0].status == "complete")
-                callback(new Page(tabs[0]));
-              else
-                afterTabLoaded(callback)(tabs[0]);
-            }
-          });
-        });
-      }
-    }
-    else
-    {
-      // Edge does not yet support runtime.openOptionsPage (tested version 38)
-      let optionsUrl = "options.html";
-
-      // Firefox for Android has its own options page.
-      if (info.application == "fennec")
-        optionsUrl = "mobile-options.html";
-
-      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:

[Thomas Greiner, 2017/09/06 12:40:02]

Detail: It's worth mentioning that this has been fixed in Firefox 56 (see
https://bugzilla.mozilla.org/show_bug.cgi?id=1271354)

[Manish Jethani, 2017/09/07 11:04:16]

Done.

-        // https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Match_patterns
-        let fullOptionsUrl = ext.getURL(optionsUrl);
-        let tab = tabs.find(element => element.url == fullOptionsUrl);
-        if (tab)
-        {
-          // Firefox for Android doesn't support the windows API, and the
-          // mobile browser has only one window anyway.
-          if ("windows" in chrome)
-            chrome.windows.update(tab.windowId, {focused: true});
-
-          chrome.tabs.update(tab.id, {active: true});
-
-          if (callback)
-            callback(new Page(tab));
-        }
-        else
-        {
-          ext.pages.open(optionsUrl, callback);

[Oleksandr, 2017/09/06 02:51:58]

Maybe add a comment above this line why we are not using fullOptionsUrl here?
Something like:

We are not using fullOptionsUrl here because of Edge issue:
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332

[Manish Jethani, 2017/09/06 10:32:22]

Acknowledged.

[Manish Jethani, 2017/09/07 11:04:16]

Done.

-        }
-      });
-    }
-  };
-
   /* Windows */
   ext.windows = {
     create(createData, callback)
     {
       chrome.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }());