Issue 6586 - Fixed WebRTC wrapper after removing ext.webRequest.onBeforeRequest

Issue 6586 - Fixed WebRTC wrapper after removing ext.webRequest.onBeforeRequest

[kzar, 2018-04-17 12:46:40]

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker....
lib/requestBlocker.js:198: let [docDomain, sitekey, specificOnly] =
getDocumentInfo(page, frame,
Perhaps it would be a nicer abstraction to have a function called matchRequests
which takes a collection of URLs and returns an iterator of the matches? It
could do the work of getDocumentInfo then, as well as optionally calling
logRequest.

[Sebastian Noack, 2018-04-17 12:56:47]

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker....
lib/requestBlocker.js:198: let [docDomain, sitekey, specificOnly] =
getDocumentInfo(page, frame,
On 2018/04/17 12:46:39, kzar wrote:
> Perhaps it would be a nicer abstraction to have a function called
matchRequests
> which takes a collection of URLs and returns an iterator of the matches? It
> could do the work of getDocumentInfo then, as well as optionally calling
> logRequest.

FWIW, I'm not too happy with this abstraction (just slightly more than repeating
myself 3 times in the same file). However, at least I'm pretty sure that this
way, the code just gets inlined by the JIT. If I move the loop in here, I'm
mildly concerned that this might effect the performance in the onBeforeRequest
listener. In particular when turning it into a generator, I know for sure that
this will cause the JIT to struggle.

As for calling logRequest(), I don't see how this can be generalized (in a
reasonable way), since the onBeforeRequest listener must call getRelatedTabIds()
first, while the message listener for the WebRTC wrapper must pass in page.id.

[kzar, 2018-04-17 13:54:06]

LGTM

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker....
lib/requestBlocker.js:198: let [docDomain, sitekey, specificOnly] =
getDocumentInfo(page, frame,
On 2018/04/17 12:56:47, Sebastian Noack wrote:
> On 2018/04/17 12:46:39, kzar wrote:
> > Perhaps it would be a nicer abstraction to have a function called
> matchRequests
> > which takes a collection of URLs and returns an iterator of the matches? It
> > could do the work of getDocumentInfo then, as well as optionally calling
> > logRequest.
> 
> FWIW, I'm not too happy with this abstraction (just slightly more than
repeating
> myself 3 times in the same file). However, at least I'm pretty sure that this
> way, the code just gets inlined by the JIT. If I move the loop in here, I'm
> mildly concerned that this might effect the performance in the onBeforeRequest
> listener. In particular when turning it into a generator, I know for sure that
> this will cause the JIT to struggle.
> 
> As for calling logRequest(), I don't see how this can be generalized (in a
> reasonable way), since the onBeforeRequest listener must call
getRelatedTabIds()
> first, while the message listener for the WebRTC wrapper must pass in page.id.

Yea, fair enough.