Issue 5464 - Ignore and whitelist WebRTC if not supported

Issue 5464 - Ignore and whitelist WebRTC if not supported

If a filter covers all three of WebSocket, WebRTC, and at least one HTTP content type (e.g. a filter like just "foo"), we generate a rule with a generic URL scheme pattern, like "^[^:]+:(//)?". But since WebRTC is still not supported in the safari bookmark's version of core, we end up generating two rules in Safari for such a filter, one for "^wss?://" and one for "^https?://". If WebRTC is not supported, we need to ignore WebRTC.

Also since we'll be generating a generic URL scheme pattern that covers WebRTC anyway, we need to whitelist WebRTC so no WebRTC URLs are inadvertently blocked.

[Manish Jethani, 2017-08-01 08:24:21]

Patch Set 1

If a filter covers all three of WebSocket, WebRTC, and at least one HTTP content
type (e.g. a filter like just "foo"), we generate a rule with a generic URL
scheme pattern, like "^[^:]+:(//)?". But since WebRTC is still not supported in
the safari bookmark's version of core, we end up generating two rules in Safari
for such a filter, one for "^wss?://" and one for "^https?://". If WebRTC is not
supported, we need to ignore WebRTC.

Also since we'll be generating a generic URL scheme pattern that covers WebRTC
anyway, we need to whitelist WebRTC so no WebRTC URLs are inadvertently blocked.

The alternative is to update the core dependency in adblockpluschrome's safari
bookmarklet, but this is currently giving some errors and looks like more work
that is out of the scope of what we're trying to do here.

[Manish Jethani, 2017-08-01 08:40:53]

Patch Set 2: Add comment

[Manish Jethani, 2017-08-01 08:48:58]

Patch Set 3: Fix bug

Doh, there was a bug in the call to the array's push function. Fixed now.

[kzar, 2017-08-01 11:15:31]

Maybe I'm missing something, but if WebRTC connections aren't supported why do
we need to whitelist them? Surely they won't work anyway? Also please could you
file an issue for this change?

[Manish Jethani, 2017-08-01 11:33:24]

On 2017/08/01 11:15:31, kzar wrote:
> Maybe I'm missing something, but if WebRTC connections aren't supported why do
> we need to whitelist them? Surely they won't work anyway? Also please could
you
> file an issue for this change?

Sorry the description was a bit misleading. The support we're talking about here
is the support in core. If core doesn't support WebRTC (because we're using an
older version of core), then we need to ignore WebRTC in our code here.

But the browser may support WebRTC (Safari 11) so we still have to whitelist
those URLs.

[kzar, 2017-08-01 13:08:54]

After discussing with Sebastian I'm not sure about the best approach here. IMO
the bitwise logic is starting to get rather confusing. Please could you open an
issue so the discussion can continue there?

One option could be to set up a safari bookmark for adblockpluscore, point that
at whatever the most recent revision Safari can handle is and then backport the
WebRTC change on top.

Another option might be to modify typeMap in abp2blocklist.js to simply add the
WEBRTC property. A simple change, but the question there is if changing the
typeMap would have any unexpected consequences.

[Manish Jethani, 2017-08-01 13:13:43]

On 2017/08/01 13:08:54, kzar wrote:

> Another option might be to modify typeMap in abp2blocklist.js to simply add
the
> WEBRTC property. A simple change, but the question there is if changing the
> typeMap would have any unexpected consequences.

Actually this is what I was doing initially. We can clone typeMap in that file
and then add WEBRTC to it, so no external effects. I like this approach for now.
We can add a comment explaining why we're doing it.

I'll open an issue for further discussion.

[Manish Jethani, 2017-08-02 06:32:32]

On 2017/08/01 13:13:43, Manish Jethani wrote:

> I'll open an issue for further discussion.

https://issues.adblockplus.org/ticket/5464

[kzar, 2017-08-21 14:01:08]

This can be closed, since we're going with the other approach[1] right? 

[1] - https://codereview.adblockplus.org/29503587/

[Manish Jethani, 2017-08-21 14:32:38]

On 2017/08/21 14:01:08, kzar wrote:
> This can be closed, since we're going with the other approach[1] right? 
> 
> [1] - https://codereview.adblockplus.org/29503587/

True, I've closed it.