Report more relevant error code for windows base::ReplaceFile impl

Report more relevant error code for windows base::ReplaceFile impl

The windows version of base::ReplaceFile performs a MoveFile, followed by
a ReplaceFile if the move fails.

However, if the to_path does not exist, and the MoveFile fails, the
error reported by ReplaceFile will always be a NOT_FOUND, because
ReplaceFile requires the to_path to exist.

This patch just fowards the MoveFile error when the ReplaceFile's error
is NOT_FOUND. It will be more or equally as relevant.

BUG=689655
Review-Url: https://codereview.chromium.org/2681643005
Cr-Commit-Position: refs/heads/master@{#448970}
Committed: https://chromium.googlesource.com/chromium/src/+/b281ba4e35f9d2f1b28c6e92cae971f048025eb6

[Charlie Harrison, 2017-02-07 22:20:55]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:22:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-02-07 22:31:21]

csharrison@chromium.org changed reviewers:
+ dcheng@chromium.org

[Charlie Harrison, 2017-02-07 22:31:21]

Daniel, is this crazy? I think that if ReplaceFile returns NOT_FOUND and it is
*not* because to_path doesn't exist, then MoveFile will have also returned
NOT_FOUND.

[dcheng, 2017-02-07 23:24:51]

LGTM. I would personally find it more readable if it were broken out into two
distinct ifs, though I don't feel super strongly about this.

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc...
base/files/file_util_win.cc:137: File::Error move_error =
File::OSErrorToFileError(GetLastError());
DWORD error_code = ::GetLastError();

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc...
base/files/file_util_win.cc:152: *error = replace_error ==
File::FILE_ERROR_NOT_FOUND ? move_error
if ((replace_error = ::GetLastError()) != ERROR_FILE_NOT_FOUND)
  error_code = replace_error;
if (error)
  *error = File::OSErrorToFileError(error_code);

[commit-bot: I haz the power, 2017-02-07 23:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 23:30:51]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-07 23:31:22]

csharrison@chromium.org changed reviewers:
+ engedy@chromium.org

[Charlie Harrison, 2017-02-07 23:31:23]

Thanks!

+engedy, how do you feel about this as a quick chance to start getting better
windows metrics?

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc...
base/files/file_util_win.cc:152: *error = replace_error ==
File::FILE_ERROR_NOT_FOUND ? move_error
On 2017/02/07 23:24:50, dcheng wrote:
> if ((replace_error = ::GetLastError()) != ERROR_FILE_NOT_FOUND)
>   error_code = replace_error;
> if (error)
>   *error = File::OSErrorToFileError(error_code);

Going to push back on this, only because I think if we're comparing the raw
error we'll have to check PATH_NOT_FOUND as well. OSErrorToFileError combines
them into a single NOT_FOUND.

[dcheng, 2017-02-07 23:32:39]

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/2681643005/diff/1/base/files/file_util_win.cc...
base/files/file_util_win.cc:152: *error = replace_error ==
File::FILE_ERROR_NOT_FOUND ? move_error
On 2017/02/07 23:31:23, Charlie Harrison wrote:
> On 2017/02/07 23:24:50, dcheng wrote:
> > if ((replace_error = ::GetLastError()) != ERROR_FILE_NOT_FOUND)
> >   error_code = replace_error;
> > if (error)
> >   *error = File::OSErrorToFileError(error_code);
> 
> Going to push back on this, only because I think if we're comparing the raw
> error we'll have to check PATH_NOT_FOUND as well. OSErrorToFileError combines
> them into a single NOT_FOUND.

Makes sense, I'm OK with that.

[engedy, 2017-02-08 12:41:28]

The plan sounds awesome, thanks for jumping on this! LGTM.

[Charlie Harrison, 2017-02-08 12:59:29]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-02-08 12:59:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 13:03:32]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486558769180950, "parent_rev":
"cf2dce6a6dcb889081057acdd10448448813bdda", "commit_rev":
"b281ba4e35f9d2f1b28c6e92cae971f048025eb6"}

[commit-bot: I haz the power, 2017-02-08 13:04:05]

Description was changed from

==========
Report more relevant error code for windows base::ReplaceFile impl

The windows version of base::ReplaceFile performs a MoveFile, followed by
a ReplaceFile if the move fails.

However, if the to_path does not exist, and the MoveFile fails, the
error reported by ReplaceFile will always be a NOT_FOUND, because
ReplaceFile requires the to_path to exist.

This patch just fowards the MoveFile error when the ReplaceFile's error
is NOT_FOUND. It will be more or equally as relevant.

BUG=689655
==========

to

==========
Report more relevant error code for windows base::ReplaceFile impl

The windows version of base::ReplaceFile performs a MoveFile, followed by
a ReplaceFile if the move fails.

However, if the to_path does not exist, and the MoveFile fails, the
error reported by ReplaceFile will always be a NOT_FOUND, because
ReplaceFile requires the to_path to exist.

This patch just fowards the MoveFile error when the ReplaceFile's error
is NOT_FOUND. It will be more or equally as relevant.

BUG=689655

Review-Url: https://codereview.chromium.org/2681643005
Cr-Commit-Position: refs/heads/master@{#448970}
Committed:
https://chromium.googlesource.com/chromium/src/+/b281ba4e35f9d2f1b28c6e92cae9...
==========

[commit-bot: I haz the power, 2017-02-08 13:04:06]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b281ba4e35f9d2f1b28c6e92cae9...