Issue 6030 - Ensure RTCPeerConnection exists before wrapping it

Issue 6030 - Ensure RTCPeerConnection exists before wrapping it

[kzar, 2017-11-14 10:36:29]

Patch Set 1

[Manish Jethani, 2017-11-14 10:57:56]

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js#n...
inject.preload.js:229: // Firefox has the option (media.peerconnection.enabled)
to disable WebRTC
Wouldn't it be better to wrap the entire code for RTCPeerConnection here into
the if block?

[Wladimir Palant, 2017-11-14 11:12:50]

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js#n...
inject.preload.js:229: // Firefox has the option (media.peerconnection.enabled)
to disable WebRTC
On 2017/11/14 10:57:56, Manish Jethani wrote:
> Wouldn't it be better to wrap the entire code for RTCPeerConnection here into
> the if block?

I agree. While I'm normally not a big fan of increasing indentation level,
returning here might skip unrelated code that some future change adds below.

[kzar, 2017-11-14 11:36:36]

Patch Set 2 : Wrap WebRTC wrapping code in the if block

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29607555/diff/29607556/inject.preload.js#n...
inject.preload.js:229: // Firefox has the option (media.peerconnection.enabled)
to disable WebRTC
On 2017/11/14 11:12:49, Wladimir Palant wrote:
> On 2017/11/14 10:57:56, Manish Jethani wrote:
> > Wouldn't it be better to wrap the entire code for RTCPeerConnection here
into
> > the if block?
> 
> I agree. While I'm normally not a big fan of increasing indentation level,
> returning here might skip unrelated code that some future change adds below.

Done.

https://codereview.adblockplus.org/29607555/diff/29607558/inject.preload.js
File inject.preload.js (right):

https://codereview.adblockplus.org/29607555/diff/29607558/inject.preload.js#n...
inject.preload.js:240: let normalizeUrl = url =>
This change - and several like it below - are necessary since we would be
violating the no-inner-declaration ESLint rule otherwise.

[Wladimir Palant, 2017-11-14 12:17:53]

LGTM assuming that all that changed is converting regular functions to arrow
functions.

[Manish Jethani, 2017-11-14 13:14:23]

LGTM