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} =
+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();
 
 function isActivePanel(panel)
 {
@@ skipping 116 matching lines @@
   {
     browser.tabs.reload(tabId, {bypassCache: true});
 
     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;
         }
 
         record.filter = matchRequest(record.request);
@@ skipping 64 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);
 });