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,10 +918,22 @@
 
   function onSubscriptionMessage(action, subscription)
   {
+    if (subscription.url in subscriptionsMap)
+    {
+      var knownSubscription = subscriptionsMap[subscription.url];
+      for (var property in subscription)
+      {
+        if (property == "title" && knownSubscription.recommended)
+          knownSubscription.originalTitle = subscription.title;
+        else
+          knownSubscription[property] = subscription[property];
+      }
+      subscription = knownSubscription;
+    }
     switch (action)
     {
       case "disabled":
-        subscription = updateSubscription(subscription);
+        updateSubscription(subscription);
         updateLanguageCollections(subscription);
         break;
       case "downloading":
@@ -943,17 +944,15 @@
         updateSubscription(subscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+        if (subscription)

[Thomas Greiner, 2016/06/16 16:10:34]

This will always be `true`. Compare it with the base revision and you'll find
that this check worked before.

[saroyanm, 2016/06/16 16:51:44]

Well spotted, thanks, Done.

+          updateLanguageCollections(subscription);
         else
           addSubscription(subscription);
 
+        updateSubscription(subscription);

[Thomas Greiner, 2016/06/16 16:10:34]

I noticed that those two function calls swapped places. How is that related to
the issue?

[saroyanm, 2016/06/16 16:51:44]

Logic here is bit confusing and yes swiping the function calls doesn't make any
easier or in this case sense.
I've update the implementation, so currently I'm checking if the Subscription is
of type=="ads" only then I'm calling the "updateLanguageCollections" method,
which we were calling for most of the subscriptions which are not type of "ads"
which making the logic confusing.

Let me know if current implementation makes more sense from your point of view
and if we can make keep this fix in this review as well, to make the code more
understandable.

         collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
-
         if (subscription.url == acceptableAdsUrl || subscription.recommended)
         {
           subscription.disabled = true;
@@ -961,10 +960,10 @@
         }
         else
         {
-          collections.custom.removeItem(knownSubscription);
+          collections.custom.removeItem(subscription);
           delete subscriptionsMap[subscription.url];
         }
-        collections.filterLists.removeItem(knownSubscription);
+        collections.filterLists.removeItem(subscription);
         break;
     }