Make base:ReplaceFile return an informative error.

Make base:ReplaceFile return an informative error.

It currently just returns true/false to indicate success.
More information to diagnose and log failures would be
helpful in IndexedDB.

This patch also logs the new error when ReplaceFile fails in
IndexedDB.

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

[dgrogan, 2013-05-06 19:55:35]

Jim, could you review this? Again, let me know if you'd rather me find a
different base/ OWNER.

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

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h#newcode104
base/file_util.h:104: // Returns true on success and optionally sets *error to
base::PLATFORM_FILE_OK.
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.

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h#newcode108
base/file_util.h:108: base::PlatformFileError* error = NULL);
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.

https://codereview.chromium.org/14886003/diff/1002/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/14886003/diff/1002/base/file_util_posix.cc#ne...
base/file_util_posix.cc:245: return true;
nit: switch the order of the results around, and you won't need curlies, or
extra indent.  This also puts the common case up front, and makes reading
(across an early return) easier.

if(...)
  return true
...

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
Who else uses these? Why do they need to be visible in the common header?  Why
wouldn't a caller that needed such include a platform specific header?

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

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h#newcode104
base/file_util.h:104: // Returns true on success and optionally sets *error to
base::PLATFORM_FILE_OK.
On 2013/05/07 01:12:36, jar wrote:
> 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.

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.

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 01:12:36, jar wrote:
> Who else uses these?

platform_file_{win,posix}.cc and file_util_{win,posix}.cc use them.

Why do they need to be visible in the common header?  Why
> wouldn't a caller that needed such include a platform specific header?

I think a platform-specific header would be fine. Is there an appropriate
existing header? Or should I create platform_file_win.h and
platform_file_posix.h?

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

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/file_util.h#newcode104
base/file_util.h:104: // Returns true on success and optionally sets *error to
base::PLATFORM_FILE_OK.
On 2013/05/07 19:02:05, dgrogan wrote:
> On 2013/05/07 01:12:36, jar wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 19:02:05, dgrogan wrote:
> On 2013/05/07 01:12:36, jar wrote:
> > Who else uses these?
> 
> platform_file_{win,posix}.cc and file_util_{win,posix}.cc use them.
> 
> Why do they need to be visible in the common header?  Why
> > wouldn't a caller that needed such include a platform specific header?
> 
> I think a platform-specific header would be fine. Is there an appropriate
> existing header? Or should I create platform_file_win.h and
> platform_file_posix.h?

If there are shared usage of these platform specific methods, then it is
reasonable to put them in platform specific header (perhaps as you suggested
mirroring the platform specific source).  I guess if this is the only thing in
that header, the above format would be OK.... but...

I didn't see anyone else calling these though.  Who else is calling them? Why
are they needed in a header?

[dgrogan, 2013-05-07 21:24:06]

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 19:44:50, jar wrote:
> On 2013/05/07 19:02:05, dgrogan wrote:
> > On 2013/05/07 01:12:36, jar wrote:
> > > Who else uses these?
> > 
> > platform_file_{win,posix}.cc and file_util_{win,posix}.cc use them.
> > 
> I didn't see anyone else calling these though.  Who else is calling them? Why
> are they needed in a header?

