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

ext/background.js

Index: ext/background.js
===================================================================
--- a/ext/background.js
+++ b/ext/background.js
@@ -318,17 +318,41 @@
   /* Browser actions */
 
   // On Firefox for Android, open the options page directly when the browser
   // action is clicked.
   if (!("getPopup" in chrome.browserAction))

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

Is this check still correct? We don't want the popup but, go right to the
options page, on mobile, even in (future) Firefox versions that support browser
action popups.

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

You're right, this check is not correct. We don't have the info module available
at this point either. I think we'll have to add the listener unconditionally and
do the check in the handler.

   {
     chrome.browserAction.onClicked.addListener(() =>
     {
-      ext.showOptions();
+      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;
   };
@@ -697,21 +721,19 @@
 
   /* Options */
 
   ext.showOptions = callback =>
   {
     let info = require("info");
 
     if ("openOptionsPage" in chrome.runtime &&
-        // Some versions of Firefox for Android before version 57 do have a

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

There's no longer any need to mention this stuff about Firefox for Android's
poor support for runtime.openOptionsPage, since that's no longer the reason why
we're not using it.

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

Hmm, the "options_ui.page" given in manifest.json, which is opened by
runtime.openOptionsPage(), is also opened directly by the browser, e.g. through
about:addons. So it seems we'd have to make sure anyway, one way or another,
that when this page is opened, the user ends up with mobile UI.

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

This is a difficult one. Our options are to have a separate build target for
Android, which I'm not sure is going to help if we can't release a separate
mobile version of ABP for Firefox, or to have a single options.html and have it
decide dynamically what UI to show to the user.

Thomas, any thoughts on this?

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

I've been wondering about that too but couldn't come up with anything reasonable
so I hoped that you guys had some input on that.

Distributing separate builds is likely not an option so one idea would be to set
up an automatic redirect in options.html but that's a very ugly workaround,
obviously.

Fortunately, any workaround we come up with will no longer be necessary as soon
as the new options page is made responsive at which point we'll no longer need
to bundle mobile-options.html.

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

How about having the default options page embed the mobile options page in a
frame, and hiding all other content, if running on Android?

[Sebastian Noack, 2017/09/07 16:36:45]

What about this? I would be open for other suggestions, but one way or another
we should address this.

-        // 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))
+        // 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(() =>
@@ -727,55 +749,53 @@
                 callback(new Page(tabs[0]));
               else
                 afterTabLoaded(callback)(tabs[0]);
             }
           });
         });
       }
     }
-    else if ("windows" in chrome)

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

Now this code is used for Firefox for Android as well. We need to do this
because runtime.openOptionsPage, even if supported well, will only open
options.html. We want to open mobile-options.html.

+    else
     {
       // Edge does not yet support runtime.openOptionsPage (tested version 38)
-      // and so this workaround needs to stay for now.

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

Removed this part of the comment since this is no longer a "workaround", at
least for Firefox for Android. Also, we're already using extension.getURL here,
part of the comment is outdated.

-      // We are not using extension.getURL to get the absolute path here
-      // because of the Edge issue:
-      // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10276332/
       let optionsUrl = "options.html";
-      let fullOptionsUrl = ext.getURL(optionsUrl);
+
+      // 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)
         {
-          chrome.windows.update(tab.windowId, {focused: true});
+          // 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.

         }
       });
     }
-    else
-    {
-      // 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.
-      ext.pages.open("options.html", callback);
-    }
   };
 
   /* Windows */
   ext.windows = {
     create(createData, callback)
     {
       chrome.windows.create(createData, createdWindow =>
       {