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

messageResponder.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 114 matching lines @@
         if (!listenerFilters)
         {
           listenerFilters = Object.create(null);
           changeListeners.set(sender.page, listenerFilters);
         }
         break;
     }
 
     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")
         {
           var subscriptionInit;
           try
           {
             subscriptionInit = require("subscriptionInit");
           }
           catch (e)
           {
@@ skipping 139 matching lines @@
 
         if (!subscription)
           FilterStorage.removeFilter(filter);
         else
           FilterStorage.removeFilter(filter, subscription, message.index);
         break;
       case "prefs.get":
         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)
         {
           if (message.ignoreDisabled && s.disabled)
             return false;
           if (s instanceof DownloadableSubscription && message.downloadable)
             return true;
           if (s instanceof SpecialSubscription && message.special)
             return true;
@@ skipping 44 matching lines @@
           if (subscription instanceof DownloadableSubscription)
             Synchronizer.execute(subscription, true);
         }
         break;
       case "subscriptions.isDownloading":
         callback(Synchronizer.isExecuting(message.url));
         break;
     }
   });
 })(this);