Bind: Merge FileUtilProxy and FileSystemFileUtilProxy: CreateOrOpen/Close

Merge FileUtilProxy and FileSystemFileUtilProxy using PostTaskAndReply: CreateOrOpen/Close

Deprecating MessageProxyRelay class and getting rid of duplicated code.

BUG=none
TEST=test_shell_tests:\*FileSystem\*

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

[kinuko, 2011-10-31 06:54:49]

Hi, this is a refactoring attempt trying to: 1) replace MessageLoopRelay (which
is potentially leaky in the current code) in favor of PostTaskAndReply and 2)
drop duplicated code in FileUtilProxy and FileSystemFileUtilProxy.  Could you
PTAL?  Thanks,

[tzik, 2011-11-01 04:47:46]

http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:115: FileUtilProxy::StatusCallback());
close_task_?

http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.cc:79:
base::Unretained(&operation_context_)),
base::Owned(new FileSystemOperation(operation_context_)) ?

[willchan no longer on Chromium, 2011-11-01 04:56:24]

Kinuko, sorry for the delay on these CLs. I'm busy moving into a new
apartment. I'll hopefully get around to this tomorrow PST if no one gets to
it first. My apologies.

On Mon, Oct 31, 2011 at 9:47 PM, <tzik@chromium.org> wrote:

>
> http://codereview.chromium.**org/8424006/diff/1006/base/**
>
file_util_proxy.cc<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc>
> File base/file_util_proxy.cc (right):
>
> http://codereview.chromium.**org/8424006/diff/1006/base/**
>
file_util_proxy.cc#newcode115<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc#newcode115>
> base/file_util_proxy.cc:115: FileUtilProxy::StatusCallback(**));
> close_task_?
>
> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
>
fileapi/file_system_operation.**cc<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc>
> File webkit/fileapi/file_system_**operation.cc (right):
>
> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
>
fileapi/file_system_operation.**cc#newcode79<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc#newcode79>
> webkit/fileapi/file_system_**operation.cc:79:
> base::Unretained(&operation_**context_)),
> base::Owned(new FileSystemOperation(operation_**context_)) ?
>
>
http://codereview.chromium.**org/8424006/<http://codereview.chromium.org/8424...
>

[willchan no longer on Chromium, 2011-11-01 04:57:04]

And I would rubberstamp an approval given by someone like ajwong or akalin
if you need to get this in sooner.

