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
@@ -15,19 +15,19 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 const {RegExpFilter,
        WhitelistFilter,
        ElemHideFilter} = require("../adblockpluscore/lib/filterClasses");
-const {SpecialSubscription} =
+const {SpecialSubscription, Subscription} =
   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();
 
@@ -154,50 +154,55 @@
 
     panel.reload = false;
     panel.reloading = true;
   }
 }
 
 function updateFilters(filters, added)
 {
+  let includes = filters.includes.bind(filters);

[Manish Jethani, 2018/12/07 09:14:47]

Sorry about my slightly misleading example earlier, this will fail if `filters`
is not an array.

We need something like:

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

[Manish Jethani, 2018/12/07 09:14:47]

We also need to pass the `Subscription` object to this function in
`onSubscriptionAdded`.

[Manish Jethani, 2018/12/07 09:18:22]

On second thoughts, I think we should just add a new `subscription` parameter to
this function.

  function updateFilters(subscription, filters, added)

We can pass null everywhere except `onSubscriptionAdded`, in which case
`filters` will be null instead.

[Jon Sonesen, 2018/12/09 15:52:09]

What do you think about making it an optional parameter with a default of null?

`function updateFilters(subscriptions=null, filters, added)`

[Manish Jethani, 2018/12/10 07:48:27]

That would be fine but not very useful since the last parameter `added` must
always be given.

The current version is odd, let's make it like this:

  function updateFilters(subscription, filters, added)

Also I notice that the code still bombs on the first line because
`filters.includes` does not exist.

In general, as part of the dependency update (or any patch), you have to test
every single code path you are touching. We can't just commit something that has
not been tested.

+
+  if (filters instanceof Subscription)
+    includes = filter => filters.searchFilter(filter) != -1;
+
   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))
+        if (includes(record.request.type))

[Manish Jethani, 2018/12/07 09:14:47]

This appears to be incorrect.

[Jon Sonesen, 2018/12/09 15:52:09]

Acknowledged.

           continue;
 
         let filter = matchRequest(record.request);
-        if (!filters.includes(filter))
+        if (!includes(filter))
           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))
+        if (includes(record.request.type))

[Manish Jethani, 2018/12/07 09:14:47]

This too appears to be incorrect.

[Jon Sonesen, 2018/12/09 15:52:09]

Acknowledged.

         {
           panel.port.postMessage({
             type: "remove-record",
             index: i
           });
           panel.records.splice(i--, 1);
           continue;
         }