Issue 1841 - Replaced URI class with built-in URL objects

Issue 1841 - Replaced URI class with built-in URL objects

[kzar, 2015-01-21 14:49:37]

Definitely one for Wladimir to double check but this actually LGTM. I can't see
anything obviously wrong or missed URI references etc.

[Wladimir Palant, 2015-01-21 15:33:44]

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/basedomain.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/basedomain.js:145: var host = new URL(url).hostname;
It's probably better to keep try..catch here - new URL("foo") still throws.

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:21: let URLproperties = ["href", "protocol", "host", "hostname",
"port", "pathname", "search"];
Nit: This isn't proper camel case, should be URLProperties or UrlProperties (the
latter seems to be more in line with common guidelines but we don't do this
consistently).

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:23: if (!URL || !URLproperties.every(prop => prop in new URL("")))
This will create seven URL instances, seems unnecessarily wasteful.

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:28: doc.head.appendChild(base);
I was about to suggest using xml:base (wouldn't require adding anything to a
document) but discovered that it's apparently deprecated, without any
replacement currently:
https://code.google.com/p/chromium/issues/detail?id=341854 :-(

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:35: base.href = baseUrl;
This will stringify baseUrl, meaning typically assigning "undefined" which is
rather ugly. Maybe use baseUrl || "" here?

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:38: for (var prop of URLproperties)
Nit: let prop?

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/whitelisting.js:68: url.host,                  // HTTP_HOST
I was trying to figure out whether you are using an implementation detail here
(the fact that url.host is always ASCII in Chrome and Safari). However, this
seems to be correct behavior according to https://url.spec.whatwg.org/ (the
logic there is rather complicated but I think I can follow it now). The behavior
implemented in Firefox right now is wrong then
(https://bugzilla.mozilla.org/show_bug.cgi?id=1007125).

[kzar, 2015-01-21 15:39:28]

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/basedomain.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/basedomain.js:145: var host = new URL(url).hostname;
On 2015/01/21 15:33:44, Wladimir Palant wrote:
> It's probably better to keep try..catch here - new URL("foo") still throws.

Oh dang, sad I missed that. I tested new URL("") but not new URL("foo"). Sorry

[Sebastian Noack, 2015-01-21 15:58:04]

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/basedomain.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/basedomain.js:145: var host = new URL(url).hostname;
On 2015/01/21 15:39:28, kzar wrote:
> On 2015/01/21 15:33:44, Wladimir Palant wrote:
> > It's probably better to keep try..catch here - new URL("foo") still throws.
> 
> Oh dang, sad I missed that. I tested new URL("") but not new URL("foo"). Sorry

This code is matter to be removed. In the furutre, wherever we get an URL from
browser APIs we imediatelly create an URL object. And I'd rather not spread
try..catch blocks across the code. Also considering that this would merely
protect against browser bugs. Since we don't process user input here.

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:23: if (!URL || !URLproperties.every(prop => prop in new URL("")))
On 2015/01/21 15:33:44, Wladimir Palant wrote:
> This will create seven URL instances, seems unnecessarily wasteful.

So what? This code runs only once, and an empty string should be fast to parse.
The alternative would be:

  let urlObj;
  if (!URL || (urlObj = new URL(""), !URLproperties.every(prop => prop in
urlObj)))
    ...

Or IMO even worse:

  function hasBuiltinURL()
  {
     if (!URL)
       return false;
     
     let urlObj = new URL("");
     return URLproperties.every(prop => prop in urlObj);
  }

  if (hasBuiltinURL())
    ...

I'd rather don't bloat the code just to save some negligible cycles during
initialization.

[Sebastian Noack, 2015-01-21 15:59:20]

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/basedomain.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/basedomain.js:145: var host = new URL(url).hostname;
On 2015/01/21 15:58:04, Sebastian Noack wrote:
> On 2015/01/21 15:39:28, kzar wrote:
> > On 2015/01/21 15:33:44, Wladimir Palant wrote:
> > > It's probably better to keep try..catch here - new URL("foo") still
throws.
> > 
> > Oh dang, sad I missed that. I tested new URL("") but not new URL("foo").
Sorry
> 
> This code is matter to be removed. In the furutre, wherever we get an URL from
> browser APIs we imediatelly create an URL object. And I'd rather not spread
> try..catch blocks across the code. Also considering that this would merely
> protect against browser bugs. Since we don't process user input here.

For reference, the reason this check where previously required was that the URI
class couldn't handle non-HTTP URLs.

[Sebastian Noack, 2015-01-21 17:48:18]

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:21: let URLproperties = ["href", "protocol", "host", "hostname",
"port", "pathname", "search"];
On 2015/01/21 15:33:44, Wladimir Palant wrote:
> Nit: This isn't proper camel case, should be URLProperties or UrlProperties
(the
> latter seems to be more in line with common guidelines but we don't do this
> consistently).

Done.

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:35: base.href = baseUrl;
On 2015/01/21 15:33:44, Wladimir Palant wrote:
> This will stringify baseUrl, meaning typically assigning "undefined" which is
> rather ugly. Maybe use baseUrl || "" here?

Done.

http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/...
lib/url.js:38: for (var prop of URLproperties)
On 2015/01/21 15:33:44, Wladimir Palant wrote:
> Nit: let prop?

Done.

[Wladimir Palant, 2015-01-21 19:38:24]

LGTM, the other comments indeed aren't important.