Move PostTaskAndReplyWithStatus into task_runner_helpers.h

Move PostTaskAndReplyWithStatus into task_runner_helpers.h


BUG=TBD
TEST=no


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

[battre, 2012-02-17 17:37:47]

Hi Fred,

what do you think of moving PostTaskAndReplyWithStatus from file_util_proxy.cc
into a separate header file task_runner_helpers.h? I think this functionality is
pretty handy in several places of chrome. Who would you recommend to review
this? willchan? I picked you because of your work on TaskRunner.

Best regards,
Dominic

[akalin, 2012-02-17 20:00:16]

On 2012/02/17 17:37:47, battre wrote:
> Hi Fred,
> 
> what do you think of moving PostTaskAndReplyWithStatus from file_util_proxy.cc
> into a separate header file task_runner_helpers.h? I think this functionality
is
> pretty handy in several places of chrome. Who would you recommend to review
> this? willchan? I picked you because of your work on TaskRunner.
> 
> Best regards,
> Dominic

I think willchan or darin would probably be a better reviewer, since they're
owners anyway.  I'll take a look also, though.

+willchan

[akalin, 2012-02-22 22:44:19]

On 2012/02/17 20:00:23, akalin wrote:

Hmm, I feel like this would better be merged with the existing PostTaskAndReply
function if possible.  Basically, PostTaskAndReply can be seen as a special case
of PostTaskAndReplyWithStatus, with the return type being 'void'.  C++'s type
system may be able to handle this elegantly, with maybe only a specialization
needed for 'void' (since 'new T()' for T = void probably won't work).

[akalin, 2012-02-22 22:44:32]

+darin, what do you think?

[darin (slow to review), 2012-02-23 19:17:54]

it might be helpful to convert at least one other consumers to this new method
in this CL.

[satorux1, 2012-02-28 23:58:12]

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h
File base/task_runner_helpers.h (right):

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:27: //      RelayHelper(Bind(&Callback),
Owned(result)));
drive-by: Where is RelayHelper defined? 'git grep' only gave me only this:

% git grep RelayHelper
base/file_util_proxy.cc://      RelayHelper(Bind(&Callback), Owned(result)));

Would be nice to include RealyHelper definition in the comment?

[darin (slow to review), 2012-02-29 00:27:48]

