Make base:ReplaceFile return an informative error.

base/file_util.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file contains utility functions for dealing with the local
 // filesystem.
 
 #ifndef BASE_FILE_UTIL_H_
 #define BASE_FILE_UTIL_H_
 
@@ skipping 83 matching lines @@
 
 // Same as Move but allows paths with traversal components.
 // Use only with extreme care.
 BASE_EXPORT bool MoveUnsafe(const base::FilePath& from_path,
                             const base::FilePath& to_path);
 
 // Renames file |from_path| to |to_path|. Both paths must be on the same
 // volume, or the function will fail. Destination file will be created
 // if it doesn't exist. Prefer this function over Move when dealing with
 // temporary files. On Windows it preserves attributes of the target file.
-// Returns true on success.
+// Returns true on success and optionally sets *error to base::PLATFORM_FILE_OK.

[jar (doing other things), 2013/05/07 01:12:36]

nit: I think you should avoid specing things as "optional."
Better (and simpler?) would be return true on success, and does not change the
value of |error|.  Returns false if there is an error, and places an the values
into |*error| if |error| is non-null.

[dgrogan, 2013/05/07 19:02:05]

That sounds fine. But it will require changing all the callers. I was aiming for
least disruptive. But if you think doing that is the way to go then I'm all for
it.

[jar (doing other things), 2013/05/07 19:44:50]

Default args would indeed be less disruptive... but that is very clearly against
the style guide.

The suggestion about removing "optionally sets" and specing it to always set the
errorr IFF returning false shouldn't make it more disruptive.

It is fine to have the "no arg" method call the method with a return |*error|
arg, so that your code doesn't have to be replicated.

+// Returns false on failure and optionally sets *error appropriately.
 BASE_EXPORT bool ReplaceFile(const base::FilePath& from_path,
-                             const base::FilePath& to_path);
+                             const base::FilePath& to_path,
+                             base::PlatformFileError* error = NULL);

[jar (doing other things), 2013/05/07 01:12:36]

nit: default arguments are not allowed according to the style guide.  This sure
seems like a plausibly reasonable exception... but there are none of this sort.

 
 // Copies a single file. Use CopyDirectory to copy directories.
 // This function fails if either path contains traversal components ('..').
 BASE_EXPORT bool CopyFile(const base::FilePath& from_path,
                           const base::FilePath& to_path);
 
 // Same as CopyFile but allows paths with traversal components.
 // Use only with extreme care.
 BASE_EXPORT bool CopyFileUnsafe(const base::FilePath& from_path,
                                 const base::FilePath& to_path);
@@ skipping 429 matching lines @@
 
 // Attempts determine the FileSystemType for |path|.
 // Returns false if |path| doesn't exist.
 BASE_EXPORT bool GetFileSystemType(const base::FilePath& path,
                                    FileSystemType* type);
 #endif
 
 }  // namespace file_util
 
 #endif  // BASE_FILE_UTIL_H_