Issue 1258 - Block ads in YouTube's HTML5 player on Safari

safari/include.youtube.js

Index: safari/include.youtube.js
===================================================================
--- a/safari/include.youtube.js
+++ b/safari/include.youtube.js
@@ -22,11 +22,13 @@
   if (!ext.backgroundPage.sendMessageSync({type: "get-domain-enabled-state"}).enabled)
     return;
 
+  var badArgumentsRegex = /^((.*_)?(ad|ads|afv|adsense)(_.*)?|(ad3|st)_module|prerolls|interstitial|infringe|iv_cta_url)$/;
+
   function rewriteFlashvars(flashvars)
   {
     var pairs = flashvars.split("&");
     for (var i = 0; i < pairs.length; i++)
-      if (/^((ad|afv|adsense)(_.*)?|(ad3|st)_module|prerolls|interstitial|infringe|iv_cta_url)=/.test(pairs[i]))
+      if (badArgumentsRegex.test(pairs[i].split("=")[0]))
         pairs.splice(i--, 1);
     return pairs.join("&");
   }
@@ -66,21 +68,94 @@
       player.parentNode.replaceChild(newPlayer, player);
   }
 
+  function runInPage(fn, arg)
+  {
+    var script = document.createElement("script");
+    script.type = "application/javascript";
+    script.async = false;
+    script.textContent = "(" + fn + ")(" + arg + ");";
+    document.documentElement.appendChild(script);
+    document.documentElement.removeChild(script);
+  }
+
   document.addEventListener("beforeload", function(event)
   {
     if ((event.target.localName == "object" || event.target.localName == "embed") && /:\/\/[^\/]*\.ytimg\.com\//.test(event.url))
       patchPlayer(event.target);
   }, true);
 
-  // if history.pushState is available, YouTube uses the history API
-  // when navigation from one video to another, and tells the flash
-  // player with JavaScript which video and which ads to show next,
-  // bypassing our flashvars rewrite code. So we disable
-  // history.pushState before YouTube's JavaScript runs.
-  var script = document.createElement("script");
-  script.type = "application/javascript";
-  script.async = false;
-  script.textContent = "History.prototype.pushState = undefined;";
-  document.documentElement.appendChild(script);
-  document.documentElement.removeChild(script);
+  runInPage(function(badArgumentsRegex)
+  {
+    // if history.pushState is available, YouTube uses the history API
+    // when navigation from one video to another, and tells the flash
+    // player with JavaScript which video and which ads to show next,
+    // bypassing our flashvars rewrite code. So we disable
+    // history.pushState before YouTube's JavaScript runs.
+    History.prototype.pushState = undefined;
+
+    // the HMLT5 player is configured via ytplayer.config.args. We have

[Wladimir Palant, 2014/08/28 16:43:38]

Nit: "The HTML5 player" (properly capitalized and spelled correctly).

[Sebastian Noack, 2014/08/29 14:51:24]

Done.

+    // to make sure that ad-related arguments are ignored as they are set.
+    var ytplayer = undefined;

[Wladimir Palant, 2014/08/28 16:43:38]

What if ytplayer variable was set already? I'd recommend saving window.ytplayer
first and assigning the original value after creating the property.

[Sebastian Noack, 2014/08/29 14:51:24]

This hack only works anyway, if our JavaScript runs before YouTube's JavaScript,
which is made sure by http://codereview.adblockplus.org/4757413714460672/. And
in that case there is no way ytplayer is already set.

+    Object.defineProperty(window, "ytplayer",
+    {
+      get: function()
+      {
+        return ytplayer;
+      },
+      set: function(rawYtplayer)
+      {
+        if (typeof rawYtplayer != "object")

[Wladimir Palant, 2014/08/28 16:43:38]

console.log(typeof null);

This should be:

  if (!rawYtplayer || typeof rawYtplayer != "object")

[Sebastian Noack, 2014/08/29 14:51:24]

Thanks for reminding me that null is an object. I'm used to know that. :(

+        {
+          ytplayer = rawYtplayer;
+          return;
+        }
+
+        var config = undefined;

[Wladimir Palant, 2014/08/28 16:43:38]

Here as well: what if the config property is already set?

[Sebastian Noack, 2014/08/29 14:51:24]

This was handled by the loop below:

  for (var prop in rawYtplayer)
    ytplayer[prop] = rawYtplayer[prop];

However, now where I use inheritance instead, I have to explicitly handle that
case.

+        ytplayer = {
+          get config()
+          {
+            return config;
+          },
+          set config(rawConfig)
+          {
+            if (typeof rawConfig != "object")

[Wladimir Palant, 2014/08/28 16:43:38]

Here as well: please consider the possibility that rawConfig is null.

[Sebastian Noack, 2014/08/29 14:51:24]

Done.

+            {
+              config = rawConfig;
+              return;
+            }
+
+            var args = undefined;
+            config = {
+              get args()
+              {
+                return args;
+              },
+              set args(rawArgs)
+              {
+                if (typeof rawArgs != "object")
+                {
+                  args = rawArgs;
+                  return;
+                }
+
+                args = {};
+                for (var arg in rawArgs)
+                {
+                  if (!badArgumentsRegex.test(arg))
+                    args[arg] = rawArgs[arg];
+                }
+              }
+            };

[Wladimir Palant, 2014/08/28 16:43:38]

There is a recurring pattern here: you wrap objects to change the value of a
particular property when it is being set. Maybe put this into a helper function
instead of repeating the same code three times?

[Sebastian Noack, 2014/08/29 14:51:24]

Of course, I already considered that. However, the pattern differs.
window.ytplayer needs to be defined with Object.defineProperty instead wrapping
the original object. And ytplayer.config.args must be processed differently,
than the other properties. So abstracting those cases in a reusable function
would just make the code even more complex.

+
+            for (var prop in rawConfig)
+              config[prop] = rawConfig[prop];
+          }
+        };
+
+        for (var prop in rawYtplayer)
+          ytplayer[prop] = rawYtplayer[prop];

[Wladimir Palant, 2014/08/28 16:43:38]

I think that inheriting from rawYtplayer would be a better idea than copying the
properties:

  ytplayer = Object.create(rawYtplayer);

Same for the config object.

[Sebastian Noack, 2014/08/29 14:51:24]

Done. However, I'm not convinced that it is a better or worse idea. The
resulting amount of code is the same.

+      },
+      configurable: true
+    });
+  }, badArgumentsRegex);
 })();