Send OnModifyFile Notification when File Write Finishes

Send OnModifyFile Notification when File Write completes successfully or fails but something was written. e.g. partial write before failure.

BUG=150060
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158784

[calvinlo, 2012-09-24 08:26:58]

Please note the changes to the
change_observer()->get_and_reset_modify_file_count() in each of the test. I'm
not sure if this is the desired behavior but this is my best guess.

[kinuko, 2012-09-25 04:12:26]

http://codereview.chromium.org/10956064/diff/1/webkit/fileapi/local_file_syst...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/1/webkit/fileapi/local_file_syst...
webkit/fileapi/local_file_system_operation.cc:693:
&FileChangeObserver::OnModifyFile, MakeTuple(url));
On the second thought (and on the contrary to what I said), can we try not to
fire ModifyFile when we failed before starting to write anything?

I think we can keep a writing status in WriterDelegate, not necessarily as a
total_bytes_writtenbn format but may be a boolean or an enum status flag (we
generally prefer enum than boolean) which could be either
WRITER_DELEGATE_STATE_INIT or WRITING.

To propagate the delegate's status we could add another parameter to
WriterDelegate::DidWrite but just passing a special value (i.e -1) would also
work.

[calvinlo, 2012-09-25 09:42:14]

As discussed in person, I ended up solving this by changing the value of boolean
"complete" to an enum.

There are 4 values
    NOT_COMPLETE,
    COMPLETE_SUCCESSFUL,
    ERROR_WRITE_STARTED,
    ERROR_WRITE_NOT_STARTED

So now the onModifyFile event only fires when write status = COMPLETE_SUCCESSFUL
|| ERROR_WRITE_STARTED (i.e. the two situations in which fileWriteDelegate is
done and at last some writing occurred)

[kinuko, 2012-09-25 14:00:49]

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
File webkit/chromeos/fileapi/remote_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
webkit/chromeos/fileapi/remote_file_system_operation.h:107: );
style-nit: can you format this function like the one above so that we don't need
to put ');' on a separate line

