Issue 5093 - Indicate whether request in devtools panel belongs to tab

devtools-panel.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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";
 
 let lastFilterQuery = null;
 
 browser.runtime.sendMessage({type: "types.get"},
   (filterTypes) =>
   {
     let filterTypesElem = document.getElementById("filter-type");
-    let filterStyleElem = document.createElement("style");
     for (let type of filterTypes)
     {
-      filterStyleElem.innerHTML +=
-        `#items[data-filter-type=${type}] tr:not([data-type=${type}])` +
-        "{display: none;}";
       let optionNode = document.createElement("option");
       optionNode.appendChild(document.createTextNode(type));
       filterTypesElem.appendChild(optionNode);
     }
-    document.body.appendChild(filterStyleElem);
   });
 
 function generateFilter(request, domainSpecific)
 {
   let filter = request.url.replace(/^[\w-]+:\/+(?:www\.)?/, "||");
   let options = [];
 
   if (request.type == "POPUP")
   {
     options.push("popup");
@@ skipping 136 matching lines @@
       row.classList.remove("filtered-by-search");
   }
 }
 
 function cancelSearch(table)
 {
   for (let row of table.rows)
     row.classList.remove("filtered-by-search");
 }
 
+/**

[Thomas Greiner, 2018/02/28 18:18:08]

In comparison to how we did the record filtering before using CSS, this
placeholder-based approach seems quite a bit more complicated because we have to
keep track of all records and modify the DOM to reflect each change.

So assuming that you're doing this to avoid the issues with `:nth-child(odd)`
when some elements are hidden, what about implementing it the same way as Chrome
does in the DevTools' Network tab and assign the styles via JavaScript (e.g.
`class="odd"`)?
That way all we need to do is update the class names of each row whenever the
user is filtering the list.

We can, of course, also hide/show the rows using JavaScript but that seems to be
out of scope for this particular issue.

[a.giammarchi, 2018/02/28 19:07:13]

before we were not doing anything, it was a hack with the background color :-)
not perfect, I agree, but don't we need to track all changes and change all
classes when we filter, if we go 100% JS?
not sure which one is better. I usually prefer styling without JS when possible.

Also I am not sure the the JS version would be better, the placeholders need to
be in place regardless unless we want to trash and recreate the whole table each
time, which I guess might be fine if we don't have thousand records.

I will give it a try +1
hide and show might be actually an alternative to the placeholder. If I have to
setup rows, maybe it's better to hide those filtered out instead of using
placeholders, and to the module 2 dance for visible rows only?

In that case there shouldn't be the need at all for placeholders.

+ * Return a generic <tr> placeholder
+ * double linked with the <tr> itself to simplify
+ * tbody swaps / filtering of undesired rows.
+ * @param {HTMLTableRowElement} tr
+ * @returns {Comment}
+ */
+function getPlaceholder(tr)
+{
+  let placeholder = tr.placeholder;
+  if (!placeholder)
+  {
+    placeholder = document.createComment("placeholder");
+    tr.placeholder = placeholder;
+    placeholder.placeholder = tr;
+  }
+  return placeholder;
+}
+
+/**
+ * Put back all hidden table rows and eventually
+ * filters those that do not match the state or type.
+ * @param {HTMLTableSectionElement} tbody
+ * @param {DOMStringMap} dataset
+ */
+function updateRows(tbody, dataset)
+{
+  // use filters to match nodes or retrieve them later on
+  const selector = getCSSFilter(dataset);
+  for (const node of tbody.childNodes)
+  {
+    // comments linked to table rows that
+    // won't match selector, will be used
+    // to place their linked row back in
+    if (
+      node.nodeType === Node.COMMENT_NODE &&
+      (!selector || !node.placeholder.matches(selector))
+    )
+    {
+      tbody.replaceChild(node.placeholder, node);
+    }
+  }
+
+  // if there is a list of nodes to filter
+  if (selector)
+  {
+    // every matched row should bre replaced with its placeholder
+    for (const tr of tbody.querySelectorAll(selector))
+    {
+      tbody.replaceChild(getPlaceholder(tr), tr);
+    }
+  }
+}
+
+function getCSSFilter(dataset)

[Thomas Greiner, 2018/02/28 18:18:08]

Detail: This name confused me a bit because (a) "filter" is ambiguous and may
refer to  ad blocking filters and (b) it doesn't indicate what the CSS selector
is for.