On Mon, Oct 31, 2011 at 9:56 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Kinuko, sorry for the delay on these CLs. I'm busy moving into a new
> apartment. I'll hopefully get around to this tomorrow PST if no one gets to
> it first. My apologies.
>
>
> On Mon, Oct 31, 2011 at 9:47 PM, <tzik@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/8424006/diff/1006/base/**
>>
file_util_proxy.cc<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc>
>> File base/file_util_proxy.cc (right):
>>
>> http://codereview.chromium.**org/8424006/diff/1006/base/**
>>
file_util_proxy.cc#newcode115<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc#newcode115>
>> base/file_util_proxy.cc:115: FileUtilProxy::StatusCallback(**));
>> close_task_?
>>
>> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
>>
fileapi/file_system_operation.**cc<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc>
>> File webkit/fileapi/file_system_**operation.cc (right):
>>
>> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
>>
fileapi/file_system_operation.**cc#newcode79<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc#newcode79>
>> webkit/fileapi/file_system_**operation.cc:79:
>> base::Unretained(&operation_**context_)),
>> base::Owned(new FileSystemOperation(operation_**context_)) ?
>>
>>
http://codereview.chromium.**org/8424006/<http://codereview.chromium.org/8424...
>>
>
>

[kinuko, 2011-11-01 06:39:00]

Thanks, all done.

http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:115: FileUtilProxy::StatusCallback());
On 2011/11/01 04:47:46, tzik wrote:
> close_task_?

Done.

http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.cc:79:
base::Unretained(&operation_context_)),
On 2011/11/01 04:47:46, tzik wrote:
> base::Owned(new FileSystemOperation(operation_context_)) ?

Done.

http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.cc:377: weak_factory_.GetWeakPtr()));
Most of them can also be replaced with Owned() and we can probably get rid of
delete this (except for the one in DidWrite, which is called differently).  I'll
do this in a separate cl...

[kinuko, 2011-11-01 07:27:51]

Willchan, thanks we're in no hurry.  Btw if the change in base/ looks
unnecessarily complicated I can simply get rid of the integration part in base/
and can make this change fileapi local.

On 2011/11/01 04:57:04, willchan wrote:
> And I would rubberstamp an approval given by someone like ajwong or akalin
> if you need to get this in sooner.
> 
> On Mon, Oct 31, 2011 at 9:56 PM, William Chan (陈智昌)
> <willchan@chromium.org>wrote:
> 
> > Kinuko, sorry for the delay on these CLs. I'm busy moving into a new
> > apartment. I'll hopefully get around to this tomorrow PST if no one gets to
> > it first. My apologies.
> >
> >
> > On Mon, Oct 31, 2011 at 9:47 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tzik@chromium.org> wrote:
> >
> >>
> >> http://codereview.chromium.**org/8424006/diff/1006/base/**
> >>
>
file_util_proxy.cc<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc>
> >> File base/file_util_proxy.cc (right):
> >>
> >> http://codereview.chromium.**org/8424006/diff/1006/base/**
> >>
>
file_util_proxy.cc#newcode115<http://codereview.chromium.org/8424006/diff/1006/base/file_util_proxy.cc#newcode115>
> >> base/file_util_proxy.cc:115: FileUtilProxy::StatusCallback(**));
> >> close_task_?
> >>
> >> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
> >>
>
fileapi/file_system_operation.**cc<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc>
> >> File webkit/fileapi/file_system_**operation.cc (right):
> >>
> >> http://codereview.chromium.**org/8424006/diff/1006/webkit/**
> >>
>
fileapi/file_system_operation.**cc#newcode79<http://codereview.chromium.org/8424006/diff/1006/webkit/fileapi/file_system_operation.cc#newcode79>
> >> webkit/fileapi/file_system_**operation.cc:79:
> >> base::Unretained(&operation_**context_)),
> >> base::Owned(new FileSystemOperation(operation_**context_)) ?
> >>
> >>
>
http://codereview.chromium.**org/8424006/%3Chttp://codereview.chromium.org/84...>
> >>
> >
> >

[willchan no longer on Chromium, 2011-11-01 22:57:37]

LGTM

http://codereview.chromium.org/8424006/diff/8001/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8424006/diff/8001/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:116: FileUtilProxy::StatusCallback());
Why do we use FileUtilProxy here? Shouldn't we just post the task to the
message_loop_proxy? There's no need for a reply, right?

The reason to do this is if we want to make sure that close_task_ gets destroyed
on the origin thread. I'm not sure if that's required. Maybe it's the safe thing
to do, so I guess I'm in favor of FileUtilProxy in this case, although it may be
useful to document why we use it internally here.

http://codereview.chromium.org/8424006/diff/8001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/8424006/diff/8001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.cc:83:
FileSystemFileUtilProxy::StatusCallback());
What's the long-term vision? Is FileSystemFileUtilProxy going away? If so, maybe
you should use base::FileUtilProxy::StatusCallback instead. Up to you.

[kinuko, 2011-11-02 05:48:22]

Thanks! Will be submitting if I get no further comments.  (I'll clean up / take
care of the webkit/fileapi memory related issues in the next patch)

http://codereview.chromium.org/8424006/diff/8001/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8424006/diff/8001/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:116: FileUtilProxy::StatusCallback());
On 2011/11/01 22:57:37, willchan wrote:
> Why do we use FileUtilProxy here? Shouldn't we just post the task to the
> message_loop_proxy? There's no need for a reply, right?
> 
> The reason to do this is if we want to make sure that close_task_ gets
destroyed
> on the origin thread. I'm not sure if that's required. Maybe it's the safe
thing
> to do, so I guess I'm in favor of FileUtilProxy in this case, although it may
be
> useful to document why we use it internally here.

Good point... I think simple PostTask will be enough.  Replaced it with
PostTask.

http://codereview.chromium.org/8424006/diff/8001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/8424006/diff/8001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_operation.cc:83:
FileSystemFileUtilProxy::StatusCallback());
On 2011/11/01 22:57:37, willchan wrote:
> What's the long-term vision? Is FileSystemFileUtilProxy going away? If so,
maybe
> you should use base::FileUtilProxy::StatusCallback instead. Up to you.

Yes, I'm going to deprecate this one (in the next patch).

[caseq, 2011-11-02 11:58:53]

This apparently caused caused some filesystem LayoutTests to crash on Mac:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[kinuko, 2011-11-02 12:11:24]

:( ok I'm reverting this.

On 2011/11/02 11:58:53, caseq wrote:
> This apparently caused caused some filesystem LayoutTests to crash on Mac:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...