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 optionSubscriptions = {};
-  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)
+  {
+    this.details = details;
+    this.items = [];
+  }
+
+  Collection.prototype.addItems = function() 
+  {
+    var length = Array.prototype.push.apply(this.items, arguments);
+    if (length == 0)
+      return;
+
+    this.items.sort(function(a, b)
+    {
+      var aValue = (a.title || a.url || a.text).toLowerCase();
+      var bValue = (b.title || b.url || a.text).toLowerCase();
+      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");
+      for (var i = 0; i < arguments.length; i++) 
+      {
+        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 = 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", this.details[j].onClick, false);
+          control.checked = item.disabled == false;
+        }
+
+        if (table.hasChildNodes())
+          table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
+        else
+          table.appendChild(listItem);
+      }
+    }
+    return length;
+  };
+
+  Collection.prototype.removeItem = function(item)
+  {
+    var index = this.items.indexOf(item);
+    if (index == -1)
+      return;
+
+    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 + "']");
+      element.parentElement.removeChild(element);
+    }
+  };
+
+  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 subscriptionUrl = e.target.parentNode.dataset.access;
+    if (!e.target.checked)
+      removeSubscription(subscriptionUrl);
+    else
+      addEnableSubscription(subscriptionUrl);
+  }
+
+  function onAddLanguageSubscriptionClick(e)
+  {
+    e.preventDefault();
+    var url = this.parentNode.dataset.access;
+    addEnableSubscription(url);
+  }
+
+  function onRemoveFilterClick()
+  {
+    var filter = this.parentNode.dataset.access;
+    removeFilter(filter);
+  }
+
+  collections.popular = new Collection(
+  [
+    {
+      id: "recommend-list-table",
+      onClick: onToggleSubscriptionClick
+    }
+  ]);
+  collections.langs = new Collection(
+  [
+    {
+      id: "blocking-languages-table",
+      onClick: onToggleSubscriptionClick
+    },
+    {
+      id: "blocking-languages-dialog-table"
+    }
+  ]);
+  collections.allLangs = new Collection(
+  [
+    {
+      id: "all-lang-table", 
+      onClick: onAddLanguageSubscriptionClick
+    }
+  ]);
+  collections.acceptableAds = new Collection(
+  [
+    {
+      id: "acceptableads-table", 
+      onClick: onToggleSubscriptionClick
+    }
+  ]);
+  collections.custom = new Collection(
+  [
+    {
+      id: "custom-list-table", 
+      onClick: onToggleSubscriptionClick
+    }
+  ]);
+  collections.whitelist = new Collection(
+  [
+    {
+      id: "whitelisting-table", 
+      onClick: onRemoveFilterClick
+    }
+  ]);
+
+  function updateSubscription(subscription)
+  {
+    var subscriptionUrl = subscription.url;
+    var knownSubscription = subscriptionsMap[subscriptionUrl];
+    if (knownSubscription)
+      knownSubscription.disabled = subscription.disabled;
+    else
+    {
+      getAcceptableAdsURL(function(acceptableAdsUrl)
+      {
+        function onObjectChanged()
+        {
+          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.querySelector(".control");
+            if (control.localName == "input")
+              control.checked = subscription.disabled == false;
+            if (subscriptionUrl in recommendationsMap)
+            {
+              var recommendation = recommendationsMap[subscriptionUrl];
+              if (recommendation.isAdsType)
+              {
+                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];
+            Object.defineProperty(subscription, property,
+            {
+              get: function()
+              {
+                return this["$" + property];
+              },
+              set: function(value)
+              {
+                this["$" + property] = value;
+                onObjectChanged();
+              }
+            });
+          });
+        }
+        else
+        {
+          Object.observe(subscription, onObjectChanged);
+        }
+
+        var collection = null;
+        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);
+        subscriptionsMap[subscriptionUrl] = subscription;
+      });
+    }
+  }
+
+  function updateFilter(filter)
+  {
+    var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
+    if (match && !filtersMap[filter.text])
+    {
+      filter.title = match[1];
+      collections.whitelist.addItems(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.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");
+        subscription.disabled = null;
+        subscription.downloadStatus = null;
+        subscription.homepage = null;
+        subscription.lastSuccess = null;
+        var recommendation = Object.create(null);
+        recommendation.isAdsType = false;
+        recommendation.isPopular = false;
+        var prefix = element.getAttribute("prefixes");
+        if (prefix)
+        {
+          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;
+
+        recommendationsMap[subscription.url] = recommendation;
+        updateSubscription(subscription);
+      }
+    }, false);
+    request.send(null);
+  }
+
   function onDOMLoaded()
   {
-    initTabs();
-    updateVersionNumber();
+    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 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;
+    });
     
