Implemented blocking of YouTube video ads, by rewriting flashvars, for Safari

Implemented blocking of YouTube video ads, by rewriting flashvars, for Safari

[Sebastian Noack, 2013-11-28 17:16:05]

I have implemented blocking of YouTube video ads on Safari, by stripping
flashvars that trigger ads. However apparently that doesn't work for videos that
were added at runtime by loadVideoByPlayerVars(), preloadVideoByPlayerVars(), or
cueVideoByPlayerVars(). Youtube uses that APIs when you click on an other video
in the sidebar. However it seems that you can't monkey patch methods provided by
a plugin (like Flash). However "AdBlock for Safari" has the same issue. But
maybe somebody has another idea?

[Wladimir Palant, 2013-12-06 10:58:35]

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/meta...
File metadata.common (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/meta...
metadata.common:21: document_start = ext/common.js ext/content.js
include.preload.js include.youtube.js
This shouldn't be part of metadata.common - it is Safari-specific functionality,
we shouldn't add a global content script in all other browsers. This means
duplicating this line in metadata.safari but IMHO that's a lot better than
loading a dummy in Chrome and Opera.

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
File safari/include.youtube.js (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:85: });
This code was meant for Chrome where synchronous messages aren't possible -
hence all the complexity with queuing calls until a response is received. This
isn't necessary for Safari, we can do a synchronous call when the first matching
beforeload event is received.

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:87: document.addEventListener("beforeload",
onBeforeLoad, true);
Adding this listener on non-YouTube websites is too much unnecessary overhead.
This script was originally limited to youtube.com and youtube-nocookie.com
domains, something that apparently isn't possible with Safari. Checking
document.domain in this content script should do.

[Sebastian Noack, 2013-12-06 11:32:33]

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
File safari/include.youtube.js (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:87: document.addEventListener("beforeload",
onBeforeLoad, true);
On 2013/12/06 10:58:35, Wladimir Palant wrote:
> Adding this listener on non-YouTube websites is too much unnecessary overhead.
> This script was originally limited to http://youtube.com and
http://youtube-nocookie.com
> domains, something that apparently isn't possible with Safari. Checking
> document.domain in this content script should do.

I assumed that YouTube videos embedded on other websites can have ads as well?

[Wladimir Palant, 2013-12-06 11:38:08]

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
File safari/include.youtube.js (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:87: document.addEventListener("beforeload",
onBeforeLoad, true);
On 2013/12/06 11:32:33, sebastian wrote:
> I assumed that YouTube videos embedded on other websites can have ads as well?

There are two ways to embed YouTube video:

1) The old way, with an <embed> tag. There are no flashvars to rewrite here.

2) The new way, with an <iframe> tag. There *might* be flashvars here that we
can rewrite there - but document.domain is YouTube's in that case.

[Sebastian Noack, 2013-12-10 18:51:35]

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
File safari/include.youtube.js (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:85: });
On 2013/12/06 10:58:35, Wladimir Palant wrote:
> This code was meant for Chrome where synchronous messages aren't possible -
> hence all the complexity with queuing calls until a response is received. This
> isn't necessary for Safari, we can do a synchronous call when the first
matching
> beforeload event is received.

We only use synchronous messaging under the hood in the abstraction layer, to
make things work in Safari, where Chrome has proper APIs for. However this is
high-level code. And I'm not sure whether I can register another event listener
for synchronous messages in background.js. Even if that would work, it would
slow down synchronous messaging even more, because of all synchronous messages
will be processed by two event listeners then. Of course I could add an API to
the abstraction layer that will allow synchronous messaging in high level code.
But if that is required I think a solution involving asynchronous messaging
would be way simpler.

Also note that synchronous messaging will block, and prevent other code from
running, until the background page sent a response. So I would rather avoid
synchronous messaging where possible.

[Sebastian Noack, 2013-12-20 09:23:46]

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/meta...
File metadata.common (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/meta...
metadata.common:21: document_start = ext/common.js ext/content.js
include.preload.js include.youtube.js
On 2013/12/06 10:58:35, Wladimir Palant wrote:
> This shouldn't be part of metadata.common - it is Safari-specific
functionality,
> we shouldn't add a global content script in all other browsers. This means
> duplicating this line in metadata.safari but IMHO that's a lot better than
> loading a dummy in Chrome and Opera.

Done.

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
File safari/include.youtube.js (right):

http://codereview.adblockplus.org/4922142982406144/diff/5629499534213120/safa...
safari/include.youtube.js:87: document.addEventListener("beforeload",
onBeforeLoad, true);
On 2013/12/06 11:38:10, Wladimir Palant wrote:
> On 2013/12/06 11:32:33, sebastian wrote:
> > I assumed that YouTube videos embedded on other websites can have ads as
well?
> 
> There are two ways to embed YouTube video:
> 
> 1) The old way, with an <embed> tag. There are no flashvars to rewrite here.
> 
> 2) The new way, with an <iframe> tag. There *might* be flashvars here that we
> can rewrite there - but document.domain is YouTube's in that case.

Done.

[Wladimir Palant, 2014-01-13 12:47:21]

LGTM even though I'm not convinced concerning async messaging. The synchronous
messaging would only affect YouTube, the overhead might be ok if that reduces
the annoyance of replacing the player after it is initialized.

[Sebastian Noack, 2014-01-14 17:58:31]

On 2014/01/13 12:47:21, Wladimir Palant wrote:
> LGTM even though I'm not convinced concerning async messaging. The synchronous
> messaging would only affect YouTube, the overhead might be ok if that reduces
> the annoyance of replacing the player after it is initialized.

No, if you just register another event handler for synchronous messages, it
would add an overhead to every synchronous message. And integrating it in the
existing synchronous messaging code, within the abstraction layer, will make the
code unnecessarily complex. However in any case we would need some Safari
specific code in the background page (and more code in total) to use synchronous
messaging here. However the response for the asynchronous message almost always
arrives before the first YouTube video is going to be loaded. So there won't be
any annoyance.