Issue 4156 - Adblocking filter only being removed in advanced tab fix

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -432,19 +432,8 @@
 
   function updateSubscription(subscription)
   {
-    var knownSubscription = subscriptionsMap[subscription.url];
-    for (var property in subscription)
-    {
-      if (property == "title" && subscription.recommended)
-        knownSubscription.originalTitle = subscription.title;
-      else
-        knownSubscription[property] = subscription[property];
-    }
-
     for (var name in collections)
-      collections[name].updateItem(knownSubscription);
-
-    return knownSubscription;
+      collections[name].updateItem(subscription);
   }
 
   function updateFilter(filter)
@@ -929,32 +918,42 @@
 
   function onSubscriptionMessage(action, subscription)
   {
+    var knownSubscription = subscriptionsMap[subscription.url];

[Thomas Greiner, 2016/06/16 11:09:33]

I'd prefer not having to deal with two different versions of the same
subscription so I'd propose something along this line:

if (subscription.url in subscriptionsMap)
{
  subscription = subscriptionsMap[subscription.url];
  ...
}

With that change the rest of the listener should be a bit cleaner since you no
longer have to think about which subscription object you're supposed to use.

[saroyanm, 2016/06/16 13:35:48]

Very good point!

+    if (knownSubscription)
+    {
+      for (var property in subscription)
+      {
+        if (property == "title" && knownSubscription.recommended)
+          knownSubscription.originalTitle = subscription.title;
+        else
+          knownSubscription[property] = subscription[property];
+      }
+    }
+    
     switch (action)
     {
       case "disabled":
-        subscription = updateSubscription(subscription);
-        updateLanguageCollections(subscription);
+        updateSubscription(knownSubscription);
+        updateLanguageCollections(knownSubscription);
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
-        updateSubscription(subscription);
+        updateSubscription(knownSubscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+        if (knownSubscription)
+          updateLanguageCollections(knownSubscription);
         else
           addSubscription(subscription);
 
-        collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
+        subscription = updateSubscription(knownSubscription || subscription);
+        collections.filterLists.addItems(knownSubscription || subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
-
-        if (subscription.url == acceptableAdsUrl || subscription.recommended)
+        if (subscription.url == acceptableAdsUrl || knownSubscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);