Issue 4361 - Pass all parameters to initEvent

Issue 4361 - Pass all parameters to initEvent

[kzar, 2016-09-15 17:57:59]

Patch Set 1

[Sebastian Noack, 2016-09-16 12:42:53]

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content.js
File safari/ext/content.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content....
safari/ext/content.js:156: evt.initEvent(eventName, false, true);
"load" and "error" event don't bubble. I think "beforeload" (above) doesn't
buuble either,but feel free to double check.

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
safari/include.youtube.js:26: beforeLoadEvent.initEvent("beforeload", false,
true);
Any reason, we don't simply use ext.backgroundPage.sendMessageSync() here?

[Sebastian Noack, 2016-09-16 12:48:52]

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content.js
File safari/ext/content.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content....
safari/ext/content.js:156: evt.initEvent(eventName, false, true);
On 2016/09/16 12:42:52, Sebastian Noack wrote:
> "load" and "error" event don't bubble. I think "beforeload" (above) doesn't
> buuble either,but feel free to double check.

Sorry, got it the wrong way around, the second argument (bubbles) is actually
correctly set to false. However, "load" and "error" events aren't cancelable, so
the third argument needs to be set to false too, here.

[kzar, 2016-09-16 13:00:43]

Patch Set 2 : load and error events shouldn't be cancelable

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content.js
File safari/ext/content.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/ext/content....
safari/ext/content.js:156: evt.initEvent(eventName, false, true);
On 2016/09/16 12:48:52, Sebastian Noack wrote:
> On 2016/09/16 12:42:52, Sebastian Noack wrote:
> > "load" and "error" event don't bubble. I think "beforeload" (above) doesn't
> > buuble either,but feel free to double check.
> 
> Sorry, got it the wrong way around, the second argument (bubbles) is actually
> correctly set to false. However, "load" and "error" events aren't cancelable,
so
> the third argument needs to be set to false too, here.

Done.

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
safari/include.youtube.js:26: beforeLoadEvent.initEvent("beforeload", false,
true);
On 2016/09/16 12:42:52, Sebastian Noack wrote:
> Any reason, we don't simply use ext.backgroundPage.sendMessageSync() here?

I'm not sure, I can't remember to be honest.

[Sebastian Noack, 2016-09-16 13:29:03]

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
safari/include.youtube.js:26: beforeLoadEvent.initEvent("beforeload", false,
true);
On 2016/09/16 13:00:43, kzar wrote:
> On 2016/09/16 12:42:52, Sebastian Noack wrote:
> > Any reason, we don't simply use ext.backgroundPage.sendMessageSync() here?
> 
> I'm not sure, I can't remember to be honest.

Then any reason you don't change it? That way this code would be redundant here.

[kzar, 2016-09-16 14:04:24]

Patch Set 3 : Use sendMessageSync

Note: I've not tested this patch set yet.

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353238/safari/include.yout...
safari/include.youtube.js:26: beforeLoadEvent.initEvent("beforeload", false,
true);
On 2016/09/16 13:29:03, Sebastian Noack wrote:
> On 2016/09/16 13:00:43, kzar wrote:
> > On 2016/09/16 12:42:52, Sebastian Noack wrote:
> > > Any reason, we don't simply use ext.backgroundPage.sendMessageSync() here?
> > 
> > I'm not sure, I can't remember to be honest.
> 
> Then any reason you don't change it? That way this code would be redundant
here.

Done.

[Sebastian Noack, 2016-09-16 14:06:43]

https://codereview.adblockplus.org/29353237/diff/29353297/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353297/safari/include.yout...
safari/include.youtube.js:26: key: "safariContentBlocker"} != true))
Here is a typo. You are essentially always calling
ext.backgroundPage.sendMessageSync(true).

[kzar, 2016-09-16 14:10:36]

Patch Set 4 : Fixed typo

Note: Not tested this patch set yet either.

https://codereview.adblockplus.org/29353237/diff/29353297/safari/include.yout...
File safari/include.youtube.js (right):

https://codereview.adblockplus.org/29353237/diff/29353297/safari/include.yout...
safari/include.youtube.js:26: key: "safariContentBlocker"} != true))
On 2016/09/16 14:06:43, Sebastian Noack wrote:
> Here is a typo. You are essentially always calling
> ext.backgroundPage.sendMessageSync(true).

Whoops you are right, fixed.

[Sebastian Noack, 2016-09-16 14:12:32]

LGTM