Issue 3915 - Properly handle special subscriptions

options.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 174 matching lines @@
   });
 
   // Load recommended subscriptions
   loadRecommendations();
 
   // Show user's filters
   reloadFilters();
 }
 $(loadOptions);
 
+function convertSpecialSubscription(subscription)
+{
+  getFilters(subscription.url, function(filters)
+  {
+    for (var j = 0; j < filters.length; j++)
+    {
+      var filter = filters[j].text;
+      if (whitelistedDomainRegexp.test(filter))
+        appendToListBox("excludedDomainsBox", RegExp.$1);

[Sebastian Noack, 2016/04/12 12:26:06]

RegExp.$* is deprecated. However, since this is code is matter to be replaced
with the new options page anyway, I leave it up to you whether to address this.

[kzar, 2016/04/12 12:31:49]

I would rather not mess with it. (Note this is not new code, I simply moved it
into a function.)

[Sebastian Noack, 2016/04/12 12:43:21]

I know that the logic here isn't new. But this would be a good opportunity to
clean it up. In particular since we have to test the whole options page anyway
after the recent changes. However, as this code is thrown away anyway when we
have the new options page, fine with me.

+      else
+        appendToListBox("userFiltersBox", filter);
+    }
+  });
+}
+
 // Reloads the displayed subscriptions and filters
 function reloadFilters()
 {
   // Load user filter URLs
   var container = document.getElementById("filterLists");
   while (container.lastChild)
     container.removeChild(container.lastChild);
 
   getSubscriptions(true, false, function(subscriptions)
   {
     for (var i = 0; i < subscriptions.length; i++)
     {
       var subscription = subscriptions[i];
       if (subscription.url == acceptableAdsUrl)
         $("#acceptableAds").prop("checked", !subscription.disabled);
       else
         addSubscriptionEntry(subscription);
     }
   });
 
   // User-entered filters
   getSubscriptions(false, true, function(subscriptions)
   {
     clearListBox("userFiltersBox");
     clearListBox("excludedDomainsBox");
 
     for (var i = 0; i < subscriptions.length; i++)
-      getFilters(subscriptions[i].url, function(filters)
-      {
-        for (var j = 0; j < filters.length; j++)
-        {
-          var filter = filters[j].text;
-          if (whitelistedDomainRegexp.test(filter))
-            appendToListBox("excludedDomainsBox", RegExp.$1);
-          else
-            appendToListBox("userFiltersBox", filter);
-        }
-      });
+      convertSpecialSubscription(subscriptions[i]);
   });
 }
 
 function initCheckbox(id, descriptor)
 {
   var checkbox = document.getElementById(id);
   var key = descriptor && descriptor.key || id;
   getPref(key, function(value)
   {
     if (descriptor && descriptor.get)
@@ skipping 219 matching lines @@
     case "disabled":
     case "downloading":
     case "downloadStatus":
     case "homepage":
     case "lastDownload":
     case "title":
       if (element)
         updateSubscriptionInfo(element, subscription);
       break;
     case "added":
-      if (subscription.url == acceptableAdsUrl)
+      if (subscription.special)

[Sebastian Noack, 2016/04/12 12:26:06]

If we add this property it should be isSpecial.

[kzar, 2016/04/12 12:31:49]

I disagree as this way it's consistent with the `subscriptions.get` message. I
guess let's leave this up to Thomas to decide.

[Sebastian Noack, 2016/04/12 12:43:21]

What does have the semantics of query parameters have to do with properties
describing an object? While speaking of consistency, also note that there is
already isDownloading.

[kzar, 2016/04/13 08:15:13]

Fair enough, I renamed these in the new review.

+        convertSpecialSubscription(subscription);
+      else if (subscription.url == acceptableAdsUrl)
         $("#acceptableAds").prop("checked", true);
       else if (!element)
         addSubscriptionEntry(subscription);
       break;
     case "removed":
       if (subscription.url == acceptableAdsUrl)
         $("#acceptableAds").prop("checked", false);
       else if (element)
         element.parentNode.removeChild(element);
       break;
@@ skipping 258 matching lines @@
       onFilterMessage(message.action, message.args[0]);
       break;
     case "prefs.respond":
       onPrefMessage(message.action, message.args[0]);
       break;
     case "subscriptions.respond":
       onSubscriptionMessage(message.action, message.args[0]);
       break;
   }
 });