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
@@ -72,10 +72,15 @@
 
   Collection.prototype.addItems = function()

[Thomas Greiner, 2016/08/04 15:54:02]

Note: We should really change this function to only accept a single argument
because that's the only way we're currently using it. That should make the
content of this function simpler and avoid this unnecessary duplication that we
now introduced.

Anyway, that can be tackled in a separate ticket. Just wanted to mention it.

[saroyanm, 2016/08/17 14:59:15]

Done.

   {
-    var length = Array.prototype.push.apply(this.items, arguments);
-    if (length == 0)
+    var itemsToAdd = [];
+    for (var i = 0; i < arguments.length; i++)

[Thomas Greiner, 2016/08/04 15:54:02]

Detail: I don't mind omitting the braces for a single line but, to avoid
confusion, please use them when the content stretches across multiple lines.

[saroyanm, 2016/08/17 14:59:15]

This was fixed during previous comment implementation

+      if (this.items.indexOf(arguments[i]) == -1)
+        itemsToAdd.push(arguments[i]);
+
+    if (itemsToAdd.length == 0)
       return;
 
+    Array.prototype.push.apply(this.items, itemsToAdd);
     this.items.sort(function(a, b)
     {
       // Make sure that Acceptable Ads is always last, since it cannot be
@@ -96,9 +101,9 @@
     {
       var table = E(this.details[j].id);
       var template = table.querySelector("template");
-      for (var i = 0; i < arguments.length; i++)
+      for (var i = 0; i < itemsToAdd.length; i++)
       {
-        var item = arguments[i];
+        var item = itemsToAdd[i];
         var listItem = document.createElement("li");
         listItem.appendChild(document.importNode(template.content, true));
         listItem.setAttribute("aria-label", this._getItemTitle(item, j));
@@ -335,20 +340,20 @@
     }
   ]);
 
-  function updateLanguageCollections(subscription)
+  function toggleShowLanguage(subscription)
   {
     if (subscription.recommended == "ads")
     {
-      if (subscription.disabled)
+      if (subscription.disabled == false)

[Thomas Greiner, 2016/08/04 15:54:03]

Detail: Do we need to explicitly check for `false`? Because that's the only
thing that's changed about this function when comparing it with the base
revision.

Also note that it's a double negation which would be great to avoid to make it
easier to understand.

[saroyanm, 2016/08/17 14:59:15]

This change was required because with latest changes we were also calling this
function during recommendation initialization. The problem is that when we are
creating recommendation object we are assigning "null" to the disabled status of
the recommendation, but I can't see any reason why this needs to be done that
way, so I changed to assign disable=true during initial initialization of the
recommendations.

+      {
+        collections.allLangs.removeItem(subscription);
+        collections.langs.addItems(subscription);
+      }
+      else
       {
         collections.allLangs.addItems(subscription);
         collections.langs.removeItem(subscription);
       }
-      else
-      {
-        collections.allLangs.removeItem(subscription);
-        collections.langs.addItems(subscription);
-      }
     }
   }
 
@@ -371,24 +376,16 @@
 
     collection.addItems(subscription);
     subscriptionsMap[subscription.url] = subscription;
+    toggleShowLanguage(subscription);
     updateTooltips();
   }
 
   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(subscription);
 
-    for (var name in collections)
-      collections[name].updateItem(knownSubscription);
-
-    return knownSubscription;
+    toggleShowLanguage(subscription);
   }
 
   function updateFilter(filter)
@@ -1026,11 +1023,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);
-        updateLanguageCollections(subscription);
+        updateSubscription(subscription);
         break;
       case "downloading":
       case "downloadStatus":
@@ -1041,15 +1049,13 @@
         break;
       case "added":
         if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+          updateSubscription(subscription);
         else
           addSubscription(subscription);
 
         collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
         if (subscription.url == acceptableAdsUrl || subscription.recommended)
         {
           subscription.disabled = true;
@@ -1057,10 +1063,10 @@
         }
         else
         {
-          collections.custom.removeItem(knownSubscription);
+          collections.custom.removeItem(subscription);
           delete subscriptionsMap[subscription.url];
         }
-        collections.filterLists.removeItem(knownSubscription);
+        collections.filterLists.removeItem(subscription);
         break;
     }