Refactor FileSystemOperation to take callback for each method.

Refactor FileSystemOperation to take callback for each method.

(Reland of r121620, which was reverted because it broke chromeos-clang compilation and LayoutTests/fast/filesystem/file-writer-*)

This patch is the first step for supporting cross-filesystem copy/move on
the Filesystem API implementation. To accomplish it, I'm planning to
crack FileSystemOperation::{Move,Copy} to a series of other FSO operations.
For it, per-method callback is more handy.

BUG=110121
TEST=*File*
TEST=LayoutTest fast/filesystem

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

[kinaba, 2012-02-10 03:09:57]

Kinuko-san, could you take time to review this? Thanks!

[kinuko, 2012-02-10 05:34:09]

Thanks for woking on this.  It's really great to see this is happening.

http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_file_browser_private_api.cc:615: static
fileapi::FileSystemContext::OpenFileSystemCallback CreateCallback(
Nice practical solution.

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
File content/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
content/browser/file_system/file_system_dispatcher_host.cc:375: // For Cancel
and OpenFileSystem we do not create an operation.
Now that we will not call this in DidOpenFileSystem can we assert Lookup always
returns true?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation.cc:519: LOG(ERROR) <<
"DelayedOpenFileforQuota";
This should be removed?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation.h:298: }
Can we add a brief comment for these callbacks?
(E.g. "write_callback_ is kept in this class so that we can dispatch it when the
operation is cancelled" etc)

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_interface.h (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_interface.h:158: //     // the callback
function attached to this operation.
"by invoking the callback function for the ongoing operation" ?

Can we also note that for cancellable operations the operation needs to keep the
callback around?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_interface.h:159: //    
operation_callback.Run(base::PLATFORM_FILE_ERROR_ABORT, ...);
Can you briefly note that what this operation_callback is supposed to be?

(e.g. maybe we could revise the above comment "for the ongoing operation,
operation_callback_ in this code example")

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:163: void set_status(int
status) { status_ = status; }
Now that all the callbacks became member methods probably we don't need these
setters?  (Here and in all other tests)

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:245:
FileSystemOperationInterface::StatusCallback RecordStatus() {
naming-nit: RecordStatusCallback() might be slightly clearer?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:247: base::Unretained(this));
This should be safe in theory but maybe we could use WeakPtr just to be extra
safe?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:268: if (status ==
base::PLATFORM_FILE_OK)
Not really sure if we need this if/else's here?

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:277: if (status ==
base::PLATFORM_FILE_OK) {
ditto.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_write_unittest.cc:103:
FileSystemOperationInterface::WriteCallback RecordWrite() {
ditto.  (I slightly prefer RecordwriteCallback())

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
webkit/fileapi/file_system_quota_unittest.cc:119:
FileSystemOperationInterface::StatusCallback RecordStatus() {
ditto.

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
webkit/tools/test_shell/simple_file_writer.cc:113: void DidWriteMain(int64
bytes, bool complete) {
If this is only called from DidWrite maybe we could just put the body of this
method in DidWrite?

[kinuko, 2012-02-10 05:34:42]

On 2012/02/10 05:34:09, kinuko wrote:
> Thanks for woking on this.  It's really great to see this is happening.

Forgot to add: Mostly looking good, minor comments only.

>
http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_file_browser_private_api.cc:615: static
> fileapi::FileSystemContext::OpenFileSystemCallback CreateCallback(
> Nice practical solution.
> 
>
http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
> File content/browser/file_system/file_system_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
> content/browser/file_system/file_system_dispatcher_host.cc:375: // For Cancel
> and OpenFileSystem we do not create an operation.
> Now that we will not call this in DidOpenFileSystem can we assert Lookup
always
> returns true?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> File webkit/fileapi/file_system_operation.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation.cc:519: LOG(ERROR) <<
> "DelayedOpenFileforQuota";
> This should be removed?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> File webkit/fileapi/file_system_operation.h (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation.h:298: }
> Can we add a brief comment for these callbacks?
> (E.g. "write_callback_ is kept in this class so that we can dispatch it when
the
> operation is cancelled" etc)
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> File webkit/fileapi/file_system_operation_interface.h (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_interface.h:158: //     // the callback
> function attached to this operation.
> "by invoking the callback function for the ongoing operation" ?
> 
> Can we also note that for cancellable operations the operation needs to keep
the
> callback around?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_interface.h:159: //    
> operation_callback.Run(base::PLATFORM_FILE_ERROR_ABORT, ...);
> Can you briefly note that what this operation_callback is supposed to be?
> 
> (e.g. maybe we could revise the above comment "for the ongoing operation,
> operation_callback_ in this code example")
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> File webkit/fileapi/file_system_operation_unittest.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_unittest.cc:163: void set_status(int
> status) { status_ = status; }
> Now that all the callbacks became member methods probably we don't need these
> setters?  (Here and in all other tests)
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_unittest.cc:245:
> FileSystemOperationInterface::StatusCallback RecordStatus() {
> naming-nit: RecordStatusCallback() might be slightly clearer?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_unittest.cc:247: base::Unretained(this));
> This should be safe in theory but maybe we could use WeakPtr just to be extra
> safe?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_unittest.cc:268: if (status ==
> base::PLATFORM_FILE_OK)
> Not really sure if we need this if/else's here?
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_unittest.cc:277: if (status ==
> base::PLATFORM_FILE_OK) {
> ditto.
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> File webkit/fileapi/file_system_operation_write_unittest.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
> webkit/fileapi/file_system_operation_write_unittest.cc:103:
> FileSystemOperationInterface::WriteCallback RecordWrite() {
> ditto.  (I slightly prefer RecordwriteCallback())
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
> File webkit/fileapi/file_system_quota_unittest.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
> webkit/fileapi/file_system_quota_unittest.cc:119:
> FileSystemOperationInterface::StatusCallback RecordStatus() {
> ditto.
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
> File webkit/tools/test_shell/simple_file_writer.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
> webkit/tools/test_shell/simple_file_writer.cc:113: void DidWriteMain(int64
> bytes, bool complete) {
> If this is only called from DidWrite maybe we could just put the body of this
> method in DidWrite?

[kinaba, 2012-02-10 08:22:58]

Thanks for the feedbacks. Updated according to the comments.

http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/9372044/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_file_browser_private_api.cc:615: static
fileapi::FileSystemContext::OpenFileSystemCallback CreateCallback(
On 2012/02/10 05:34:09, kinuko wrote:
> Nice practical solution.
:)

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
File content/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
content/browser/file_system/file_system_dispatcher_host.cc:375: // For Cancel
and OpenFileSystem we do not create an operation.
On 2012/02/10 05:34:09, kinuko wrote:
> Now that we will not call this in DidOpenFileSystem can we assert Lookup
always
> returns true?

Callback from Cancel still called this, so it might have been a case Lookup
returned false.
I separated DidCancel from DidFinish and dropped the Lookup here to DCHECK.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation.cc:519: LOG(ERROR) <<
"DelayedOpenFileforQuota";
On 2012/02/10 05:34:09, kinuko wrote:
> This should be removed?

Oops!!111 Right.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation.h:298: }
On 2012/02/10 05:34:09, kinuko wrote:
> Can we add a brief comment for these callbacks?
> (E.g. "write_callback_ is kept in this class so that we can dispatch it when
the
> operation is cancelled" etc)

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_interface.h (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_interface.h:158: //     // the callback
function attached to this operation.
On 2012/02/10 05:34:09, kinuko wrote:
> "by invoking the callback function for the ongoing operation" ?
> 
> Can we also note that for cancellable operations the operation needs to keep
the
> callback around?

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_interface.h:159: //    
operation_callback.Run(base::PLATFORM_FILE_ERROR_ABORT, ...);
On 2012/02/10 05:34:09, kinuko wrote:
> Can you briefly note that what this operation_callback is supposed to be?
> 
> (e.g. maybe we could revise the above comment "for the ongoing operation,
> operation_callback_ in this code example")

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:163: void set_status(int
status) { status_ = status; }
On 2012/02/10 05:34:09, kinuko wrote:
> Now that all the callbacks became member methods probably we don't need these
> setters?  (Here and in all other tests)

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:245:
FileSystemOperationInterface::StatusCallback RecordStatus() {
On 2012/02/10 05:34:09, kinuko wrote:
> naming-nit: RecordStatusCallback() might be slightly clearer?

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:247: base::Unretained(this));
On 2012/02/10 05:34:09, kinuko wrote:
> This should be safe in theory but maybe we could use WeakPtr just to be extra
> safe?

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:268: if (status ==
base::PLATFORM_FILE_OK)
On 2012/02/10 05:34:09, kinuko wrote:
> Not really sure if we need this if/else's here?

Done.
I did in this way because TestReadDirSuccess had explicitly checked status is
not set when successful, but, yeah, removing this branch seems much cleaner.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_unittest.cc:277: if (status ==
base::PLATFORM_FILE_OK) {
On 2012/02/10 05:34:09, kinuko wrote:
> ditto.

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_oper...
webkit/fileapi/file_system_operation_write_unittest.cc:103:
FileSystemOperationInterface::WriteCallback RecordWrite() {
On 2012/02/10 05:34:09, kinuko wrote:
> ditto.  (I slightly prefer RecordwriteCallback())

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/fileapi/file_system_quot...
webkit/fileapi/file_system_quota_unittest.cc:119:
FileSystemOperationInterface::StatusCallback RecordStatus() {
On 2012/02/10 05:34:09, kinuko wrote:
> ditto.

Done.

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
webkit/tools/test_shell/simple_file_writer.cc:113: void DidWriteMain(int64
bytes, bool complete) {
On 2012/02/10 05:34:09, kinuko wrote:
> If this is only called from DidWrite maybe we could just put the body of this
> method in DidWrite?

I'd rather keep it in the current form. DidWrite (more specifically,
ClearOperation) is supposed to be called from IO thread, while DidWriteMain need
to forward itself to the main thread. Mixing them up into one method may make
the code hard to understand the thread interaction.

To make the intent (separating methods at the thread boundary), I renamed
DidSucceed, DidFail, DidWriteMain. Hope it improves clarity.

[kinuko, 2012-02-10 08:49:14]

LGTM mod nits

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
File content/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/9372044/diff/1/content/browser/file_system/fil...
content/browser/file_system/file_system_dispatcher_host.cc:375: // For Cancel
and OpenFileSystem we do not create an operation.
On 2012/02/10 08:22:58, kinaba wrote:
> On 2012/02/10 05:34:09, kinuko wrote:
> > Now that we will not call this in DidOpenFileSystem can we assert Lookup
> always
> > returns true?
> 
> Callback from Cancel still called this, so it might have been a case Lookup
> returned false.
> I separated DidCancel from DidFinish and dropped the Lookup here to DCHECK.

Thanks!

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
webkit/tools/test_shell/simple_file_writer.cc:113: void DidWriteMain(int64
bytes, bool complete) {
On 2012/02/10 08:22:58, kinaba wrote:
> On 2012/02/10 05:34:09, kinuko wrote:
> > If this is only called from DidWrite maybe we could just put the body of
this
> > method in DidWrite?
> 
> I'd rather keep it in the current form. DidWrite (more specifically,
> ClearOperation) is supposed to be called from IO thread, while DidWriteMain
need
> to forward itself to the main thread. Mixing them up into one method may make
> the code hard to understand the thread interaction.
> 
> To make the intent (separating methods at the thread boundary), I renamed
> DidSucceed, DidFail, DidWriteMain. Hope it improves clarity.

Ah ok.  Could we rename DidWriteMain to DidWriteOnMainThread or something then
(per convention)?  I wasn't very sure what this 'Main' meant at first.

[kinaba, 2012-02-10 09:15:28]

Good. After checking the trybot results I'll put into CQ.

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
File webkit/tools/test_shell/simple_file_writer.cc (right):

http://codereview.chromium.org/9372044/diff/1/webkit/tools/test_shell/simple_...
webkit/tools/test_shell/simple_file_writer.cc:113: void DidWriteMain(int64
bytes, bool complete) {
On 2012/02/10 08:49:14, kinuko wrote:
> On 2012/02/10 08:22:58, kinaba wrote:
> > On 2012/02/10 05:34:09, kinuko wrote:
> > > If this is only called from DidWrite maybe we could just put the body of
> this
> > > method in DidWrite?
> > 
> > I'd rather keep it in the current form. DidWrite (more specifically,
> > ClearOperation) is supposed to be called from IO thread, while DidWriteMain
> need
> > to forward itself to the main thread. Mixing them up into one method may
make
> > the code hard to understand the thread interaction.
> > 
> > To make the intent (separating methods at the thread boundary), I renamed
> > DidSucceed, DidFail, DidWriteMain. Hope it improves clarity.
> 
> Ah ok.  Could we rename DidWriteMain to DidWriteOnMainThread or something then
> (per convention)?  I wasn't very sure what this 'Main' meant at first.

Roger.

[commit-bot: I haz the power, 2012-02-10 14:48:23]

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

[commit-bot: I haz the power, 2012-02-10 14:48:37]

Presubmit check for 9372044-12002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/tools/test_shell/simple_file_system.cc,webkit/tools/test_shell/simple_file_system.h,webkit/tools/test_shell/simple_file_writer.cc

Presubmit checks took 2.4s to calculate.

[kinuko, 2012-02-10 14:50:20]

Tony, can you take a look at (or rubber-stamp) webkit/tools changes?  Thanks!

[tony, 2012-02-10 18:40:34]

webkit/tools LGTM

[commit-bot: I haz the power, 2012-02-11 00:22:30]

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

[commit-bot: I haz the power, 2012-02-11 02:18:43]

Change committed as 121620

[kinaba, 2012-02-11 02:51:21]

On 2012/02/11 02:18:43, I haz the power (commit-bot) wrote:
> Change committed as 121620

and broke chromeos-clang build ;(. I'll prepare a fix.

[tony, 2012-02-11 03:21:18]

It looks like this regressed some layout tests.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

Can you investigate?

[kinaba, 2012-02-11 03:30:02]

On 2012/02/11 03:21:18, tony wrote:
> It looks like this regressed some layout tests.
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...
> 
> Can you investigate?

Ouch, something seems to be wrong with Write().
Thank your for pointing it out. I'll have a deeper look on Monday.

[kinaba, 2012-02-13 08:06:57]

Hello, I found the cause and fixed it.
Kinuko and Tony, sorry to bother you but could you re-check this change?

It includes three changes from the previous try.
[1] Fix compile error on clang. Removed incorrect OVERRIDEs (patch 4 -> 5).
[2] Fix layout test failure. Delayed the timing of "delete this" on Cancel for
Write (patch 5 -> 6), which was indeed the original behavior before refactoring.
[3] Added a unittest for detecting the difference due to the change [2] (crash
vs test pass).

[kinaba, 2012-02-13 08:33:53]

Oops, the new test is not working properly...
Give me a moment, I'll check it.

On 2012/02/13 08:06:57, kinaba wrote:
> Hello, I found the cause and fixed it.
> Kinuko and Tony, sorry to bother you but could you re-check this change?
> 
> It includes three changes from the previous try.
> [1] Fix compile error on clang. Removed incorrect OVERRIDEs (patch 4 -> 5).
> [2] Fix layout test failure. Delayed the timing of "delete this" on Cancel for
> Write (patch 5 -> 6), which was indeed the original behavior before
refactoring.
> [3] Added a unittest for detecting the difference due to the change [2] (crash
> vs test pass).

[kinaba, 2012-02-13 10:06:26]

Now the patchset 7 looks good (all bots passed unit_tests).
Thanks once again, could you review the patch once more?

[kinuko, 2012-02-13 10:10:21]

LGTM

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:113: if
(MessageLoop::current()->is_running())
This is ok but I guess it will never be false?

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:348:
MessageLoop::current()->RunAllPending();
Could we add a comment why we call RunAllPending() here instead of Run()?  (E.g.
We won't dispatch callbacks after Cancel() is issued (so no chance to Quit) nor
do we need to another run write cycle)

[tony, 2012-02-13 18:19:05]

LGTM

[kinaba, 2012-02-14 02:17:46]

+Zel

Kinuko-san, I found that I am soooo stupid that I've still missed a crashing
case for Cancel. Could you give a quick look on the diff from patchset 7 to 8? I
believe this is the final call for reviewing...

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:113: if
(MessageLoop::current()->is_running())
On 2012/02/13 10:10:21, kinuko wrote:
> This is ok but I guess it will never be false?

Done.

http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:348:
MessageLoop::current()->RunAllPending();
On 2012/02/13 10:10:21, kinuko wrote:
> Could we add a comment why we call RunAllPending() here instead of Run()? 
(E.g.
> We won't dispatch callbacks after Cancel() is issued (so no chance to Quit)
nor
> do we need to another run write cycle)

Done.

[kinuko, 2012-02-14 06:00:03]

still lgtm!

On 2012/02/14 02:17:46, kinaba wrote:
> +Zel
> 
> Kinuko-san, I found that I am soooo stupid that I've still missed a crashing
> case for Cancel. Could you give a quick look on the diff from patchset 7 to 8?
I
> believe this is the final call for reviewing...
> 
>
http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
> File webkit/fileapi/file_system_operation_write_unittest.cc (right):
> 
>
http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_operation_write_unittest.cc:113: if
> (MessageLoop::current()->is_running())
> On 2012/02/13 10:10:21, kinuko wrote:
> > This is ok but I guess it will never be false?
> 
> Done.
> 
>
http://codereview.chromium.org/9372044/diff/19006/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_operation_write_unittest.cc:348:
> MessageLoop::current()->RunAllPending();
> On 2012/02/13 10:10:21, kinuko wrote:
> > Could we add a comment why we call RunAllPending() here instead of Run()? 
> (E.g.
> > We won't dispatch callbacks after Cancel() is issued (so no chance to Quit)
> nor
> > do we need to another run write cycle)
> 
> Done.

[commit-bot: I haz the power, 2012-02-14 06:02:04]

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

[commit-bot: I haz the power, 2012-02-14 08:40:39]

Change committed as 121857