Issue 2375 - Implement "Blocking lists" section in new options page

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -44,6 +44,15 @@
       table.removeChild(placeholder);
   }
 
+  Collection.prototype._createElementQuery = function(item)
+  {
+    var access = (item.url || item.text).replace(/'/g, "\\'");
+    return function(container)
+    {
+      return container.querySelector("[data-access='" + access + "']");
+    };
+  };
+
   Collection.prototype.addItems = function() 
   {
     var length = Array.prototype.push.apply(this.items, arguments);
@@ -72,6 +81,7 @@
         if (text)
           listItem.setAttribute("data-search", text.toLowerCase());
 
+        updateBlockingList(listItem, item);

[Thomas Greiner, 2016/01/27 17:16:57]

This doesn't belong into Collection since it's specific to one table. Therefore
I'd suggest moving it into the observer function.

[saroyanm, 2016/01/28 17:00:11]

We are calling this method on observer as well, This method is used to update
the date/time and links of the filterList item, but in general it should be part
of updateItem method, I've added that implementation to updateItem method, hope
it's more clear now.

[Thomas Greiner, 2016/01/29 17:48:08]

This code is only relevant for subscriptions. However, Collection also manages
filters which don't specify source, homepage, etc. Therefore it should be
separate from Collection.

However, I'm fine with how it looks right now to centralize logic.

         var control = listItem.querySelector(".control");
         if (control)
         {
@@ -81,7 +91,8 @@
 
         this._setEmpty(table, null);
         if (table.hasChildNodes())
-          table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
+          table.insertBefore(listItem, 
+                            table.childNodes[this.items.indexOf(item)]);

[Thomas Greiner, 2016/01/25 15:40:32]

Detail: Please use braces around it if you occupy more than a single line.

Note that we usually indent a continuing line with two additional double-spaces.

[saroyanm, 2016/01/26 18:36:19]

Done.

[Thomas Greiner, 2016/01/27 17:16:58]

Detail: What I meant with "indent using double-spaces" was to use four spaces
instead of the usual two when starting a new line:

abcdef(ghi
....jklmno)

instead of

abcdef(ghi
      ..jklmno)

Because that way we can write longer lines while still making clear that the
second line is part of the previous statement and not another independent
statement.

[saroyanm, 2016/01/28 17:00:12]

Got it, sorry for that, I was thinking that we need to align the second line
with the related expression on line above for readability.

         else
           table.appendChild(listItem);
       }
@@ -96,10 +107,11 @@
       return;
 
     this.items.splice(index, 1);
+    var getListElement = this._createElementQuery(item);
     for (var i = 0; i < this.details.length; i++)
     {
       var table = E(this.details[i].id);
-      var element = table.childNodes[index];
+      var element = getListElement(table);
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, this.details[i].emptyText);
@@ -113,16 +125,73 @@
     {
       var table = E(this.details[i].id);
       var template = table.querySelector("template");
-      table.innerHTML = "";
-      table.appendChild(template);
+      var staticElements = [];

[Thomas Greiner, 2016/01/25 15:40:33]

The above two variables are no longer used.

[saroyanm, 2016/01/26 18:36:19]

Done.

+      var element = table.firstChild;
+      while (element)
+      {
+        if ((element.tagName == "LI" && !element.classList.contains("static")) ||
+            element.nodeType == 3)
+          table.removeChild(element);
+        element = element.nextSibling
+      }
+
       this._setEmpty(table, this.details[i].emptyText);
     }
   };
 
-  function onToggleSubscriptionClick(e)
+  Collection.prototype.hasId = function(id) 
+  {
+    for (var i = 0; i < this.details.length; i++)
+      if (this.details[i].id == id)
+        return true;
+      
+    return false;
+  };
+
+  function updateBlockingList(listItem, subscription)
+  {
+    var dateElement = listItem.querySelector(".date");
+    var timeElement = listItem.querySelector(".time");
+
+    if(dateElement && timeElement)
+    {
+      if (subscription.downloadStatus && 
+          subscription.downloadStatus != "synchronize_ok")
+      {
+        var map =
+        {
+          "synchronize_invalid_url": "options_subscription_lastDownload_invalidURL",
+          "synchronize_connection_error": "options_subscription_lastDownload_connectionError",
+          "synchronize_invalid_data": "options_subscription_lastDownload_invalidData",
+          "synchronize_checksum_mismatch": "options_subscription_lastDownload_checksumMismatch"
+        };
+        if (subscription.downloadStatus in map)
+          timeElement.textContent = ext.i18n.getMessage(map[subscription.downloadStatus]);
+        else
+          timeElement.textContent = subscription.downloadStatus;
+      }
+      else if (subscription.lastDownload > 0)
+      {
+        var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000);
+        var date = new Date(subscription.lastDownload * 1000);
+        dateElement.textContent = date.getFullYear() + "-" +
+                                    date.getMonth()+1 + "-" + date.getDate();
+        timeElement.textContent = date.getHours() + ":" + date.getMinutes();
+      }
+    }
+    var websiteElement = listItem.querySelector(".context-menu .website");
+    var sourceElement = listItem.querySelector(".context-menu .source");
+    if (websiteElement && subscription.homepage)
+      websiteElement.setAttribute("href", subscription.homepage);
+    if (sourceElement)
+      sourceElement.setAttribute("href", subscription.url);
+  }
+
+  function toggleRemoveSubscription(e)
   {
     e.preventDefault();
-    var subscriptionUrl = e.target.parentNode.getAttribute("data-access");
+    var subscriptionUrl = getParentAccessElement(e.target).
+                          getAttribute("data-access");
     if (!e.target.checked)
     {
       ext.backgroundPage.sendMessage({
@@ -134,6 +203,18 @@
       addEnableSubscription(subscriptionUrl);
   }
 
+  function toggleDisableSubscription(e)
+  {
+    e.preventDefault();
+    var subscriptionUrl = e.target.parentNode.getAttribute("data-access");
+    ext.backgroundPage.sendMessage(
+    {
+      type: "subscriptions.toggle",
+      keepInstalled: true,
+      url: subscriptionUrl
+    });
+  }
+
   function onAddLanguageSubscriptionClick(e)
   {
     e.preventDefault();
@@ -155,7 +236,7 @@
   [
     {
       id: "recommend-list-table",
-      onClick: onToggleSubscriptionClick
+      onClick: toggleRemoveSubscription
     }
   ]);
   collections.langs = new Collection(
@@ -163,7 +244,7 @@
     {
       id: "blocking-languages-table",
       emptyText: "options_dialog_language_added_empty",
-      onClick: onToggleSubscriptionClick
+      onClick: toggleRemoveSubscription
     },
     {
       id: "blocking-languages-dialog-table",
@@ -182,14 +263,14 @@
   [
     {
       id: "acceptableads-table",
-      onClick: onToggleSubscriptionClick
+      onClick: toggleRemoveSubscription
     }
   ]);
   collections.custom = new Collection(
   [
     {
       id: "custom-list-table",
-      onClick: onToggleSubscriptionClick
+      onClick: toggleRemoveSubscription
     }
   ]);
   collections.whitelist = new Collection(
@@ -207,31 +288,39 @@
       emptyText: "options_customFilters_empty"
     }
   ]);
+  collections.blockingLists = new Collection(
+  [
+    {
+      id: "blocking-lists-table",
+      onClick: toggleDisableSubscription
+    }
+  ]);
 
-  function updateSubscription(subscription)
+  function observeSubscription(subscription)
   {
-    var subscriptionUrl = subscription.url;
-    var knownSubscription = subscriptionsMap[subscriptionUrl];
-    if (knownSubscription)
-      knownSubscription.disabled = subscription.disabled;
-    else
+    function onObjectChanged(change)
     {
-      getAcceptableAdsURL(function(acceptableAdsUrl)
+      for (var i = 0; i < change.length; i++)
       {
-        function onObjectChanged()
+        var property = change[i].name;
+        if (property == "disabled")
         {
-          var access = (subscriptionUrl || subscription.text).replace(/'/g, "\\'");
+          var access = (subscription.url || 
+                        subscription.text).replace(/'/g, "\\'");
           var elements = document.querySelectorAll("[data-access='" + access + "']");
           for (var i = 0; i < elements.length; i++)
           {
             var element = elements[i];
+            var tableId = element.parentElement ? element.parentElement.id : "";
             var control = element.querySelector(".control");
-            if (control.localName == "input")
+            if (control && control.localName == "input")
               control.checked = subscription.disabled == false;
-            if (subscriptionUrl in recommendationsMap)
+            if (subscription.url in recommendationsMap)
             {
-              var recommendation = recommendationsMap[subscriptionUrl];
-              if (recommendation.type == "ads")
+              var recommendation = recommendationsMap[subscription.url];
+              if (recommendation.type == "ads" && 
+                 (collections.langs.hasId(tableId) ||
+                 collections.allLangs.hasId(tableId)))

[Thomas Greiner, 2016/01/25 15:40:33]

I've been thinking about this again. Wouldn't you agree that all of that should
be handled by the Collection?

[saroyanm, 2016/01/26 18:36:18]

I assume you are referring to the "Collection.prototype.updateItem" from
https://codereview.adblockplus.org/29332808/
While the referencing review is almost done, I think make sense to rebase
current review after pushing the changes, I'll check to finish with that review
quick.

[Thomas Greiner, 2016/01/27 17:16:57]

I guess you're right. We can leave it here for now and clean it up afterwards by
moving the relevant parts into Collection.

[saroyanm, 2016/01/28 17:00:11]

Done, hope now it's more clear.

               {
                 if (subscription.disabled == false)
                 {
@@ -247,33 +336,60 @@
             }
           }
         }
+        else 
+        {
+          var blockingListId = collections.blockingLists.details[0].id;
+          var blockingList = document.getElementById(blockingListId);
+          var listItem = blockingList.querySelector("[data-access='" +
+                          subscription.url + "']");
+          if (listItem)
+            updateBlockingList(listItem, subscription);
+        }
+      }
+    }
 
-        if (!Object.observe)
+    if (!Object.observe)
+    {
+      ["disabled", "lastDownload"].forEach(function(property)
+      {
+        subscription["$" + property] = subscription[property];
+        Object.defineProperty(subscription, property,
         {
-          // Currently only "disabled" property of subscription used for observation
-          // but with Advanced tab implementation we should also add more properties.
-          ["disabled"].forEach(function(property)
+          get: function()
           {
-            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);
-        }
+            return this["$" + property];
+          },
+          set: function(value)
+          {
+            this["$" + property] = value;
+            var change = Object.create(null);
+            change.name = property;
+            onObjectChanged([change]);
+          }
+        });
+      });
+    }
+    else
+    {
+      Object.observe(subscription, onObjectChanged);
+    }
+  }
 
+  function updateSubscription(subscription)
+  {
+    var subscriptionUrl = subscription.url;
+    var knownSubscription = subscriptionsMap[subscriptionUrl];
+    if (knownSubscription)
+    {
+      for (var param in subscription)

[Thomas Greiner, 2016/01/25 15:40:31]

Detail: It's a property, not a parameter.

[saroyanm, 2016/01/26 18:36:19]

Done.

+        if (param != "title")

[Thomas Greiner, 2016/01/25 15:40:31]

Why did you specifically exclude the "title" property.

[saroyanm, 2016/01/26 18:36:19]

To use titles specified in Recommendation, so the subscriptions in
subscriptionsMap will always have the initial title, anyway we need to update
the title as soon "title" action is triggered, otherwise we can't update the
title when lastDownload is triggered as in current situation.

[Thomas Greiner, 2016/01/27 17:16:57]

Right, forgot about that case.

It's actually trickier than that because "subscriptions.title" can be triggered
at any point (e.g. subscription updates in the background and includes a
different title than before).
So as soon as we implement the updating of titles we should somehow make sure
that the titles from recommended subscriptions stay unchanged.

Out of the scope of this review, however, so let's tackle this separately.

[saroyanm, 2016/01/28 17:00:11]

Acknowledged.

+          knownSubscription[param] = subscription[param];
+    }
+    else
+    {
+      observeSubscription(subscription);
+      getAcceptableAdsURL(function(acceptableAdsUrl)
+      {
         var collection = null;
         if (subscriptionUrl in recommendationsMap)
         {
@@ -331,7 +447,6 @@
           subscription.disabled = null;
           subscription.downloadStatus = null;
           subscription.homepage = null;
-          subscription.lastSuccess = null;
           var recommendation = Object.create(null);
           recommendation.type = element.getAttribute("type");
           var prefix = element.getAttribute("prefixes");
@@ -352,13 +467,26 @@
       });
   }
 
+  function getParentAccessElement(element)
+  {
+    while (!element.getAttribute("data-access"))
+      element = element.parentNode;

[Thomas Greiner, 2016/01/25 15:40:33]

This may cause an exception if there is no parent element that has that
attribute.

In the upcoming review for #2409 I was planning to introduce the following
function which you could use here as well so feel free to implement it here
already or improve this function based on it.

function findData(element, dataName, returnElement)
{
  while (element)
  {
    if (element.hasAttribute("data-" + dataName))
      return returnElement ? element : element.getAttribute("data-" + dataName);

    element = element.parentElement;
  }
  return null;
}

[saroyanm, 2016/01/26 18:36:19]

Like it, thanks.

+
+    return element;
+  }
+
   function onClick(e)
   {
     var element = e.target;
     while (true)
     {
       if (!element)
+      {
+        var context = document.querySelector(".context");
+        if (context)
+          context.classList.remove("context");
         return;
+      }
 
       if (element.hasAttribute("data-action"))
         break;
@@ -419,21 +547,46 @@
           document.body.setAttribute("data-tab",
             element.getAttribute("data-tab"));
           break;
+        case "update-all-subscriptions":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.update"
+          });
+          break;
+        case "open-context-menu":
+          var listItem = getParentAccessElement(element);
+          var contextMenu = listItem.querySelector(".content");
+          listItem.classList.add("context");
+          break;
+        case "update-now":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.update",
+            url: getParentAccessElement(element).getAttribute("data-access")
+          });
+          getParentAccessElement(element).classList.remove("context");

[Thomas Greiner, 2016/01/25 15:40:33]

Why not just add a "close-context-menu" action to the list elements directly
instead of introducing "website" and "source" actions and manually add it to
other ones?

[saroyanm, 2016/01/26 18:36:19]

Done.

+          break;
+        case "website":
+          getParentAccessElement(element).classList.remove("context");
+          break;
+        case "source":
+          getParentAccessElement(element).classList.remove("context");
+          break;
+        case "delete":

[Thomas Greiner, 2016/01/25 15:40:33]

Detail: This message name doesn't describe what it does (i.e. what's deleted).
Therefore I'd suggest renaming it to "remove-subscription".

[saroyanm, 2016/01/26 18:36:18]

Done.

+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.remove",
+            url: getParentAccessElement(element).getAttribute("data-access")
+          });
+          getParentAccessElement(element).classList.remove("context");
+          break;
       }
     }
   }
 
   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;
