Issue 29407555: Noissue - Add webrtc filter type to filter documentation (Closed)
|
Created:
April 9, 2017, 5:12 a.m. by kzar Modified:
May 31, 2017, 11:10 a.m. CC:
Wladimir Palant Visibility:
Public. |
DescriptionNoissue - Add webrtc filter type to filter documentation
Patch Set 1 #
Total comments: 7
Patch Set 2 : Tweak phrasing #Patch Set 3 : Reword to mention ICE servers #MessagesTotal messages: 11
Patch Set 1
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> While it's probably not worth to add support for the legacy blocking mode on Safari, I wonder whether connection to WebRTC ICE servers are actually blocked through Content Blockers? If not done, yet all it would take is mapping $webrtc filters to "raw". Same for $websocket. Manish, can you check this? Also, I wonder whether we should specifically mention ICE servers here, as the $webrtc option is essentially blocking requests to ICE servers, not to the actual peers.
Patch Set 2 : Tweak phrasing https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/22 13:59:37, Sebastian Noack wrote: > While it's probably not worth to add support for the legacy blocking mode on > Safari, I wonder whether connection to WebRTC ICE servers are actually blocked > through Content Blockers? If not done, yet all it would take is mapping $webrtc > filters to "raw". Same for $websocket. Manish, can you check this? > > Also, I wonder whether we should specifically mention ICE servers here, as the > $webrtc option is essentially blocking requests to ICE servers, not to the > actual peers. I see what you mean, but equally I'm trying to keep this short and simple. I've made a small tweak to make it clearer that the connections are being made via a peer connection instance, what do you think?
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/22 14:06:41, kzar wrote: > On 2017/05/22 13:59:37, Sebastian Noack wrote: > > While it's probably not worth to add support for the legacy blocking mode on > > Safari, I wonder whether connection to WebRTC ICE servers are actually blocked > > through Content Blockers? If not done, yet all it would take is mapping > $webrtc > > filters to "raw". Same for $websocket. Manish, can you check this? > > > > Also, I wonder whether we should specifically mention ICE servers here, as the > > $webrtc option is essentially blocking requests to ICE servers, not to the > > actual peers. > > I see what you mean, but equally I'm trying to keep this short and simple. I've > made a small tweak to make it clearer that the connections are being made via a > peer connection instance, what do you think? Why not using the correct terminology and talk about ICE servers? In fact one could argue that the actual peer connection (which is not blocked) is made by the same instance (after all that's what the API is named like), based on the peers discovered through the ICE server(s).
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/22 14:23:07, Sebastian Noack wrote: > On 2017/05/22 14:06:41, kzar wrote: > > On 2017/05/22 13:59:37, Sebastian Noack wrote: > > > While it's probably not worth to add support for the legacy blocking mode on > > > Safari, I wonder whether connection to WebRTC ICE servers are actually > blocked > > > through Content Blockers? If not done, yet all it would take is mapping > > $webrtc > > > filters to "raw". Same for $websocket. Manish, can you check this? > > > > > > Also, I wonder whether we should specifically mention ICE servers here, as > the > > > $webrtc option is essentially blocking requests to ICE servers, not to the > > > actual peers. > > > > I see what you mean, but equally I'm trying to keep this short and simple. > I've > > made a small tweak to make it clearer that the connections are being made via > a > > peer connection instance, what do you think? > > Why not using the correct terminology and talk about ICE servers? In fact one > could argue that the actual peer connection (which is not blocked) is made by > the same instance (after all that's what the API is named like), based on the > peers discovered through the ICE server(s). Do you have a suggestion of how we can do that without making it unclear to someone who isn't a developer who's familiar with how the WebRTC API works?
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/22 14:26:44, kzar wrote: > On 2017/05/22 14:23:07, Sebastian Noack wrote: > > On 2017/05/22 14:06:41, kzar wrote: > > > On 2017/05/22 13:59:37, Sebastian Noack wrote: > > > > While it's probably not worth to add support for the legacy blocking mode > on > > > > Safari, I wonder whether connection to WebRTC ICE servers are actually > > blocked > > > > through Content Blockers? If not done, yet all it would take is mapping > > > $webrtc > > > > filters to "raw". Same for $websocket. Manish, can you check this? > > > > > > > > Also, I wonder whether we should specifically mention ICE servers here, as > > the > > > > $webrtc option is essentially blocking requests to ICE servers, not to the > > > > actual peers. > > > > > > I see what you mean, but equally I'm trying to keep this short and simple. > > I've > > > made a small tweak to make it clearer that the connections are being made > via > > a > > > peer connection instance, what do you think? > > > > Why not using the correct terminology and talk about ICE servers? In fact one > > could argue that the actual peer connection (which is not blocked) is made by > > the same instance (after all that's what the API is named like), based on the > > peers discovered through the ICE server(s). > > Do you have a suggestion of how we can do that without making it unclear to > someone who isn't a developer who's familiar with how the WebRTC API works? I don't have a particular suggestion, but how is mentioning the RTCPeerConnection API/class any less technical than the existence of ICE servers? I probably would keep mentioning both, so if you know about WebRTC but not about ICE servers, you still know what this is about.
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/22 13:59:37, Sebastian Noack wrote: > While it's probably not worth to add support for the legacy blocking mode on > Safari, I wonder whether connection to WebRTC ICE servers are actually blocked > through Content Blockers? If not done, yet all it would take is mapping $webrtc > filters to "raw". Same for $websocket. Manish, can you check this? Safari does not support RTCPeerConnection. I just checked WebSocket, and it does indeed block it. I think we can map $websocket filters directly.
https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests initiated via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock Plus for Firefox and Safari not yet supported)}}</li> On 2017/05/29 13:38:19, Manish Jethani wrote: > On 2017/05/22 13:59:37, Sebastian Noack wrote: > > While it's probably not worth to add support for the legacy blocking mode on > > Safari, I wonder whether connection to WebRTC ICE servers are actually blocked > > through Content Blockers? If not done, yet all it would take is mapping > $webrtc > > filters to "raw". Same for $websocket. Manish, can you check this? > > Safari does not support RTCPeerConnection. I just checked WebSocket, and it does > indeed block it. > > I think we can map $websocket filters directly. So I guess let's map both, $websocket and $webrtc to "raw", in abp2blocklist. Even if RTCPeerConnection isn't supported yet by Safari, when they add support it might be blocked by the "raw" type as well. As for the documentation change here, let's just not mention Safari.
Patch Set 3 : Reword to mention ICE servers
On 2017/05/29 13:47:07, Sebastian Noack wrote: > https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html > File pages/filters.html (right): > > https://codereview.adblockplus.org/29407555/diff/29407556/pages/filters.html#... > pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — requests > initiated via <a > href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> > object</a> (Adblock Plus for Chrome and Opera 1.13.3 or higher required, Adblock > Plus for Firefox and Safari not yet supported)}}</li> > On 2017/05/29 13:38:19, Manish Jethani wrote: > > On 2017/05/22 13:59:37, Sebastian Noack wrote: > > > While it's probably not worth to add support for the legacy blocking mode on > > > Safari, I wonder whether connection to WebRTC ICE servers are actually > blocked > > > through Content Blockers? If not done, yet all it would take is mapping > > $webrtc > > > filters to "raw". Same for $websocket. Manish, can you check this? > > > > Safari does not support RTCPeerConnection. I just checked WebSocket, and it > does > > indeed block it. > > > > I think we can map $websocket filters directly. > > So I guess let's map both, $websocket and $webrtc to "raw", in abp2blocklist. > Even if RTCPeerConnection isn't supported yet by Safari, when they add support > it might be blocked by the "raw" type as well. I've filed an issue for this: https://issues.adblockplus.org/ticket/5283
LGTM 
|
