Append a trailing slash on file directory URLs. Thus a link to /directory wil...

Append a trailing slash on file directory URLs. Thus a link to /directory will work just as well as a link to /directory/


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5999

[Evan Stade, 2008-11-25 00:48:42]

http://codereview.chromium.org/12620/diff/10/204
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/12620/diff/10/204#newcode76
Line 76: file_util::EnsureEndsWithSeparator(&file_path))
To me it feels like the EnsureEndsWithSeparator call belongs in
FileURLToFilePath, but other callers of FileURLToFilePath depend on it not being
there.

http://codereview.chromium.org/12620/diff/10/201
File webkit/tools/test_shell/test_webview_delegate.cc (left):

http://codereview.chromium.org/12620/diff/10/201#oldcode226
Line 226: request->SetURL(GURL("testshell-error:"));
it's not very helpful to overwrite the troublesome URL with a generic error url

[tony, 2008-11-25 01:07:23]

http://codereview.chromium.org/12620/diff/10/203
File base/file_util.cc (right):

http://codereview.chromium.org/12620/diff/10/203#newcode49
Line 49: (path)[path.length() - 1] == kPathSeparator);
Nit: The extra () around path are unnecessary and makes this read funny.

http://codereview.chromium.org/12620/diff/10/203#newcode55
Line 55: // rather than making a new copy of the entire path.
Yuk.  Yeah, it seems like kPathSeparator should be moved into FilePath.  I'm ok
with deferring this until later.

http://codereview.chromium.org/12620/diff/10/202
File base/file_util.h (right):

http://codereview.chromium.org/12620/diff/10/202#newcode47
Line 47: // These two versions are both deprecated. TODO: remove them.
Nit: Please use TODO(estade) instead of TODO.

http://codereview.chromium.org/12620/diff/10/204
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/12620/diff/10/204#newcode76
Line 76: file_util::EnsureEndsWithSeparator(&file_path))
I don't have a strong opinion about where it should go.  If you wanted to fix
all the callers, that would be ok with me.

On 2008/11/25 00:48:42, Evan Stade wrote:
> To me it feels like the EnsureEndsWithSeparator call belongs in
> FileURLToFilePath, but other callers of FileURLToFilePath depend on it not
being
> there.

http://codereview.chromium.org/12620/diff/10/201
File webkit/tools/test_shell/test_webview_delegate.cc (left):

http://codereview.chromium.org/12620/diff/10/201#oldcode226
Line 226: request->SetURL(GURL("testshell-error:"));
This is more like what chrome does. I think it would be good to keep this
behavior because it's more like what chrome does (it uses the WebDataSource to
get the real URL for display in the URL bar).

How about we keep the SetURL and fix the error_text below to show the real URL?

On 2008/11/25 00:48:42, Evan Stade wrote:
> it's not very helpful to overwrite the troublesome URL with a generic error
url

[Evan Stade, 2008-11-25 02:00:18]

I'm working on another changelist to get rid of file_util::kPathSeparator

http://codereview.chromium.org/12620/diff/10/203
File base/file_util.cc (right):

http://codereview.chromium.org/12620/diff/10/203#newcode49
Line 49: (path)[path.length() - 1] == kPathSeparator);
On 2008/11/25 01:07:23, tony wrote:
> Nit: The extra () around path are unnecessary and makes this read funny.

Done.

http://codereview.chromium.org/12620/diff/10/203#newcode55
Line 55: // rather than making a new copy of the entire path.
On 2008/11/25 01:07:23, tony wrote:
> Yuk.  Yeah, it seems like kPathSeparator should be moved into FilePath.  I'm
ok
> with deferring this until later.

what do you know.. they have one in FilePath as well.

http://codereview.chromium.org/12620/diff/10/202
File base/file_util.h (right):

http://codereview.chromium.org/12620/diff/10/202#newcode47
Line 47: // These two versions are both deprecated. TODO: remove them.
On 2008/11/25 01:07:23, tony wrote:
> Nit: Please use TODO(estade) instead of TODO.

Done.

http://codereview.chromium.org/12620/diff/10/204
File net/url_request/url_request_file_job.cc (right):

http://codereview.chromium.org/12620/diff/10/204#newcode76
Line 76: file_util::EnsureEndsWithSeparator(&file_path))
On 2008/11/25 01:07:23, tony wrote:
> I don't have a strong opinion about where it should go.  If you wanted to fix
> all the callers, that would be ok with me.
> 
> On 2008/11/25 00:48:42, Evan Stade wrote:
> > To me it feels like the EnsureEndsWithSeparator call belongs in
> > FileURLToFilePath, but other callers of FileURLToFilePath depend on it not
> being
> > there.
> 

maybe later

http://codereview.chromium.org/12620/diff/10/201
File webkit/tools/test_shell/test_webview_delegate.cc (left):

http://codereview.chromium.org/12620/diff/10/201#oldcode226
Line 226: request->SetURL(GURL("testshell-error:"));
On 2008/11/25 01:07:23, tony wrote:
> This is more like what chrome does. I think it would be good to keep this
> behavior because it's more like what chrome does (it uses the WebDataSource to
> get the real URL for display in the URL bar).
> 
> How about we keep the SetURL and fix the error_text below to show the real
URL?
> 
> On 2008/11/25 00:48:42, Evan Stade wrote:
> > it's not very helpful to overwrite the troublesome URL with a generic error
> url

Done.

[tony, 2008-11-25 16:19:55]

LGTM

[Evan Stade, 2008-11-25 20:54:46]

as it turns out, a little more work is needed to make sure the URL bar displays
the proper URL. Treat the URL .../directory as a redirect to .../directory/.

[tony, 2008-11-25 21:42:52]

LGTM with the following comment addressed.

http://codereview.chromium.org/12620/diff/231/26
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/12620/diff/231/26#newcode207
Line 207: if (path[path.size() - 1] != '/') {
Does path always return at least '/' or do you need to check the size?  If it
always returns something of len >= 1, can you add a comment as such?