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

lib/notificationHelper.js

Index: lib/notificationHelper.js
===================================================================
--- a/lib/notificationHelper.js
+++ b/lib/notificationHelper.js
@@ -20,16 +20,17 @@
 "use strict";
 
 const {startIconAnimation, stopIconAnimation} = require("icon");
 const {Utils} = require("utils");
 const {Notification: NotificationStorage} = require("notification");
 const {stringifyURL} = require("url");
 const {initAntiAdblockNotification} = require("antiadblockInit");
 const {Prefs} = require("prefs");
+const {showOptions} = require("options");
 
 let activeNotification = null;
 let activeButtons = null;
 let defaultDisplayMethods = ["popup"];
 let displayMethods = Object.create(null);
 displayMethods.critical = ["icon", "notification", "popup"];
 displayMethods.question = ["notification"];
 displayMethods.normal = ["notification"];
@@ -118,17 +119,17 @@
     case "link":
       ext.pages.open(Utils.getDocLink(activeNotification.links[buttonIndex]));
       break;
     case "open-all":
       openNotificationLinks();
       break;
     case "configure":
       Prefs.notifications_showui = true;
-      ext.showOptions(page =>
+      showOptions(page =>

[Sebastian Noack, 2017/09/27 01:38:35]

I didn't go through the code, but assuming all instanced of showOptions() with
callback are in the same form:

  showOptions(page =>
  {
    page.sendMessage({
      type: "app.respond",
      action: <some_action>
      args: <some_args>
    });
  });

It seems to make more sense to make showOptions() take "action" and "args"
instead of a callback:

  showOptions([some_action[, some_args]]);

[Manish Jethani, 2017/09/27 11:20:55]

In messageResponder.js we have this:

  ext.showOptions(() =>
  {
    if (!message.action)
      return;

    sendMessage("app", message.action, ...message.args);
  });

That sendMessage function sends the message to all the listeners after running
some conversion on the arguments. We could inline some of that code here, just
for the options page, but I think still means that the options page would
receive the message twice.

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

The callback there seems to be dead code to me. I can only find one instance of
app.open in the whole code base, and message.action isn't set there. Or do I
miss something? If not, I suggest to just remove the callback from that call.

We have to adapt adblockplusui, anyway, since it is no longer ext.showOptions()
but require("options").showOptions().

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

This callback is required for the abp:subscribe links. In fact, the issue #5748
occurs because this callback is getting called too soon.
 

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

abp:subscribe links are not handled through the app.open message, and my
suggestion was not to unsupport the scenario where there are additional
information to be send to the options page, but rather streamline it by passing
the data to be send to the options page rather than a callback with duplicated
boilerplate, see the first comment in this thread.

[Manish Jethani, 2017/10/02 08:03:12]

Sorry, I referred to the wrong code.

Here's the part from the "subscriptions.add" handler in messageResponder.js:

      ext.showOptions(() =>
      { 
        sendMessage("app", "addSubscription", subscription);
      }); 

So messageResponder.js would have to handle this itself, it cannot let
showOptions send the message, for the reason I mentioned above.

There's only one place in the entire code where the callback simply sends a
message to the page, we would not be avoiding any duplication then by adding
extra parameters to showOptions.

       {
         page.sendMessage({
           type: "app.respond",
           action: "focusSection",
           args: ["notifications"]
         });
       });
       break;