Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(101)

Issue 29752559: Issue 6586 - Fixed WebRTC wrapper after removing ext.webRequest.onBeforeRequest (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by Sebastian Noack
Modified:
3 months ago
Reviewers:
kzar
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Moved request.blockedByRTCWrapper message listener below other message listener #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -50 lines) Patch
M lib/requestBlocker.js View 1 6 chunks +65 lines, -50 lines 3 comments Download

Messages

Total messages: 4
Sebastian Noack
3 months ago (2018-04-14 11:55:05 UTC) #1
kzar
https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js#newcode198 lib/requestBlocker.js:198: let [docDomain, sitekey, specificOnly] = getDocumentInfo(page, frame, Perhaps it ...
3 months ago (2018-04-17 12:46:40 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29752559/diff/29752562/lib/requestBlocker.js#newcode198 lib/requestBlocker.js:198: let [docDomain, sitekey, specificOnly] = getDocumentInfo(page, frame, On 2018/04/17 ...
3 months ago (2018-04-17 12:56:47 UTC) #3
kzar
3 months ago (2018-04-17 13:54:06 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5