-    E("find-language").setAttribute("placeholder", ext.i18n.getMessage("options_modal_language_find"));
-    setLinks("block-element-explanation", "#");
-    
-    E("add-blocking-list").addEventListener("click", Modal.open, false);
-    E("add-website-language").addEventListener("click", Modal.open, false);
-    E("modal-close").addEventListener("click", Modal.close, false);
-    E("whitelisting-add-icon").addEventListener("click", whitelistDomainBtnClick, false);
-    E("whitelisting-add-btn").addEventListener("click", whitelistDomainBtnClick, false);
-    E("whitelisting-enter-icon").addEventListener("click", whitelistDomainBtnClick, false);
-    E("whitelisting-textbox").addEventListener("keypress", function(e) {
-      if (e.keyCode == 13)
-        whitelistDomainBtnClick();
+    var placeholderValue  = ext.i18n.getMessage("options_dialog_language_find");
+    E("find-language").setAttribute("placeholder", placeholderValue);
+    E("add-blocking-list").addEventListener("click", function()
+    {
+      openDialog("customlist");
     }, false);
-    E("whitelisting-cancel-btn").addEventListener("click", function(){
-      E("whitelisting-textbox").value = "";
+    E("add-website-language").addEventListener("click", function()
+    {
+      openDialog("language");
     }, false);
-    E("allow-whitelist-cb").addEventListener("click", toggleAcceptableAds, false);
-    E("import-blockingList-btn").addEventListener("click", importListBtnCLick, false);
-    E("edit-ownBlockingList-btn").addEventListener("click", editOwnRulsBtnClick, false);
-    E("find-language").addEventListener("keyup", searchLanguage, false);
-  }
-  
-  function initTabs()
-  {
-    var showContent = function(tab)
-    {
-      var tab = tab.querySelector(".tabs li.active");
-      if (tab.dataset.show)
-        E(tab.dataset.show).classList.add("active");

[Thomas Greiner, 2015/01/30 13:55:52]

Using CSS classes instead of modifying the style attribute is already an
improvement but here's an example of what I meant:

When clicking on the "General" tab you could set the "data-tab" attribute of the
body element to "general" to show the content of the general tab. You can
achieve that using CSS to hide elements that should not be shown in the general
tab, show those that should be shown and highlight the tab in the sidebar.

e.g.

body[data-tab="general"] #tab-general {
  ...
}

body[data-tab="general"] #content-general {
  ...
}

Using that technique you could simply write `document.body.dataset.tab =
"general";` instead of the entire `initTabs()` function.

[saroyanm, 2015/02/13 10:57:11]

Yes, good point.

-    };
-    var optionList = document.querySelectorAll('.tabs li[data-show]');
-    for (var i = 0; i < optionList.length; ++i)
-    {
-      optionList[i].addEventListener("click", function(ev)
-      {
-        var tab = this.parentNode.querySelector(".active");
-        tab.classList.remove("active");
-        this.classList.add("active");
-        E(tab.dataset.show).classList.remove("active");
-        showContent(this.parentNode);
-      }, false);
-    }
-    showContent(E("main-navigation-tabs"));
-    showContent(E("blocking-list-tabs"));
-  }
-  
-  var Modal =
-  {
-    open: function (content)
-    {
-      var modal = E("modal");
-      var content = E(this && this.dataset ? this.dataset.show : content);
-      content.classList.add("active");
-      document.body.classList.add("modal-active");

[Thomas Greiner, 2015/01/30 13:55:52]

You could get rid of this entire function by setting the "data-modal" attribute
on the body by using the same technique I outlined above for `initTabs()`.

e.g.

To show the "add-subscription" dialog: `document.body.dataset.modal =
"add-subscription"`
To hide the dialog: `delete document.body.dataset.modal`

[saroyanm, 2015/02/13 10:57:11]

Done.

-      if (content.dataset.title)
-        E("modal-title").innerHTML = ext.i18n.getMessage(content.dataset.title);
-      modal.style.marginTop = -(modal.clientHeight/2)+"px";
-    },
-    close: function ()
-    {
-      var contents = E("modal-content").childNodes;
-      for (var i = 0; i < contents.length; ++i)
-      {
-        if (contents[i].style)
-          contents[i].classList.remove("active");
-      }
-      document.body.classList.remove("modal-active");
-    }
-  }
-  
+    E("dialog-close").addEventListener("click", function()
+    {
+      delete document.body.dataset.dialog;
+    }, 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)
+    {
+      // 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("import-blockingList-button").addEventListener("click", function()
+    {
+      var url = E("blockingList-textbox").value;
+      addEnableSubscription(url);
+      delete document.body.dataset.dialog;
+    }, false);
+  }
+
+  function openDialog(name)
+  {
+    document.body.dataset.dialog = name;
+  }
+
   function populateLists()
   {
-    ext.backgroundPage.sendMessage({
+    subscriptionsMap = Object.create(null);
+    filtersMap = Object.create(null);
+    recommendationsMap = Object.create(null);
+
+    // Empty collections and lists
+    for (var property in collections)
+      collections[property].clearAll();
+
+    ext.backgroundPage.sendMessage(
+    {
       type: "subscriptions.get",
       special: true
-    }, function(subscriptions)
-    {
+    },
+    function(subscriptions)
+    {
+      // Load filters
       for (var i = 0; i < subscriptions.length; i++)
       {
-        ext.backgroundPage.sendMessage({
+        ext.backgroundPage.sendMessage(
+        {
           type: "filters.get",
           subscriptionUrl: subscriptions[i].url
-        }, function(filters)
-        {
-          var whitelistArray = [];
+        }, 
+        function(filters)
+        {
           for (var i = 0; i < filters.length; i++)
-          {
-            var match = filters[i].text.match(/^@@\|\|([^\/:]+)\^\$document$/);
-            if (match[1])
-            {
-              whitelistArray.push(match[1]);
-            }
-            else
-            {
-              // TODO: add `filters[i].text` to list of custom filters
-            }
-          }
-          
-          if (whitelistArray.length > 0)
-          {
-            whitelistArray.sort();
-            for (var i = 0; i < whitelistArray.length; i++)
-            {
-              var domain = whitelistArray[i];
-              E("whitelisting-table").appendChild(createWhitelistElem(domain));
-            }
-          }
+            updateFilter(filters[i]);
         });
       }
     });
-    
-    loadRecommendations(function(recommends)
-    {
-      ext.backgroundPage.sendMessage({
+    loadRecommendations();
+    getAcceptableAdsURL(function(acceptableAdsUrl)
+    {
+      var subscription = Object.create(null);
+      subscription.url = acceptableAdsUrl;
+      subscription.disabled = true;
+      subscription.title = ext.i18n.getMessage("options_acceptableAds_description");
+      updateSubscription(subscription);
+
+      // Load user subscriptions
+      ext.backgroundPage.sendMessage(
+      {
         type: "subscriptions.get",
         downloadable: true
-      }, function(subscriptions)
-      {
-        getAcceptableAdsURL(function(url)
-        {
-          acceptableAdsUrl = url;
-          for (var i = 0; i < subscriptions.length; i++)
-          {
-            if (subscriptions[i].url == acceptableAdsUrl)
-            {
-              E("allow-whitelist-cb").checked = !subscriptions[i].disabled;
-              continue;
-            }
- 
-            var subscription = recommends[subscriptions[i].url];
-            if (!subscription)
-              recommends[subscriptions[i].url] = subscriptions[i];
-            else
-            {
-              subscription.disabled = subscriptions[i].disabled;
-              if (subscription.type == "ads")
-                subscription.isAdded = true;
-            }
-          }
-          for (var key in recommends)
-            addOptionItem(recommends[key]);
-        });
+      }, 
+      function(subscriptions)
+      {
+        for (var i = 0; i < subscriptions.length; i++)
+          onSubscriptionMessage("added", subscriptions[i]);
       });
     });
   }
-  
-  function loadRecommendations(callback)
-  {
-    var recommendations = {};
-    var request = new XMLHttpRequest();
-    request.open("GET", "subscriptions.xml");

[Thomas Greiner, 2015/01/30 13:55:52]

This function call is missing the third parameter which indicates whether it
should be a synchronous or asynchronous XHR.

[saroyanm, 2015/02/13 10:57:11]

Done.

-    request.onload = function()
-    {
-      var list = document.getElementById("subscriptionSelector");
-      var elements = request.responseXML.documentElement.getElementsByTagName("subscription");
-      for (var i = 0; i < elements.length; i++)
-      {
-        var element = elements[i];
-        var subscription = {};

[Thomas Greiner, 2015/01/30 13:55:52]

Use `var subscription = Object.create(null);` instead since this object doesn't
need a prototype.

[saroyanm, 2015/02/13 10:57:11]

Done.

-        subscription.title = element.getAttribute("title");
-        subscription.url = element.getAttribute("url");
-        subscription.disabled = true;
-        var prefix = element.getAttribute("prefixes");
-        if (prefix)
-        {
-          subscription.prefixes = element.getAttribute("prefixes");
-          subscription.type = "ads";
-          subscription.display = ext.i18n.getMessage("options_language_"+subscription.prefixes.replace(/,/g, '_'));

[Thomas Greiner, 2015/01/30 13:55:52]

This line doesn't need to be that long. I'd suggest splitting it up into two
lines:

prefix = prefixes.replace(/,/g, "_");
subscription.display = ext.i18n.getMessage("subscriptions_langauge_" + prefix);

[saroyanm, 2015/02/13 10:57:11]

Done.

-        }
-        else
-          subscription.display = element.getAttribute("specialization");
-        
-        var popular = element.getAttribute("popular");

[Thomas Greiner, 2015/01/30 13:55:52]

It's safer to convert this string into a boolean.

[saroyanm, 2015/02/13 10:57:11]

Done.

-        if (popular)
-          subscription.popular = element.getAttribute("popular");
-        
-        recommendations[subscription.url] = subscription;
-      }
-      optionSubscriptions = recommendations;
-      callback(recommendations);
-    }
-    request.send();
-  }
-  
-  function searchLanguage()
-  {
-    var searchVal = this.value;
-    var items = E("all-lang-table").childNodes;
-    for (var i = 0; i < items.length; ++i)
-    {
-      var item = items[i];
-      var language = item.getElementsByTagName("span")[1].innerHTML;

[Thomas Greiner, 2015/01/30 13:55:52]

Don't rely on the content of an element because it might change (e.g. if we
decide to add child nodes). Rather use an approach similar to this one:
http://www.redotheweb.com/2013/05/15/client-side-full-text-search-in-css.html

-      if (language.toLowerCase().indexOf(searchVal.toLowerCase()) > -1)
-        item.style.display = "block";
-      else
-        item.style.display = "none";
-    }
-  }
-  
-  function addOptionItem(subscription)
-  {
-    var display = subscription.display ? subscription.display : subscription.title;

[Thomas Greiner, 2015/01/30 13:55:52]

You can reduce this down to `var display = subscriptions.display ||
subscription.title;`

-    var getPossition = function(elements, subscription)

[Thomas Greiner, 2015/01/30 13:55:52]

This variable name is spelled incorrectly.

[Thomas Greiner, 2015/01/30 13:55:52]

You were able to use a much nicer approach for populating the list of
whitelisted domains by using the data structure to sort the elements instead of
going through the DOM.

[saroyanm, 2015/02/13 10:57:11]

Changed to use arrays for sorting.

-    {
-      var localArray = [];
-      for (var i = 0; i < elements.length; i++)
-      {
-        var elem = elements[i];
-        localArray.push(elem);
-      }
-      
-      localArray.push(subscription);
-      return localArray.sort(function(a, b) {
-        var aPopular = a.getElementsByClassName("popular").length > 0;
-        var bPopular = b.getElementsByClassName("popular").length > 0;
-        if(aPopular == bPopular)
-        {
-          var aValue = a.getElementsByClassName("display")[0].innerHTML.toLowerCase();
-          var bValue = b.getElementsByClassName("display")[0].innerHTML.toLowerCase();
-          if (aValue < bValue)
-            return -1;
-          if (aValue > bValue)
-            return 1;
-          return 0;
-        }
-        if (aPopular == "true")
-          return 1;
-        else
-          return -1;
-      }).indexOf(subscription);
-    };
-    
-    var checkBoxClick = function(e)
-    {
-      e.preventDefault();
-      toggleSubscription(subscription);
-    };
-    
-    var appendToTable = function(table, elem)
-    {
-      var elements = table.getElementsByTagName("li");
-      if (elements.length == 0)
-        table.appendChild(elem);
-      else
-      {
-        var possition = getPossition(elements, elem);

[Thomas Greiner, 2015/01/30 13:55:52]

This variable name is spelled incorrectly.

-        table.insertBefore(elem, table.childNodes[possition]);
-      }
-    };
-    
-    if (subscription.type && subscription.type == "ads")
-    {
-      if (!subscription.isAdded)
-      {
-        var listElem = generateListElement(subscription, subscription.display, "add");
-        listElem.dataset.url = subscription.url;
-        listElem._subscription = subscription;
-        listElem.getElementsByTagName("button")[0].addEventListener("click", function(e)
-        {
-          addSubscription(this.dataset.url);
-        }.bind(listElem), false);
-        appendToTable(E("all-lang-table"), listElem);
-      }
-      else
-      {
-        var listElem = generateListElement(subscription, display, "checkbox");
-        listElem.dataset.url = subscription.url;
-        listElem._subscription = subscription;
-        listElem.getElementsByTagName("input")[0].addEventListener("click", checkBoxClick, false);
-        appendToTable(E("blocking-languages-table"), listElem);
-        var listElem = generateListElement(subscription, display);
-        listElem.dataset.url = subscription.url;
-        listElem._subscription = subscription;
-        appendToTable(E("blocking-languages-modal-table"), listElem);
-      }
-    }
-    else
-    {
-      var listElem = generateListElement(subscription, display, "checkbox");
-      listElem.dataset.url = subscription.url;
-      listElem._subscription = subscription;
-      listElem.getElementsByTagName("input")[0].addEventListener("click", checkBoxClick, false);
-      appendToTable(E("further-list-table"), listElem);
-    }
-  }
-  
-  function addLanguageSubscription(subscription)
-  {
-    var optionSubscription = getOptionSubscription(subscription.url);
-    var elems = getElementsByUrl(subscription.url);
-    for (var i = 0; i < elems.length; i++)

[Thomas Greiner, 2015/01/30 13:55:52]

Why are you removing elements when adding a subscription? The list of language
subscriptions in the dialog can be populated everytime you load it.

[saroyanm, 2015/02/13 10:57:11]

The item will stay there when it's added while we don't need that item in all
languages list anymore not to confuse the user.

-      elems[i].parentNode.removeChild(elems[i]);
-    optionSubscription.isAdded = true;
-    optionSubscription.disabled = false;
-    addOptionItem(optionSubscription);
-  }
-  
-  function createWhitelistElem(domain)
-  {
-    var listElem = generateListElement(null, domain, "delete");
-    listElem.dataset.domain = domain;
-    listElem.getElementsByTagName("button")[0].addEventListener("click", removeWhitelistBtnClick.bind(listElem), false);
-    return listElem;
-  }
-  
-  function addFurtherList(subscription)

[Thomas Greiner, 2015/01/30 13:55:52]

We tend to call them "custom subscriptions" in our code. "Further blocking
lists" is used only in the texts.

[saroyanm, 2015/02/13 10:57:11]

Done.

-  {
-    var optionSubscription = getOptionSubscription(subscription.url);
-    if (optionSubscription)
-    {
-      optionSubscription.disabled = false;
-      addOptionItem(optionSubscription);
-    }
-    else
-    {
-      optionSubscriptions[subscription.url] = subscription;
-      addOptionItem(subscription);
-    }
-  }
-  
-  function updateSubscriptionState(subscription, state)
-  {
-    var elem = getElementsByUrl(subscription.url);
-    if (elem.length > 0)
-    {
-      for (var i = 0; i < elem.length; i++)

[Thomas Greiner, 2015/01/30 13:55:52]

A subscription should only be shown once on the page, right?

[saroyanm, 2015/02/13 10:57:11]

if you mean tab, then Language subscriptions shown in modal also.

-      {
-        var checkbox = elem[i].getElementsByTagName("input")[0];
-        if (checkbox)
-          checkbox.checked = state;

[Thomas Greiner, 2015/01/30 13:55:52]

Might make sense to implement some simple data-binding for that to make sure
that model and view are always kept in sync. Unfortunately, `Object.observe()`
is not widely supported yet so we'd have to implement that ourselves.

I'd like to hear Wladimir's thoughts on that though.

-      }
-    }
-    else
-    {
-      if (subscription.url == acceptableAdsUrl)
-        E("allow-whitelist-cb").checked = state;
-      else
-        addFurtherList(subscription);
-    }
-  }
-  
-  function getElementsByUrl(url)
-  {
-    return document.querySelectorAll("[data-url='"+url+"']");
-  }
-  
-  function generateListElement(subscription, text, type)
-  {
-    var list = document.createElement("li");
-    if (type == "checkbox")
-    {
-      var input = document.createElement("input");
-      input.setAttribute("type", "checkbox");
-      if (subscription.disabled == false)
-        input.checked = true;
-      list.appendChild(input);
-    }
-    else if (type == "delete")
-    {
-      var button = document.createElement("button");
-      button.setAttribute("class", "delete");
-      list.appendChild(button);
-    }
-    else if (type == "add")
-    {
-      var button = document.createElement("button");
-      button.setAttribute("class", "button-add");
-      var span = document.createElement("span");
-      span.innerHTML = "+" + ext.i18n.getMessage("options_btn_add");
-      button.appendChild(span);
-      list.appendChild(button);
-    }
-    var span = document.createElement("span");
-    span.setAttribute("class", "display");
-    span.innerHTML = text;
-    list.appendChild(span);
-    
-    if (subscription && subscription.popular == "true")
-    {
-      var popular = document.createElement("span");
-      popular.setAttribute("class", "popular");
-      popular.innerHTML = "popular";
-      list.appendChild(popular);
-    }
-    
-    return list;
-  }
-  
-  function getOptionSubscription(url)
-  {
-    return optionSubscriptions[url];
-  }
-
-  function importListBtnCLick()
-  {
-    var url = E("blockingList-textbox").value;
-    addSubscription(url);
-    Modal.close();
-  }
-  
-  function whitelistDomainBtnClick()
-  {
-    var domain = E("whitelisting-textbox").value;
-    if (domain)
-      addWhitelistedDomain(domain);
-  }
-  
-  function removeWhitelistBtnClick()
-  {
-    removeWhitelistedDomain(this.dataset.domain);
-  }
-  
-  function editOwnRulsBtnClick()
-  {
-    
-  }
-  
-  function showAddSubscriptionDialog(action, subscription)

[Thomas Greiner, 2015/01/30 13:55:52]

The `action` parameter is unused.

[saroyanm, 2015/02/13 10:57:11]

Done.

-  {
-    E("blockingList-textbox").value = subscription.url;
-    Modal.open("further-blocking-modal");
-  }
-  
+
+  function addWhitelistedDomain()
+  {
+    var domain = E("whitelisting-textbox");
+    if (domain.value)
+    {
+      ext.backgroundPage.sendMessage(
+      {
+        type: "filters.add",
+        text: "@@||" + domain.value.toLowerCase() + "^$document"
+      });
+    }
+
+    domain.value = "";
+  }
+
+  function editCustomFilters()
+  {
+    //TODO: NYI
+  }
+
   function getAcceptableAdsURL(callback)
   {
-    ext.backgroundPage.sendMessage({
+    ext.backgroundPage.sendMessage(
+    {
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
-    }, function(value)
+    },
+    function(value)
     {
       getAcceptableAdsURL = function(callback)
       {
         callback(value);
       }
       getAcceptableAdsURL(callback);
     });
   }
 
-  function toggleSubscription(subscription)
-  {
-    ext.backgroundPage.sendMessage({
-      type: "subscriptions.toggle",
-      url: subscription.url,
-      title: subscription.title,
-      homepage: subscription.homepage
-    });
-  }
-
-  function toggleAcceptableAds(e)
-  {
-    e.preventDefault();
-    var acceptableCheckbox = this;

[Thomas Greiner, 2015/01/30 13:55:52]

This variable is not necessary. You can access the element on which the event
was dispatched on using the event's `target` property.

-    getAcceptableAdsURL(function(url)
-    {
-      var isChecked = !acceptableCheckbox.checked;
-      var title = "Allow non-intrusive advertising";

[Thomas Greiner, 2015/01/30 13:55:52]

Those two values are only used once so no need to define variables for them.

if (!ev.target.checked)
  removeSubscription(url);
else
  addSubscription(url, "Allow non-intrusive advertising");

-      if (isChecked)
-        removeSubscription(url);
-      else
-        addSubscription(url, title);
-    });
-  }
-
-  function addSubscription(url, title, homepage)
-  {
+  function addEnableSubscription(url, title, homepage)
+  {
+    var messageType = null;
+    var knownSubscription = subscriptionsMap[url];
+    if (knownSubscription && knownSubscription.disabled == true)
+      messageType = "subscriptions.toggle"
+    else
+      messageType = "subscriptions.add"
+
     var message = {
-      type: "subscriptions.add",
+      type: messageType,
       url: url
     };
     if (title)
       message.title = title;
     if (homepage)
       message.homepage = homepage;
 
     ext.backgroundPage.sendMessage(message);
   }
 
   function removeSubscription(url)
   {
-    ext.backgroundPage.sendMessage({
+    ext.backgroundPage.sendMessage(
+    {
       type: "subscriptions.remove",
       url: url
     });
   }
 
-  function addWhitelistedDomain(domain)
-  {
-    ext.backgroundPage.sendMessage({
-      type: "filters.add",
-      text: "@@||" + domain.toLowerCase() + "^$document"
-    });
-  }
-
-  function removeWhitelistedDomain(domain)
-  {
-    ext.backgroundPage.sendMessage({
+  function removeFilter(filter)
+  {
+    ext.backgroundPage.sendMessage(
+    {
       type: "filters.remove",
-      text: "@@||" + domain.toLowerCase() + "^$document"
+      text: filter
     });
   }
 
   function onFilterMessage(action, filter)
   {
     switch (action)
     {
       case "added":
-        var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
-        if (match[1])

[Thomas Greiner, 2015/01/30 13:55:52]

`match` could be null which can cause an exception to be thrown here. Therefore
`if (match)` would be a safer approach.

[saroyanm, 2015/02/13 10:57:11]

Done.

-        {
-          var whitelistTbl =  E("whitelisting-table");
-          var items = whitelistTbl.getElementsByClassName("display");
-          var domains = [];
-          for (var i = 0; i < items.length; i++)
-          {
-            domains.push(items[i].innerHTML);

[Thomas Greiner, 2015/01/30 13:55:52]

The content of an HTML element might change so better not to rely on that and
take the data directly from an array.

[saroyanm, 2015/02/13 10:57:11]

Done.

-          }
-          var domain = match[1];
-          domains.push(domain);
-          domains.sort();
-          
-          whitelistTbl.insertBefore(createWhitelistElem(domain), whitelistTbl.childNodes[domains.indexOf(domain)]);
-          E("whitelisting-textbox").value = "";
-        }
-        else
-        {
-          // TODO: add `filters[i].text` to list of custom filters
-        }
+        updateFilter(filter);
+        updateShareLink();
         break;
       case "loaded":
         populateLists();
         break;
       case "removed":
-        var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
-        if (match[1])

[Thomas Greiner, 2015/01/30 13:55:52]

As mentioned above, `match` could be null so better check `if (match)`.

[saroyanm, 2015/02/13 10:57:11]

Done.

-        {
-          var elem = document.querySelector("[data-domain='"+match[1]+"']");

[Thomas Greiner, 2015/01/30 13:55:52]

We're not restricted by bandwidth so you don't need to use abbreviated variable
names.

[Thomas Greiner, 2015/01/30 13:55:52]

Nit: Missing spaces around `+`s

-          elem.parentNode.removeChild(elem);
-        }
-        break;
-    }
-  }
-  
+        var knownFilter = filtersMap[filter.text];
+        collections.whitelist.removeItem(knownFilter);
+        delete filtersMap[filter.text];
+        updateShareLink();
+        break;
+    }
+  }
+
   function onSubscriptionMessage(action, subscription)
   {
     switch (action)
     {
       case "added":
-        var optionSubscription = getOptionSubscription(subscription.url);
-        if (optionSubscription)
-        {
-          var isAdsType = optionSubscription.type && optionSubscription.type == "ads";
-          if (isAdsType && !optionSubscription.isAdded)
-            addLanguageSubscription(subscription);
-          else
-            updateSubscriptionState(subscription, true);
-        }
-        else if (subscription.url == acceptableAdsUrl)
-          updateSubscriptionState(subscription, true);
-        else
-          addFurtherList(subscription);
-        break;
       case "disabled":
-        updateSubscriptionState(subscription, false);
+        updateSubscription(subscription);
+        updateShareLink();
         break;
       case "homepage":
         // TODO: NYI
         break;
       case "removed":
-        updateSubscriptionState(subscription, false);
+        getAcceptableAdsURL(function(acceptableAdsUrl)
+        {
+          if (subscription.url == acceptableAdsUrl)
+          {
+            subscription.disabled = true;
+            updateSubscription(subscription);
+          }
+          else
+          {
+            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;
+    openDialog("customlist");
+  }
+
   function updateShareLink()
   {
-    ext.backgroundPage.sendMessage({
+    ext.backgroundPage.sendMessage(
+    {
       type: "filters.blocked",
       url: "https://platform.twitter.com/widgets/",
       requestType: "SCRIPT",
       docDomain: "adblockplus.org",
       thirdParty: true
-    }, function(blocked)
+    },
+    function(blocked)
     {
       // TODO: modify "share" link accordingly
     });
   }
-  
-  function updateVersionNumber()
-  {
-    ext.backgroundPage.sendMessage({
-    method: "app.get",

[Thomas Greiner, 2015/01/30 13:55:52]

Nit: Adjust indentation level

[saroyanm, 2015/02/13 10:57:11]

Done.

-    what: "addonVersion"
-    }, function(addonVersion)
-    {
-      E("abp-version").innerHTML = addonVersion;

[Thomas Greiner, 2015/01/30 13:55:52]

You're only changing the text content so use `textContent` instead of
`innerHTML`.

[saroyanm, 2015/02/13 10:57:11]

Done.

-    });
-  }
-  
-  function getDocLink(link, callback)
-  {
-    ext.backgroundPage.sendMessage({
-      type: "app.get",
-      what: "doclink",
-      link: link
-    }, callback);
-  }
-  
-  function setLinks(id)

[Thomas Greiner, 2015/01/30 13:55:52]

Seems like you should be able to get rid of this function after addressing my
comment regarding line 33.

-  {
-    var element = E(id);
-    if (!element)
-    {
-      return;
-    }
-
-    var links = element.getElementsByTagName("a");
-
-    for (var i = 0; i < links.length; i++)
-    {
-      if (typeof arguments[i + 1] == "string")
-      {
-        links[i].href = arguments[i + 1];
-        links[i].setAttribute("target", "_blank");
-      }
-      else if (typeof arguments[i + 1] == "function")
-      {
-        links[i].href = "javascript:void(0);";
-        links[i].addEventListener("click", arguments[i + 1], false);
-      }
-    }
-  }
-  
+
   function E(id)
   {
     return document.getElementById(id);
   }
-  
+
   ext.onMessage.addListener(function(message)
   {
     switch (message.type)
     {
       case "app.listen":
         if (message.action == "addSubscription")
-        {
-          message.args.unshift(message.action);

[Thomas Greiner, 2015/01/30 13:55:52]

You don't even use `message.action` in `showAddSubscriptionDialog` because
you're already checking for it here so no need to add it to `message.args`.

[saroyanm, 2015/02/13 10:57:11]

Done.

-          showAddSubscriptionDialog.apply(null, message.args);

[Thomas Greiner, 2015/01/30 13:55:52]

`showAddSubscription` only requires the first argument so let's limit it to
that:

`showAddSubscriptionDialog(message.args[0]);`

[saroyanm, 2015/02/13 10:57:11]

Done.

-        }
+          showAddSubscriptionDialog(message.args[0]);
         break;
       case "filters.listen":
-        message.args.unshift(message.action);
-        onFilterMessage.apply(null, message.args);

[Thomas Greiner, 2015/01/30 13:55:52]

`onFilterMessage` only expects two parameters so let's limit it to that:

`onFilterMessage(message.action, message.args[0]);`

Same applies to `onSubscriptionMessage` below.

[saroyanm, 2015/02/13 10:57:11]

Done.

+        onFilterMessage(message.action, message.args[0]);
         break;
       case "subscriptions.listen":
-        message.args.unshift(message.action);
-        onSubscriptionMessage.apply(null, message.args);
+        onSubscriptionMessage(message.action, message.args[0]);
         break;
     }
   });
-  
-  ext.backgroundPage.sendMessage({
+
+  ext.backgroundPage.sendMessage(
+  {
     type: "app.listen",
     filter: ["addSubscription"]
   });
-  ext.backgroundPage.sendMessage({
+  ext.backgroundPage.sendMessage(
+  {
     type: "filters.listen",
     filter: ["added", "loaded", "removed"]
   });
-  ext.backgroundPage.sendMessage({
+  ext.backgroundPage.sendMessage(
+  {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
-  
+
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();