Issue 3829 - Merge add-subscriptions and subscriptions.add messages

messageResponder.js

Index: messageResponder.js
===================================================================
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -132,14 +132,6 @@
 
     switch (message.type)
     {
-      case "add-subscription":
-        ext.showOptions(function()
-        {
-          var subscription = Subscription.fromURL(message.url);
-          subscription.title = message.title;
-          onFilterChange("addSubscription", subscription);
-        });
-        break;
       case "app.get":
         if (message.what == "issues")
         {
@@ -299,22 +291,24 @@
         callback(Prefs[message.key]);
         break;
       case "subscriptions.add":
-        if (message.url in FilterStorage.knownSubscriptions)

[Sebastian Noack, 2016/03/17 21:41:17]

This check seems like a bug to me. If I understand the FilterStorage code
correctly the only thing that it does is ignoring subscriptions that have
previously been added, even if they got removed again. Also note that we removed
a similar check from the subscription initialization code.

[Thomas Greiner, 2016/03/18 10:48:20]

No, you're probably thinking about `Filter.knownFilters` or
`ElemHide.knownSubscriptions` which are never being cleared (see #3468). But
according to
https://hg.adblockplus.org/adblockpluscore/file/tip/lib/filterStorage.js#l171
`FilterStorage.knownSubscriptions` is cleared properly.

[Sebastian Noack, 2016/03/18 11:35:07]

You're right. However, FilterStorage.addSubscription already bails out if the
subscription is in FilterStorage.knownSubscriptions. So this check is still
redundant.

[Thomas Greiner, 2016/03/18 13:20:39]

I see what you mean. Seems to be more of a design decision whether or not we'd
want the confirmation dialog to show up if a subscription has already been
added.
Currently, there's no feedback for the user that this is the case so it
shouldn't make too much of a difference whether the dialog is shown or not, I
assume.

-          return;
-
         var subscription = Subscription.fromURL(message.url);
-        if (!subscription)

[Sebastian Noack, 2016/03/17 21:41:17]

Subscription.fromURL() never returns null or similar. Similar checks also have
been removed from the subscription initialization code.

[Thomas Greiner, 2016/03/18 10:48:20]

You're right, nice catch.

-          return;
-
-        subscription.disabled = false;
         if ("title" in message)
           subscription.title = message.title;
         if ("homepage" in message)
           subscription.homepage = message.homepage;
-        FilterStorage.addSubscription(subscription);
 
-        if (subscription instanceof DownloadableSubscription && !subscription.lastDownload)
-          Synchronizer.execute(subscription);
+        if (message.ask)

[Thomas Greiner, 2016/03/18 10:48:20]

Detail: Just a personal preference but what do you think about "confirm" instead
since our dialog is equivalent in its purpose to `window.confirm()`?

[Sebastian Noack, 2016/03/18 11:35:07]

Fine with me.

+        {
+          ext.showOptions(onFilterChange.bind(null, "addSubscription", subscription));

[Thomas Greiner, 2016/03/18 10:48:20]

Coding style: Mind the maximum line length.

[Sebastian Noack, 2016/03/18 11:35:07]

Done.

+        }
+        else
+        {
+          subscription.disabled = false;
+          FilterStorage.addSubscription(subscription);
+
+          if (subscription instanceof DownloadableSubscription && !subscription.lastDownload)
+            Synchronizer.execute(subscription);
+        }
         break;
       case "subscriptions.get":
         var subscriptions = FilterStorage.subscriptions.filter(function(s)