issue 1526 - Implement new options page design for Chrome/Opera/Safari

options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 (function()
 {
-  var acceptableAdsUrl = null;
   var subscriptionsMap = Object.create(null);
   var recommendationsMap = Object.create(null);
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
 
   function Collection(details)

[saroyanm, 2015/02/26 12:18:33]

We can rethink about the name of current Class in case.

[Thomas Greiner, 2015/03/05 11:36:03]

I guess "Collection" should be fine (see Java's Collection interface
http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html). Otherwise
"ItemList" would be another option we could go with in this case since its
methods are called "addItems" and "removeItem".

[Thomas Greiner, 2015/03/05 11:36:03]

Why "details"? It's an array of items and not a configuration object or
something similar.

[saroyanm, 2015/03/06 11:54:32]

I used details as I thought that it's corresponds to table details (tableId and
item's onclick listener).
Maybe naming it tablePreferences would be better ?
I think naming them items can make confusion, while we are adding items using
addItems and RemoveItem, what you think ?

[Thomas Greiner, 2015/03/12 11:41:56]

Makes sense, let's stick with "details" then.

   {
     this.details = details;
-  }
-
-  Collection.prototype = Object.create(Array.prototype);
+    this.items = [];
+  }
+
   Collection.prototype.addItems = function() 

[Thomas Greiner, 2015/03/05 11:36:03]

I couldn't find a case where you add multiple items at once so accepting one
parameter and renaming it to "addItem" would be sufficient.

[saroyanm, 2015/03/06 11:54:32]

I actually were thinking that this could be useful for upcoming patch when we
will implement lazy loading, anyway I can change this to only add one item for
now, let me know what you think about that.

[Thomas Greiner, 2015/03/12 11:41:56]

Ok, then let's leave it like it is for now (no need to add that parameter then)
to keep things simple.

[saroyanm, 2015/03/12 14:03:22]

Done.

   {
-    var length = Array.prototype.push.apply(this, arguments);
+    var length = Array.prototype.push.apply(this.items, arguments);
     if (length == 0)
       return;
 
-    this.sort(function(a, b)
+    this.items.sort(function(a, b)
     {
       var aValue = (a.title || a.url || a.text).toLowerCase();
       var bValue = (b.title || b.url || a.text).toLowerCase();
-      if (aValue < bValue)
-        return -1;
-      if (aValue > bValue)
-        return 1;
-      return 0;
+      return aValue.localeCompare(bValue);
     });
 
     for (var j = 0; j < this.details.length; j++) 
     {
       var table = E(this.details[j].id);
       var template = table.querySelector("template");
-      var listener = this.details[j].listener;
       for (var i = 0; i < arguments.length; i++) 
       {
-        var object = arguments[i];
-        var text = object.title || object.text;

[Thomas Greiner, 2015/03/05 11:36:03]

A subscription might not have a title. In those cases we should at least show
its URL.

-        var access = object.url || object.text;

[Thomas Greiner, 2015/03/05 11:36:03]

You're only using this variable once so no need to have a variable in the first
place. Same goes for "index".

[saroyanm, 2015/03/06 11:54:32]

Done, but now I'm using access, to shorten the line.

[Thomas Greiner, 2015/03/12 11:41:56]

That'd be ok but "index" is only used in that one if-case so  if you want to
have a separate "index" variable to make lines shorter then initialize it where
you're using it and not here.

[saroyanm, 2015/03/12 14:03:22]

This is already fixed in current patch:
http://codereview.adblockplus.org/6088024630755328/diff/5637036128075776/opti...

-        var index = this.indexOf(object);
+        var item = arguments[i];
+        var text = item.title || item.url || item.text;
         var listItem = document.createElement("li");
         listItem.appendChild(document.importNode(template.content, true));
-        listItem.dataset.access = access;

[Thomas Greiner, 2015/03/05 11:36:03]

You need to normalize this string or otherwise
`Element.querySelector("[data-access='" + access + "']")` will break for filters
or subscription titles that contain `'`.

Replacing `'` with `\'` should be sufficient but it needs to be done everywhere
you're constructing this access variable.

[saroyanm, 2015/03/06 11:54:32]

while current query throw error:
document.querySelector("[data-search='test\'.com']");

This works fine:
document.querySelector("[data-search='test\\'.com']");

But not sure if we can stick to that solution, what you think ?

[Thomas Greiner, 2015/03/12 11:41:56]

Should be fine.

Alternatively, you could use `encodeURIComponent` to normalize it. However, for
that you'd need to replace

table.querySelector("[data-access='" + access + "']")

with

table.querySelector('[data-access="' + access + '"]')

+        listItem.dataset.access = item.url || item.text;
         listItem.querySelector(".display").textContent = text;
         if (text)
           listItem.dataset.search = text.toLowerCase();
 
         var control = listItem.querySelector(".control");
         if (control)
         {
-          control.addEventListener("click", listener, false);

[Thomas Greiner, 2015/03/05 11:36:03]

Just FYI: I'd still prefer a single global event listener instead of adding a
listener to each control element individually but let's go with that for now.

-          control.checked = object.disabled == false;
+          control.addEventListener("click", this.details[j].onClick, false);
+          control.checked = item.disabled == false;
         }
 
-        var popular = listItem.querySelector(".popular");
-        if (popular)
-          popular.textContent = ext.i18n.getMessage("options_popular");

[Thomas Greiner, 2015/03/05 11:36:03]

Setting that text right inside the template tag once at startup should be
sufficient. No need to set it each time a new item is added to the list.

[saroyanm, 2015/03/06 11:54:32]

Done.

-
-        if (table.hasChildNodes)
-          table.insertBefore(listItem, table.childNodes[index]);
+        if (table.hasChildNodes())
+          table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
         else
           table.appendChild(listItem);
       }
     }
     return length;
   };
 
-  Collection.prototype.removeItem = function(obj)

[Thomas Greiner, 2015/03/05 11:36:03]

The function is called "removeItem" but the item is called "obj" and not "item".

[saroyanm, 2015/03/06 11:54:32]

Done.

-  {
-    var index = this.indexOf(obj);
+  Collection.prototype.removeItem = function(item)
+  {
+    var index = this.items.indexOf(item);
     if (index == -1)
       return;
 
-    this.splice(index, 1);
-    var access = obj.url || obj.text;
+    this.items.splice(index, 1);
+    var access = (item.url || item.text).replace(/'/g, "\\'");
     for (var i = 0; i < this.details.length; i++)
     {
       var table = E(this.details[i].id);
-      var element = table.querySelector("[data-access='"+access+"']");
+      var element = table.querySelector("[data-access='" + access + "']");
       element.parentElement.removeChild(element);
     }
   };
 
-  function toggleSubscription(e)
+  Collection.prototype.clearAll = function()
+  {
+    for (var i = 0; i < this.details.length; i++)
+    {
+      var table = E(this.details[i].id);
+      var template = table.querySelector("template");
+      table.innerHTML = "";
+      table.appendChild(template);
+    }
+    this.items.length = 0;
+  };
+
+  function onToggleSubscriptionClick(e)
   {
     e.preventDefault();
-    var isChecked = e.target.checked;

[Thomas Greiner, 2015/03/05 11:36:03]

Again, this variable is only used once.

[saroyanm, 2015/03/06 11:54:32]

Done.

     var subscriptionUrl = e.target.parentNode.dataset.access;
-    if (!isChecked)
+    if (!e.target.checked)
       removeSubscription(subscriptionUrl);
     else
       addEnableSubscription(subscriptionUrl);
   }
 
-  function addLanguageSubscription(e)
+  function onAddLanguageSubscriptionClick(e)
   {
     e.preventDefault();
     var url = this.parentNode.dataset.access;
     addEnableSubscription(url);
   }
 
-  function triggerRemoveFilter()
+  function onRemoveFilterClick()
   {
     var filter = this.parentNode.dataset.access;
     removeFilter(filter);
   }
 
   collections.popular = new Collection(
   [
     {
       id: "recommend-list-table",
-      listener: toggleSubscription

[Thomas Greiner, 2015/03/05 11:36:03]

I'd rename "listener" to "onClick". Otherwise it's not clear when that function
will be triggered.

[saroyanm, 2015/03/06 11:54:32]

Done.

+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.langs = new Collection(
   [
     {
       id: "blocking-languages-table",
-      listener: toggleSubscription
+      onClick: onToggleSubscriptionClick
     },
     {
-      id: "blocking-languages-modal-table"
+      id: "blocking-languages-dialog-table"
     }
   ]);
   collections.allLangs = new Collection(
   [
     {
       id: "all-lang-table", 
-      listener: addLanguageSubscription
+      onClick: onAddLanguageSubscriptionClick
     }
   ]);
   collections.acceptableAds = new Collection(
   [
     {
       id: "acceptableads-table", 
-      listener: toggleSubscription
+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.custom = new Collection(
   [
     {
       id: "custom-list-table", 
-      listener: toggleSubscription
+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.whitelist = new Collection(
   [
     {
       id: "whitelisting-table", 
-      listener: triggerRemoveFilter
+      onClick: onRemoveFilterClick
     }
   ]);
 
   function updateSubscription(subscription)
   {
     var subscriptionUrl = subscription.url;
-    var knownSubscription = getKnownSubscription(subscriptionUrl);
+    var knownSubscription = subscriptionsMap[subscriptionUrl];
     if (knownSubscription)
       knownSubscription.disabled = subscription.disabled;
     else
     {
       getAcceptableAdsURL(function(acceptableAdsUrl)
       {
         function onObjectChanged()
         {
-          var access = subscriptionUrl || subscription.text;
+          var access = (subscriptionUrl || subscription.text).replace(/'/g, "\\'");
           var elements = document.querySelectorAll("[data-access='" + access + "']");
           for (var i = 0; i < elements.length; i++)
           {
             var element = elements[i];
-            var control = element.getElementsByClassName("control")[0];

[Thomas Greiner, 2015/03/05 11:36:03]

Usually, you use `element.querySelector(".control")` for that.

[saroyanm, 2015/03/06 11:54:32]

Done.

+            var control = element.querySelector(".control");
             if (control.localName == "input")
               control.checked = subscription.disabled == false;
-            if (isRecommendation(subscriptionUrl))
+            if (subscriptionUrl in recommendationsMap)
             {
-              var recommendation = getRecommendation(subscriptionUrl);
-              if (recommendation.isAdsType && subscription.disabled == false)
+              var recommendation = recommendationsMap[subscriptionUrl];
+              if (recommendation.isAdsType)
               {
-                collections.allLangs.removeItem(subscription);
-                collections.langs.addItems(subscription);
-              }
-              else
-              {
-                collections.allLangs.addItems(subscription);
-                collections.langs.removeItem(subscription);
+                if (subscription.disabled == false)
+                {
+                  collections.allLangs.removeItem(subscription);
+                  collections.langs.addItems(subscription);
+                }
+                else
+                {
+                  collections.allLangs.addItems(subscription);
+                  collections.langs.removeItem(subscription);
+                }
               }
             }
           }
         }
 
         if (!Object.observe)
         {
+          // Currently only "disabled" property of subscription used for observation
+          // but with Advanced tab implementation we should also add more properties.
           ["disabled"].forEach(function(property)
           {
-            subscription["$"+property] = subscription[property];

[Thomas Greiner, 2015/03/05 11:36:03]

By now I'd expect you to know that operators, like `+` in this case, need to
have spaces around them.

[saroyanm, 2015/03/06 11:54:32]

Done.

+            subscription["$" + property] = subscription[property];
             Object.defineProperty(subscription, property,
             {
               get: function()
               {
-                return this["$"+property];
+                return this["$" + property];
               },
               set: function(value)
               {
-                this["$"+property] = value;
+                this["$" + property] = value;
                 onObjectChanged();
               }
             });
           });
         }
         else
         {
-          Object.observe(subscription, function(changes)
-          {
-            onObjectChanged();
-          });
+          Object.observe(subscription, onObjectChanged);
         }
 
         var collection = null;
-        if (isRecommendation(subscriptionUrl))
-        {
-          var recommendation = getRecommendation(subscriptionUrl);
+        if (subscriptionUrl in recommendationsMap)
+        {
+          var recommendation = recommendationsMap[subscriptionUrl];
           if (recommendation.isPopular)
             collection = collections.popular;
           else if (recommendation.isAdsType && subscription.disabled == false)
             collection = collections.langs;
           else
             collection = collections.allLangs;
         }
         else if (subscriptionUrl == acceptableAdsUrl)
           collection = collections.acceptableAds;
         else
           collection = collections.custom;
 
         collection.addItems(subscription);
-        setKnownSubscription(subscriptionUrl, subscription);
+        subscriptionsMap[subscriptionUrl] = subscription;
       });
     }
   }
 
   function updateFilter(filter)
   {
-    var knownFilter = getKnownFilter(filter.text);
     var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
-    if (match && !knownFilter)
+    if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItems(filter);
-      setKnownFilter(filter.text, filter);
+      filtersMap[filter.text] = filter
     }
     else
     {
       // TODO: add `filters[i].text` to list of custom filters
     }
   }
 
   function loadRecommendations()
   {
     var request = new XMLHttpRequest();
     request.open("GET", "subscriptions.xml", false);
-    request.onload = function()
+    request.addEventListener("load", function()
     {
       var list = document.getElementById("subscriptionSelector");
       var docElem = request.responseXML.documentElement;
       var elements = docElem.getElementsByTagName("subscription");
       for (var i = 0; i < elements.length; i++)
       {
         var element = elements[i];
         var subscription = Object.create(null);
         subscription.title = element.getAttribute("title");
         subscription.url = element.getAttribute("url");
@@ skipping 10 matching lines @@
           var prefix = element.getAttribute("prefixes").replace(/,/g, "_");
           subscription.title = ext.i18n.getMessage("options_language_" + prefix);
           recommendation.isAdsType = true;
         }
         else
           subscription.title = element.getAttribute("specialization");
         
         if (element.getAttribute("popular"))
           recommendation.isPopular = true;
 
-        setRecommendation(subscription.url, recommendation);
+        recommendationsMap[subscription.url] = recommendation;
         updateSubscription(subscription);
       }
-    };
+    }, false);
     request.send(null);
   }
 
   function onDOMLoaded()
   {
+    var recommendationTemplate = document.querySelector("#recommend-list-table template");
+    var popularText = ext.i18n.getMessage("options_popular");
+    recommendationTemplate.content.querySelector(".popular").textContent = popularText;
+    var languagesTemplate = document.querySelector("#all-lang-table template");
+    var buttonText = ext.i18n.getMessage("options_button_add");
+    languagesTemplate.content.querySelector(".button-add span").textContent = buttonText;
+
     updateShareLink();
     populateLists();
-    
+
     var tabList = document.querySelectorAll("#main-navigation-tabs li");
     for (var i = 0; i < tabList.length; i++)
     {
       tabList[i].addEventListener("click", function(e)
       {
         document.body.dataset.tab = e.currentTarget.dataset.show;
       }, false);
     }
-    
-    function searchLanguage()
+
+    function onFindLanguageKeyUp()
     {
       var searchStyle = E("search-style");
       if (!this.value)
         searchStyle.innerHTML = "";
       else
         searchStyle.innerHTML = "#all-lang-table li:not([data-search*=\"" + this.value.toLowerCase() + "\"]) { display: none; }";
     }
-    
+
     // Update version number in navigation sidebar
     ext.backgroundPage.sendMessage(
     {
       method: "app.get",
       what: "addonVersion"
     },
     function(addonVersion)
     {
       E("abp-version").textContent = addonVersion;
     });
     
-    var placeholderValue  = ext.i18n.getMessage("options_modal_language_find");
+    var placeholderValue  = ext.i18n.getMessage("options_dialog_language_find");
     E("find-language").setAttribute("placeholder", placeholderValue);
     E("add-blocking-list").addEventListener("click", function()
     {
-      openModal("customlist");
+      openDialog("customlist");
     }, false);
     E("add-website-language").addEventListener("click", function()
     {
-      openModal("language");
+      openDialog("language");
     }, false);
-    E("modal-close").addEventListener("click", function()
-    {
-      delete document.body.dataset.modal;
+    E("dialog-close").addEventListener("click", function()
+    {
+      delete document.body.dataset.dialog;
     }, false);
-    E("edit-ownBlockingList-btn").addEventListener("click", editCustomFilters, false);
-    E("find-language").addEventListener("keyup", searchLanguage, false);
-    E("find-language").addEventListener("search", searchLanguage, false);

[Thomas Greiner, 2015/03/05 11:36:03]

Turns out that the "onsearch" event listener is WebKit-only but seems like you
don't need it anyway so please remove this line.

[saroyanm, 2015/03/06 11:54:32]

I'm using this event to listen for "X" button click inside the input element of
type "search":
http://stackoverflow.com/questions/2977023/how-do-you-detect-the-clearing-of-...

[Thomas Greiner, 2015/03/12 11:41:56]

Since that "X" is also not in the standard I'd suggest hiding it since it also
doesn't appear in any of the options page designs.

Example:
input[type="search"]::-webkit-search-cancel-button {
  display: none;
}

According to the standard "the difference between the Text state and the Search
state is primarily stylistic" (see
http://www.w3.org/TR/html5/forms.html#text-(type=text)-state-and-search-state...
and also https://bugzilla.mozilla.org/show_bug.cgi?id=456229#c1).

[saroyanm, 2015/03/12 14:03:22]

Done.

-    E("whitelisting-add-icon").addEventListener("click", addWhitelistedDomain, false);
-    E("whitelisting-add-btn").addEventListener("click", addWhitelistedDomain, false);
-    E("whitelisting-enter-icon").addEventListener("click", addWhitelistedDomain, false);
+    E("edit-ownBlockingList-button").addEventListener("click", editCustomFilters, false);
+    E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
+    E("whitelisting").addEventListener("click", function(e)
+    {
+      var id = e.target.id;
+      if (id == "whitelisting-add-icon" || id == "whitelisting-enter-icon")
+        addWhitelistedDomain();
+      else if (id == "whitelisting-cancel-button")
+        E("whitelisting-textbox").value = "";
+    }, false);
+    E("whitelisting-add-button").addEventListener("click", addWhitelistedDomain, false);
     E("whitelisting-textbox").addEventListener("keypress", function(e)
     {
-      // keyCode value of 13 corresponds to "Enter" key

[Thomas Greiner, 2015/03/05 11:36:03]

Please also explain why you need to check for both "key" and "keyCode".

Example:

// KeyboardEvent.keyCode has been deprecated so we use KeyboardEvent.key instead
if it's available
if (...)
{
  ...
}
// keyCode value 13 corresponds to "Enter" key
else
{
  ...
}

[saroyanm, 2015/03/06 11:54:32]

Done.

-      if (e.key && e.key == "Enter")
-        addWhitelistedDomain();
-      else if (!e.key && e.keyCode == 13)
+      // e.keyCode has been deprecated so we attempt to use e.key
+      // keyCode "13" corresponds to "Enter"
+      if ((e.key && e.key == "Enter") || (!e.key && e.keyCode == 13))
         addWhitelistedDomain();
     }, false);
-    E("whitelisting-cancel-btn").addEventListener("click", function()
-    {
-      E("whitelisting-textbox").value = "";
-    }, false);
-    E("import-blockingList-btn").addEventListener("click", function()
+    E("import-blockingList-button").addEventListener("click", function()
     {
       var url = E("blockingList-textbox").value;
       addEnableSubscription(url);
-      delete document.body.dataset.modal;
+      delete document.body.dataset.dialog;
     }, false);
   }
 
-  function openModal(name)
-  {
-    document.body.dataset.modal = name;
+  function openDialog(name)
+  {
+    document.body.dataset.dialog = name;
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
     filtersMap = Object.create(null);
     recommendationsMap = Object.create(null);
 
     // Empty collections and lists
     for (var property in collections)
-    {
-      var collection = collections[property];
-      collection.details.forEach(function(detail)

[Thomas Greiner, 2015/03/05 11:36:03]

This property does not need to be exposed. It'd be cleaner to have a
`Collection.clearAll` method. Furthermore using a regular for-loop would work
just as fine.

[saroyanm, 2015/03/06 11:54:32]

Done.

-      {
-        var table = E(detail.id);
-        var template = table.querySelector("template");
-        table.innerHTML = "";
-        table.appendChild(template);
-      });
-      collection.length = 0;
-    }
+      collections[property].clearAll();
 
     ext.backgroundPage.sendMessage(
     {
       type: "subscriptions.get",
       special: true
     },
     function(subscriptions)
     {
       // Load filters
       for (var i = 0; i < subscriptions.length; i++)
@@ skipping 43 matching lines @@
         type: "filters.add",
         text: "@@||" + domain.value.toLowerCase() + "^$document"
       });
     }
 
     domain.value = "";
   }
 
   function editCustomFilters()
   {
-    
+    //TODO: NYI
   }
 
   function getAcceptableAdsURL(callback)
   {
-    if (acceptableAdsUrl)

[Thomas Greiner, 2015/03/05 11:36:03]

This if-statement is not necessary and neither is the "acceptableAdsUrl"
variable. You're replacing the function as soon as you have the value and from
then on only return the value directly.

[saroyanm, 2015/03/06 11:54:32]

Hmm :/ Totally missed your implementation, good point :)

-      callback(acceptableAdsUrl);
-    else
-    {
-      ext.backgroundPage.sendMessage(
-      {
-        type: "prefs.get",
-        key: "subscriptions_exceptionsurl"
-      },
-      function(value)
-      {
-        getAcceptableAdsURL = function(callback)
-        {
-          acceptableAdsUrl = value;
-          callback(value);
-        }
-        getAcceptableAdsURL(callback);
-      });
-    }
-  }
-
-  function getKnownSubscription(url)

[Thomas Greiner, 2015/03/05 11:36:03]

The next few functions are all one-liners. There's no added value in having
them.

[saroyanm, 2015/03/06 11:54:32]

Done.

-  {
-    return subscriptionsMap[url];
-  }
-
-  function setKnownSubscription(url, subscription)
-  {
-    subscriptionsMap[url] = subscription;
-  }
-
-  function getKnownFilter(text)
-  {
-    return filtersMap[text];
-  }
-
-  function setKnownFilter(text, filter)
-  {
-    filtersMap[text] = filter;
-  }
-
-  function getRecommendation(url)
-  {
-    return recommendationsMap[url];
-  }
-
-  function setRecommendation(url, recommendation)
-  {
-    recommendationsMap[url] = recommendation;
-  }
-
-  function isRecommendation(url)
-  {
-    return url in recommendationsMap;
+    ext.backgroundPage.sendMessage(
+    {
+      type: "prefs.get",
+      key: "subscriptions_exceptionsurl"
+    },
+    function(value)
+    {
+      getAcceptableAdsURL = function(callback)
+      {
+        callback(value);
+      }
+      getAcceptableAdsURL(callback);
+    });
   }
 
   function addEnableSubscription(url, title, homepage)
   {
     var messageType = null;
-    var knownSubscription = getKnownSubscription(url);
+    var knownSubscription = subscriptionsMap[url];
     if (knownSubscription && knownSubscription.disabled == true)
       messageType = "subscriptions.toggle"
     else
       messageType = "subscriptions.add"
 
     var message = {
       type: messageType,
       url: url
     };
     if (title)
@@ skipping 27 matching lines @@
     switch (action)
     {
       case "added":
         updateFilter(filter);
         updateShareLink();
         break;
       case "loaded":
         populateLists();
         break;
       case "removed":
-        var knownFilter = getKnownFilter(filter.text);
+        var knownFilter = filtersMap[filter.text];
         collections.whitelist.removeItem(knownFilter);
+        delete filtersMap[filter.text];
         updateShareLink();
         break;
     }
   }
 
   function onSubscriptionMessage(action, subscription)
   {
     switch (action)
     {
       case "added":
-        updateSubscription(subscription);
-        updateShareLink();
-        break;
       case "disabled":
         updateSubscription(subscription);
         updateShareLink();
         break;
       case "homepage":
         // TODO: NYI
         break;
       case "removed":
         getAcceptableAdsURL(function(acceptableAdsUrl)
         {
           if (subscription.url == acceptableAdsUrl)
           {
             subscription.disabled = true;
             updateSubscription(subscription);
           }
           else
           {
-            var knownSubscription = getKnownSubscription(subscription.url);
-            var subscriptionUrl = subscription.url;

[Thomas Greiner, 2015/03/05 11:36:03]

This variable is not necessary. It's only used once and it doesn't abstract any
complexity.

[saroyanm, 2015/03/06 11:54:32]

Done.

-            if (isRecommendation(subscriptionUrl))
+            var knownSubscription = subscriptionsMap[subscription.url];
+            if (subscription.url in recommendationsMap)
               knownSubscription.disabled = true;
             else
+            {
               collections.custom.removeItem(knownSubscription);
+              delete subscriptionsMap[subscription.url];
+            }
           }
           updateShareLink();
         });
         break;
       case "title":
         // TODO: NYI
         break;
     }
   }
 
   function showAddSubscriptionDialog(subscription)
   {
     E("blockingList-textbox").value = subscription.url;
-    openModal("customlist");
+    openDialog("customlist");
   }
 
   function updateShareLink()
   {
     ext.backgroundPage.sendMessage(
     {
       type: "filters.blocked",
       url: "https://platform.twitter.com/widgets/",
       requestType: "SCRIPT",
       docDomain: "adblockplus.org",
@@ skipping 38 matching lines @@
     filter: ["added", "loaded", "removed"]
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();