What about, for instance, renaming it to something like "getRecordSelector"?

[a.giammarchi, 2018/02/28 19:07:13]

it actually returns a CSS selector that filters rows but since we're going with
manual classes updates this whole part won't be needed anymore.

+{
+  const {filterState, filterType} = dataset;
+
+  // create a list of possible CSS filter
+  // excluding #background-items from the list of rows to consider
+  const query = [];
+  if (filterState)
+  {
+    query.push(`tr:not(#background-items):not([data-state="${filterState}"])`);
+  }
+  if (filterType)
+  {
+    query.push(`tr:not(#background-items):not([data-type="${filterType}"])`);
+  }
+  return query.join(",");
+}
+
 document.addEventListener("DOMContentLoaded", () =>
 {
   let container = document.getElementById("items");
   let table = container.querySelector("tbody");
+  let bgItems = container.querySelector("#background-items");
   let template = document.querySelector("template").content.firstElementChild;
+  const records = [];
 
   document.getElementById("reload").addEventListener("click", () =>
   {
     ext.devtools.inspectedWindow.reload();
   }, false);
 
   document.getElementById("filter-state").addEventListener("change", (event) =>
   {
     container.dataset.filterState = event.target.value;
+    updateRows(table, container.dataset);
   }, false);
 
   document.getElementById("filter-type").addEventListener("change", (event) =>
   {
     container.dataset.filterType = event.target.value;
+    updateRows(table, container.dataset);
   }, false);
 
   ext.onMessage.addListener((message) =>
   {
     switch (message.type)
     {
       case "add-record":
-        table.appendChild(createRecord(message.request, message.filter,
-                                       template));
+        let record = createRecord(message.request, message.filter, template);
+        records.push(record);
+        // avoid pushing rows that are being filtered
+        // and use their placeholder when that is the case
+        let selector = getCSSFilter(container.dataset);
+        let node = (!selector || !record.matches(selector)) ?
+                    record : getPlaceholder(record);
+        if (message.request.docDomain == null)

[Thomas Greiner, 2018/02/28 18:18:08]

Note: We'll be able to check `message.request.pageId == -1` after #6402 lands,
according to the code that's currently under review. No need to make any changes
at this point but I thought it's worth mentioning that this part of the code is
subject to change.

[a.giammarchi, 2018/02/28 19:07:13]

then I prefer making that one a blocking of this one, for the simple reason I
don't think this approach I've implemented is bullet proof at all.

What do you think?

+        {
+          table.appendChild(node);
+        }
+        else
+        {
+          table.insertBefore(node, bgItems);
+        }
         break;
 
       case "update-record":
-        let oldRow = table.getElementsByTagName("tr")[message.index];
+        let oldRow = records[message.index];
         let newRow = createRecord(message.request, message.filter, template);
-        oldRow.parentNode.replaceChild(newRow, oldRow);
+        records[message.index] = newRow;
+        let liveRow = oldRow.parentNode ? oldRow : oldRow.placeholder;
+        liveRow.parentNode.replaceChild(newRow, liveRow);
         newRow.classList.add("changed");
         container.classList.add("has-changes");
         break;
 
       case "remove-record":
-        let row = table.getElementsByTagName("tr")[message.index];
-        row.parentNode.removeChild(row);
+        let row = records.splice(message.index, 1)[0];
+        let live = row.parentNode ? row : row.placeholder;
+        live.parentNode.removeChild(live);
         container.classList.add("has-changes");
         break;
 
       case "reset":
+        records.splice(0);
         table.innerHTML = "";
+        table.appendChild(bgItems);
         container.classList.remove("has-changes");
         break;
     }
   });
 
   window.addEventListener("message", (event) =>
   {
     switch (event.data.type)
     {
       case "performSearch":
         performSearch(table, event.data.queryString);
         lastFilterQuery = event.data.queryString;
         break;
       case "cancelSearch":
         cancelSearch(table);
         lastFilterQuery = null;
         break;
     }
   });
 
   // Since Chrome 54 the themeName is accessible, for earlier versions we must
   // assume the default theme is being used.
   // https://bugs.chromium.org/p/chromium/issues/detail?id=608869
   let theme = browser.devtools.panels.themeName || "default";
   document.body.classList.add(theme);
 }, false);