Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issue 564011: Remove the rule that "://" means a standard URL. This fixes a number of bugs... (Closed)

Created:
10 years, 10 months ago by brettw
Modified:
9 years, 7 months ago
Reviewers:
bauerb at google
CC:
chromium-reviews, darin (slow to review)
Visibility:
Public.

Description

Remove the rule that "://" means a standard URL. This fixes a number of bugs and layout tests. The only thing we use now to determine whether a scheme is standard scheme is whether it is on a known list. Fix some bugs around setting protocols. Previously, we wouldn't reparse the entire URL, which would give weird results in some cases, since which part actually gets replaced changes if you change the protocol. This change has us replace the protocol, then completely reparse the URL which ensures we don't miss any edge cases. It is a bit slower, but changing the protocol is very unusual. This differs from KURL in several cases. On the fast/dom/HTMLAnchorElement/set-href-attribute-hostname.html layout test, we fail setting a hostname on a URL with "foo" protocol because we no longer treat a "foo" URL as a standard protocol. However, this matches Firefox's behavior, so I think there should not be compat problems. Other differences in that test are that we allow setting hosts with spaces in them for IE compat, and that when we generate the URL "c:/path/testurl.html" on Windows, we reinterpret that as a file URL to file:///C:/path/testurl.html. This is how "c:/path/testurl.html" would be interpreted if we just found it on a page, so I think this behavior is correct, even though no other browser does this (our handling of these generally matches IE, but it fails to set a "c" protocol with an exception in this case). http://crbug.com/160 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 3

Patch Set 7 : '' #

Patch Set 8 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -104 lines) Patch
M src/gurl.h View 6 7 1 chunk +3 lines, -3 lines 0 comments Download
M src/gurl.cc View 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M src/gurl_unittest.cc View 2 3 4 5 6 7 4 chunks +29 lines, -30 lines 0 comments Download
M src/url_canon_stdurl.cc View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
M src/url_util.h View 2 3 4 5 6 7 1 chunk +3 lines, -6 lines 0 comments Download
M src/url_util.cc View 1 2 3 4 5 6 7 8 chunks +77 lines, -63 lines 0 comments Download
M src/url_util_unittest.cc View 1 2 3 4 5 6 7 1 chunk +48 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
brettw
Bernhard: Please review everything Darin: In case you're curious, I decided we're OK with the ...
10 years, 10 months ago (2010-02-05 20:21:52 UTC) #1
darin (slow to review)
On Fri, Feb 5, 2010 at 12:21 PM, <brettw@chromium.org> wrote: > Reviewers: bauerb, > > ...
10 years, 10 months ago (2010-02-05 22:17:53 UTC) #2
bauerb at google
Hi Brett, so far the patch looks really good to me, but I have a ...
10 years, 10 months ago (2010-02-08 16:46:40 UTC) #3
brettw
On Mon, Feb 8, 2010 at 8:46 AM, <bauerb@google.com> wrote: > Hi Brett, > > ...
10 years, 10 months ago (2010-02-08 17:48:10 UTC) #4
bauerb at google
On 2010/02/08 17:48:10, brettw wrote: > This is already a very challenging patch to land, ...
10 years, 10 months ago (2010-02-08 18:11:02 UTC) #5
brettw
On Mon, Feb 8, 2010 at 10:11 AM, <bauerb@google.com> wrote: > On 2010/02/08 17:48:10, brettw ...
10 years, 10 months ago (2010-02-08 18:30:41 UTC) #6
bauerb at google
Hi Brett, attached is a small additional patch for gurl.h. Bernhard. http://codereview.chromium.org/564011
10 years, 10 months ago (2010-02-08 18:57:53 UTC) #7
brettw
On Mon, Feb 8, 2010 at 10:57 AM, Bernhard Bauer <bauerb@google.com> wrote: > Hi Brett, ...
10 years, 10 months ago (2010-02-08 19:19:56 UTC) #8
bauerb at google
http://codereview.chromium.org/564011/diff/14001/14003 File src/url_util.cc (left): http://codereview.chromium.org/564011/diff/14001/14003#oldcode100 src/url_util.cc:100: // "://", use IsStandard for that. Remove this sentence ...
10 years, 10 months ago (2010-02-10 18:09:18 UTC) #9
brettw
Nice catches. New snap up.
10 years, 10 months ago (2010-02-11 21:30:06 UTC) #10
brettw
10 years, 10 months ago (2010-02-12 21:20:47 UTC) #11
Committed in googleurl@123. I did not yet pull deps.

Powered by Google App Engine
This is Rietveld 408576698