it'd be nice to see a unit test for this new function.  that way the usage will
be more clear.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h
File base/task_runner_helpers.h (right):

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:19: //   void Callback(R& result);
why does Callback take a non-const reference?  should it be a const reference
instead?

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:85: bool PostTaskAndReplyWithStatus(
why is this function named *WithStatus?

[willchan no longer on Chromium, 2012-03-01 02:17:24]

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h
File base/task_runner_helpers.h (right):

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:36: template <typename R1, typename R2>
I don't think I like ReturnValueTranslator. Do the translation in the callback
function, why define converters here? I dislike it even more because it looks
like we only get link errors for this, not a compile error. Lame =/

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:50: if (!func.is_null())
I don't think we should allow func to be NULL. Why not DCHECK?

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:51: *result = ReturnValueTranslator<R1,
R2>::Value(func.Run());
I think this should just be *result = fun.Run(); and only have one template
parameter.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:61: void ReturnAsParamAdapter1(const Callback<R(A1)>&
func, A1 a1, R* result) {
Do we need pump to generate multiple variants of this?

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:75: if (!callback.is_null())
I think we should DCHECK in PostTaskAndReplyWithStatus(), so this should be
non-NULL.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:79: template <typename R, typename OWNED>
I think we should only have one parameter, let's not provide functionality here
to do type conversion of values. Let people do that themselves in their
callbacks.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:85: bool PostTaskAndReplyWithStatus(
I think this should be the only function in base. Everything else should be in
base::internal.

[battre, 2012-03-07 20:01:48]

I have addressed your issues and implemented a unit test.

The difference between Patch Set 3 and 5 is where I have put the
PostTaskAndReplyWithResult function. In Patch Set 3 it is in the helpers file;
in Patch Set 5 it is in the MessageLoop class itself. Which version do you
prefer?

We can go a step further and rename PostTaskAndReplyWithResult to
PostTaskAndReply. The C++ type system takes care of the details, but I am not
sure whether this improves the readability. WDYT?

Best regards,
Dominic

P.S. Sorry for the long delay since my last update. I was extremely busy with a
ton of other work.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h
File base/task_runner_helpers.h (right):

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:19: //   void Callback(R& result);
On 2012/02/29 00:27:48, darin wrote:
> why does Callback take a non-const reference?  should it be a const reference
> instead?

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:27: //      RelayHelper(Bind(&Callback),
Owned(result)));
On 2012/02/28 23:58:13, satorux1 wrote:
> drive-by: Where is RelayHelper defined? 'git grep' only gave me only this:
> 
> % git grep RelayHelper
> base/file_util_proxy.cc://      RelayHelper(Bind(&Callback), Owned(result)));
> 
> Would be nice to include RealyHelper definition in the comment?

This was a copy&pasted typo (->ReplyHelper).

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:36: template <typename R1, typename R2>
On 2012/03/01 02:17:24, willchan wrote:
> I don't think I like ReturnValueTranslator. Do the translation in the callback
> function, why define converters here? I dislike it even more because it looks
> like we only get link errors for this, not a compile error. Lame =/

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:50: if (!func.is_null())
On 2012/03/01 02:17:24, willchan wrote:
> I don't think we should allow func to be NULL. Why not DCHECK?

Some unit tests rely on this (see the failed tests for Patch Set 2 of this CL.)
We may change them.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:51: *result = ReturnValueTranslator<R1,
R2>::Value(func.Run());
On 2012/03/01 02:17:24, willchan wrote:
> I think this should just be *result = fun.Run(); and only have one template
> parameter.

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:61: void ReturnAsParamAdapter1(const Callback<R(A1)>&
func, A1 a1, R* result) {
On 2012/03/01 02:17:24, willchan wrote:
> Do we need pump to generate multiple variants of this?

No, we can achieve the same effect by using Bind. I have deleted this.

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:75: if (!callback.is_null())
On 2012/03/01 02:17:24, willchan wrote:
> I think we should DCHECK in PostTaskAndReplyWithStatus(), so this should be
> non-NULL.

See above.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:79: template <typename R, typename OWNED>
On 2012/03/01 02:17:24, willchan wrote:
> I think we should only have one parameter, let's not provide functionality
here
> to do type conversion of values. Let people do that themselves in their
> callbacks.

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:85: bool PostTaskAndReplyWithStatus(
On 2012/03/01 02:17:24, willchan wrote:
> I think this should be the only function in base. Everything else should be in
> base::internal.

Done.

http://codereview.chromium.org/9416060/diff/1/base/task_runner_helpers.h#newc...
base/task_runner_helpers.h:85: bool PostTaskAndReplyWithStatus(
On 2012/02/29 00:27:48, darin wrote:
> why is this function named *WithStatus?

I have renamed it to PostTaskAndReplyWithResult. I think this name fits better.

Done.

[battre, 2012-03-26 12:44:43]

friendly ping

[willchan no longer on Chromium, 2012-04-04 17:35:48]

Sorry for the delay, I totally missed this last week while I was at IETF.

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h
File base/task_runner.h (right):

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h#...
base/task_runner.h:11: #include "base/bind.h"
I have a problem with all these new headers. I see why you added them, but
TaskRunner is used very very widely in chrome code, due to MessageLoop
inheriting from it. That means we should be careful about what we include. I'm
torn, but sorta leaning towards moving this into a utility header and making it
a non-member function that takes a TaskRunner as a parameter, rather than being
a member function of TaskRunner.

I'd love to hear thoughts on this.

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h#...
base/task_runner.h:52: template <typename ReturnType, typename OWNED>
s/OWNED/OwnedType/

[battre, 2012-04-04 19:47:41]

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h
File base/task_runner.h (right):

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h#...
base/task_runner.h:11: #include "base/bind.h"
On 2012/04/04 17:35:49, willchan wrote:
> I have a problem with all these new headers. I see why you added them, but
> TaskRunner is used very very widely in chrome code, due to MessageLoop
> inheriting from it. That means we should be careful about what we include. I'm
> torn, but sorta leaning towards moving this into a utility header and making
it
> a non-member function that takes a TaskRunner as a parameter, rather than
being
> a member function of TaskRunner.
> 
> I'd love to hear thoughts on this.

Either way is fine with me. I'll follow your decision.

[willchan no longer on Chromium, 2012-04-05 16:53:03]

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h
File base/task_runner.h (right):

https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h#...
base/task_runner.h:11: #include "base/bind.h"
On 2012/04/04 19:47:41, battre wrote:
> On 2012/04/04 17:35:49, willchan wrote:
> > I have a problem with all these new headers. I see why you added them, but
> > TaskRunner is used very very widely in chrome code, due to MessageLoop
> > inheriting from it. That means we should be careful about what we include.
I'm
> > torn, but sorta leaning towards moving this into a utility header and making
> it
> > a non-member function that takes a TaskRunner as a parameter, rather than
> being
> > a member function of TaskRunner.
> > 
> > I'd love to hear thoughts on this.
> 
> Either way is fine with me. I'll follow your decision.

darin/akalin, feel free to chime in, but otherwise I'm going to ask for a
task_runner_util.h. I figure that in the future, we may have more of these
helpers anyway, so I'd like to defer to a helper file and reduce #includes for
the common case.

battre: please give them another day, and if there's no NACK, proceed.

[willchan no longer on Chromium, 2012-04-11 11:39:16]

No one responded here, so I guess I'll just say that yes, please create a
task_runner_util.h instead and move this there.

On 2012/04/05 16:53:03, willchan wrote:
> https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h
> File base/task_runner.h (right):
> 
>
https://chromiumcodereview.appspot.com/9416060/diff/31001/base/task_runner.h#...
> base/task_runner.h:11: #include "base/bind.h"
> On 2012/04/04 19:47:41, battre wrote:
> > On 2012/04/04 17:35:49, willchan wrote:
> > > I have a problem with all these new headers. I see why you added them, but
> > > TaskRunner is used very very widely in chrome code, due to MessageLoop
> > > inheriting from it. That means we should be careful about what we include.
> I'm
> > > torn, but sorta leaning towards moving this into a utility header and
making
> > it
> > > a non-member function that takes a TaskRunner as a parameter, rather than
> > being
> > > a member function of TaskRunner.
> > > 
> > > I'd love to hear thoughts on this.
> > 
> > Either way is fine with me. I'll follow your decision.
> 
> darin/akalin, feel free to chime in, but otherwise I'm going to ask for a
> task_runner_util.h. I figure that in the future, we may have more of these
> helpers anyway, so I'd like to defer to a helper file and reduce #includes for
> the common case.
> 
> battre: please give them another day, and if there's no NACK, proceed.

[battre, 2012-04-12 06:46:33]

http://codereview.chromium.org/9416060/diff/31001/base/task_runner.h
File base/task_runner.h (right):

http://codereview.chromium.org/9416060/diff/31001/base/task_runner.h#newcode11
base/task_runner.h:11: #include "base/bind.h"
On 2012/04/05 16:53:03, willchan wrote:
> On 2012/04/04 19:47:41, battre wrote:
> > On 2012/04/04 17:35:49, willchan wrote:
> > > I have a problem with all these new headers. I see why you added them, but
> > > TaskRunner is used very very widely in chrome code, due to MessageLoop
> > > inheriting from it. That means we should be careful about what we include.
> I'm
> > > torn, but sorta leaning towards moving this into a utility header and
making
> > it
> > > a non-member function that takes a TaskRunner as a parameter, rather than
> > being
> > > a member function of TaskRunner.
> > > 
> > > I'd love to hear thoughts on this.
> > 
> > Either way is fine with me. I'll follow your decision.
> 
> darin/akalin, feel free to chime in, but otherwise I'm going to ask for a
> task_runner_util.h. I figure that in the future, we may have more of these
> helpers anyway, so I'd like to defer to a helper file and reduce #includes for
> the common case.
> 
> battre: please give them another day, and if there's no NACK, proceed.

Done.

http://codereview.chromium.org/9416060/diff/31001/base/task_runner.h#newcode52
base/task_runner.h:52: template <typename ReturnType, typename OWNED>
On 2012/04/04 17:35:49, willchan wrote:
> s/OWNED/OwnedType/

Done.

[willchan no longer on Chromium, 2012-04-16 23:57:33]

LGTM

[commit-bot: I haz the power, 2012-04-17 00:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9416060/41009

[commit-bot: I haz the power, 2012-04-17 00:01:48]

Presubmit check for 9416060-41009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  base/task_runner_util.h

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2012-04-17 01:25:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9416060/49002

[commit-bot: I haz the power, 2012-04-17 02:48:08]

Change committed as 132527