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

Issue 5919468974768128: Issue 2062 - Preserve trailing question mark when converting URL into string (Closed)

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

Description

The problem is that the URL object doesn't distinguish between no query string and an empty query string, e.g: new URL("http://foobar").search == new URL("http://foobar?").search == "" Hence filters that expect a "?" to be given don't match anymore if there is an empty query string. First I considered handling this particular case, writing following code: function stringifyURL(url) { let protocol = url.protocol; if (protocol != "http:" && protocol != "https:") return url.href; let host = getDecodedHostname(url); if (url.port) host += ":" + url.port; let search = url.search; if (!search) { let href = url.href; let queryPos = href.indexOf("?"); if (queryPos != -1) { let hashPos = href.indexOf("#"); if (hashPos == -1 || hashPos > queryPos) search = "?"; } } return protocol + "//" + host + url.pathname + search; } However, this would make the logic even more complex than it already is, and might still not consider all corner cases. So I went with a more naive approach, which is way simpler and also faster (in particular in the common case, without IDN and no hash). Basically, I reduced the assumptions to a minimum, by just replacing IDN domains and stripping the hash part from URL.href.

Patch Set 1 : #

Total comments: 1

Patch Set 2 : URL.port isn't needed anymore #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -10 lines) Patch
M lib/url.js View 1 3 chunks +17 lines, -9 lines 3 comments Download
M qunit/tests/url.js View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 10
Sebastian Noack
http://codereview.adblockplus.org/5919468974768128/diff/5629499534213120/qunit/tests/url.js File qunit/tests/url.js (left): http://codereview.adblockplus.org/5919468974768128/diff/5629499534213120/qunit/tests/url.js#oldcode85 qunit/tests/url.js:85: testNormalizedURL("http://user:password@example.com/","http://example.com/", "stripped auth credentials"); The behavior regarding credentials changed. ...
4 years, 6 months ago (2015-02-27 20:01:59 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5919468974768128/diff/5741031244955648/qunit/tests/url.js File qunit/tests/url.js (left): http://codereview.adblockplus.org/5919468974768128/diff/5741031244955648/qunit/tests/url.js#oldcode85 qunit/tests/url.js:85: testNormalizedURL("http://user:password@example.com/","http://example.com/", "stripped auth credentials"); The behavior regarding credentials changed. ...
4 years, 6 months ago (2015-02-27 20:06:29 UTC) #2
kzar
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js#newcode21 lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"]; ...
4 years, 6 months ago (2015-03-02 15:04:13 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js#newcode21 lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"]; ...
4 years, 6 months ago (2015-03-02 15:10:37 UTC) #4
kzar
On 2015/03/02 15:10:37, Sebastian Noack wrote: > http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js > File lib/url.js (right): > > http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js#newcode21 ...
4 years, 6 months ago (2015-03-02 15:14:30 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js File lib/url.js (right): http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/url.js#newcode21 lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname", "pathname", "search"]; ...
4 years, 6 months ago (2015-03-02 15:18:20 UTC) #6
kzar
LGTM
4 years, 6 months ago (2015-03-02 15:23:06 UTC) #7
Wladimir Palant
In the end, it is up to you to support this code. As I said, ...
4 years, 6 months ago (2015-03-02 15:27:18 UTC) #8
Sebastian Noack
For reference, we discussed this on Friday on IRC, and Wladimir prefers the alternative approach ...
4 years, 6 months ago (2015-03-02 15:35:33 UTC) #9
kzar
4 years, 6 months ago (2015-03-02 15:41:12 UTC) #10
Well I love all my children equally... no but seriously I actually prefer your
approach here Sebastian. It's simpler and I found it easy to understand. (It's
possible we're missing some corner cases but I couldn't think of any and the
same could be said for the other approach.)
Sign in to reply to this message.

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