Cleanup: Deprecate FileSystemCallbackDispatcher

Cleanup: Deprecate FileSystemCallbackDispatcher

Replace all FileSystemCallbackDispatcher* with our usual
base::Callback.

BUG=157537
TEST=existing tests
R=michaeln@chromium.org, tzik@chromium.org, yzshen@chromium.org

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

[kinuko, 2013-05-14 08:46:30]

Our long-due cleanup.

Taiju and/or Michael: can you take a look at changes in:
webkit/fileapi/*
content/common/fileapi/*

Yuzhu: can you or someone in your team take a look at:
content/renderer/pepper/*
webkit/plugins/ppapi/*

Thanks!

[tzik, 2013-05-14 12:22:29]

lgtm

[yzshen1, 2013-05-14 16:50:09]

LGTM

I am glad to see this CL. Thanks!

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_system_host.h:40: void
OpenFileSystemReply(base::PlatformFileError platform_error,
nit: It could be moved to the private section.

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:92: const
std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
nit: put each parameter on a separate line, please.

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:40: class FileCallbacks {
It seems good to change ppb_file_ref_impl and get rid of this class entirely.

But I am fine if you would like to add a TODO and make it a separate CL later.

[michaeln, 2013-05-14 22:18:58]

lgtm, but i'm not really sure we win in the trade?

https://chromiumcodereview.appspot.com/14796018/diff/54001/content/common/fil...
File content/common/fileapi/file_system_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/14796018/diff/54001/content/common/fil...
content/common/fileapi/file_system_dispatcher.cc:50:
dispatcher->onerror_callback_ =
I see, here's the code path by which error value seen on DidSomething may be
!OK. A late bound closure.

https://chromiumcodereview.appspot.com/14796018/diff/54001/content/common/fil...
content/common/fileapi/file_system_dispatcher.cc:411:
dispatcher->DidReadMetadata(file_info, platform_path, base::PLATFORM_FILE_OK);
Are there any code paths where the error parameter is not OK for this
DidDoSomething call (and the others)? It looks like errors come back from the
browser process in the form of OnDidFail ipcs.

https://chromiumcodereview.appspot.com/14796018/diff/54001/webkit/fileapi/fil...
File webkit/fileapi/file_system_callback_dispatcher.h (left):

https://chromiumcodereview.appspot.com/14796018/diff/54001/webkit/fileapi/fil...
webkit/fileapi/file_system_callback_dispatcher.h:23: // but uses chromium types.
The code looks correct and all but I don't really see the need for the change.

I think i'm less enthusiastic about this change because i think it loses some
clarity. The old way, the relationship between this chromium interface, the set
of ipc messages, and the WebKit interface, and the internal WebCore interface,
and the IDL defined scriptable interface... are pretty clear given how similar
those all are. Now there's a layer of 'closure' indirection that obscures that
self similarity. The previous comments demonstrate that sort of loss of clarity.

[kinuko, 2013-05-15 13:58:03]

Update the patch further. Michael/Yuzhu, can you take one more look?

1. Got rid of webkit/plugins/ppapi/file_callbacks.h
2. Got rid of lazy-bound error closures so that one Blink/IDL callback is mapped
to one base::Callback

Thanks,

https://codereview.chromium.org/14796018/diff/54001/content/common/fileapi/fi...
File content/common/fileapi/file_system_dispatcher.cc (right):

https://codereview.chromium.org/14796018/diff/54001/content/common/fileapi/fi...
content/common/fileapi/file_system_dispatcher.cc:50:
dispatcher->onerror_callback_ =
On 2013/05/14 22:18:58, michaeln wrote:
> I see, here's the code path by which error value seen on DidSomething may be
> !OK. A late bound closure.

I removed these lazy bound closures as this may confuse readers.

https://codereview.chromium.org/14796018/diff/54001/content/common/fileapi/fi...
content/common/fileapi/file_system_dispatcher.cc:411:
dispatcher->DidReadMetadata(file_info, platform_path, base::PLATFORM_FILE_OK);
On 2013/05/14 22:18:58, michaeln wrote:
> Are there any code paths where the error parameter is not OK for this
> DidDoSomething call (and the others)? It looks like errors come back from the
> browser process in the form of OnDidFail ipcs.

If so it must be a coding error, but anyway I dropped these weird error
handlings but instead changed some methods. Hope it clarifies the code.

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_system_host.h:40: void
OpenFileSystemReply(base::PlatformFileError platform_error,
On 2013/05/14 16:50:09, yzshen1 wrote:
> nit: It could be moved to the private section.

Hmm, looks like this needs to be public as it's called from outside of this
class in the end.

https://codereview.chromium.org/14796018/diff/54001/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_callback_dispatcher.h (left):

https://codereview.chromium.org/14796018/diff/54001/webkit/fileapi/file_syste...
webkit/fileapi/file_system_callback_dispatcher.h:23: // but uses chromium types.
On 2013/05/14 22:18:58, michaeln wrote:
> The code looks correct and all but I don't really see the need for the change.
> 
> I think i'm less enthusiastic about this change because i think it loses some
> clarity. The old way, the relationship between this chromium interface, the
set
> of ipc messages, and the WebKit interface, and the internal WebCore interface,
> and the IDL defined scriptable interface... are pretty clear given how similar
> those all are. Now there's a layer of 'closure' indirection that obscures that
> self similarity. The previous comments demonstrate that sort of loss of
clarity.
> 

In the new patch one callback in Blink/WebKit is always mapped to one
base::Callback, so it should be adding no extra layers.

(If we keep this class anyone who wants to send FileSystem IPC needs to
implement this... which feels unfortunate to me and I felt this patch is 'win'
at least in pepper code)

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.cc (right):

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.cc:92: const
std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
On 2013/05/14 16:50:09, yzshen1 wrote:
> nit: put each parameter on a separate line, please.

Done (in ppb_file_ref_impl.cc)

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
File webkit/plugins/ppapi/file_callbacks.h (right):

https://codereview.chromium.org/14796018/diff/54001/webkit/plugins/ppapi/file...
webkit/plugins/ppapi/file_callbacks.h:40: class FileCallbacks {
On 2013/05/14 16:50:09, yzshen1 wrote:
> It seems good to change ppb_file_ref_impl and get rid of this class entirely.
> 
> But I am fine if you would like to add a TODO and make it a separate CL later.

Done.

I was going to cut a separate CL, but have integrated it into this one :)

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:152: return;
I think this needs return here, but let me know if this's wrong...

[yzshen1, 2013-05-15 18:13:54]

Only a few nits. Thanks!

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_system_host.h (right):

https://codereview.chromium.org/14796018/diff/54001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_system_host.h:40: void
OpenFileSystemReply(base::PlatformFileError platform_error,
I think they could be private since they are base::Bind()-ed in this class.
Right?

On 2013/05/15 13:58:03, kinuko wrote:
> On 2013/05/14 16:50:09, yzshen1 wrote:
> > nit: It could be moved to the private section.
> 
> Hmm, looks like this needs to be public as it's called from outside of this
> class in the end.

https://codereview.chromium.org/14796018/diff/67001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1027: const
MetadataCallback& success_callback,
IMO, it is easier to have only one callback instead of two. What do you think?

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:152: return;
It makes sense to me. Thanks!
On 2013/05/15 13:58:03, kinuko wrote:
> I think this needs return here, but let me know if this's wrong...

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:172: if (callback->completed())
nit: it is more common to use IsPending since it also checks null.

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:521: callback, base::Unretained(this),
files, file_types),
Why using unretained here? I think it may die before the callback is run, right?

[michaeln, 2013-05-15 19:54:06]

lgtm

> > I see, here's the code path by which error value seen on DidSomething may be
> > !OK. A late bound closure.
> 
> I removed these lazy bound closures as this may confuse readers.
> 
>
https://codereview.chromium.org/14796018/diff/54001/content/common/fileapi/fi...
> content/common/fileapi/file_system_dispatcher.cc:411:
> dispatcher->DidReadMetadata(file_info, platform_path, base::PLATFORM_FILE_OK);
> On 2013/05/14 22:18:58, michaeln wrote:
> > Are there any code paths where the error parameter is not OK for this
> > DidDoSomething call (and the others)? It looks like errors come back from
the
> > browser process in the form of OnDidFail ipcs.
> 
> If so it must be a coding error, but anyway I dropped these weird error
> handlings but instead changed some methods. Hope it clarifies the code.

Yes, I think it does, the new patch is an improvement from my pov.

> content/renderer/pepper/pepper_plugin_delegate_impl.cc:1027: const
> MetadataCallback& success_callback,
> IMO, it is easier to have only one callback instead of two. What do you think?

The thing is that there are multiple reflections of this same/similar api from
end-to-end: IDL for script, WebKitAPI, IPC message set, FileAPIMessageFilter,
FileSystemDispatcher interface... and a good amount of code stitch those layers
together. The less conditional branching that happens at those stitch points,
the better from my pov.

It might be nicer to have a single callback/completion routine that can express
ERROR and good RESULT, but only if that flavor of completion was used from
end-to-end. I think having small impedance mismatches in the middle makes for
less maintainable code. I might be valuing that 'self-similarity' from
end-to-end too much... but that's my pov about why I think the new snapshot is
an improvement over the first patch.

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
File content/common/fileapi/webfilewriter_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
content/common/fileapi/webfilewriter_impl.cc:42:
base::Bind(&WebFileWriterImpl::DidFinish, AsWeakPtr()));
since this is a subclass, shouldn't need to be public for this guy

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
File webkit/fileapi/webfilewriter_base.h (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
webkit/fileapi/webfilewriter_base.h:33: void DidWrite(int64 bytes, bool
complete);
do these need to be public now?

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:521: callback, base::Unretained(this),
files, file_types),
On 2013/05/15 18:13:54, yzshen1 wrote:
> Why using unretained here? I think it may die before the callback is run,
right?

I don't see where the old FileCallbacks did anytihng other than an 'unretained'
storage of this pointer value either. Was that a bug or valid code? Either way,
I don't see how this patch introduces a new bug here (could be wrong?).

[michaeln, 2013-05-15 20:18:26]

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
File content/common/fileapi/webfilewriter_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
content/common/fileapi/webfilewriter_impl.cc:42:
base::Bind(&WebFileWriterImpl::DidFinish, AsWeakPtr()));
since this is a subclass, shouldn't need to be public for this guy

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
File webkit/fileapi/webfilewriter_base.h (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
webkit/fileapi/webfilewriter_base.h:33: void DidWrite(int64 bytes, bool
complete);
do these need to be public now?

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:521: callback, base::Unretained(this),
files, file_types),
On 2013/05/15 18:13:54, yzshen1 wrote:
> Why using unretained here? I think it may die before the callback is run,
right?

I don't see where the old FileCallbacks did anytihng other than an 'unretained'
storage of this pointer value either. Was that a bug or valid code?

[yzshen1, 2013-05-15 20:26:49]

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:521: callback, base::Unretained(this),
files, file_types),
I think it is a bug.
But I agree that this is not a new bug introduced by this CL.

On 2013/05/15 20:18:26, michaeln wrote:
> On 2013/05/15 18:13:54, yzshen1 wrote:
> > Why using unretained here? I think it may die before the callback is run,
> right?
> 
> I don't see where the old FileCallbacks did anytihng other than an
'unretained'
> storage of this pointer value either. Was that a bug or valid code?
>

[kinuko, 2013-05-16 09:29:38]

Thanks, fixed public/private attribute of some callback methods. Will be
submitting...

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
File content/common/fileapi/webfilewriter_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/content/common/fileapi/we...
content/common/fileapi/webfilewriter_impl.cc:42:
base::Bind(&WebFileWriterImpl::DidFinish, AsWeakPtr()));
On 2013/05/15 20:18:26, michaeln wrote:
> since this is a subclass, shouldn't need to be public for this guy

Done.

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
File webkit/fileapi/webfilewriter_base.h (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/fileapi/webfilewri...
webkit/fileapi/webfilewriter_base.h:33: void DidWrite(int64 bytes, bool
complete);
On 2013/05/15 20:18:26, michaeln wrote:
> do these need to be public now?

Made them protected.

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:172: if (callback->completed())
On 2013/05/15 18:13:54, yzshen1 wrote:
> nit: it is more common to use IsPending since it also checks null.

Done.

https://codereview.chromium.org/14796018/diff/67001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:521: callback, base::Unretained(this),
files, file_types),
On 2013/05/15 20:26:49, yzshen1 wrote:
> I think it is a bug.
> But I agree that this is not a new bug introduced by this CL.
> 
> On 2013/05/15 20:18:26, michaeln wrote:
> > On 2013/05/15 18:13:54, yzshen1 wrote:
> > > Why using unretained here? I think it may die before the callback is run,
> > right?
> > 
> > I don't see where the old FileCallbacks did anytihng other than an
> 'unretained'
> > storage of this pointer value either. Was that a bug or valid code?
> > 
> 

Could TrackedCallback::IsPending(callback) catch if 'this' go away earlier? 
Anyway I'm submitting this but added a TODO comment.

[kinuko, 2013-05-16 14:19:35]

Committed patchset #3 manually as r200531.