Flush at the end of local file writing in FileWriter API.

Flush at the end of local file writing in FileWriter API.

This CL ensures the written content is flushed physically before
fileWriter.write() invokes the "onwriteend" event, for native local files.
Remote files (Google Drive files in Chrome OS) nor FileSystem API
files (in PERSISTENT or TEMPORARY storage) aren't affected.

The summary of the changes:
* Call Flush() before the final callback.
(webkit/fileapi/file_writer_delegate.cc)
* Delegate Flush() to net::FileStream::Flush 
(webkit/fileapi/local_file_stream_writer.cc)
* No-op implementation for Flush().
(webkit/fileapi/sanbox_file_stream_writer.cc)
(webkit/chromeos/fileapi/remote_file_stream_writer.cc)
* Implementation of asynchronous Flush.
(net/base/file_stream_{posix,win}.cc)
* Other files are just for reflecting the rename Flush -> FlushSync.

BUG=144790
R=willchan@chromium.org,kinuko@chromium.org,benjhayden@chromium.org
TBR=marja@chromium.org
TEST=out/Debug/browser_tests --gtest_filter='*FileSystemApi*'
TEST=./webkit/tools/layout_tests/run_webkit_tests.sh fast/filesystem
TEST=Manual steps reported in the issue.

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

[kinuko, 2012-09-26 08:34:08]

http://codereview.chromium.org/10986045/diff/1/webkit/fileapi/file_writer_del...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10986045/diff/1/webkit/fileapi/file_writer_del...
webkit/fileapi/file_writer_delegate.cc:210: }
Do we need to do this when error has happend after some writes?

Btw fyi we're making a following change so that it'll become easier to tell if
there' were any writes on error case
http://codereview.chromium.org/10956064/

[kinaba, 2012-09-28 11:15:25]

http://codereview.chromium.org/10986045/diff/1/webkit/fileapi/file_writer_del...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10986045/diff/1/webkit/fileapi/file_writer_del...
webkit/fileapi/file_writer_delegate.cc:210: }
On 2012/09/26 08:34:08, kinuko wrote:
> Do we need to do this when error has happend after some writes?
> 
> Btw fyi we're making a following change so that it'll become easier to tell if
> there' were any writes on error case
> http://codereview.chromium.org/10956064/

Thanks. I rebased this CL on top of it.

I've also implemented Flush for the error case (PTAL for the latest patchset).

[kinaba, 2012-09-28 11:26:13]

Hi, could you review the CL, specifically on the following areas?
+willchan: net/base changes.
+kinuko: webkit/(chromeos/)fileapi changes.

