Remaining cleanup (base::Bind): Replacing FileUtilProxy calls with new callback

Remaining cleanup (base::Bind): Replacing FileUtilProxy calls with new callback

- removing unused methods
- making some remaining cleanups

BUG=none
TEST=green bots

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

[awong, 2011-10-12 08:09:27]

Need to look at this closer, but here's a high level comment.

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h#newco...
base/file_util_proxy.h:75: class MessageLoopRelay : public
RefCountedThreadSafe<MessageLoopRelay> {
Is it possible to not have this class at All?
It looks like what it's essentially doing is allowing one to return a value via
a return type, and then pass that in as the parameter to the next callback.

We had considered that when making MessageLoopProsy::PostTaskAndreply(), and had
decided to try instead for sharing a data structure between two closures via
currying.  So instead of

R DoWork()
void DoReply(R)

We do

void DoWork(R*)
void DoReply(R*);

R* r = new R();
ml_->PostTaskAndReply(FROM_HERE, Bind(&DoWork, r), Bind(&DoReply, r));

[kinuko, 2011-10-12 11:23:37]

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h#newco...
base/file_util_proxy.h:75: class MessageLoopRelay : public
RefCountedThreadSafe<MessageLoopRelay> {
On 2011/10/12 08:09:27, awong wrote:
> Is it possible to not have this class at All?
> It looks like what it's essentially doing is allowing one to return a value
via
> a return type, and then pass that in as the parameter to the next callback.
> 
> We had considered that when making MessageLoopProsy::PostTaskAndreply(), and
had
> decided to try instead for sharing a data structure between two closures via
> currying.  So instead of
> 
> R DoWork()
> void DoReply(R)
> 
> We do
> 
> void DoWork(R*)
> void DoReply(R*);
> 
> R* r = new R();
> ml_->PostTaskAndReply(FROM_HERE, Bind(&DoWork, r), Bind(&DoReply, r));

Thanks for your early comments!
Hmm... I see.  The issue was almost all of the file related functions return
bool or PlatformFileError to report return status.

What if I do like this using two helper templates?

template <typename R> static void ReplyWithReturnAdapter(
    const Callback<R(void)>& func, R* result) {
  *result = func.Run();
}


// the R* pointer will be unretained (or do something smarter)
template <typename R>
static Closure ReplyWithReturn(const Callback<R(void)>& func, R* result) {
  return base::Bind(&ReplyWithReturnAdapter<R>, func,
                    base::Unretained(result));
}

Then we can call a function with return value like:

R DoWork();
void DoReply(*R);

ml_->PostTaskAndReply(FROM_HERE,           
   FUP::ReplyWithReturn(Bind(&DoWork), &result),
   Bind(&DoReply));

[awong, 2011-10-15 00:17:12]

Sorry for the slow response.  I think your latest proposal sounds pretty good.

I'm going to be out for the next two weeks.  Fred or Will have volunteered to
help in that time period if you have questions or want to bounce ideas :)

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h#newco...
base/file_util_proxy.h:75: class MessageLoopRelay : public
RefCountedThreadSafe<MessageLoopRelay> {
I think that'd work pretty well.  About the memory management, I think using
Owned() in the reply will take care of deleting |result| correctly. 
Specifically:


R* result = new R();
ml_->PostTaskAndReply(FROM_HERE,
  ReplyWithReturnAdapter(Bind(&DoWork, a, b, c), result),
  Bind(&DoReply, Owned(result));


Also, in ReplyWithReturn, you don't need base::Unretained(result) if R is not a
refcounted type.

Lastly, I think these templates can go completely inside an anonymous namespace
the .cc file right?

-Albert



On 2011/10/12 11:23:38, kinuko wrote:
> On 2011/10/12 08:09:27, awong wrote:
> > Is it possible to not have this class at All?
> > It looks like what it's essentially doing is allowing one to return a value
> via
> > a return type, and then pass that in as the parameter to the next callback.
> > 
> > We had considered that when making MessageLoopProsy::PostTaskAndreply(), and
> had
> > decided to try instead for sharing a data structure between two closures via
> > currying.  So instead of
> > 
> > R DoWork()
> > void DoReply(R)
> > 
> > We do
> > 
> > void DoWork(R*)
> > void DoReply(R*);
> > 
> > R* r = new R();
> > ml_->PostTaskAndReply(FROM_HERE, Bind(&DoWork, r), Bind(&DoReply, r));
> 
> Thanks for your early comments!
> Hmm... I see.  The issue was almost all of the file related functions return
> bool or PlatformFileError to report return status.
> 
> What if I do like this using two helper templates?
> 
> template <typename R> static void ReplyWithReturnAdapter(
>     const Callback<R(void)>& func, R* result) {
>   *result = func.Run();
> }
> 
> 
> // the R* pointer will be unretained (or do something smarter)
> template <typename R>
> static Closure ReplyWithReturn(const Callback<R(void)>& func, R* result) {
>   return base::Bind(&ReplyWithReturnAdapter<R>, func,
>                     base::Unretained(result));
> }
> 
> Then we can call a function with return value like:
> 
> R DoWork();
> void DoReply(*R);
> 
> ml_->PostTaskAndReply(FROM_HERE,           
>    FUP::ReplyWithReturn(Bind(&DoWork), &result),
>    Bind(&DoReply));

[kinuko, 2011-10-17 15:21:03]

Thanks, I've added will and fred to the reviewers.

Willchan, Fred, this patch replaces old callbacks around FileUtilProxy and also
obsoletes some unused methods.  Could you take a look?  Thanks!

(James if some of your changes get in first I'll try removing the conflicts.  I
think most of your change should just fit with this one)

On 2011/10/15 00:17:12, awong wrote:
> Sorry for the slow response.  I think your latest proposal sounds pretty good.
> 
> I'm going to be out for the next two weeks.  Fred or Will have volunteered to
> help in that time period if you have questions or want to bounce ideas :)
> 
> http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
>
http://codereview.chromium.org/8231004/diff/7001/base/file_util_proxy.h#newco...
> base/file_util_proxy.h:75: class MessageLoopRelay : public
> RefCountedThreadSafe<MessageLoopRelay> {
> I think that'd work pretty well.  About the memory management, I think using
> Owned() in the reply will take care of deleting |result| correctly. 
> Specifically:
> 
> 
> R* result = new R();
> ml_->PostTaskAndReply(FROM_HERE,
>   ReplyWithReturnAdapter(Bind(&DoWork, a, b, c), result),
>   Bind(&DoReply, Owned(result));
> 
> 
> Also, in ReplyWithReturn, you don't need base::Unretained(result) if R is not
a
> refcounted type.

Ah cool.  Removed the Unretained part.

> Lastly, I think these templates can go completely inside an anonymous
namespace
> the .cc file right?

Yup.  Done.

> -Albert
> 
> 
> 
> On 2011/10/12 11:23:38, kinuko wrote:
> > On 2011/10/12 08:09:27, awong wrote:
> > > Is it possible to not have this class at All?
> > > It looks like what it's essentially doing is allowing one to return a
value
> > via
> > > a return type, and then pass that in as the parameter to the next
callback.
> > > 
> > > We had considered that when making MessageLoopProsy::PostTaskAndreply(),
and
> > had
> > > decided to try instead for sharing a data structure between two closures
via
> > > currying.  So instead of
> > > 
> > > R DoWork()
> > > void DoReply(R)
> > > 
> > > We do
> > > 
> > > void DoWork(R*)
> > > void DoReply(R*);
> > > 
> > > R* r = new R();
> > > ml_->PostTaskAndReply(FROM_HERE, Bind(&DoWork, r), Bind(&DoReply, r));
> > 
> > Thanks for your early comments!
> > Hmm... I see.  The issue was almost all of the file related functions return
> > bool or PlatformFileError to report return status.
> > 
> > What if I do like this using two helper templates?
> > 
> > template <typename R> static void ReplyWithReturnAdapter(
> >     const Callback<R(void)>& func, R* result) {
> >   *result = func.Run();
> > }
> > 
> > 
> > // the R* pointer will be unretained (or do something smarter)
> > template <typename R>
> > static Closure ReplyWithReturn(const Callback<R(void)>& func, R* result) {
> >   return base::Bind(&ReplyWithReturnAdapter<R>, func,
> >                     base::Unretained(result));
> > }
> > 
> > Then we can call a function with return value like:
> > 
> > R DoWork();
> > void DoReply(*R);
> > 
> > ml_->PostTaskAndReply(FROM_HERE,           
> >    FUP::ReplyWithReturn(Bind(&DoWork), &result),
> >    Bind(&DoReply));

[James Hawkins, 2011-10-17 17:05:41]

Looks like we conflicted :-/

http://codereview.chromium.org/8311010/
http://codereview.chromium.org/8315011/
http://codereview.chromium.org/8315012/
http://codereview.chromium.org/8311012/

[willchan no longer on Chromium, 2011-10-17 22:30:30]

Uh, should I be reviewing this? There's a conflict right now? Let me know if I
should proceed.

On 2011/10/17 17:05:41, James Hawkins wrote:
> Looks like we conflicted :-/
> 
> http://codereview.chromium.org/8311010/
> http://codereview.chromium.org/8315011/
> http://codereview.chromium.org/8315012/
> http://codereview.chromium.org/8311012/

[James Hawkins, 2011-10-17 22:34:59]

Neg, all of the CLs for this file have landed (got the go-head from kinuko).

[willchan no longer on Chromium, 2011-10-17 22:39:43]

OK, thx. Will ignore this CL.

On Mon, Oct 17, 2011 at 3:34 PM, <jhawkins@chromium.org> wrote:

> Neg, all of the CLs for this file have landed (got the go-head from
> kinuko).
>
>
http://codereview.chromium.**org/8231004/<http://codereview.chromium.org/8231...
>

[kinuko, 2011-10-18 08:45:32]

Willchan, Fred,
I rebased this change with the James' ones and separated out the file_util_proxy
internal change to another change.  This left some remaining fixes that are just
nits/automatic ones (like: converting more callbacks, removing unused methods
and removing duplicated WeakPtrFactory).  Could you PTAL?  Thanks!

[akalin, 2011-10-18 19:05:11]

LGTM

http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.cc
File base/file_util_proxy.cc (left):

http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.cc#old...
base/file_util_proxy.cc:147: &created_, &error_code);
indent

http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.h
File base/file_util_proxy.h (right):

http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.h#newc...
base/file_util_proxy.h:35: // valid to pass StatusCallback() to any function
that takes a StatusCallback,
I think "a null callback" is clearer

[willchan no longer on Chromium, 2011-10-18 19:53:58]

I'm concerned about FileUtilProxy. The callbacks that run on the origin thread
may have bound parameters. There's a race condition on callback destruction
happening on the origin thread or the file thread. Therefore, it's not clear
which thread the bound parameters will be destroyed on. We should employ the
PostTaskAndReply() solution where we use the RefCountedThreadSafe to control
destruction.

On 2011/10/18 19:05:11, akalin wrote:
> LGTM
> 
> http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.cc
> File base/file_util_proxy.cc (left):
> 
>
http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.cc#old...
> base/file_util_proxy.cc:147: &created_, &error_code);
> indent
> 
> http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.h
> File base/file_util_proxy.h (right):
> 
>
http://codereview.chromium.org/8231004/diff/23002/base/file_util_proxy.h#newc...
> base/file_util_proxy.h:35: // valid to pass StatusCallback() to any function
> that takes a StatusCallback,
> I think "a null callback" is clearer

[darin (slow to review), 2011-10-18 20:05:39]

> I'm concerned about FileUtilProxy. The callbacks that run on the origin 
> thread may have bound parameters. There's a race condition on callback 
> destruction happening on the origin thread or the file thread. Therefore, 
> it's not clear which thread the bound parameters will be destroyed on. We 
> should employ the PostTaskAndReply() solution where we use the 
> RefCountedThreadSafe to control destruction.

Yes, exactly!  FileUtilProxy needs to be rewritten on top of PostTaskAndReply. 
It is not safe to just change old callbacks to new callbacks here.

[darin (slow to review), 2011-10-18 20:06:18]

Incidentally, FileUtilProxy was the inspiration for PostTaskAndReply!

[akalin, 2011-10-18 20:07:18]

I'm confused.  This CL isn't converting to new callbacks, it's already
using new callbacks, and this CL is just removing the base:: prefix.

Is this perhaps based on another pending CL?

On Tue, Oct 18, 2011 at 1:05 PM,  <darin@chromium.org> wrote:
>> I'm concerned about FileUtilProxy. The callbacks that run on the origin
>> thread may have bound parameters. There's a race condition on callback
>> destruction happening on the origin thread or the file thread. Therefore,
>> it's not clear which thread the bound parameters will be destroyed on. We
>> should employ the PostTaskAndReply() solution where we use the
>> RefCountedThreadSafe to control destruction.
>
> Yes, exactly!  FileUtilProxy needs to be rewritten on top of
> PostTaskAndReply.
> It is not safe to just change old callbacks to new callbacks here.
>
> http://codereview.chromium.org/8231004/
>

[willchan no longer on Chromium, 2011-10-18 20:33:00]

The conversion to new callbacks is recent here. Yeah, this CL itself is
benign, it was the earlier conversion which had the issue.

On Tue, Oct 18, 2011 at 1:07 PM, Fred Akalin <akalin@chromium.org> wrote:

> I'm confused.  This CL isn't converting to new callbacks, it's already
> using new callbacks, and this CL is just removing the base:: prefix.
>
> Is this perhaps based on another pending CL?
>
> On Tue, Oct 18, 2011 at 1:05 PM,  <darin@chromium.org> wrote:
> >> I'm concerned about FileUtilProxy. The callbacks that run on the origin
> >> thread may have bound parameters. There's a race condition on callback
> >> destruction happening on the origin thread or the file thread.
> Therefore,
> >> it's not clear which thread the bound parameters will be destroyed on.
> We
> >> should employ the PostTaskAndReply() solution where we use the
> >> RefCountedThreadSafe to control destruction.
> >
> > Yes, exactly!  FileUtilProxy needs to be rewritten on top of
> > PostTaskAndReply.
> > It is not safe to just change old callbacks to new callbacks here.
> >
> > http://codereview.chromium.org/8231004/
> >
>

[willchan no longer on Chromium, 2011-10-18 20:55:09]

I'll send a LGTM for this right now since it's just minor cleanup, but we should
either revert this and the previous CL here or just fix in place to use
PostTaskAndReply(). The reason is the new callbacks support currying, whereas
the old callback system didn't, so now we are exposing FileUtilProxy to
incorrect use due to ambiguity about which thread the callback will be destroyed
on.

On 2011/10/18 20:33:00, willchan wrote:
> The conversion to new callbacks is recent here. Yeah, this CL itself is
> benign, it was the earlier conversion which had the issue.
> 
> On Tue, Oct 18, 2011 at 1:07 PM, Fred Akalin <mailto:akalin@chromium.org>
wrote:
> 
> > I'm confused.  This CL isn't converting to new callbacks, it's already
> > using new callbacks, and this CL is just removing the base:: prefix.
> >
> > Is this perhaps based on another pending CL?
> >
> > On Tue, Oct 18, 2011 at 1:05 PM,  <mailto:darin@chromium.org> wrote:
> > >> I'm concerned about FileUtilProxy. The callbacks that run on the origin
> > >> thread may have bound parameters. There's a race condition on callback
> > >> destruction happening on the origin thread or the file thread.
> > Therefore,
> > >> it's not clear which thread the bound parameters will be destroyed on.
> > We
> > >> should employ the PostTaskAndReply() solution where we use the
> > >> RefCountedThreadSafe to control destruction.
> > >
> > > Yes, exactly!  FileUtilProxy needs to be rewritten on top of
> > > PostTaskAndReply.
> > > It is not safe to just change old callbacks to new callbacks here.
> > >
> > > http://codereview.chromium.org/8231004/
> > >
> >

[James Hawkins, 2011-10-18 20:56:59]

On 2011/10/18 20:55:09, willchan wrote:
> I'll send a LGTM for this right now since it's just minor cleanup, but we
should
> either revert this and the previous CL here or just fix in place to use
> PostTaskAndReply(). The reason is the new callbacks support currying, whereas
> the old callback system didn't, so now we are exposing FileUtilProxy to
> incorrect use due to ambiguity about which thread the callback will be
destroyed
> on.
> 

Kinuko is not online right now to answer, but she informed me yesterday that she
is planning to convert this to PostTaskAndReply. Likely it'll happen in this CL.

[willchan no longer on Chromium, 2011-10-18 21:02:17]

OIC, ok.

On Tue, Oct 18, 2011 at 1:56 PM, <jhawkins@chromium.org> wrote:

> On 2011/10/18 20:55:09, willchan wrote:
>
>> I'll send a LGTM for this right now since it's just minor cleanup, but we
>>
> should
>
>> either revert this and the previous CL here or just fix in place to use
>> PostTaskAndReply(). The reason is the new callbacks support currying,
>> whereas
>> the old callback system didn't, so now we are exposing FileUtilProxy to
>> incorrect use due to ambiguity about which thread the callback will be
>>
> destroyed
>
>> on.
>>
>
>
> Kinuko is not online right now to answer, but she informed me yesterday
> that she
> is planning to convert this to PostTaskAndReply. Likely it'll happen in
> this CL.
>
>
http://codereview.chromium.**org/8231004/<http://codereview.chromium.org/8231...
>