Add support for real full filesystem URL parsing and canonicalization.

Add support for real full filesystem URL parsing and canonicalization.

See also the chromium [http://codereview.chromium.org/7811006/] and webkit [http://codereview.chromium.org/8856006/] changes that will depend on this.

[abarth, 2011-11-16 22:02:43]

http://codereview.appspot.com/4961060/diff/28001/src/gurl.h
File src/gurl.h (right):

http://codereview.appspot.com/4961060/diff/28001/src/gurl.h#newcode382
src/gurl.h:382: GURL* inner_url_;
Should this be a scoped_ptr?

[ericu, 2011-11-16 22:13:36]

http://codereview.appspot.com/4961060/diff/28001/src/gurl.h
File src/gurl.h (right):

http://codereview.appspot.com/4961060/diff/28001/src/gurl.h#newcode382
src/gurl.h:382: GURL* inner_url_;
On 2011/11/16 22:02:43, abarth wrote:
> Should this be a scoped_ptr?

I was wondering about that myself.  I'd love to use one, but googleurl doesn't
use them anywhere, so I wanted to ask Brett if that was a style choice or just
that it hadn't needed any.  I see that scoped_ptr.h is in the repo, but it's
only used in logging code in base, not in the src directory.

[abarth, 2011-11-17 01:16:37]

I'll defer to Brett here, but I suspect it's fine.

[ericu, 2011-12-21 21:56:40]

On 2011/11/17 01:16:37, abarth wrote:
> I'll defer to Brett here, but I suspect it's fine.

Making that be a scoped_ptr requires adding an include of scoped_ptr.h in the
header file.  That grabs both the one in this repo and the one in Chromium's
repo [when you build Chromium], which causes an explosion.  I'm going to leave
it as a raw pointer for now.

[brettw, 2011-12-22 19:18:52]

I just looked at a few parts so far, and I hate this much less than I feared.
Nice.

One question: are we / should we be doing anything to reject
"filesystem:filesystem:filesystem....."?

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc
File src/gurl.cc (right):

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc#newcode123
src/gurl.cc:123: if (is_valid_ && SchemeIsFileSystem())
Use {} here since the contents is multi-line (same in the cases below as well)

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc#newcode313
src/gurl.cc:313: if (SchemeIsFileSystem()) {
No {}

http://codereview.appspot.com/4961060/diff/38007/src/url_util.cc
File src/url_util.cc (right):

http://codereview.appspot.com/4961060/diff/38007/src/url_util.cc#newcode104
src/url_util.cc:104: const url_parse::Component& component,
Can you fix the indenting here?

[ericu, 2011-12-22 23:26:11]

On Thu, Dec 22, 2011 at 11:18 AM,  <brettw@chromium.org> wrote:
> I just looked at a few parts so far, and I hate this much less than I
> feared. Nice.
>
> One question: are we / should we be doing anything to reject
> "filesystem:filesystem:filesystem....."?

See url_parse.cc:379.

[ericu, 2012-01-03 19:22:30]

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc
File src/gurl.cc (right):

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc#newcode123
src/gurl.cc:123: if (is_valid_ && SchemeIsFileSystem())
On 2011/12/22 19:18:52, brettw wrote:
> Use {} here since the contents is multi-line (same in the cases below as well)

Done.

http://codereview.appspot.com/4961060/diff/38007/src/gurl.cc#newcode313
src/gurl.cc:313: if (SchemeIsFileSystem()) {
On 2011/12/22 19:18:52, brettw wrote:
> No {}

Done.

http://codereview.appspot.com/4961060/diff/38007/src/url_util.cc
File src/url_util.cc (right):

http://codereview.appspot.com/4961060/diff/38007/src/url_util.cc#newcode104
src/url_util.cc:104: const url_parse::Component& component,
On 2011/12/22 19:18:52, brettw wrote:
> Can you fix the indenting here?

Done.

[brettw, 2012-01-03 21:09:25]

http://codereview.appspot.com/4961060/diff/50002/src/gurl.cc
File src/gurl.cc (right):

http://codereview.appspot.com/4961060/diff/50002/src/gurl.cc#newcode499
src/gurl.cc:499: void GURL::Swap(GURL* other) {
You need to add the inner_url_ stuff to this function.

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl.cc
File src/url_canon_filesystemurl.cc (right):

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:77: &new_inner_parsed.path);
Indentation

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:95: &new_parsed->path);
Indentation.

http://codereview.appspot.com/4961060/diff/50002/src/url_parse.h
File src/url_parse.h (right):

http://codereview.appspot.com/4961060/diff/50002/src/url_parse.h#newcode229
src/url_parse.h:229: Parsed* inner_parsed_;
Can you add a comment here that this pointer is owned by this struct?

[ericu, 2012-01-03 21:18:42]

I've made the small changes; I'm now looking at the larger layering change Brett
and I talked about offline.

http://codereview.appspot.com/4961060/diff/50002/src/gurl.cc
File src/gurl.cc (right):

http://codereview.appspot.com/4961060/diff/50002/src/gurl.cc#newcode499
src/gurl.cc:499: void GURL::Swap(GURL* other) {
On 2012/01/03 21:09:26, brettw wrote:
> You need to add the inner_url_ stuff to this function.

Done.

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl.cc
File src/url_canon_filesystemurl.cc (right):

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:77: &new_inner_parsed.path);
On 2012/01/03 21:09:26, brettw wrote:
> Indentation

Done.

http://codereview.appspot.com/4961060/diff/50002/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:95: &new_parsed->path);
On 2012/01/03 21:09:26, brettw wrote:
> Indentation.

Done.

http://codereview.appspot.com/4961060/diff/50002/src/url_parse.h
File src/url_parse.h (right):

http://codereview.appspot.com/4961060/diff/50002/src/url_parse.h#newcode229
src/url_parse.h:229: Parsed* inner_parsed_;
On 2012/01/03 21:09:26, brettw wrote:
> Can you add a comment here that this pointer is owned by this struct?

Done.

[brettw, 2012-01-04 19:22:17]

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_filesystemurl.cc
File src/url_canon_filesystemurl.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:107: } // namespace
Two spaces before //

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_relative.cc
File src/url_canon_relative.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_relative.cc#ne...
src/url_canon_relative.cc:161: if (url_util::CompareSchemeComponent(url, scheme,
"filesystem")) {
Can you comment this code, and also add test cases in TEST(URLCanonTest,
ResolveRelativeURL) {

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc
File src/url_canon_unittest.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc#ne...
src/url_canon_unittest.cc:1451: {"filesystem:file:///temporary/gaba?query#ref",
NULL, NULL, NULL, NULL, NULL, "interesting/", NULL, NULL,
"filesystem:file:///temporary/interesting/?query#ref"},
There are no tests here for setting the scheme, username, etc. (the 5 NULLs). We
should have tests for this. We should be sure to have one where we replace the
scheme from filesystem, and also what happens when you take an http URL and set
the scheme to filesystem.

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc#ne...
src/url_canon_unittest.cc:1675: {"filesystem:http://www.foo.com//",
"filesystem:http://www.foo.com//", false},
For one of these cases, can you do stuff like adding some uppercase to the
scheme (both filesystem and http) and hostname to make sure that the proper
transformations are done?

[ericu, 2012-01-04 21:57:36]

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_filesystemurl.cc
File src/url_canon_filesystemurl.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_filesystemurl....
src/url_canon_filesystemurl.cc:107: } // namespace
On 2012/01/04 19:22:17, brettw wrote:
> Two spaces before //

Done.

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_relative.cc
File src/url_canon_relative.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_relative.cc#ne...
src/url_canon_relative.cc:161: if (url_util::CompareSchemeComponent(url, scheme,
"filesystem")) {
On 2012/01/04 19:22:17, brettw wrote:
> Can you comment this code, and also add test cases in TEST(URLCanonTest,
> ResolveRelativeURL) {

Done, although now I've deleted most of it, as per our discussion.

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc
File src/url_canon_unittest.cc (right):

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc#ne...
src/url_canon_unittest.cc:1451: {"filesystem:file:///temporary/gaba?query#ref",
NULL, NULL, NULL, NULL, NULL, "interesting/", NULL, NULL,
"filesystem:file:///temporary/interesting/?query#ref"},
On 2012/01/04 19:22:17, brettw wrote:
> There are no tests here for setting the scheme, username, etc. (the 5 NULLs).
We
> should have tests for this. We should be sure to have one where we replace the
> scheme from filesystem, and also what happens when you take an http URL and
set
> the scheme to filesystem.

Done.
Setting scheme is a no-op, as with file, so I've added a test for file as well.
Replacing http->filesystem gives you garbage, but I've added a test as we
discussed, so that we notice if the garbage changes.

http://codereview.appspot.com/4961060/diff/49018/src/url_canon_unittest.cc#ne...
src/url_canon_unittest.cc:1675: {"filesystem:http://www.foo.com//",
"filesystem:http://www.foo.com//", false},
On 2012/01/04 19:22:17, brettw wrote:
> For one of these cases, can you do stuff like adding some uppercase to the
> scheme (both filesystem and http) and hostname to make sure that the proper
> transformations are done?

Done.

[brettw, 2012-01-04 22:22:28]

LGTM

[brettw, 2012-01-04 22:47:42]

Committed as googleurl@166.

I did not pull deps. You should submit a change to do that (you can TBR it).

[ericu, 2012-01-04 23:08:29]

Will do; thanks!

On Wed, Jan 4, 2012 at 2:47 PM,  <brettw@chromium.org> wrote:
> Committed as googleurl@166.
>
> I did not pull deps. You should submit a change to do that (you can TBR
> it).
>
> http://codereview.appspot.com/4961060/