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 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.

+    this.items = [];
+  }
+
   Collection.prototype.addItems = function() 
   {
-    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)

[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;
+      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;

[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)]);
+        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.indexOf(item);
+    var index = this.items.indexOf(item);
     if (index == -1)
       return;
 
-    this.splice(index, 1);
+    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.length = 0;
+    this.items.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.

+  function onToggleSubscriptionClick(e)
   {
     e.preventDefault();
     var subscriptionUrl = e.target.parentNode.dataset.access;
     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",
-      onClick: toggleSubscription
+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.langs = new Collection(
   [
     {
       id: "blocking-languages-table",
-      onClick: toggleSubscription
+      onClick: onToggleSubscriptionClick
     },
     {
-      id: "blocking-languages-modal-table"
+      id: "blocking-languages-dialog-table"
     }
   ]);
   collections.allLangs = new Collection(
   [
     {
       id: "all-lang-table", 
-      onClick: addLanguageSubscription
+      onClick: onAddLanguageSubscriptionClick
     }
   ]);
   collections.acceptableAds = new Collection(
   [
     {
       id: "acceptableads-table", 
-      onClick: toggleSubscription
+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.custom = new Collection(
   [
     {
       id: "custom-list-table", 
-      onClick: toggleSubscription
+      onClick: onToggleSubscriptionClick
     }
   ]);
   collections.whitelist = new Collection(
   [
     {
       id: "whitelisting-table", 
-      onClick: triggerRemoveFilter
+      onClick: onRemoveFilterClick
     }
   ]);
 
   function updateSubscription(subscription)
   {
     var subscriptionUrl = subscription.url;
     var knownSubscription = subscriptionsMap[subscriptionUrl];
     if (knownSubscription)
       knownSubscription.disabled = subscription.disabled;
     else
@@ skipping 25 matching lines @@
                   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)

[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();
-          });
+          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;
@@ skipping 23 matching lines @@
     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.

+    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 13 matching lines @@
         }
         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()
   {
     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.

+    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-button").addEventListener("click", editCustomFilters, false);
-    E("find-language").addEventListener("keyup", searchLanguage, 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 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"
+      // 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();

[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;
+      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)
@@ skipping 54 matching lines @@
         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.

+    //TODO: NYI
   }
 
   function getAcceptableAdsURL(callback)
   {
     ext.backgroundPage.sendMessage(
     {
       type: "prefs.get",
       key: "subscriptions_exceptionsurl"
     },
     function(value)
@@ skipping 63 matching lines @@
         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)
@@ skipping 17 matching lines @@
         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",
       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")
@@ skipping 19 matching lines @@
     filter: ["added", "loaded", "removed"]
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();