Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(961)

Issue 4544964214128640: Issue 1841 - Replaced URI class with built-in URL objects (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Sebastian Noack
Modified:
4 years, 9 months ago
Reviewers:
Wladimir Palant, kzar
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 14

Patch Set 2 : Added support for URL(string, URL) #

Patch Set 3 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -235 lines) Patch
M lib/basedomain.js View 2 chunks +1 line, -108 lines 0 comments Download
A lib/url.js View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
M lib/whitelisting.js View 1 chunk +4 lines, -4 lines 0 comments Download
M metadata.common View 1 chunk +1 line, -0 lines 0 comments Download
M qunit/tests/baseDomain.js View 1 chunk +0 lines, -123 lines 0 comments Download

Messages

Total messages: 8
Sebastian Noack
4 years, 10 months ago (2015-01-21 07:55:11 UTC) #1
kzar
Definitely one for Wladimir to double check but this actually LGTM. I can't see anything ...
4 years, 10 months ago (2015-01-21 14:49:37 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js File lib/basedomain.js (right): http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js#newcode145 lib/basedomain.js:145: var host = new URL(url).hostname; It's probably better to ...
4 years, 10 months ago (2015-01-21 15:33:44 UTC) #3
kzar
http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js File lib/basedomain.js (right): http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js#newcode145 lib/basedomain.js:145: var host = new URL(url).hostname; On 2015/01/21 15:33:44, Wladimir ...
4 years, 10 months ago (2015-01-21 15:39:28 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js File lib/basedomain.js (right): http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js#newcode145 lib/basedomain.js:145: var host = new URL(url).hostname; On 2015/01/21 15:39:28, kzar ...
4 years, 10 months ago (2015-01-21 15:58:04 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js File lib/basedomain.js (right): http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/basedomain.js#newcode145 lib/basedomain.js:145: var host = new URL(url).hostname; On 2015/01/21 15:58:04, Sebastian ...
4 years, 10 months ago (2015-01-21 15:59:20 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/4544964214128640/diff/5629499534213120/lib/url.js#newcode21 lib/url.js:21: let URLproperties = ["href", "protocol", "host", "hostname", "port", "pathname", ...
4 years, 10 months ago (2015-01-21 17:48:18 UTC) #7
Wladimir Palant
4 years, 10 months ago (2015-01-21 19:38:24 UTC) #8
LGTM, the other comments indeed aren't important.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5