void DidWrite(
    base::PlatformFileError result,
    int64 bytes,
    ...);

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:181: return (writing_started_) ?
ERROR_WRITE_STARTED : ERROR_WRITE_NOT_STARTED;
nit: no parens needed around writing_started_

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:204: WriteCompletionStatus status =
(done) ? COMPLETE_SUCCESSFUL : NOT_COMPLETE;
nit: no parens needed

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:28: enum WriteCompletionStatus {
Since DidWrite could be called with a status repeatedly maybe we should call
this enum WriteProgressStatus?

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:32: ERROR_WRITE_NOT_STARTED,
NOT_COMPLETE always indicates successful partial write (write was successful but
have more pending data), right?  Then I think I prefer naming these enums like:

SUCCESS_IO_PENDING,
SUCCESS_COMPLETED,
ERROR_WRITE_STARTED,
ERROR_WRITE_NOT_STARTED,

so that one can easily tell the status indicates error or not.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:38: DelegateWriteCallback;
nit: indentation looks a bit off

typedef base::Callback<void(
    base...,
    int64 bytes,
    ...)> DelegateWriteCallback;

or

typedef base::Callback<
    void(base...,
         int64 bytes,
         ...)> DelegateWriteCallback;

looks more common.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:82: WriteCompletionStatus
ErrorCompletionStatus();
Please make this method const if it doesn't change its internal state.

Also how about renaming this to:
GetCompletionStatusOnError() const

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate_unittest.cc:43: bool complete() const {
return complete_; }
Can we change this to return WriteCompletionStatus so that we can check the
completion status more specifically?

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:692: write_status ==
FileWriterDelegate::ERROR_WRITE_STARTED) {
This might be only me but I found this code kinda makes me worry if we're
covering all possible situations we should fire the notification.

How about moving line 697 before this if condition and change it like:

if (complete && write_status != ERROR_WRITE_NOT_STARTED)

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:699: if (write_status ==
FileWriterDelegate::COMPLETE_SUCCESSFUL ||
shouldn't this just use 'complete'?

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.h:24: #include
"webkit/quota/quota_manager.h"
please sort headers.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation_write_unittest.cc:28: 
nit: extra empty line

[tzik, 2012-09-26 01:41:11]

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.h:24: #include
"webkit/quota/quota_manager.h"
maybe, quota_manager.h is not needed here.

[calvinlo, 2012-09-26 05:23:07]

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
File webkit/chromeos/fileapi/remote_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
webkit/chromeos/fileapi/remote_file_system_operation.h:107: );
On 2012/09/25 14:00:49, kinuko wrote:
> style-nit: can you format this function like the one above so that we don't
need
> to put ');' on a separate line
> 
> void DidWrite(
>     base::PlatformFileError result,
>     int64 bytes,
>     ...);

Originally on its own new line because the previous line was 81 chars long.
Added typedef to shorten the type so it's not as long and moved the ); back up
one line.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:181: return (writing_started_) ?
ERROR_WRITE_STARTED : ERROR_WRITE_NOT_STARTED;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: no parens needed around writing_started_

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:204: WriteCompletionStatus status =
(done) ? COMPLETE_SUCCESSFUL : NOT_COMPLETE;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: no parens needed

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:28: enum WriteCompletionStatus {
On 2012/09/25 14:00:49, kinuko wrote:
> Since DidWrite could be called with a status repeatedly maybe we should call
> this enum WriteProgressStatus?

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:38: DelegateWriteCallback;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: indentation looks a bit off
> 
> typedef base::Callback<void(
>     base...,
>     int64 bytes,
>     ...)> DelegateWriteCallback;
> 
> or
> 
> typedef base::Callback<
>     void(base...,
>          int64 bytes,
>          ...)> DelegateWriteCallback;
> 
> looks more common.

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:82: WriteCompletionStatus
ErrorCompletionStatus();
On 2012/09/25 14:00:49, kinuko wrote:
> Please make this method const if it doesn't change its internal state.
> 
> Also how about renaming this to:
> GetCompletionStatusOnError() const

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate_unittest.cc:43: bool complete() const {
return complete_; }
On 2012/09/25 14:00:49, kinuko wrote:
> Can we change this to return WriteCompletionStatus so that we can check the
> completion status more specifically?

Done. Please note that a lot more lines in this file have been changed because
of the conversion of (bool complete_) to (WriteProgressStatus write_status_).

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:692: write_status ==
FileWriterDelegate::ERROR_WRITE_STARTED) {
On 2012/09/25 14:00:49, kinuko wrote:
> This might be only me but I found this code kinda makes me worry if we're
> covering all possible situations we should fire the notification.
> 
> How about moving line 697 before this if condition and change it like:
> 
> if (complete && write_status != ERROR_WRITE_NOT_STARTED)

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:699: if (write_status ==
FileWriterDelegate::COMPLETE_SUCCESSFUL ||
On 2012/09/25 14:00:49, kinuko wrote:
> shouldn't this just use 'complete'?

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.h:24: #include
"webkit/quota/quota_manager.h"
On 2012/09/26 01:41:12, tzik wrote:
> maybe, quota_manager.h is not needed here.

Oops, good catch. Removing second include call. The header is still needed
though for DidGetUsageAndQuotaAndRunTask().

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation_write_unittest.cc:28: 
On 2012/09/25 14:00:49, kinuko wrote:
> nit: extra empty line

Done.

[calvinlo, 2012-09-26 05:23:07]

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
File webkit/chromeos/fileapi/remote_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/chromeos/fileapi/rem...
webkit/chromeos/fileapi/remote_file_system_operation.h:107: );
On 2012/09/25 14:00:49, kinuko wrote:
> style-nit: can you format this function like the one above so that we don't
need
> to put ');' on a separate line
> 
> void DidWrite(
>     base::PlatformFileError result,
>     int64 bytes,
>     ...);

Originally on its own new line because the previous line was 81 chars long.
Added typedef to shorten the type so it's not as long and moved the ); back up
one line.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:181: return (writing_started_) ?
ERROR_WRITE_STARTED : ERROR_WRITE_NOT_STARTED;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: no parens needed around writing_started_

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.cc:204: WriteCompletionStatus status =
(done) ? COMPLETE_SUCCESSFUL : NOT_COMPLETE;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: no parens needed

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:28: enum WriteCompletionStatus {
On 2012/09/25 14:00:49, kinuko wrote:
> Since DidWrite could be called with a status repeatedly maybe we should call
> this enum WriteProgressStatus?

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:38: DelegateWriteCallback;
On 2012/09/25 14:00:49, kinuko wrote:
> nit: indentation looks a bit off
> 
> typedef base::Callback<void(
>     base...,
>     int64 bytes,
>     ...)> DelegateWriteCallback;
> 
> or
> 
> typedef base::Callback<
>     void(base...,
>          int64 bytes,
>          ...)> DelegateWriteCallback;
> 
> looks more common.

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate.h:82: WriteCompletionStatus
ErrorCompletionStatus();
On 2012/09/25 14:00:49, kinuko wrote:
> Please make this method const if it doesn't change its internal state.
> 
> Also how about renaming this to:
> GetCompletionStatusOnError() const

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate_unittest.cc:43: bool complete() const {
return complete_; }
On 2012/09/25 14:00:49, kinuko wrote:
> Can we change this to return WriteCompletionStatus so that we can check the
> completion status more specifically?

Done. Please note that a lot more lines in this file have been changed because
of the conversion of (bool complete_) to (WriteProgressStatus write_status_).

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:692: write_status ==
FileWriterDelegate::ERROR_WRITE_STARTED) {
On 2012/09/25 14:00:49, kinuko wrote:
> This might be only me but I found this code kinda makes me worry if we're
> covering all possible situations we should fire the notification.
> 
> How about moving line 697 before this if condition and change it like:
> 
> if (complete && write_status != ERROR_WRITE_NOT_STARTED)

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.cc:699: if (write_status ==
FileWriterDelegate::COMPLETE_SUCCESSFUL ||
On 2012/09/25 14:00:49, kinuko wrote:
> shouldn't this just use 'complete'?

Done.

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation.h (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation.h:24: #include
"webkit/quota/quota_manager.h"
On 2012/09/26 01:41:12, tzik wrote:
> maybe, quota_manager.h is not needed here.

Oops, good catch. Removing second include call. The header is still needed
though for DidGetUsageAndQuotaAndRunTask().

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/6001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_operation_write_unittest.cc:28: 
On 2012/09/25 14:00:49, kinuko wrote:
> nit: extra empty line

Done.

[kinuko, 2012-09-26 06:31:15]

lgtm

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate_unittest.cc:38: bool write_status() const {
return write_status_; }
should this return WriteProgressStatus?

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:691: bool complete = (write_status
!= FileWriterDelegate::SUCCESS_IO_PENDING);
nit: const

[calvinlo, 2012-09-26 07:43:40]

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate_unittest.cc:38: bool write_status() const {
return write_status_; }
On 2012/09/26 06:31:15, kinuko wrote:
> should this return WriteProgressStatus?

Done.

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10956064/diff/11002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:691: bool complete = (write_status
!= FileWriterDelegate::SUCCESS_IO_PENDING);
On 2012/09/26 06:31:15, kinuko wrote:
> nit: const

Done.

[commit-bot: I haz the power, 2012-09-26 07:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10956064/16002

[commit-bot: I haz the power, 2012-09-26 07:44:42]

Presubmit check for 10956064-16002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Add a description.

[commit-bot: I haz the power, 2012-09-26 08:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10956064/16002

[commit-bot: I haz the power, 2012-09-26 11:21:22]

Change committed as 158784