-
     populateLists();
-
     function onFindLanguageKeyUp()
     {
       var searchStyle = E("search-style");
@@ -705,14 +858,27 @@
     switch (action)
     {
       case "added":
+        updateSubscription(subscription);
+        updateShareLink();
+
+        var knownSubscription = subscriptionsMap[subscription.url];
+        if (knownSubscription)
+          collections.blockingLists.addItems(knownSubscription);
+        else
+          collections.blockingLists.addItems(subscription);
+        break;
       case "disabled":
         updateSubscription(subscription);
         updateShareLink();
         break;
+      case "lastDownload":
+        updateSubscription(subscription);
+        break;
       case "homepage":
         // TODO: NYI
         break;
       case "removed":
+        var knownSubscription = subscriptionsMap[subscription.url];
         getAcceptableAdsURL(function(acceptableAdsUrl)
         {
           if (subscription.url == acceptableAdsUrl)
@@ -722,7 +888,6 @@
           }
           else
           {
-            var knownSubscription = subscriptionsMap[subscription.url];
             if (subscription.url in recommendationsMap)
               knownSubscription.disabled = true;
             else
@@ -733,6 +898,7 @@
           }
           updateShareLink();
         });
+        collections.blockingLists.removeItem(knownSubscription);

[Thomas Greiner, 2016/01/25 15:40:38]

Detail: Semantically, updating the UI should happen after the storage was
modified so I'd suggest moving this code one line up.

[saroyanm, 2016/01/26 18:36:18]

Done.

         break;
       case "title":
         // TODO: NYI
@@ -821,7 +987,7 @@
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
-    filter: ["added", "disabled", "homepage", "removed", "title"]
+    filter: ["added", "disabled", "homepage", "lastDownload", "removed", "title", "updated"]

[Thomas Greiner, 2016/01/25 15:40:32]

Detail: You removed the handling for "updated" so I guess you can remove it from
here.

[saroyanm, 2016/01/26 18:36:20]

Done.

   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);