Also cc-ing +benwells, please warn me if you feel concern on this CL regarding
effects on platform apps (refer also the comment #16 of the bug).

Thanks!

[willchan no longer on Chromium, 2012-09-28 21:32:30]

+paivanof

paivanof: Can you take a quick look to make sure this will be consistent with
your refactorings?

[pivanof, 2012-09-28 22:21:05]

On 2012/09/28 21:32:30, willchan wrote:
> +paivanof
> 
> paivanof: Can you take a quick look to make sure this will be consistent with
> your refactorings?

Consistent enough.

I wonder is usage of WeakPtrFactory really necessary in LocalFileStreamWriter?
It's alright as a boilerplate for the general pattern but it's used only in
callbacks passed to FileStream. And FileStream is always deleted in
LocalFileStreamWriter's destructor, so there's no way these callbacks will be
used after reset of WeakPtrFactory.

[kinaba, 2012-09-29 00:25:06]

On 2012/09/28 22:21:05, pivanof wrote:
> I wonder is usage of WeakPtrFactory really necessary in LocalFileStreamWriter?
> It's alright as a boilerplate for the general pattern but it's used only in
> callbacks passed to FileStream. And FileStream is always deleted in
> LocalFileStreamWriter's destructor, so there's no way these callbacks will be
> used after reset of WeakPtrFactory.

Good point. Thanks.

In my vague memory, I thought I was afraid of the case where the callbacks are
used while FileStream's destructor is waiting for IO completion; however this
seems not the case, after I re-examined the implementation of FileStream. If it
is safe to rely on this assumption (i.e., ~FileStream shuts up all in-flight
callbacks), it will be possible to remove the weak pointers. Is it?

Either way, could you let me go this patch as in the current form, and do the
refactoring in a separate patch? I'd like to merge this one to M23 branch, so I
want to minimize any possible effects it might cause.

[willchan no longer on Chromium, 2012-09-29 00:30:28]

LGTM

If it's M23 blocking, feel free to call it out and I'll review it sooner.

[pivanof, 2012-09-29 00:45:19]

On 2012/09/29 00:25:06, kinaba wrote:
> On 2012/09/28 22:21:05, pivanof wrote:
> If it
> is safe to rely on this assumption (i.e., ~FileStream shuts up all in-flight
> callbacks), it will be possible to remove the weak pointers. Is it?

Yes, it's safe to assume that especially after my pending refactoring patch.

> Either way, could you let me go this patch as in the current form, and do the
> refactoring in a separate patch?

Sure, such refactoring can wait. I've brought it up because when I looked at the
patch the thought immediately jumped into my mind: "WeakPtr is used, so
FileStream can outlive caller. But how can that happen?"

[kinaba, 2012-09-29 00:52:28]

@willchan, thanks.

Adding reviewers
+benjhayden for content/browser/download/OWNERS
+marja for chrome/browser/sessions/OWNERS
Could you take a brief look?
The change to the directories is just a simple renaming so I'm 99.9% sure it
should be fine, but just in case (while finishing fileapi-side review).

[kinaba, 2012-09-29 00:54:38]

On 2012/09/29 00:45:19, pivanof wrote:
> On 2012/09/29 00:25:06, kinaba wrote:
> > On 2012/09/28 22:21:05, pivanof wrote:
> > If it
> > is safe to rely on this assumption (i.e., ~FileStream shuts up all in-flight
> > callbacks), it will be possible to remove the weak pointers. Is it?
> 
> Yes, it's safe to assume that especially after my pending refactoring patch.
> 
> > Either way, could you let me go this patch as in the current form, and do
the
> > refactoring in a separate patch?
> 
> Sure, such refactoring can wait. I've brought it up because when I looked at
the
> patch the thought immediately jumped into my mind: "WeakPtr is used, so
> FileStream can outlive caller. But how can that happen?"

Makes sense. I'll revisit this point after M23 is stabilized.

[benjhayden, 2012-09-29 16:15:24]

LGTM
It looks like you renamed Flush() to FlushSync() and added a new Flush() which
is async. Any reason not to keep Flush() sync and add a new FlushAsync()
instead?

[pivanof, 2012-09-29 16:36:35]

On 2012/09/29 16:15:24, benjhayden_chromium wrote:
> It looks like you renamed Flush() to FlushSync() and added a new Flush() which
> is async. Any reason not to keep Flush() sync and add a new FlushAsync()
> instead?

Because the encouraged and default way of using FileStream should be async. All
other methods (Open(), Read(), Write(), Seek()) follow the same pattern.

[Randy Smith (Not in Mondays), 2012-09-30 18:17:40]

On 2012/09/29 16:36:35, pivanof wrote:
> On 2012/09/29 16:15:24, benjhayden_chromium wrote:
> > It looks like you renamed Flush() to FlushSync() and added a new Flush()
which
> > is async. Any reason not to keep Flush() sync and add a new FlushAsync()
> > instead?
> 
> Because the encouraged and default way of using FileStream should be async.
All
> other methods (Open(), Read(), Write(), Seek()) follow the same pattern.

Could you say a little more or give a pointer for the rationale for this
pattern, and when it should and shouldn't be applied?  I can certainly see it
for file operations in general (why wait for an operation when you don't need
to) but for Flush() and Close() I see some value in it.  Flush() because it's
not really doing anything (other than saying "hurry up getting this out to
disk!") if it's not synchronous, and Close() because some consumers may care
about whether or not they can rely on the data as having been saved to
persistent store.

Note: I'm not trying to delay this checkin or change what you're doing.  I'm
just looking for any information that might affect whether the downloads system
should keep exporting its current functionality (when we report a download as
complete, the download has been fully flushed to disk) or not.

[pivanof, 2012-09-30 21:26:51]

On 2012/09/30 18:17:40, rdsmith wrote:
> On 2012/09/29 16:36:35, pivanof wrote:
> > On 2012/09/29 16:15:24, benjhayden_chromium wrote:
> > > It looks like you renamed Flush() to FlushSync() and added a new Flush()
> which
> > > is async. Any reason not to keep Flush() sync and add a new FlushAsync()
> > > instead?
> > 
> > Because the encouraged and default way of using FileStream should be async.
> All
> > other methods (Open(), Read(), Write(), Seek()) follow the same pattern.
> 
> Could you say a little more or give a pointer for the rationale for this
> pattern, and when it should and shouldn't be applied?

If you are asking about rationale behind decision of Flush+FlushSync against
FlushAsync+Flush then willchan and satorux could probably answer that better (as
it all started with http://codereview.chromium.org/9349005). My understanding of
this is FileStream is used most often in async mode to not block UI and IO
threads. So if someone uses FileStream without much thinking about the issue he
should use default methods like Flush(). If someone absolutely knows what he's
doing he can use FlushSync() and it will be immediately visible during review,
so reviewer will have to give that another thought to understand whether it's
absolutely necessary to use sync methods and whether it could hurt.

> I can certainly see it
> for file operations in general (why wait for an operation when you don't need
> to) but for Flush() and Close() I see some value in it.  Flush() because it's
> not really doing anything (other than saying "hurry up getting this out to
> disk!") if it's not synchronous

It also notifies the caller when flush is complete and everything is on disk.

> and Close() because some consumers may care
> about whether or not they can rely on the data as having been saved to
> persistent store.

Note: FileStream::Close*() methods will disappear soon --
http://codereview.chromium.org/10701050/. Besides closing file doesn't guarantee
that its contents went to persistent store.

[kinuko, 2012-10-01 03:03:02]

webkit/fileapi/* lgtm

[kinaba, 2012-10-01 04:37:30]

On 2012/09/30 18:17:40, rdsmith wrote:
> ... and Close() because some consumers may care
> about whether or not they can rely on the data as having been saved to
> persistent store.

This is the exact reason why I'm adding _async_ Flush in this CL.
IMO, sync vs async is not about wait vs not-wait. Rather, it is about
blocking vs yielding to other operations on the same thread while waiting.

For some reason the client code (webkit/fileapi/*) touched in this CL is
driving net::FileStream from IO thread, which should not block, yet still
I want to wait for its physical flush.

# Perhaps my CL title was a little misleading...

> Note: I'm not trying to delay this checkin or change what you're doing.  I'm
> just looking for any information that might affect whether the downloads
system
> should keep exporting its current functionality (when we report a download as
> complete, the download has been fully flushed to disk) or not.

I think the functionality should be kept (for the same reason I'm introducing it
for FileAPI).

[kinaba, 2012-10-01 05:06:38]

On 2012/10/01 03:03:02, kinuko wrote:
> webkit/fileapi/* lgtm
Thanks, Kinuko.

Please allow me to go to CQ with TRB=marja for chrome/browser/sessions/*, since
- The change under sessions/* is just a mechanical renaming.
- This is M23 blocking and hence timely fix is desired.

[commit-bot: I haz the power, 2012-10-01 05:06:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/10986045/10001

[commit-bot: I haz the power, 2012-10-01 05:12:01]

Retried try job too often for step(s) compile

[marja, 2012-10-01 07:33:05]

post-commit LGTM (chrome/browser/sessions/session_backend.cc)

[kinaba, 2012-10-01 07:33:54]

On 2012/10/01 07:33:05, marja wrote:
> post-commit LGTM (chrome/browser/sessions/session_backend.cc)

Thank you very much!