Switch [String]FileWriter to use new file_io.h functions/types

Switch [String]FileWriter to use new file_io.h functions/types

Also add ScopedFileHandle as cross-platform version of ScopedFD/ScopedFileHANDLE.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/5f5e342584355bb495d12b54846a29041163e88c

[scottmg, 2014-12-19 18:22:52]

Patchset #4 (id:60001) has been deleted

[Mark Mentovai, 2014-12-19 18:57:44]

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.cc
File util/file/file_writer.cc (right):

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.cc...
util/file/file_writer.cc:129: #ifndef NDEBUG
Can you do this in the non-POSIX implementation too?

Maybe it’s easier if the #if is inside the method body.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:57: //! \brief Wraps `write()` or `WriteFile()` or
provides an alternate
Aside from WriteIoVec, none of these functions directly wrap what they say, they
just wrap the things from file_io. You can simplify the comments by just
referencing the file_io functions.

The advantage of that is that the file_io functions have defined their semantics
by making up the differences between underlying callsl ike write() and
WriteFile(). Since the comments in this file also say “alternate implementation
with identical semantics,” that’d leave open the question: identical to which?
write() or WriteFile().

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:67: //!     identical semantics on POSIX. This method
will write entire buffers,
“on POSIX” is confusing here, put it after writev() to clear it up.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:102: bool Open(const base::FilePath& path,
There’s one caller of this method, in tools/generate_dump.cc, that needs to be
updated.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:135: base::ScopedFD file_;
Maybe file_io.h should provide a typedef for the scoper too.

[scottmg, 2014-12-19 19:32:52]

Patchset #5 (id:100001) has been deleted

[scottmg, 2014-12-19 19:36:29]

Patchset #5 (id:120001) has been deleted

[scottmg, 2014-12-19 19:37:54]

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.cc
File util/file/file_writer.cc (right):

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.cc...
util/file/file_writer.cc:129: #ifndef NDEBUG
On 2014/12/19 18:57:44, Mark Mentovai wrote:
> Can you do this in the non-POSIX implementation too?
> 
> Maybe it’s easier if the #if is inside the method body.

Done.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:57: //! \brief Wraps `write()` or `WriteFile()` or
provides an alternate
On 2014/12/19 18:57:44, Mark Mentovai wrote:
> Aside from WriteIoVec, none of these functions directly wrap what they say,
they
> just wrap the things from file_io. You can simplify the comments by just
> referencing the file_io functions.
> 
> The advantage of that is that the file_io functions have defined their
semantics
> by making up the differences between underlying callsl ike write() and
> WriteFile(). Since the comments in this file also say “alternate
implementation
> with identical semantics,” that’d leave open the question: identical to which?
> write() or WriteFile().

Done.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:67: //!     identical semantics on POSIX. This method
will write entire buffers,
On 2014/12/19 18:57:44, Mark Mentovai wrote:
> “on POSIX” is confusing here, put it after writev() to clear it up.

Done.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:102: bool Open(const base::FilePath& path,
On 2014/12/19 18:57:44, Mark Mentovai wrote:
> There’s one caller of this method, in tools/generate_dump.cc, that needs to be
> updated.

Done.

https://codereview.chromium.org/815053004/diff/80001/util/file/file_writer.h#...
util/file/file_writer.h:135: base::ScopedFD file_;
On 2014/12/19 18:57:44, Mark Mentovai wrote:
> Maybe file_io.h should provide a typedef for the scoper too.

Done.

[Mark Mentovai, 2014-12-19 20:42:57]

LGTM

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h#new...
util/file/file_io.h:44: using ScopedFileHandle = base::ScopedFD;
Good thing this name is available. :)

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h#new...
util/file/file_io.h:162: //! \sa ScopedFD
These two \sa can become one now, same for the open-for-write function.

https://codereview.chromium.org/815053004/diff/160001/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/815053004/diff/160001/util/file/file_writer.h...
util/file/file_writer.h:49: //! \brief Wraps WriteFile(), or provides and
implementation with identical
and → an

https://codereview.chromium.org/815053004/diff/160001/util/win/scoped_handle.cc
File util/win/scoped_handle.cc (right):

https://codereview.chromium.org/815053004/diff/160001/util/win/scoped_handle....
util/win/scoped_handle.cc:24: PCHECK(CloseHandle(handle));
Did you mean to change this one instead or in addition? You only did
ScopedKernelHANDLE.

[scottmg, 2014-12-19 21:32:41]

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h#new...
util/file/file_io.h:44: using ScopedFileHandle = base::ScopedFD;
On 2014/12/19 20:42:57, Mark Mentovai wrote:
> Good thing this name is available. :)

You can see the future! :)

https://codereview.chromium.org/815053004/diff/160001/util/file/file_io.h#new...
util/file/file_io.h:162: //! \sa ScopedFD
On 2014/12/19 20:42:57, Mark Mentovai wrote:
> These two \sa can become one now, same for the open-for-write function.

Done.

https://codereview.chromium.org/815053004/diff/160001/util/file/file_writer.h
File util/file/file_writer.h (right):

https://codereview.chromium.org/815053004/diff/160001/util/file/file_writer.h...
util/file/file_writer.h:49: //! \brief Wraps WriteFile(), or provides and
implementation with identical
On 2014/12/19 20:42:57, Mark Mentovai wrote:
> and → an

Done.

https://codereview.chromium.org/815053004/diff/160001/util/win/scoped_handle.cc
File util/win/scoped_handle.cc (right):

https://codereview.chromium.org/815053004/diff/160001/util/win/scoped_handle....
util/win/scoped_handle.cc:24: PCHECK(CloseHandle(handle));
On 2014/12/19 20:42:57, Mark Mentovai wrote:
> Did you mean to change this one instead or in addition? You only did
> ScopedKernelHANDLE.

Urgh, I meant to change only the File one. It would work for the Kernel one, but
seems odd to call a "...File" function on the kernel handle.

[scottmg, 2014-12-19 21:33:07]

Committed patchset #7 (id:180001) manually as
5f5e342584355bb495d12b54846a29041163e88c (presubmit successful).