As above (I suspect you just didn't see that comment last time around) file_util
and platform_file use them:
https://codereview.chromium.org/14886003/diff/1002/base/file_util_posix.cc#ne...
https://codereview.chromium.org/14886003/diff/1002/base/platform_file_posix.c...

If you DID see that comment, and know that both of those places use those
methods, then I am confused. They are in platform_file.h so that
file_util_{win,posix} can call them. platform_file_{win,posix} also calls it. Am
I missing something fundamental?

[jar (doing other things), 2013-05-07 21:39:01]

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 21:24:06, dgrogan wrote:
> On 2013/05/07 19:44:50, jar wrote:
> > On 2013/05/07 19:02:05, dgrogan wrote:
> > > On 2013/05/07 01:12:36, jar wrote:
> > > > Who else uses these?
> > > 
> > > platform_file_{win,posix}.cc and file_util_{win,posix}.cc use them.
> > > 
> > I didn't see anyone else calling these though.  Who else is calling them?
Why
> > are they needed in a header?
> 
> As above (I suspect you just didn't see that comment last time around)
file_util
> and platform_file use them:
>
https://codereview.chromium.org/14886003/diff/1002/base/file_util_posix.cc#ne...
>
https://codereview.chromium.org/14886003/diff/1002/base/platform_file_posix.c...
> 
> If you DID see that comment, and know that both of those places use those
> methods, then I am confused. They are in platform_file.h so that
> file_util_{win,posix} can call them. platform_file_{win,posix} also calls it.
Am
> I missing something fundamental?

OK.  I didn't spot the uses.  Sorry to have missed your comment.

Is it worth moving these into the ifdef on line 22?  I don't think there is a
style rule, and seeing the related typdefs nearby probably makes this a tad
easier to read.

[dgrogan, 2013-05-07 21:44:43]

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 21:39:02, jar wrote:
> Is it worth moving these into the ifdef on line 22?  I don't think there is a
> style rule, and seeing the related typdefs nearby probably makes this a tad
> easier to read.

Heh, that's where I tried to put them originally. The problem is the functions
return PlatformFileError, which isn't defined until line 65.

[dgrogan, 2013-05-07 22:05:57]

Latest patch that addresses comments is uploaded, take a look when you can.

[jar (doing other things), 2013-05-07 22:31:32]

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 21:44:43, dgrogan wrote:
> On 2013/05/07 21:39:02, jar wrote:
> > Is it worth moving these into the ifdef on line 22?  I don't think there is
a
> > style rule, and seeing the related typdefs nearby probably makes this a tad
> > easier to read.
> 
> Heh, that's where I tried to put them originally. The problem is the functions
> return PlatformFileError, which isn't defined until line 65.

How about splitting the difference... and uniting the ifdefs up before line 121,
which may be the first users of the typdefs seen on line 22?

(personal nit): The fewer conditional ifdef contexts my mind has to go
in-and-out of in a read, the better.

https://codereview.chromium.org/14886003/diff/51001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/14886003/diff/51001/base/file_util.h#newcode106
base/file_util.h:106: BASE_EXPORT bool ReplaceFile(const base::FilePath&
from_path,
nit: You can't overload functions, so you need a different name from line 111
(which is why you can't use default arguments either).

Maybe:
ReplaceFileGetError
ReplaceFileAndGetError
other?

https://codereview.chromium.org/14886003/diff/51001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/14886003/diff/51001/base/file_util_win.cc#new...
base/file_util_win.cc:151: bool ReplaceFile(const FilePath& from_path, const
FilePath& to_path,
nit: One arg per line when you wrap args in declaration or definition.

[dgrogan, 2013-05-07 22:45:16]

Thanks for the comments. Next patch is up.

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/14886003/diff/1002/base/platform_file.h#newco...
base/platform_file.h:211: #endif
On 2013/05/07 22:31:32, jar wrote:
> On 2013/05/07 21:44:43, dgrogan wrote:
> > On 2013/05/07 21:39:02, jar wrote:
> > > Is it worth moving these into the ifdef on line 22?  I don't think there
is
> a
> > > style rule, and seeing the related typdefs nearby probably makes this a
tad
> > > easier to read.
> > 
> > Heh, that's where I tried to put them originally. The problem is the
functions
> > return PlatformFileError, which isn't defined until line 65.
> 
> How about splitting the difference... and uniting the ifdefs up before line
121,
> which may be the first users of the typdefs seen on line 22?
> 
> (personal nit): The fewer conditional ifdef contexts my mind has to go
> in-and-out of in a read, the better.

Done.

https://codereview.chromium.org/14886003/diff/51001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/14886003/diff/51001/base/file_util.h#newcode106
base/file_util.h:106: BASE_EXPORT bool ReplaceFile(const base::FilePath&
from_path,
On 2013/05/07 22:31:32, jar wrote:
> nit: You can't overload functions, so you need a different name from line 111
> (which is why you can't use default arguments either).
> 
> Maybe:
> ReplaceFileGetError
> ReplaceFileAndGetError
> other?

Done. I went with ReplaceFileAndGetError.

https://codereview.chromium.org/14886003/diff/51001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/14886003/diff/51001/base/file_util_win.cc#new...
base/file_util_win.cc:151: bool ReplaceFile(const FilePath& from_path, const
FilePath& to_path,
On 2013/05/07 22:31:32, jar wrote:
> nit: One arg per line when you wrap args in declaration or definition.

Done.

[jar (doing other things), 2013-05-07 22:52:45]

lgtm

[commit-bot: I haz the power, 2013-05-07 22:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/14886003/62001

[commit-bot: I haz the power, 2013-05-07 23:04:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-07 23:08:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/14886003/70002

[commit-bot: I haz the power, 2013-05-08 00:14:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-08 14:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/14886003/81001

[commit-bot: I haz the power, 2013-05-08 22:02:38]

Change committed as 199025