issue 3741 - Add "remove" option to list items in new options page

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -302,7 +302,7 @@
     {
       id: "blocking-languages-table",
       emptyText: "options_dialog_language_added_empty",
-      onClick: toggleRemoveSubscription
+      onClick: toggleDisableSubscription

[saroyanm, 2016/03/23 16:34:38]

I think we do not need this listeners being specified on collections, I think we
can specify them in HTML data-action="". 
I can handle that in this in this review or separately, please let me know know
what you think.

[Thomas Greiner, 2016/05/23 12:43:25]

I'd be fine with either but if it's not too much effort to do it'd be great to
include it in this review.

[saroyanm, 2016/06/08 15:21:06]

Done.

     },
     {
       id: "blocking-languages-dialog-table",
@@ -328,7 +328,7 @@
   [
     {
       id: "custom-list-table",
-      onClick: toggleRemoveSubscription
+      onClick: toggleDisableSubscription
     }
   ]);
   collections.whitelist = new Collection(
@@ -360,23 +360,6 @@
     {
       for (var i = 0; i < change.length; i++)
       {
-        if (change[i].name == "disabled")

[saroyanm, 2016/03/23 16:34:38]

This implementation doesn't belong to here as well, we should have handled this
when subscription were added or removed, I think currently the implementation
should be more understandable.

[saroyanm, 2016/05/10 14:26:16]

Please ignore this change as well, as specified below.

-        {
-          var recommendation = recommendationsMap[subscription.url];
-          if (recommendation && recommendation.type == "ads")
-          {
-            if (subscription.disabled == false)
-            {
-              collections.allLangs.removeItem(subscription);
-              collections.langs.addItems(subscription);
-            }
-            else
-            {
-              collections.allLangs.addItems(subscription);
-              collections.langs.removeItem(subscription);
-            }
-          }
-        }
         for (var i in collections)
           collections[i].updateItem(subscription);
       }
@@ -423,31 +406,6 @@
         if (property != "title")
           knownSubscription[property] = subscription[property];
     }
-    else
-    {

[saroyanm, 2016/03/23 16:34:38]

This doesn't have anything to do with updateSubscription() function, this should
be done whenever we are adding a new subscription, so I've updated this as well
in scope of current review, I think now it's make more sense. 

[saroyanm, 2016/05/10 14:26:16]

The implementation was changed, I decided to introduce this change and the one
above separately in case needed, so please ignore this and comment above, it
will make the review quicker and rebasing as well.

-      observeSubscription(subscription);
-      getAcceptableAdsURL(function(acceptableAdsUrl)
-      {
-        var collection = null;
-        if (subscriptionUrl in recommendationsMap)
-        {
-          var recommendation = recommendationsMap[subscriptionUrl];
-          if (recommendation.type != "ads")
-            collection = collections.popular;
-          else if (subscription.disabled == false)
-            collection = collections.langs;
-          else
-            collection = collections.allLangs;
-        }
-        else if (subscriptionUrl == acceptableAdsUrl)
-          collection = collections.acceptableAds;
-        else
-          collection = collections.custom;
-
-        collection.addItems(subscription);
-        subscriptionsMap[subscriptionUrl] = subscription;
-      });
-    }
   }
 
   function updateFilter(filter)
@@ -482,7 +440,7 @@
           var subscription = Object.create(null);
           subscription.title = element.getAttribute("title");
           subscription.url = element.getAttribute("url");
-          subscription.disabled = null;
+          subscription.disabled = true;
           subscription.downloadStatus = null;
           subscription.homepage = null;
           var recommendation = Object.create(null);
@@ -499,8 +457,16 @@
             subscription.title = getMessage("common_feature_" + type + "_title");
           }
 
+          var collection = null;
+          if (recommendation.type != "ads")
+            collection = collections.popular;
+          else
+            collection = collections.allLangs;
+
+          collection.addItems(subscription);
           recommendationsMap[subscription.url] = recommendation;
-          updateSubscription(subscription);
+          subscriptionsMap[subscription.url] = subscription;
+          observeSubscription(subscription);
         }
       });
   }
@@ -834,7 +800,9 @@
       subscription.url = acceptableAdsUrl;
       subscription.disabled = true;
       subscription.title = getMessage("options_acceptableAds_description");
-      updateSubscription(subscription);
+      collections.acceptableAds.addItems(subscription);
+      subscriptionsMap[subscription.url] = subscription;
+      observeSubscription(subscription);
 
       // Load user subscriptions
       ext.backgroundPage.sendMessage(
@@ -934,14 +902,31 @@
     switch (action)
     {
       case "added":
-        updateSubscription(subscription);
-        updateShareLink();
+        getAcceptableAdsURL(function(acceptableAdsUrl)
+        {
+          updateSubscription(subscription);
+          if (subscriptionsMap[subscription.url])
+            subscription = subscriptionsMap[subscription.url];
+          else
+            subscriptionsMap[subscription.url] = subscription;
 
-        var knownSubscription = subscriptionsMap[subscription.url];
-        if (knownSubscription)
-          collections.filterLists.addItems(knownSubscription);
-        else
+          var recommendation = recommendationsMap[subscription.url];
+          if (recommendation)
+          {
+            if (recommendation.type == "ads")
+            {
+              collections.langs.addItems(subscription);
+              collections.allLangs.removeItem(subscription);
+            }
+          }
+          else if (subscription.url != acceptableAdsUrl)
+          {
+            collections.custom.addItems(subscription);
+            observeSubscription(subscription);
+          }
           collections.filterLists.addItems(subscription);
+          updateShareLink();
+        });
         break;
       case "disabled":
         updateSubscription(subscription);
@@ -954,26 +939,31 @@
         // TODO: NYI
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
+        subscription = subscriptionsMap[subscription.url];
         getAcceptableAdsURL(function(acceptableAdsUrl)
         {
+          collections.filterLists.removeItem(subscription);
           if (subscription.url == acceptableAdsUrl)
           {
             subscription.disabled = true;
-            updateSubscription(subscription);
           }
           else
           {
-            if (subscription.url in recommendationsMap)
-              knownSubscription.disabled = true;
+            var recommendation = recommendationsMap[subscription.url];
+            if (recommendation && recommendation.type == "ads")
+            {
+              collections.allLangs.addItems(subscription);
+              collections.langs.removeItem(subscription);
+            }
+            else if (recommendation)
+              subscription.disabled = true;
             else
             {
-              collections.custom.removeItem(knownSubscription);
+              collections.custom.removeItem(subscription);
               delete subscriptionsMap[subscription.url];
             }
           }
           updateShareLink();
-          collections.filterLists.removeItem(knownSubscription);
         });
         break;
       case "title":