Issue 154 - Added UI for devtools panel on Chrome

devtools-panel.js

 /*
- * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 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 generateFilter(request)
 {
   var filter = request.url.replace(/^[\w\-]+:\/+(?:www\.)?/, "||");
 
   if (request.type == "POPUP")
   {
     filter += "$popup";
 
     if (request.url == "about:blank")
       filter += ",domain=" + request.docDomain;
   }
 
   return filter;
 }
 
-function createActionButton(action, label, filter, port)
+function createActionButton(action, label, filter)
 {
   var button = document.createElement("span");
 
   button.textContent = label;
   button.classList.add("action");
 
   button.addEventListener("click", function()
   {
-    port.postMessage({action: action, filter: filter});
-  });
+    ext.backgroundPage.sendMessage({
+      type: "filters." + action,
+      text: filter
+    });
+  }, false);
 
   return button;
 }
 
-function createRecord(request, filter, port)

[Thomas Greiner, 2015/01/12 10:37:54]

Seems like HTML templates is what you'd want to use here which should be
supported by Chrome 28.

[Sebastian Noack, 2015/01/12 11:55:07]

Sounds good. I will look into it.

+function createRecord(request, filter, template)
 {
-  var row = document.createElement("tr");
+  var row = document.importNode(template, true);
   row.dataset.type = request.type;
 
-  var requestCell = document.createElement("td");
-  row.appendChild(requestCell);
+  row.querySelector(".domain").textContent = request.docDomain;
+  row.querySelector(".type").textContent = request.type;
 
-  var requestWrapper = document.createElement("div");
-  requestWrapper.classList.add("request-wrapper");
-  requestCell.appendChild(requestWrapper);
-
-  var urlElement = document.createElement("div");
-  urlElement.classList.add("url");
-  urlElement.innerHTML = " ";

[Thomas Greiner, 2015/01/12 10:37:54]

I assume you do this to ensure a minimum height for this element. If so you
should rather use the `min-height` CSS property.

However, why would there be no URL in the first place?

[Sebastian Noack, 2015/01/12 11:55:07]

Doesn't seem to do the trick when using flexbox.
See below.

[Thomas Greiner, 2015/01/16 18:17:21]

That's strange because it works for me when emptying the content of this DIV and
adding the style `.url {min-height: 17px;}` to set the minimum height to the
same value as the defined line height.

[Sebastian Noack, 2015/02/04 14:17:29]

I don't recall anymore what the particular problem was here. But hard-coding the
min-height for the cell, would be just another hack, and IMO not any better than
using " ".

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

Assuring that the element has a minimum height is what you're trying to achieve
here. Modifying the DOM to work around a styling issue is a hack.

[Sebastian Noack, 2015/03/13 13:07:38]

I don't want a minimum height, I want each line to be exact 17px large in
height. Therefore I use the line-height property of the table. However,
line-height is only applied to text. So I have to make every line contain some
text, that's what I have to use the non-breaking-space for. IMO this is
certainly better and not more a hack than redundantly specifying the size with
the min-height property for all potential empty <div> elements.

-  requestWrapper.appendChild(urlElement);
-
-  var domainElement = document.createElement("div");
-  domainElement.classList.add("domain");
-  domainElement.textContent = request.docDomain;
-  requestWrapper.appendChild(domainElement);
-
-  var typeElement = document.createElement("div");
-  typeElement.classList.add("type");
-  typeElement.textContent = request.type;
-  requestWrapper.appendChild(typeElement);
-
-  var filterCell = document.createElement("td");
-  row.appendChild(filterCell);
-
-  var actionWrapper = document.createElement("div");
-  actionWrapper.classList.add("action-wrapper");
-  filterCell.appendChild(actionWrapper);
-
-  var filterWrapper = document.createElement("div");
-  filterWrapper.classList.add("filter-wrapper");
-  actionWrapper.appendChild(filterWrapper);
-
-  var filterElement = document.createElement("div");
-  filterElement.innerHTML = " ";
-  filterElement.classList.add("filter");
-  filterWrapper.appendChild(filterElement);
-
-  var originElement = document.createElement("div");
-  originElement.innerHTML = " ";
-  originElement.classList.add("origin");
-  filterWrapper.appendChild(originElement);
+  var urlElement = row.querySelector(".url");
+  var actionWrapper = row.querySelector(".action-wrapper");
 
   if (request.url)

[Thomas Greiner, 2015/01/12 10:37:54]

Which requests don't have URLs associated to them?

[Sebastian Noack, 2015/01/12 11:55:07]

Element hiding filter matches.

   {
     urlElement.textContent = request.url;
 
     if ("openResource" in ext.devtools.panels && request.type != "POPUP")

[Thomas Greiner, 2015/01/12 10:37:54]

Why would `ext.devtools.panels.openResource` not be available?

[Sebastian Noack, 2015/01/12 11:55:07]

It's new in Chrome 38.

     {
       urlElement.classList.add("resourceLink");
       urlElement.addEventListener("click", function()

[Thomas Greiner, 2015/01/12 10:37:54]

No need to add an event listener to each row individually. You can catch the
event higher up in the hierarchy.

[Sebastian Noack, 2015/01/12 11:55:07]

I know. But then I'd need to double check whether the element clicked on is an
action button, and extract the URL from the DOM. It's much simpler this way. And
I don't care that too many event listeners slow down the page in IE6.

[Thomas Greiner, 2015/01/16 18:17:21]

I'd agree if `urlElement` had child elements that we'd have to consider. But it
doesn't appear much more complicated to, instead of:

if ("openResource" in ext.devtools.panel && request.type != "POPUP")
{
  urlElement.classList.add("resourceLink");
  urlElement.addEventListener("click", function()
  {
    ext.devtools.panels.openResource(request.url);
  }, false);
}

write:

if (request.type != "POPUP")
  urlElement.dataset.resourceUrl = request.url;

and somewhere else:

if ("openResource" in ext.devtools.panels)
{
  document.addEventListener("click", function(ev)
  {
    var resourceUrl = ev.target.dataset.resourceUrl;
    if (resourceUrl)
      ext.devtools.panels.openResource(resourceUrl);
  }, true);
}

You could even use that attribute in your CSS instead of `.resourceLink` by
writing `[data-resource-url]`.

[Sebastian Noack, 2015/02/04 14:17:29]

I'd still need to check whether ext.devtools.panels.openResource is available
when creating the element. Otherwise it would still look like a link even when
you can't actually click it on old Chrome versions.

This really is just premature optimization, adding complexity and duplicating
logic. Not that, it would make the code any easier to read, when having the
event listener defined in an entire different place than the rest of the DOM
code related to the element.

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

That makes sense. Separating the logic from the view creation would've been nice
but since it's not always clickable too much of an overhead, true.

       {
         ext.devtools.panels.openResource(request.url);
-      });
+      }, false);
     }
   }
 
   if (filter)
   {
+    var filterElement = row.querySelector(".filter");
+    var originElement = row.querySelector(".origin");
+
     filterElement.textContent = filter.text;
     row.dataset.state = filter.whitelisted ? "whitelisted" : "blocked";
 
     if (filter.subscription)
       originElement.textContent = filter.subscription;
     else
     {
       if (filter.userDefined)
         originElement.textContent = "user-defined";
       else
         originElement.textContent = "unnamed subscription";
 
       originElement.classList.add("unnamed");
     }
 
     if (!filter.whitelisted && request.type != "ELEMHIDE")
-      actionWrapper.appendChild(createActionButton("add", "Whitelist request", "@@" + generateFilter(request), port));
+      actionWrapper.appendChild(createActionButton("add", "Add exception", "@@" + generateFilter(request)));
 
     if (filter.userDefined)
-      actionWrapper.appendChild(createActionButton("remove", "Remove filter", filter.text, port));
+      actionWrapper.appendChild(createActionButton("remove", "Remove rule", filter.text));
   }
   else
-    actionWrapper.appendChild(createActionButton("add", "Block request", generateFilter(request), port));
+    actionWrapper.appendChild(createActionButton("add", "Block item", generateFilter(request)));
 
   return row;
 }
 
 document.addEventListener("DOMContentLoaded", function()
 {
-  var port = ext.devtools.connect();
   var container = document.getElementById("items");
-  var table = container.getElementsByTagName("table")[0];
+  var table = container.querySelector("tbody");
+  var template = document.querySelector("template").content.firstElementChild;
 
-  port.onMessage.addListener(function(msg)
+  document.getElementById("reload").addEventListener("click", function()
   {
-    switch (msg.type)
+    ext.devtools.inspectedWindow.reload();
+  }, false);
+
+  document.getElementById("filter-state").addEventListener("change", function(event)
+  {
+    container.dataset.filterState = event.target.value;
+  }, false);
+
+  document.getElementById("filter-type").addEventListener("change", function(event)
+  {
+    container.dataset.filterType = event.target.value;
+  }, false);
+
+  ext.onMessage.addListener(function(message)
+  {
+    switch (message.type)
     {
       case "add-record":
-        table.appendChild(createRecord(msg.request, msg.filter, port));
+        table.appendChild(createRecord(message.request, message.filter, template));
         break;
 
       case "update-record":
-        var oldRow = table.getElementsByTagName("tr")[msg.index];
-        var newRow = createRecord(msg.request, msg.filter, port);
+        var oldRow = table.getElementsByTagName("tr")[message.index];
+        var newRow = createRecord(message.request, message.filter, template);
         oldRow.parentNode.replaceChild(newRow, oldRow);
         newRow.classList.add("changed");
         container.classList.add("has-changes");
         break;
 
       case "remove-record":
-        var row = table.getElementsByTagName("tr")[msg.index];
+        var row = table.getElementsByTagName("tr")[message.index];
         row.parentNode.removeChild(row);
         container.classList.add("has-changes");
         break;
 
       case "reset":
         table.innerHTML = "";
         container.classList.remove("has-changes");
         break;
     }
   });
-
-  document.getElementById("reload").addEventListener("click", function()
-  {
-    ext.devtools.inspectedWindow.reload();
-  });
-
-  document.getElementById("filter-state").addEventListener("change", function(event)
-  {
-    container.dataset.filterState = event.target.value;
-  });
-
-  document.getElementById("filter-type").addEventListener("change", function(event)
-  {
-    container.dataset.filterType = event.target.value;
-  });
-});
+}, false);