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

safari/include.youtube.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 68 matching lines @@
   }
 
   document.addEventListener("beforeload", function(event)
   {
     if ((event.target.localName == "object" || event.target.localName == "embed") && /:\/\/[^\/]*\.ytimg\.com\//.test(event.url))
       patchPlayer(event.target);
   }, true);
 
   runInPage(function(badArgumentsRegex)
   {
-    // if history.pushState is available, YouTube uses the history API
+    // 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.

+    // The HTML5 player is configured via ytplayer.config.args. We have
     // 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",
     {
+      configurable: true,
       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. :(

+        if (!rawYtplayer || typeof rawYtplayer != "object")
         {
           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.

+        ytplayer = Object.create(rawYtplayer, {
+          config: {
+            enumerable: true,
+            get: function()
             {
-              config = rawConfig;
-              return;
+              return config;
+            },
+            set: function(rawConfig)
+            {
+              if (!rawConfig || typeof rawConfig != "object")
+              {
+                config = rawConfig;
+                return;
+              }
+
+              var args = undefined;
+              config = Object.create(rawConfig, {
+                args: {
+                  enumerable: true,
+                  get: function()
+                  {
+                    return args;
+                  },
+                  set: function(rawArgs)
+                  {
+                    if (!rawArgs || typeof rawArgs != "object")
+                    {
+                      args = rawArgs;
+                      return;
+                    }
+
+                    args = {};
+                    for (var arg in rawArgs)
+                    {
+                      if (!badArgumentsRegex.test(arg))
+                        args[arg] = rawArgs[arg];
+                    }
+                  }
+                }
+              });
+
+              config.args = rawConfig.args;
             }
+          }
+        });
 
-            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
+        ytplayer.config = rawYtplayer.config;
+      }
     });
   }, badArgumentsRegex);
 })();