Make FileSystemOperation's lifetime more explicit.

Make FileSystemOperation's lifetime more explicit.

In the current code calling dispatcher->DidXxx in an operation's DidXxx method MAY indirectly delete the operation itself depending on the dispatcher's implementation.  I was confused by this several times and I want to make this flow more explicit.

This patch lets FileSystemOperation control its lifetime by itself so that each callback dispatcher implementation does not need to take care of it.

Also moved BrowserFileSystemCallbackDispatcher into file_system_dispatcher_host.cc as it's only used in it and its implementation is tightly coupled with the DispatcherHost.

BUG=60243
TEST=none

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

[ericu, 2010-11-16 19:26:54]

I have some small comments below, but overall I'm iffy on this changelist.  I'm
not really seeing that it makes things clearer.  It makes some parts simpler,
but seems to make others more complex.  Dumi, Michael: what do you think?

http://codereview.chromium.org/4821005/diff/10001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/4821005/diff/10001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:79:
ALLOW_THIS_IN_INITIALIZER_LIST(this), dispatcher)),
This seems a bit odd to me.  We're creating a wrapper object that will most
likely be used only once, and then if we're actually going to reuse it [call for
a nondestructive one], we need to reach down inside it and return a private
member.  Why not just store the underlying FSCD and create a destructive
dispatcher only when asked for one?  That way the nondestructive accessor would
be simpler.

http://codereview.chromium.org/4821005/diff/10001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:369:
destructive_dispatcher()->DidWrite(bytes, complete);
Given that we explicitly don't want to delete the operation unless complete, it
seems really unintuitive to ask for the destructive_dispatcher, and then not
have it destroy anything.  Why not just use the nondistructive dispatcher when
you don't want to destroy the FSO, and make the destructive dispatcher always
destroy?

http://codereview.chromium.org/4821005/diff/10001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_system.cc (right):

http://codereview.chromium.org/4821005/diff/10001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:38: callbacks_->didSucceed();
It looks to me like these test operations now never get unregistered...is that
right?

[kinuko, 2010-11-16 20:29:08]

Removed the destructive_dispatcher stuff (it's a last min hacky device).
How does it look now?

On 2010/11/16 19:26:54, ericu wrote:
> I have some small comments below, but overall I'm iffy on this changelist. 
I'm
> not really seeing that it makes things clearer.  It makes some parts simpler,
> but seems to make others more complex.  Dumi, Michael: what do you think?

Let me explain a bit more.
The current code works, and may look simpler in some way than this one, but I
think it also has some code maintenance problem.
In the current code ownership is formed in a cyclic way that goes across
boundaries between webkit/fileapi and chrome.  (dispatcher_host owns operations,
operation owns dispatcher implemented by chrome, and dispatcher owns
dispatcher_host.)
Application logic/callbacks for each operation needs to be implemented by a
dispatcher but the dispatcher also needs to take care of the operation's
lifetime, which happens to be an owner of the dispatcher itself.  In the new
code dispatcher can only focus on its application logic.
Operation cannot control its lifetime, and whenever someone rewrites operation
each dispatcher implementation may need to be updated too to keep the lifetime
control correct.

http://codereview.chromium.org/4821005/diff/10001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_system.cc (right):

http://codereview.chromium.org/4821005/diff/10001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:38: callbacks_->didSucceed();
On 2010/11/16 19:26:54, ericu wrote:
> It looks to me like these test operations now never get unregistered...is that
> right?

They're not registered now.  There's no need to track them in test filesystem.

[michaeln, 2010-11-18 01:49:05]

This CL is definitely more involved than the other one.

http://codereview.chromium.org/4821005/diff/19001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_file_system.cc (left):

http://codereview.chromium.org/4821005/diff/19001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:87: SimpleFileSystem*
file_system_;
Would a WeakPtr make sense here now or maybe you don't need this data member any
more?

http://codereview.chromium.org/4821005/diff/19001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_file_system.cc:98:
operations_.Remove(iter.GetCurrentKey());
In the chrome case, FileSystemDispatcherHost is refcounted and pending
operations hold a reference to it (by way of the CallbackDispatcher for the
operation), so that upon operation completion all is still there.

How does that work in the SimpleFileSystem case with delete semantics? What
happens to pending operations when the SimpleFileSystem instance has been
deleted? Previously, the WebFileSystemCallbacks would not get called (i think),
but now it looks like they will be.

[ericu, 2010-11-23 03:18:20]

LGTM

http://codereview.chromium.org/4821005/diff/33001/webkit/fileapi/sandboxed_fi...
File webkit/fileapi/sandboxed_file_system_operation.cc (right):

http://codereview.chromium.org/4821005/diff/33001/webkit/fileapi/sandboxed_fi...
webkit/fileapi/sandboxed_file_system_operation.cc:162: if
(!file_system_context_->path_manager()->CrackFileSystemPath(
In general, any dispatcher()->didFoo call is followed by a "delete this;".  It's
not for these last two utility functions; that's worth a comment.