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

Issue 29721716: Issue 6473 - Remove WebSocket wrapper (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by Manish Jethani
Modified:
6 months, 1 week ago
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 6473 - Remove WebSocket wrapper

Patch Set 1 #

Total comments: 8

Patch Set 2 : Address comments to Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -69 lines) Patch
M inject.preload.js View 1 3 chunks +13 lines, -53 lines 0 comments Download
M lib/requestBlocker.js View 1 2 chunks +4 lines, -16 lines 0 comments Download

Messages

Total messages: 12
Manish Jethani
6 months, 1 week ago (2018-03-13 16:33:22 UTC) #1
Manish Jethani
Patch Set 1
6 months, 1 week ago (2018-03-13 16:34:19 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js#newcode141 inject.preload.js:141: * Shared request checking code, used by the RTCPeerConnection ...
6 months, 1 week ago (2018-03-13 17:43:01 UTC) #3
Manish Jethani
Patch Set 2: Address comments to Patch Set 1 https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js#newcode141 inject.preload.js:141: ...
6 months, 1 week ago (2018-03-13 18:05:12 UTC) #4
Sebastian Noack
LGTM
6 months, 1 week ago (2018-03-13 18:31:34 UTC) #5
Sebastian Noack
Argh, wait. There is also some legacy code, which is only used in combination with ...
6 months, 1 week ago (2018-03-13 18:33:35 UTC) #6
kzar
On 2018/03/13 18:33:35, Sebastian Noack wrote: > Argh, wait. There is also some legacy code, ...
6 months, 1 week ago (2018-03-13 20:55:16 UTC) #7
Manish Jethani
On 2018/03/13 18:33:35, Sebastian Noack wrote: > Argh, wait. There is also some legacy code, ...
6 months, 1 week ago (2018-03-13 22:12:11 UTC) #8
Manish Jethani
On 2018/03/13 20:55:16, kzar wrote: > On 2018/03/13 18:33:35, Sebastian Noack wrote: > > Argh, ...
6 months, 1 week ago (2018-03-13 22:13:30 UTC) #9
Sebastian Noack
On 2018/03/13 22:12:11, Manish Jethani wrote: > On 2018/03/13 18:33:35, Sebastian Noack wrote: > > ...
6 months, 1 week ago (2018-03-13 22:51:05 UTC) #10
Manish Jethani
On 2018/03/13 22:51:05, Sebastian Noack wrote: > On 2018/03/13 22:12:11, Manish Jethani wrote: > > ...
6 months, 1 week ago (2018-03-13 23:03:34 UTC) #11
Sebastian Noack
6 months, 1 week ago (2018-03-13 23:13:10 UTC) #12
On 2018/03/13 23:03:34, Manish Jethani wrote:
> Correct me if I'm wrong, but setting the content security policy to
"connect-src
> http:" in a document's response headers effectively blocks all WebSocket
> connections, which is what is desired here. How is this related to the API
> wrapper?

True, filters like $webrtc,domain=example.com would still work with this
workaround, regardless of the WebSocket wrapper. But leaving this code around
just for Chrome <=57 still wouldn't be worth it. Anyway, I guess we can tackle
that separately (with Dave's upcoming changes). LGTM.
Sign in to reply to this message.

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