Issue 3515 - Adapted polyfill for URL class for consistency

lib/polyfill/url.js

Index: lib/polyfill/url.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/lib/polyfill/url.js
@@ -0,0 +1,55 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+"use strict";

[kzar, 2016/01/16 16:05:28]

Again shouldn't this be inside the IIFE?

[Sebastian Noack, 2016/01/19 14:13:21]

Again, it shouldn't. ;)

Putting "use strict" on the top-level will turn on strict mode for the whole
file, but not for other files even if they share the same context.

[kzar, 2016/01/19 15:24:02]

Acknowledged.

+
+(function(global)
+{
+  // Chrome <35 and Safari 6 used the non-standard name webkitURL
+  if (!("URL" in global) && "webkitURL" in global)
+    global.URL = global.webkitURL;
+
+  // Chrome <32 didn't implement any of those properties

[kzar, 2016/01/16 16:05:28]

Couldn't we avoid checking these properties exist for versions 35 onwards?
Something like:

if ("URL" in global)
  return;
else if ("webkitURL" in global)
{
  global.URL = global.webkitURL;
  var URLProperties = ...

[Sebastian Noack, 2016/01/19 14:13:21]

Note that Safari 6 and Chrome 32-34 uses the non-standard name webkitURL while
it already provides these properties. And we need that list of properties anyway
below.

[kzar, 2016/01/19 15:24:02]

Yea I understand that, just suggesting changing the logic so we don't check the
properties if URL already exists. If URL already exists we know that we're
running on a new enough version of Chrome / Safari as to provide URL with the
correct properties, right?

As far as I see it, the only time we're not sure if the correct properties are
given is when webkitURL exists but not URL.

[Sebastian Noack, 2016/01/19 16:03:39]

That seems to be true. But skipping that check if window.URL exist doesn't make
the code here any simpler. In fact it would be two additional lines, without
much simplifying the remainder.

However, if the check is always executed, that code path is always tested, even
when running on the latest Chrome version.

And performance-wise, it's only executed once at startup and the check is quite
fast. So not worth any optimization.

[kzar, 2016/01/19 16:12:07]

Acknowledged.

+  var URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"];

[kzar, 2016/01/16 16:05:28]

Nit: this line is too long

[Sebastian Noack, 2016/01/19 14:13:21]

Done.

+  if ("URL" in global)
+  {
+    var dummy = new URL("about:blank");
+    if (URLProperties.every(function(prop) { return prop in dummy; }))
+      return;
+  }
+
+  var doc = document.implementation.createHTMLDocument();
+  var base = doc.createElement("base");
+  doc.head.appendChild(base);
+  var anchor = doc.createElement("a");
+  doc.body.appendChild(anchor);
+
+  global.URL = function(url, baseUrl)

[kzar, 2016/01/16 16:05:28]

A few things that I noticed behave differently with this:

 - Calling with a relative URL but no base URL does not throw an exception.
 - The hash part of the URL does not seem to be parsed.
 - The username and password parts of the URL also aren't parsed.
 - Calling with no arguments at all does not throw an exception.

[Sebastian Noack, 2016/01/19 14:13:21]

As for the other two polyfills, it's not the objective to be 100% complete with
the standard. But to only implement the properties we need. In particular with
the approach here adding additional properties would increase the memory
footprint.
It's probably not worth adding complexity to handle that scenario. The reason
this polyfill is as simple as it is is that we merely delegate to an <a>
element. Also since we don't catch any exceptions thrown here, that
inconsistency wouldn't make anything worse.

And after all, the behavior doesn't change with this patch.

[kzar, 2016/01/19 15:24:02]

I think with the missing properties, especially hash, we should include them. Or
if not we should at least leave a comment here, I can imagine someone writing
code that uses the hash of a URL, testing it on a modern browser and not
thinking to check that the polyfill also supports the properties used.

We could end up with undefined instead of the hash string, which depending on
how it's used might not even throw an exception.

[Sebastian Noack, 2016/01/19 16:03:39]

The way URL objects are currently used in Adblock Plus username, password and
hash are explicitly ignored in stringifyURL anyway. That behavior is even
documented there.

And if in the future, URL objects should be used in another way, a comment
wouldn't help either, if one only tests in the latest version of any browser.

And still, this is unrelated of the change here, which is about unifying the
polyfills, not about changing their behavior.

[kzar, 2016/01/19 16:12:07]

Acknowledged.

+  {
+    if (baseUrl instanceof URL)

[kzar, 2016/01/16 16:05:28]

should be `... instanceof global.URL)`?

[Sebastian Noack, 2016/01/19 14:13:21]

As in the other review, that isn't necessary, but let's put it into a local
variable.

[kzar, 2016/01/19 15:24:02]

Acknowledged.

+      base.href = baseUrl.href;
+    else
+      base.href = baseUrl || "";
+    anchor.href = url;
+
+    for (var i = 0; i < URLProperties.length; i++)
+    {
+      var prop = URLProperties[i];
+      this[prop] = anchor[prop];
+    }
+  };
+})(this);