Issue 4580 - Make filter/request logging use plain tabIds, prepare for multi-tab requests

Issue 4580 - Make filter/request logging use plain tabIds, prepare for multi-tab requests

[Sebastian Noack, 2018-03-31 01:18:53]

https://codereview.adblockplus.org/29737602/diff/29737603/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737602/diff/29737603/lib/requestBlocker....
lib/requestBlocker.js:80: let tabIds = tabId != -1 ? [tabId] : [];
This is in preparation for requests that relate to n tabs (i.e. request send
from inside Service/Shared Workers). Currently, those requests are still not
associated with any tab, but I work on that, and in order to not change APIs
here again (and since the main change will be quite big already) I already went
ahead and made the tabs passed an array, here.

[kzar, 2018-04-03 17:14:04]

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js#new...
lib/devtools.js:34: // and recorded items. We can't use a PageMap here,
Maybe remove/change this comment since it wouldn't be consistent to use a
PageMap anymore anyway?

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js#new...
lib/devtools.js:357: ext.pages.onLoading.addListener(onLoading);
What about these? Well I don't mind if we remove them later, but in which case I
guess the codereview title (commit message) should be tweaked?

https://codereview.adblockplus.org/29737602/diff/29739586/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/requestBlocker....
lib/requestBlocker.js:80: let tabIds = tabId != -1 ? [tabId] : [];
Nit: Seems like switching the order and using == would be slightly easier to
grok?

https://codereview.adblockplus.org/29737602/diff/29739586/lib/stats.js
File lib/stats.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/stats.js#newcode77
lib/stats.js:77: Prefs.blocked_total++;
Seems like before we didn't increment this counter for hits not associated with
a tab, but now we do?

[Sebastian Noack, 2018-04-04 01:50:34]

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js#new...
lib/devtools.js:34: // and recorded items. We can't use a PageMap here,
On 2018/04/03 17:14:03, kzar wrote:
> Maybe remove/change this comment since it wouldn't be consistent to use a
> PageMap anymore anyway?

Well spotted. Done.

https://codereview.adblockplus.org/29737602/diff/29739586/lib/devtools.js#new...
lib/devtools.js:357: ext.pages.onLoading.addListener(onLoading);
On 2018/04/03 17:14:03, kzar wrote:
> What about these? Well I don't mind if we remove them later, but in which case
I
> guess the codereview title (commit message) should be tweaked?

There is quite some magic around the ext.pages.onLoading event. Hence there is
no trivial replacement using the plain browser.* API. It might be possible
though that not all of that magic is necessary for the devtools panel, but I
would have to verify that. So I'd rather get rid of ext.page.onLoading
(potentially not only here) with a separate change. But I updated the review
title (and commit message) to be more specific, as you requested.

https://codereview.adblockplus.org/29737602/diff/29739586/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/requestBlocker....
lib/requestBlocker.js:80: let tabIds = tabId != -1 ? [tabId] : [];
On 2018/04/03 17:14:03, kzar wrote:
> Nit: Seems like switching the order and using == would be slightly easier to
> grok?

If you think in numbers, yes, this way around seems slightly backwards:

  ~is(id, -1) => use(id)

However, if you think in terms of what "tabId != -1" actually means, the logic
seems simpler to follow this way:

  valid(id) => use(id)

Also it doesn't matter much, for the time being as my next patch under review is
already removing this line again anyway.

https://codereview.adblockplus.org/29737602/diff/29739586/lib/stats.js
File lib/stats.js (right):

https://codereview.adblockplus.org/29737602/diff/29739586/lib/stats.js#newcode77
lib/stats.js:77: Prefs.blocked_total++;
On 2018/04/03 17:14:03, kzar wrote:
> Seems like before we didn't increment this counter for hits not associated
with
> a tab, but now we do?

Yes. To me it seems a mistake that we didn't before. Mind that this code exists
for longer than we actually process requests that aren't associated with a tab.

[Sebastian Noack, 2018-04-04 02:01:34]

Also note that this patch is based on
https://codereview.adblockplus.org/29737568/ (which you didn't comment on yet).

[kzar, 2018-04-05 10:13:00]

LGTM