Issue 7054 - Update the adblockpluscore dependency to 5cb695da5a40, adblockplusui to f86abf2efdfd

lib/devtools.js

Index: lib/devtools.js
===================================================================
--- a/lib/devtools.js
+++ b/lib/devtools.js
@@ -17,17 +17,17 @@
 
 "use strict";
 
 const {RegExpFilter,
        WhitelistFilter,
        ElemHideFilter} = require("../adblockpluscore/lib/filterClasses");
 const {SpecialSubscription} =
   require("../adblockpluscore/lib/subscriptionClasses");
-const {FilterStorage} = require("../adblockpluscore/lib/filterStorage");
+const {filterStorage} = require("../adblockpluscore/lib/filterStorage");
 const {defaultMatcher} = require("../adblockpluscore/lib/matcher");
 const {filterNotifier} = require("../adblockpluscore/lib/filterNotifier");
 const {extractHostFromFrame} = require("./url");
 const {port} = require("./messaging");
 const {HitLogger, nonRequestTypes} = require("./hitLogger");
 
 let panels = new Map();
 
@@ -152,18 +152,22 @@
   {
     browser.tabs.reload(tabId, {bypassCache: true});
 
     panel.reload = false;
     panel.reloading = true;
   }
 }
 
-function updateFilters(filters, added)
+function updateFilters(subscription, filters, added)
 {
+  let includes = subscription ?

[Manish Jethani, 2018/12/18 17:12:16]

It seems the parentheses are not required.

For indentation, I would normally do it like this:

  let includes = subscription ?
                   filter => subscription.searchFilter(filter) != -1 :
                   filters.includes.bind(filters);

[Jon Sonesen, 2018/12/19 10:05:20]

Done.

[Manish Jethani, 2018/12/19 10:25:16]

It should be aligned with the "b" in "subscription" (two spaces).

(We disabled the strict checks in ESLint because they did not entirely agree
with our style, otherwise ESLint would have complained.)

+    (filter => subscription.searchFilter(filter) != -1) :
+    filters.includes.bind(filters);
+
   for (let panel of panels.values())
   {
     for (let i = 0; i < panel.records.length; i++)
     {
       let record = panel.records[i];
 
       // If an added filter matches a request shown in the devtools panel,
       // update that record to show the new filter. Ignore filters that aren't
@@ -171,30 +175,32 @@
       // if they don't already match. In particular, in case of element hiding
       // filters, we also wouldn't know if any new element matches.
       if (added)
       {
         if (nonRequestTypes.includes(record.request.type))
           continue;
 
         let filter = matchRequest(record.request);
-        if (!filters.includes(filter))
-          continue;
+
+        if (filter)

[kzar, 2018/12/18 11:29:57]

Nit: Please add braces since the body spans more than one line. (Did this pass
linting?)

+          if (!includes(filter))

[kzar, 2018/12/18 11:29:57]

Woudln't it make more sense to do this:

if (filter && !includes(filter))

Than what you've got so far?:

if (filter)
  if (!includes(filter))

[Manish Jethani, 2018/12/18 17:12:16]

This would have to be:

  if (!filter || !includes(filter))
    ...

If the filter is null then the list doesn't include it.

[Jon Sonesen, 2018/12/19 10:05:20]

Done.

+            continue;
 
         record.filter = filter;
       }
 
       // If a filter shown in the devtools panel got removed, update that
       // record to show the filter that matches now, or none, instead.
       // For filters that aren't associated with any sub-resource request,
       // just remove the record. We wouldn't know whether another filter
       // matches instead until the page is reloaded.
       else
       {
-        if (!filters.includes(record.filter))
+        if (!includes(record.filter))
           continue;
 
         if (nonRequestTypes.includes(record.request.type))
         {
           panel.port.postMessage({
             type: "remove-record",
             index: i
           });
@@ -210,30 +216,30 @@
         index: i,
         request: record.request,
         filter: getFilterInfo(record.filter)
       });
     }
   }
 }
 
-function onFilterAdded(filter)
+function onFilterAdded(filter, subscription)

[kzar, 2018/12/18 11:29:57]

It seems weird to add the `subscription` parameter which is then never used?
(Same below.)

[Jon Sonesen, 2018/12/19 10:05:20]

Done.

[kzar, 2018/12/19 10:16:52]

Thanks, but why did you add it in the first place? How come you can safely
remove it again now?

 {
-  updateFilters([filter], true);
+  updateFilters(null, [filter], true);
 }
 
-function onFilterRemoved(filter)
+function onFilterRemoved(filter, subscription)
 {
-  updateFilters([filter], false);
+  updateFilters(null, [filter], false);
 }
 
 function onSubscriptionAdded(subscription)
 {
   if (subscription instanceof SpecialSubscription)
-    updateFilters(subscription.filters, true);
+    updateFilters(subscription, null, true);
 }
 
 browser.runtime.onConnect.addListener(newPort =>
 {
   let match = newPort.name.match(/^devtools-(\d+)$/);
   if (!match)
     return;