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=

Review URL: https://codereview.chromium.org/13196006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@193855 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 105 insertions, 148 deletions

diff --git a/base/file_util.cc b/base/file_util.cc
index 3e7dc445..16fee2c 100644
--- a/base/file_util.cc
+++ b/base/file_util.cc
@@ -37,28 +37,6 @@ namespace file_util {
 
 bool g_bug108724_debug = false;
 
-bool EndsWithSeparator(const FilePath& path) {
-  FilePath::StringType value = path.value();
-  if (value.empty())
-    return false;
-
-  return FilePath::IsSeparator(value[value.size() - 1]);
-}
-
-bool EnsureEndsWithSeparator(FilePath* path) {
-  if (!DirectoryExists(*path))
-    return false;
-
-  if (EndsWithSeparator(*path))
-    return true;
-
-  FilePath::StringType& path_str =
-      const_cast<FilePath::StringType&>(path->value());
-  path_str.append(&FilePath::kSeparators[0], 1);
-
-  return true;
-}
-
 void InsertBeforeExtension(FilePath* path, const FilePath::StringType& suffix) {
   FilePath::StringType& value =
       const_cast<FilePath::StringType&>(path->value());
@@ -289,33 +267,6 @@ int GetUniquePathNumber(
   return -1;
 }
 
-bool ContainsPath(const FilePath &parent, const FilePath& child) {
-  FilePath abs_parent = FilePath(parent);
-  FilePath abs_child = FilePath(child);
-
-  if (!file_util::AbsolutePath(&abs_parent) ||
-      !file_util::AbsolutePath(&abs_child))
-    return false;
-
-#if defined(OS_WIN)
-  // file_util::AbsolutePath() does not flatten case on Windows, so we must do
-  // a case-insensitive compare.
-  if (!StartsWith(abs_child.value(), abs_parent.value(), false))
-#else
-  if (!StartsWithASCII(abs_child.value(), abs_parent.value(), true))
-#endif
-    return false;
-
-  // file_util::AbsolutePath() normalizes '/' to '\' on Windows, so we only need
-  // to check kSeparators[0].
-  if (abs_child.value().length() <= abs_parent.value().length() ||
-      abs_child.value()[abs_parent.value().length()] !=
-          FilePath::kSeparators[0])
-    return false;
-
-  return true;
-}
-
 int64 ComputeDirectorySize(const FilePath& root_path) {
   int64 running_size = 0;
   FileEnumerator file_iter(root_path, true, FileEnumerator::FILES);
diff --git a/base/file_util.h b/base/file_util.h
index d4a6471..874d5ca 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -39,30 +39,17 @@
 
 namespace base {
 class Time;
-}
 
-namespace file_util {
-
-extern bool g_bug108724_debug;
-
-//-----------------------------------------------------------------------------
-// Functions that operate purely on a path string w/o touching the filesystem:
-
-// Returns true if the given path ends with a path separator character.
-BASE_EXPORT bool EndsWithSeparator(const base::FilePath& path);
+// Returns an absolute version of a relative path. Returns an empty path on
+// error. On POSIX, this function fails if the path does not exist. This
+// function can result in I/O so it can be slow.
+BASE_EXPORT FilePath MakeAbsoluteFilePath(const FilePath& input);
 
-// Makes sure that |path| ends with a separator IFF path is a directory that
-// exists. Returns true if |path| is an existing directory, false otherwise.
-BASE_EXPORT bool EnsureEndsWithSeparator(base::FilePath* path);
+}  // namespace base
 
-// Convert provided relative path into an absolute path.  Returns false on
-// error. On POSIX, this function fails if the path does not exist.
-BASE_EXPORT bool AbsolutePath(base::FilePath* path);
+namespace file_util {
 
-// Returns true if |parent| contains |child|. Both paths are converted to
-// absolute paths before doing the comparison.
-BASE_EXPORT bool ContainsPath(const base::FilePath& parent,
-                              const base::FilePath& child);
+extern bool g_bug108724_debug;
 
 //-----------------------------------------------------------------------------
 // Functions that involve filesystem access or modification:
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 8bf164a..8b36812 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -59,6 +59,19 @@
 #endif
 
 using base::FilePath;
+using base::MakeAbsoluteFilePath;
+
+namespace base {
+
+FilePath MakeAbsoluteFilePath(const FilePath& input) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  char full_path[PATH_MAX];
+  if (realpath(input.value().c_str(), full_path) == NULL)
+    return FilePath();
+  return FilePath(full_path);
+}
+
+}  // namespace base
 
 namespace file_util {
 
@@ -150,15 +163,6 @@ static std::string TempFileName() {
 #endif
 }
 
-bool AbsolutePath(FilePath* path) {
-  base::ThreadRestrictions::AssertIOAllowed();  // For realpath().
-  char full_path[PATH_MAX];
-  if (realpath(path->value().c_str(), full_path) == NULL)
-    return false;
-  *path = FilePath(full_path);
-  return true;
-}
-
 // TODO(erikkay): The Windows version of this accepts paths like "foo/bar/*"
 // which works both with and without the recursive flag.  I'm not sure we need
 // that functionality. If not, remove from file_util_win.cc, otherwise add it
@@ -253,15 +257,16 @@ bool CopyDirectory(const FilePath& from_path,
   // This function does not properly handle destinations within the source
   FilePath real_to_path = to_path;
   if (PathExists(real_to_path)) {
-    if (!AbsolutePath(&real_to_path))
+    real_to_path = MakeAbsoluteFilePath(real_to_path);
+    if (real_to_path.empty())
       return false;
   } else {
-    real_to_path = real_to_path.DirName();
-    if (!AbsolutePath(&real_to_path))
+    real_to_path = MakeAbsoluteFilePath(real_to_path.DirName());
+    if (real_to_path.empty())
       return false;
   }
-  FilePath real_from_path = from_path;
-  if (!AbsolutePath(&real_from_path))
+  FilePath real_from_path = MakeAbsoluteFilePath(from_path);
+  if (real_from_path.empty())
     return false;
   if (real_to_path.value().size() >= real_from_path.value().size() &&
       real_to_path.value().compare(0, real_from_path.value().size(),
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index 77dd7ac..5cfc677 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -1961,50 +1961,6 @@ TEST_F(FileUtilTest, AppendToFile) {
   EXPECT_EQ(L"hellohello", read_content);
 }
 
-TEST_F(FileUtilTest, Contains) {
-  FilePath data_dir =
-      temp_dir_.path().Append(FILE_PATH_LITERAL("FilePathTest"));
-
-  // Create a fresh, empty copy of this directory.
-  if (file_util::PathExists(data_dir)) {
-    ASSERT_TRUE(file_util::Delete(data_dir, true));
-  }
-  ASSERT_TRUE(file_util::CreateDirectory(data_dir));
-
-  FilePath foo(data_dir.Append(FILE_PATH_LITERAL("foo")));
-  FilePath bar(foo.Append(FILE_PATH_LITERAL("bar.txt")));
-  FilePath baz(data_dir.Append(FILE_PATH_LITERAL("baz.txt")));
-  FilePath foobar(data_dir.Append(FILE_PATH_LITERAL("foobar.txt")));
-
-  // Annoyingly, the directories must actually exist in order for realpath(),
-  // which Contains() relies on in posix, to work.
-  ASSERT_TRUE(file_util::CreateDirectory(foo));
-  std::string data("hello");
-  ASSERT_TRUE(file_util::WriteFile(bar, data.c_str(), data.length()));
-  ASSERT_TRUE(file_util::WriteFile(baz, data.c_str(), data.length()));
-  ASSERT_TRUE(file_util::WriteFile(foobar, data.c_str(), data.length()));
-
-  EXPECT_TRUE(file_util::ContainsPath(foo, bar));
-  EXPECT_FALSE(file_util::ContainsPath(foo, baz));
-  EXPECT_FALSE(file_util::ContainsPath(foo, foobar));
-  EXPECT_FALSE(file_util::ContainsPath(foo, foo));
-
-  // Platform-specific concerns.
-  FilePath foo_caps(data_dir.Append(FILE_PATH_LITERAL("FOO")));
-#if defined(OS_WIN)
-  EXPECT_TRUE(file_util::ContainsPath(foo,
-      foo_caps.Append(FILE_PATH_LITERAL("bar.txt"))));
-  EXPECT_TRUE(file_util::ContainsPath(foo,
-      FilePath(foo.value() + FILE_PATH_LITERAL("/bar.txt"))));
-#elif defined(OS_MACOSX)
-  // We can't really do this test on OS X since the case-sensitivity of the
-  // filesystem is configurable.
-#elif defined(OS_POSIX)
-  EXPECT_FALSE(file_util::ContainsPath(foo,
-      foo_caps.Append(FILE_PATH_LITERAL("bar.txt"))));
-#endif
-}
-
 TEST_F(FileUtilTest, TouchFile) {
   FilePath data_dir =
       temp_dir_.path().Append(FILE_PATH_LITERAL("FilePathTest"));
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index 6c7f093..964302a 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -28,6 +28,18 @@
 
 using base::FilePath;
 
+namespace base {
+
+FilePath MakeAbsoluteFilePath(const FilePath& input) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  wchar_t file_path[MAX_PATH];
+  if (!_wfullpath(file_path, input.value().c_str(), MAX_PATH))
+    return FilePath();
+  return FilePath(file_path);
+}
+
+}  // namespace base
+
 namespace file_util {
 
 namespace {
@@ -37,15 +49,6 @@ const DWORD kFileShareAll =
 
 }  // namespace
 
-bool AbsolutePath(FilePath* path) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  wchar_t file_path_buf[MAX_PATH];
-  if (!_wfullpath(file_path_buf, path->value().c_str(), MAX_PATH))
-    return false;
-  *path = FilePath(file_path_buf);
-  return true;
-}
-
 bool Delete(const FilePath& path, bool recursive) {
   base::ThreadRestrictions::AssertIOAllowed();
 
diff --git a/base/files/file_path.cc b/base/files/file_path.cc
index 01d9eab..e9495a1 100644
--- a/base/files/file_path.cc
+++ b/base/files/file_path.cc
@@ -521,6 +521,24 @@ bool FilePath::IsAbsolute() const {
   return IsPathAbsolute(path_);
 }
 
+bool FilePath::EndsWithSeparator() const {
+  if (empty())
+    return false;
+  return IsSeparator(path_[path_.size() - 1]);
+}
+
+FilePath FilePath::AsEndingWithSeparator() const {
+  if (EndsWithSeparator() || path_.empty())
+    return *this;
+
+  StringType path_str;
+  path_str.reserve(path_.length() + 1);  // Only allocate string once.
+
+  path_str = path_;
+  path_str.append(&kSeparators[0], 1);
+  return FilePath(path_str);
+}
+
 FilePath FilePath::StripTrailingSeparators() const {
   FilePath new_path(path_);
   new_path.StripTrailingSeparatorsInternal();
diff --git a/base/files/file_path.h b/base/files/file_path.h
index cf3dc62..d66d631 100644
--- a/base/files/file_path.h
+++ b/base/files/file_path.h
@@ -285,6 +285,13 @@ class BASE_EXPORT FilePath {
   // platforms, an absolute path begins with a separator character.
   bool IsAbsolute() const;
 
+  // Returns true if the patch ends with a path separator character.
+  bool EndsWithSeparator() const WARN_UNUSED_RESULT;
+
+  // Returns a copy of this FilePath that ends with a trailing separator. If
+  // the input path is empty, an empty FilePath will be returned.
+  FilePath AsEndingWithSeparator() const WARN_UNUSED_RESULT;
+
   // Returns a copy of this FilePath that does not end with a trailing
   // separator.
   FilePath StripTrailingSeparators() const WARN_UNUSED_RESULT;
diff --git a/base/files/file_path_unittest.cc b/base/files/file_path_unittest.cc
index 9a690a9..28778d9 100644
--- a/base/files/file_path_unittest.cc
+++ b/base/files/file_path_unittest.cc
@@ -1196,7 +1196,34 @@ TEST_F(FilePathTest, NormalizePathSeparators) {
               "i: " << i << ", input: " << input.value();
   }
 }
-
 #endif
 
+TEST_F(FilePathTest, EndsWithSeparator) {
+  const UnaryBooleanTestData cases[] = {
+    { FPL(""), false },
+    { FPL("/"), true },
+    { FPL("foo/"), true },
+    { FPL("bar"), false },
+    { FPL("/foo/bar"), false },
+  };
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    FilePath input = FilePath(cases[i].input).NormalizePathSeparators();
+    EXPECT_EQ(cases[i].expected, input.EndsWithSeparator());
+  }
+}
+
+TEST_F(FilePathTest, AsEndingWithSeparator) {
+  const UnaryTestData cases[] = {
+    { FPL(""), FPL("") },
+    { FPL("/"), FPL("/") },
+    { FPL("foo"), FPL("foo/") },
+    { FPL("foo/"), FPL("foo/") }
+  };
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    FilePath input = FilePath(cases[i].input).NormalizePathSeparators();
+    FilePath expected = FilePath(cases[i].expected).NormalizePathSeparators();
+    EXPECT_EQ(expected.value(), input.AsEndingWithSeparator().value());
+  }
+}
+
 }  // namespace base
diff --git a/base/path_service.cc b/base/path_service.cc
index 1fd2f6f..32c6909 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -18,6 +18,7 @@
 #include "base/synchronization/lock.h"
 
 using base::FilePath;
+using base::MakeAbsoluteFilePath;
 
 namespace base {
   bool PathProvider(int key, FilePath* result);
@@ -217,9 +218,9 @@ bool PathService::Get(int key, FilePath* result) {
 
   if (path.ReferencesParent()) {
     // Make sure path service never returns a path with ".." in it.
-    if (!file_util::AbsolutePath(&path)) {
+    path = MakeAbsoluteFilePath(path);
+    if (path.empty())
       return false;
-    }
   }
   *result = path;
 
@@ -250,17 +251,16 @@ bool PathService::OverrideAndCreateIfNeeded(int key,
   // fore we protect this call with a flag.
   if (create) {
     // Make sure the directory exists. We need to do this before we translate
-    // this to the absolute path because on POSIX, AbsolutePath fails if called
-    // on a non-existent path.
+    // this to the absolute path because on POSIX, MakeAbsoluteFilePath fails
+    // if called on a non-existent path.
     if (!file_util::PathExists(file_path) &&
         !file_util::CreateDirectory(file_path))
       return false;
   }
 
-  // We need to have an absolute path, as extensions and plugins don't like
-  // relative paths, and will gladly crash the browser in CHECK()s if they get a
-  // relative path.
-  if (!file_util::AbsolutePath(&file_path))
+  // We need to have an absolute path.
+  file_path = MakeAbsoluteFilePath(file_path);
+  if (file_path.empty())
     return false;
 
   base::AutoLock scoped_lock(path_data->lock);
diff --git a/base/test/test_file_util_posix.cc b/base/test/test_file_util_posix.cc
index 6be1c82..731d976 100644
--- a/base/test/test_file_util_posix.cc
+++ b/base/test/test_file_util_posix.cc
@@ -17,6 +17,8 @@
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 
+using base::MakeAbsoluteFilePath;
+
 namespace file_util {
 
 namespace {
@@ -90,11 +92,12 @@ bool CopyRecursiveDirNoCache(const base::FilePath& source_dir,
   // This function does not properly handle destinations within the source
   base::FilePath real_to_path = dest_dir;
   if (PathExists(real_to_path)) {
-    if (!AbsolutePath(&real_to_path))
+    real_to_path = MakeAbsoluteFilePath(real_to_path);
+    if (real_to_path.empty())
       return false;
   } else {
-    real_to_path = real_to_path.DirName();
-    if (!AbsolutePath(&real_to_path))
+    real_to_path = MakeAbsoluteFilePath(real_to_path.DirName());
+    if (real_to_path.empty())
       return false;
   }
   if (real_to_path.value().compare(0, source_dir.value().size(),