Move path functions from file_util to FilePath object.

Move path functions from file_util to FilePath object.

EnsureEndsWithSeparator used to check whether the file existed. This seems bad and unnecessary so I removed it.

I removed file_util::ContainsPath and used the existing file_util::IsParent instead. The functions descriptions are the same but the implementations do slightly different things, which is worrying. The only non-test use of this function to worry about is content/browser/storage_partition_impl_map.cc. As far as I see, the requirements for this seem OK, but I'm not very familiar with this.

After some discussion with akalin, I changed sync/internal_api/sync_manager_impl.cc to be a DCHECK that the path is absolute rather than make it absolute. The old code relied on the behavior of the old function that the argument would be unchanged if the file didn't exist, and this (possibly relative) path would be used later. This behavior doesn't make a lot of sense, and it looks like now that the path is always absolute, so I replaced this call with a DCHECK.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193855

[brettw, 2013-03-30 04:19:38]

Self review

https://codereview.chromium.org/13196006/diff/1/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/13196006/diff/1/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.cc:66: *file_path =
file_path->AsAbsolute();
Fix me.

https://codereview.chromium.org/13196006/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util_unittest.cc (right):

https://codereview.chromium.org/13196006/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util_unittest.cc:308: root_path =
root_path.AsAbsolute();
Fix me

https://codereview.chromium.org/13196006/diff/1/chrome/test/gpu/gpu_pixel_bro...
File chrome/test/gpu/gpu_pixel_browsertest.cc (right):

https://codereview.chromium.org/13196006/diff/1/chrome/test/gpu/gpu_pixel_bro...
chrome/test/gpu/gpu_pixel_browsertest.cc:61: base::FilePath
abs_path(file_path).AsAbsolute();
Fix me

https://codereview.chromium.org/13196006/diff/1/extensions/common/extension_r...
File extensions/common/extension_resource_unittest.cc (right):

https://codereview.chromium.org/13196006/diff/1/extensions/common/extension_r...
extensions/common/extension_resource_unittest.cc:153: expected_path =
root_resource.AsAbsolute();
Fixme

https://codereview.chromium.org/13196006/diff/1/webkit/support/webkit_support.cc
File webkit/support/webkit_support.cc (right):

https://codereview.chromium.org/13196006/diff/1/webkit/support/webkit_support...
webkit/support/webkit_support.cc:579: base::FilePath
path(UTF8ToWide(utf8_path)).AsAbsolute();
Fixme

[rvargas (doing something else), 2013-04-01 19:50:31]

https://codereview.chromium.org/13196006/diff/24001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/13196006/diff/24001/base/files/file_path.cc#n...
base/files/file_path.cc:525: FilePath FilePath::AsAbsolute() const {
I'm not sure moving this code here is a good idea. An explicit design parameter
of FilePath is that it doesn't ever touch the actual file system, relying
instead on pure inspection of what paths look like.

Operations that cannot be implemented without looking at disk were part of
file_util for that reason.

The other changes seem fine.

[brettw, 2013-04-01 22:42:24]

(No new patch up, will let you know when I'm done.)

https://codereview.chromium.org/13196006/diff/24001/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/13196006/diff/24001/base/files/file_path.cc#n...
base/files/file_path.cc:525: FilePath FilePath::AsAbsolute() const {
I thought that was the case but it didn't seem like anybody was following that
rule. and was just using AsAbsolute all over the place. But looking closer it
looks like that's only in the tests and we actually do a pretty good job of
keeping IsAbsolute calls off the UI thread.

I'll keep AsAbsolute there although I think I'll change it to return a copy
rather than modify the param in-place; I think that made the code cleaner when I
was doing this patch.

[brettw, 2013-04-01 23:51:18]

I moved AsAbsolute out as base::MakeAbsoluteFilePath. I'm transitioning
everything in file_util to the base namespace with the word "File" in the name,
so I went ahead ad did the base namespace at this time even though it doesn't
(yet) match anything else.

[rvargas (doing something else), 2013-04-02 01:39:55]

LGTM after the small fix for AsEndingWithSeparator

https://codereview.chromium.org/13196006/diff/20003/base/files/file_path.cc
File base/files/file_path.cc (right):

https://codereview.chromium.org/13196006/diff/20003/base/files/file_path.cc#n...
base/files/file_path.cc:19: #include "base/threading/thread_restrictions.h"
remove this.

https://codereview.chromium.org/13196006/diff/20003/chrome/browser/platform_u...
File chrome/browser/platform_util_win.cc (right):

https://codereview.chromium.org/13196006/diff/20003/chrome/browser/platform_u...
chrome/browser/platform_util_win.cc:36: if (dir.empty())
The old code rejected empty paths while the new version continues.

We should probably make AsEndingWithSeparator fail for empty paths.

https://codereview.chromium.org/13196006/diff/20003/webkit/tools/test_shell/t...
File webkit/tools/test_shell/test_shell_main.cc (right):

https://codereview.chromium.org/13196006/diff/20003/webkit/tools/test_shell/t...
webkit/tools/test_shell/test_shell_main.cc:1: /)/ Copyright (c) 2012 The
Chromium Authors. All rights reserved.
typo

[brettw, 2013-04-08 23:40:19]

https://codereview.chromium.org/13196006/diff/20003/chrome/browser/platform_u...
File chrome/browser/platform_util_win.cc (right):

https://codereview.chromium.org/13196006/diff/20003/chrome/browser/platform_u...
chrome/browser/platform_util_win.cc:36: if (dir.empty())
Good catch, I also added tests for this case.

[akalin, 2013-04-08 23:42:14]

sync lgtm

[rvargas (doing something else), 2013-04-09 00:06:35]

lgtm

[brettw, 2013-04-11 21:02:52]

The original patch broke a sync test. I had to make this change:

https://codereview.chromium.org/13196006/diff2/71003:77001/sync/internal_api/...

The function is being called for a nonexistant path, and is counting on the old
behavior of not touching the argument on failure. Since the new version returns
empty in this case, the path got clobbered.

I don't really understand why this case is being hit. It seems weird or possibly
accidental that the code was written this way. It may also only be hit during
tests.

Fred: Do you have any more input about this? Is there are more useful comment I
can put there about why we expect this behavior?

[brettw, 2013-04-12 04:41:27]

cloud_print:
TBR=vitalybuka@chromium.org

extensions:
TBR=finnur@chromium.org

ui
TBR=sky@chromium.org

[brettw, 2013-04-12 04:41:27]

cloud_print:
TBR=vitalybuka@chromium.org

extensions:
TBR=finnur@chromium.org

ui
TBR=sky@chromium.org

[brettw, 2013-04-12 04:43:19]

chrome_frame
TBR=robertshield@chromium.org

content
TBR=jam@chromium.org

skia
TBR=alokp@chromium.org

[Vitaly Buka (NO REVIEWS), 2013-04-12 04:43:32]

lgtm

[brettw, 2013-04-12 05:17:24]

Committed patchset #11 manually as r193855.

[Finnur, 2013-04-12 12:43:28]

Extensions LGTM

[Steve Block, 2013-05-07 06:46:27]

https://codereview.chromium.org/13196006/diff/89001/webkit/plugins/npapi/plug...
File webkit/plugins/npapi/plugin_list_posix.cc (right):

https://codereview.chromium.org/13196006/diff/89001/webkit/plugins/npapi/plug...
webkit/plugins/npapi/plugin_list_posix.cc:221: base::FilePath orig_path =
base::MakeAbsoluteFilePath(path);
Shouldn't this be 'path = ...', and orig_path should be left as it was?