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

options.js

Index: options.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/options.js
@@ -0,0 +1,666 @@
+/*
+ * 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 subscriptionsMap = Object.create(null);
+  var recommendationsMap = Object.create(null);
+  var filtersMap = Object.create(null);
+  var collections = Object.create(null);
+
+  function Collection(details)

[Felix Dahlke, 2015/06/07 21:09:59]

Does details have to be an array? From what I've seen below, you're only ever
passing a one element array to it.

[saroyanm, 2015/06/09 15:29:13]

on line 139 I'm passing array from 2 elements.
"Adblocking for websites in.." section and "Added languages" in dialog have have
same data in there and handled same way.

[Felix Dahlke, 2015/06/11 14:40:20]

True, fair enough.

+  {
+    this.details = details;
+  }
+
+  Collection.prototype = Object.create(Array.prototype);

[Felix Dahlke, 2015/06/07 21:09:59]

From what I've seen, the only member functions used on Collection instances are
addItems(), removeItem() and clearAll(). In that case I believe we should prefer
composition over inheritance here, i.e. just store the items in a member
variable rather than inheriting from Array.

[saroyanm, 2015/06/09 15:29:13]

I would like to understand why it's more preferable to use composition in this
case instead of inheritance, I personally think it's more readable, for example
in the example of addItems method for me it's more intuitive that I'm sorting
and pushing the items into the object itself, I don't mind to use composition
instead I just would like to understand whether there is a reason I'm missing or
it's personal preference.

[Felix Dahlke, 2015/06/11 14:40:20]

It's a pretty common principle in OOP
(http://en.wikipedia.org/wiki/Composition_over_inheritance). The whole thing
about being a "has a" relationship versus being a "is a" relationship.

More practically, inheritance gives clients API they are not supposed to use,
since they would be violating invariants. In our concrete case here, there's an
invariant that the elements should be sorted, which is upheld by addItems and
removeItems. If clients were to use inherited API such as push, they could
easily violate it.

[saroyanm, 2015/06/12 10:51:18]

Done.

+  Collection.prototype.addItems = function() 
+  {
+    var length = Array.prototype.push.apply(this, arguments);
+    if (length == 0)
+      return;
+
+    this.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)

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Would be shorter using localeCompare: `return
aValue.localeCompare(bValue);`

[saroyanm, 2015/06/09 15:29:13]

I like this :) Done.

+        return -1;
+      if (aValue > bValue)
+        return 1;
+      return 0;
+    });
+
+    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;

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: "Do not compare x == true or x == false. Use (x) or (!x)"
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

There are more instances of this below.

[Felix Dahlke, 2015/06/07 21:15:24]

Sorry, wrong link, should have been:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

[saroyanm, 2015/06/09 15:29:13]

In this case is bit complicated, while the design itself is bit inconsistent
with logic, we now have subscription which are disabled and aren't added which
are treated differently on view, 
so please note the case when we need to show the subscription in the case it's
"disabled" property is null or true. for this case we are using
subscription.desabled == false.
Or we should go with the case of !subscription.desabled && subscription.desabled
!= null ? It's still bit confusing.

[Felix Dahlke, 2015/06/11 14:40:20]

Oh I didn't realise that. No then it's fine.

+        }
+
+        if (table.hasChildNodes)

[Felix Dahlke, 2015/06/07 21:09:59]

Shouldn't this be `table.hasChildNodes()`?

[saroyanm, 2015/06/09 15:29:13]

Done.

+          table.insertBefore(listItem, table.childNodes[this.indexOf(item)]);
+        else
+          table.appendChild(listItem);
+      }
+    }
+    return length;
+  };
+
+  Collection.prototype.removeItem = function(item)
+  {
+    var index = this.indexOf(item);
+    if (index == -1)
+      return;
+
+    this.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.length = 0;
+  };
+
+  function toggleSubscription(e)

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Wouldn't insist on it, but being an event handler that does something with
the event, I'd prefer a name that makes this more obvious, such as
`onToggleSubscriptionClick`. Same for the other control click handlers.

[saroyanm, 2015/06/09 15:29:13]

Done.

+  {
+    e.preventDefault();
+    var subscriptionUrl = e.target.parentNode.dataset.access;
+    if (!e.target.checked)
+      removeSubscription(subscriptionUrl);
+    else
+      addEnableSubscription(subscriptionUrl);
+  }
+
+  function addLanguageSubscription(e)
+  {
+    e.preventDefault();
+    var url = this.parentNode.dataset.access;
+    addEnableSubscription(url);
+  }
+
+  function triggerRemoveFilter()
+  {
+    var filter = this.parentNode.dataset.access;
+    removeFilter(filter);
+  }
+
+  collections.popular = new Collection(
+  [
+    {
+      id: "recommend-list-table",
+      onClick: toggleSubscription
+    }
+  ]);
+  collections.langs = new Collection(
+  [
+    {
+      id: "blocking-languages-table",
+      onClick: toggleSubscription
+    },
+    {
+      id: "blocking-languages-modal-table"
+    }
+  ]);
+  collections.allLangs = new Collection(
+  [
+    {
+      id: "all-lang-table", 
+      onClick: addLanguageSubscription
+    }
+  ]);
+  collections.acceptableAds = new Collection(
+  [
+    {
+      id: "acceptableads-table", 
+      onClick: toggleSubscription
+    }
+  ]);
+  collections.custom = new Collection(
+  [
+    {
+      id: "custom-list-table", 
+      onClick: toggleSubscription
+    }
+  ]);
+  collections.whitelist = new Collection(
+  [
+    {
+      id: "whitelisting-table", 
+      onClick: triggerRemoveFilter
+    }
+  ]);
+
+  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)
+        {
+          ["disabled"].forEach(function(property)

[Felix Dahlke, 2015/06/07 21:09:59]

This could be simplified :D

[saroyanm, 2015/06/09 15:29:13]

For future we are going to not only listen to "disbled" property of
subscription, but also to other properties like -> title, lastDownload, homepage
and etc. 
I've added comment regarding that, please let me know if think it make sense to
modify it now.

[Felix Dahlke, 2015/06/11 14:40:20]

If we're absolutely sure we'll need it, I'm OK with it.

+          {
+            subscription["$" + property] = subscription[property];
+            Object.defineProperty(subscription, property,
+            {
+              get: function()
+              {
+                return this["$" + property];
+              },
+              set: function(value)
+              {
+                this["$" + property] = value;
+                onObjectChanged();
+              }
+            });
+          });
+        }
+        else
+        {
+          Object.observe(subscription, function(changes)

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Why not just `Object.observe(subscription, onObjectChanged);`?

[saroyanm, 2015/06/09 15:29:13]

Done.

+          {
+            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.onload = function()

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Use addEventListener()?

[saroyanm, 2015/06/09 15:29:13]

Done.

+    {
+      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);
+      }
+    };
+    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()

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Similar to above, I'd find this easier to understand if it was named to
indicate it's an event handler, e.g. `onFindLanguageKeyUp`.

[saroyanm, 2015/06/09 15:29:13]

Done.

+    {
+      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");
+    E("find-language").setAttribute("placeholder", placeholderValue);
+    E("add-blocking-list").addEventListener("click", function()
+    {
+      openModal("customlist");
+    }, false);
+    E("add-website-language").addEventListener("click", function()
+    {
+      openModal("language");
+    }, false);
+    E("modal-close").addEventListener("click", function()
+    {
+      delete document.body.dataset.modal;
+    }, false);
+    E("edit-ownBlockingList-button").addEventListener("click", editCustomFilters, false);
+    E("find-language").addEventListener("keyup", searchLanguage, 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 use e.key instead

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: "so we attempt to use e.key"? Since we're actually falling back to keyCode.

[saroyanm, 2015/06/09 15:29:13]

Done.

+      if (e.key && e.key == "Enter")
+        addWhitelistedDomain();
+      else if (!e.key && e.keyCode == 13) //keyCode "13" corresponds to "Enter"
+        addWhitelistedDomain();

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Seeing how the else body is identical to the if body, why not merge the
conditions?

[saroyanm, 2015/06/09 15:29:13]

Done.

+    }, false);
+    E("import-blockingList-button").addEventListener("click", function()
+    {
+      var url = E("blockingList-textbox").value;
+      addEnableSubscription(url);
+      delete document.body.dataset.modal;
+    }, false);
+  }
+
+  function openModal(name)
+  {
+    document.body.dataset.modal = name;
+  }
+
+  function populateLists()
+  {
+    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)
+    {
+      // Load filters
+      for (var i = 0; i < subscriptions.length; i++)
+      {
+        ext.backgroundPage.sendMessage(
+        {
+          type: "filters.get",
+          subscriptionUrl: subscriptions[i].url
+        }, 
+        function(filters)
+        {
+          for (var i = 0; i < filters.length; i++)
+            updateFilter(filters[i]);
+        });
+      }
+    });
+    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)
+      {
+        for (var i = 0; i < subscriptions.length; i++)
+          onSubscriptionMessage("added", subscriptions[i]);
+      });
+    });
+  }
+
+  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()
+  {
+    

[Felix Dahlke, 2015/06/07 21:09:59]

Nit: Seeing how we have a bunch of TODO comments here, I guess this deserves one
too?

[saroyanm, 2015/06/09 15:29:13]

Done.

+  }
+
+  function getAcceptableAdsURL(callback)
+  {
+    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 = subscriptionsMap[url];
+    if (knownSubscription && knownSubscription.disabled == true)
+      messageType = "subscriptions.toggle"
+    else
+      messageType = "subscriptions.add"
+
+    var message = {
+      type: messageType,
+      url: url
+    };
+    if (title)
+      message.title = title;
+    if (homepage)
+      message.homepage = homepage;
+
+    ext.backgroundPage.sendMessage(message);
+  }
+
+  function removeSubscription(url)
+  {
+    ext.backgroundPage.sendMessage(
+    {
+      type: "subscriptions.remove",
+      url: url
+    });
+  }
+
+  function removeFilter(filter)
+  {
+    ext.backgroundPage.sendMessage(
+    {
+      type: "filters.remove",
+      text: filter
+    });
+  }
+
+  function onFilterMessage(action, filter)
+  {
+    switch (action)
+    {
+      case "added":
+        updateFilter(filter);
+        updateShareLink();
+        break;
+      case "loaded":
+        populateLists();
+        break;
+      case "removed":
+        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":

[Felix Dahlke, 2015/06/07 21:09:59]

Merge with case "added" above?

[saroyanm, 2015/06/09 15:29:13]

Done.

+        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 = 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");
+  }
+
+  function updateShareLink()
+  {
+    ext.backgroundPage.sendMessage(
+    {
+      type: "filters.blocked",
+      url: "https://platform.twitter.com/widgets/",
+      requestType: "SCRIPT",
+      docDomain: "adblockplus.org",
+      thirdParty: true
+    },
+    function(blocked)
+    {
+      // TODO: modify "share" link accordingly
+    });
+  }
+
+  function E(id)

[Felix Dahlke, 2015/06/07 21:09:59]

Not a fan of this function, made things harder to read since I first had to look
up what it does. If Thomas is OK with it I can live with it too, but then we
should at least name it with a lower case E, so it doesn't look like a
constructor.

[Felix Dahlke, 2015/06/08 19:44:51]

Just realised that we have E() as well in the Firefox code. Still not a fan, but
for consistency's sake I'm OK with keeping it as it is here.

+  {
+    return document.getElementById(id);
+  }
+
+  ext.onMessage.addListener(function(message)
+  {
+    switch (message.type)
+    {
+      case "app.listen":
+        if (message.action == "addSubscription")
+          showAddSubscriptionDialog(message.args[0]);
+        break;
+      case "filters.listen":
+        onFilterMessage(message.action, message.args[0]);
+        break;
+      case "subscriptions.listen":
+        onSubscriptionMessage(message.action, message.args[0]);
+        break;
+    }
+  });
+
+  ext.backgroundPage.sendMessage(
+  {
+    type: "app.listen",
+    filter: ["addSubscription"]
+  });
+  ext.backgroundPage.sendMessage(
+  {
+    type: "filters.listen",
+    filter: ["added", "loaded", "removed"]
+  });
+  ext.backgroundPage.sendMessage(
+  {
+    type: "subscriptions.listen",
+    filter: ["added", "disabled", "homepage", "removed", "title"]
+  });
+
+  window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
+})();