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

lib/devtools.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";
 
 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();
 
 function isActivePanel(panel)
 {
@@ skipping 114 matching lines @@
   // previous URL.
   if (panel && panel.reload)
   {
     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
       // associated with any sub-resource request. There is no record for these
       // 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
           });
           panel.records.splice(i--, 1);
           continue;
         }
 
         record.filter = matchRequest(record.request);
       }
 
       panel.port.postMessage({
         type: "update-record",
         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;
 
   let inspectedTabId = parseInt(match[1], 10);
   let localOnBeforeRequest = onBeforeRequest.bind();
@@ skipping 28 matching lines @@
       ext.pages.onLoading.removeListener(onLoading);
       filterNotifier.off("filter.added", onFilterAdded);
       filterNotifier.off("filter.removed", onFilterRemoved);
       filterNotifier.off("subscription.added", onSubscriptionAdded);
     }
   });
 
   HitLogger.addListener(inspectedTabId, hitListener);
   panels.set(inspectedTabId, panel);
 });