mojo filesystem: Simplify full file reading/writing.

mojo filesystem: Simplify full file reading/writing.

One common pattern that's coming up multiple times is that we want to
read or write the full contents of a file. The first attempt at an
interface to that had was put on the File object.

However, file seeking behaviour differs between platforms. Performing a
seek on an empty file is safe on posix and errors on Windows. And when
dealing with arbitrary File objects, we want to seek to the beginning
just in case there was any previous usage on the File object.

It was also cumbersome. The user was still responsible for opening the
file and closing it once they were done with it. Putting these
operations on Directory not only removes a bug, but also simplifies the
interface.

BUG=557405
Committed: https://crrev.com/fd918ae590f61f145d75c800abb8eb050ca32b06
Cr-Commit-Position: refs/heads/master@{#371845}

[Elliot Glaysher, 2016-01-26 22:46:55]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-01-27 01:08:40]

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/d...
File components/filesystem/directory_impl.cc (right):

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/d...
components/filesystem/directory_impl.cc:243: while ((len =
base_file.ReadAtCurrentPos(buf.get(), kBufferSize)) > 0)
ReadAtCurrentPos returns an int.

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
File components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc
(right):

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc:376:
CHECK_EQ(FileError::OK, err);
Seems like we shouldn't crash here, but have some sort of error handling.

[Elliot Glaysher, 2016-01-27 01:13:42]

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
File components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc
(right):

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc:376:
CHECK_EQ(FileError::OK, err);
On 2016/01/27 01:08:40, sky wrote:
> Seems like we shouldn't crash here, but have some sort of error handling.

There's no real error checking here because there's no real error checking on
write in the JsonPrefStore. While I could add a call to
error_delegate_->OnError(), this would be a divergence in the behaviour.

(I agree with you that it is bizarre that the PrefService system doesn't seem to
verify that it's successfully written preferences to disk.)

[sky, 2016-01-27 02:48:13]

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
File components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc
(right):

https://codereview.chromium.org/1634293002/diff/40001/components/filesystem/p...
components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc:376:
CHECK_EQ(FileError::OK, err);
On 2016/01/27 01:13:42, Elliot Glaysher wrote:
> On 2016/01/27 01:08:40, sky wrote:
> > Seems like we shouldn't crash here, but have some sort of error handling.
> 
> There's no real error checking here because there's no real error checking on
> write in the JsonPrefStore. While I could add a call to
> error_delegate_->OnError(), this would be a divergence in the behaviour.
> 
> (I agree with you that it is bizarre that the PrefService system doesn't seem
to
> verify that it's successfully written preferences to disk.)

Error checking is different than crashing if there is an error. Shouldn't this
ignore the error, like the rest of code does?

[Elliot Glaysher, 2016-01-27 18:10:20]

ptal

[sky, 2016-01-27 19:21:09]

LGTM

[Elliot Glaysher, 2016-01-27 19:23:47]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-01-27 19:26:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634293002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634293002/80001

[commit-bot: I haz the power, 2016-01-27 19:42:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-27 19:43:00]

Description was changed from

==========
mojo filesystem: Simplify full file reading/writing.

One common pattern that's coming up multiple times is that we want to
read or write the full contents of a file. The first attempt at an
interface to that had was put on the File object.

However, file seeking behaviour differs between platforms. Performing a
seek on an empty file is safe on posix and errors on Windows. And when
dealing with arbitrary File objects, we want to seek to the beginning
just in case there was any previous usage on the File object.

It was also cumbersome. The user was still responsible for opening the
file and closing it once they were done with it. Putting these
operations on Directory not only removes a bug, but also simplifies the
interface.

BUG=557405
==========

to

==========
mojo filesystem: Simplify full file reading/writing.

One common pattern that's coming up multiple times is that we want to
read or write the full contents of a file. The first attempt at an
interface to that had was put on the File object.

However, file seeking behaviour differs between platforms. Performing a
seek on an empty file is safe on posix and errors on Windows. And when
dealing with arbitrary File objects, we want to seek to the beginning
just in case there was any previous usage on the File object.

It was also cumbersome. The user was still responsible for opening the
file and closing it once they were done with it. Putting these
operations on Directory not only removes a bug, but also simplifies the
interface.

BUG=557405

Committed: https://crrev.com/fd918ae590f61f145d75c800abb8eb050ca32b06
Cr-Commit-Position: refs/heads/master@{#371845}
==========

[commit-bot: I haz the power, 2016-01-27 19:43:01]

Patchset 5 (id:??) landed as
https://crrev.com/fd918ae590f61f145d75c800abb8eb050ca32b06
Cr-Commit-Position: refs/heads/master@{#371845}

[Garrett Casto, 2016-01-27 20:38:28]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1643733002/ by gcasto@chromium.org.

The reason for reverting is: Broke the Windows build. 

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/1043...

FAILED: ninja -t msvc -e environment.x64 -- E:\b\build\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64/cl.exe" /nologo
/showIncludes /FC @obj/components/filesystem/lib/directory_impl.obj.rsp /c
../../components/filesystem/directory_impl.cc
/Foobj/components/filesystem/lib/directory_impl.obj
/Fdobj/components/filesystem/lib_cc.pdb
e:\b\build\slave\win_x64_gn\build\src\components\filesystem\directory_impl.cc(274)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_x64_gn\build\src\components\filesystem\directory_impl.cc(274)
: warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss
of data
ninja: build stopped: subcommand failed..