Pass Callback to TaskRunner by value and consume it on invocation (1)

Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2726523002
Cr-Commit-Position: refs/heads/master@{#460288}
Committed: https://chromium.googlesource.com/chromium/src/+/070c8ffb44d04075bd7400691a2325cb6233558a

[tzik, 2017-02-28 10:29:35]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

BUG=
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-02-28 10:30:05]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 10:30:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 10:45:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 10:45:26]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tzik, 2017-02-28 12:12:49]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 12:13:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 12:50:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 12:50:25]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[tzik, 2017-02-28 13:03:47]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is less controversial part of http://crrev.com/2637843002.

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-02-28 13:25:26]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 13:25:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-02-28 13:26:57]

tzik@chromium.org changed reviewers:
+ gab@chromium.org

[tzik, 2017-02-28 13:26:59]

PTAL

[commit-bot: I haz the power, 2017-02-28 14:08:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 14:08:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-03-01 06:41:29]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 06:41:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 06:45:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 06:45:11]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tzik, 2017-03-07 07:36:30]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 07:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 08:43:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 08:43:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-03-07 09:08:27]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 09:08:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 09:58:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 09:58:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-03-07 13:39:44]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 13:40:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 16:07:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 16:07:07]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-03-13 08:43:34]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is less controversial part of http://crrev.com/2637843002.

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is less controversial part of http://crrev.com/2637843002.

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-14 11:50:29]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 11:50:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 13:42:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 13:42:09]

Dry run: This issue passed the CQ dry run.

[gab, 2017-03-15 19:13:42]

//base lgtm w/ nits (didn't look at non-base code -- maybe it can be split to
other CLs?)

https://codereview.chromium.org/2726523002/diff/140001/base/deferred_sequence...
File base/deferred_sequenced_task_runner.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/deferred_sequence...
base/deferred_sequenced_task_runner.cc:101: *i = DeferredTask();
Isn't this handled by the move now?

https://codereview.chromium.org/2726523002/diff/140001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/task_scheduler/sc...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:152: auto
task = MakeUnique<Task>(from_here, std::move(closure), traits_, delay);
#include <utility>

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:1023:
base::ResetAndReturn(&task.task).Run();
What's the difference between this and std::move(task.task).Run()? The
documentation on base::ResetAndReturn doesn't make it obvious why this should be
preferred, should it? Should the documentation be updated?

[tzik, 2017-03-21 04:52:53]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 04:53:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-21 05:43:20]

https://codereview.chromium.org/2726523002/diff/140001/base/deferred_sequence...
File base/deferred_sequenced_task_runner.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/deferred_sequence...
base/deferred_sequenced_task_runner.cc:101: *i = DeferredTask();
On 2017/03/15 19:13:42, gab wrote:
> Isn't this handled by the move now?

Done.

https://codereview.chromium.org/2726523002/diff/140001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/task_scheduler/sc...
base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:152: auto
task = MakeUnique<Task>(from_here, std::move(closure), traits_, delay);
On 2017/03/15 19:13:42, gab wrote:
> #include <utility>

Done.

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:1023:
base::ResetAndReturn(&task.task).Run();
On 2017/03/15 19:13:42, gab wrote:
> What's the difference between this and std::move(task.task).Run()? The
> documentation on base::ResetAndReturn doesn't make it obvious why this should
be
> preferred, should it? Should the documentation be updated?

std::move(cb).Run() for Callback and OnceCallback behaves differently until
https://codereview.chromium.org/2753953003/ lands.
It doesn't clear |cb| if it's Callback, and does clear it if it's OnceCallback.

[tzik, 2017-03-21 05:43:38]

tzik@chromium.org changed reviewers:
+ sky@chromium.org

[tzik, 2017-03-21 05:44:41]

tzik@chromium.org changed reviewers:
+ kinuko@chromium.org

[tzik, 2017-03-21 05:45:41]

tzik@chromium.org changed reviewers:
+ oshima@chromium.org, vmpstr@chromium.org

[tzik, 2017-03-21 05:48:02]

tzik@chromium.org changed reviewers:
+ alexclarke@chromium.org

[tzik, 2017-03-21 05:49:32]

tzik@chromium.org changed reviewers:
+ wez@chromium.org

[tzik, 2017-03-21 05:51:20]

tzik@chromium.org changed reviewers:
+ ccameron@chromium.org, mmenke@chromium.org

[tzik, 2017-03-21 05:53:22]

tzik@chromium.org changed reviewers:
+ xhwang@chromium.org

[tzik, 2017-03-21 05:54:14]

tzik@chromium.org changed reviewers:
+ sdefresne@chromium.org

[tzik, 2017-03-21 05:57:30]

tzik@chromium.org changed reviewers:
+ slan@chromium.org

[tzik, 2017-03-21 06:00:36]

On 2017/03/15 19:13:42, gab wrote:
> //base lgtm w/ nits (didn't look at non-base code -- maybe it can be split to
> other CLs?)

I think it's hard. Since most of changes are signature change of subclasses of
TaskRunner, we need to update them at once.

[tzik, 2017-03-21 06:01:22]

Adding owners. PTAL to:
sky: //chrome, //components/history, //mojo
kinuko: //content, tp/WK/Source/platform/WebTaskRunner.h
oshima: //chromeos
vmpstr: //cc/tiles, //cc/test
alexclarke: tp/WK/Source/platform/scheduler
wez: //remoting
ccameron: //ui/accelerated_widget_mac
mmenke: //net
sdefresne: //ios
xhwang: //media
slan: //chromecast

[commit-bot: I haz the power, 2017-03-21 08:13:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 08:13:46]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-03-21 12:10:07]

So this doesn't still stop new code to leave a ref / make a copy but the
intention is to land an incremental change before we actually turn these to use
OnceCallback, is that the case?

lgtm

[mmenke, 2017-03-21 12:50:31]

On 2017/03/21 12:10:07, kinuko wrote:
> So this doesn't still stop new code to leave a ref / make a copy but the
> intention is to land an incremental change before we actually turn these to
use
> OnceCallback, is that the case?
> 
> lgtm

That's my impression a well.  This should be made clear in the CL description.

net/ LGTM, if the description is updated.

[oshima, 2017-03-21 14:55:12]

chromeos/ lgtm

Can you add/file a bug? I know you explained in the description, but for the CL
this large, it's better to have a bug (including answer to questions above).

[sky, 2017-03-21 14:56:30]

Question for you on this:

> With the pass-by-const-ref manner as the old code does, we can't avoid
> leaving a reference to the callback object on the original thread. That
> is, the callback object may be destroyed either on the target thread or
> the original thread. That's problematic when a non-thread-safe object is
> bound to the callback.

If the callback object needs to be destroyed on a particular thread shouldn't
the code calling PostTask ensure that happens?

In looking at this patch I'm having a hard time knowing when pass-by-value
should be used vs const-ref.

[mmenke, 2017-03-21 15:03:37]

On 2017/03/21 14:56:30, sky wrote:
> Question for you on this:
> 
> > With the pass-by-const-ref manner as the old code does, we can't avoid
> > leaving a reference to the callback object on the original thread. That
> > is, the callback object may be destroyed either on the target thread or
> > the original thread. That's problematic when a non-thread-safe object is
> > bound to the callback.
> 
> If the callback object needs to be destroyed on a particular thread shouldn't
> the code calling PostTask ensure that happens?
> 
> In looking at this patch I'm having a hard time knowing when pass-by-value
> should be used vs const-ref.

With the old API, the code can't easily ensure that happens - the callback owns
a ref, and if you pass the callback by ref, you can't release all references to
the callback on the current thread until after the call to PostTask.  With this
API call, you can pass ownership of the callback to PostTask, so avoid that
issue.

[slan, 2017-03-21 15:27:03]

On 2017/03/21 15:03:37, mmenke wrote:
> On 2017/03/21 14:56:30, sky wrote:
> > Question for you on this:
> > 
> > > With the pass-by-const-ref manner as the old code does, we can't avoid
> > > leaving a reference to the callback object on the original thread. That
> > > is, the callback object may be destroyed either on the target thread or
> > > the original thread. That's problematic when a non-thread-safe object is
> > > bound to the callback.
> > 
> > If the callback object needs to be destroyed on a particular thread
shouldn't
> > the code calling PostTask ensure that happens?
> > 
> > In looking at this patch I'm having a hard time knowing when pass-by-value
> > should be used vs const-ref.
> 
> With the old API, the code can't easily ensure that happens - the callback
owns
> a ref, and if you pass the callback by ref, you can't release all references
to
> the callback on the current thread until after the call to PostTask.  With
this
> API call, you can pass ownership of the callback to PostTask, so avoid that
> issue.

cast/ lgtm

[xhwang, 2017-03-21 16:23:24]

media/ lgtm

[Wez, 2017-03-21 16:43:36]

https://codereview.chromium.org/2726523002/diff/160001/remoting/client/plugin...
File remoting/client/plugin/pepper_main_thread_task_runner.cc (right):

https://codereview.chromium.org/2726523002/diff/160001/remoting/client/plugin...
remoting/client/plugin/pepper_main_thread_task_runner.cc:60:
base::ResetAndReturn(&task).Run();
Why do you need ResetAndReturn() here?

[sky, 2017-03-21 17:54:00]

On Tue, Mar 21, 2017 at 8:03 AM,  <mmenke@chromium.org> wrote:
> On 2017/03/21 14:56:30, sky wrote:
>> Question for you on this:
>>
>> > With the pass-by-const-ref manner as the old code does, we can't avoid
>> > leaving a reference to the callback object on the original thread. That
>> > is, the callback object may be destroyed either on the target thread or
>> > the original thread. That's problematic when a non-thread-safe object is
>> > bound to the callback.
>>
>> If the callback object needs to be destroyed on a particular thread
>> shouldn't
>> the code calling PostTask ensure that happens?
>>
>> In looking at this patch I'm having a hard time knowing when pass-by-value
>> should be used vs const-ref.
>
> With the old API, the code can't easily ensure that happens - the callback
> owns
> a ref, and if you pass the callback by ref, you can't release all references
> to
> the callback on the current thread until after the call to PostTask. With
> this
> API call, you can pass ownership of the callback to PostTask, so avoid that
> issue.

With the new api do you *always* have to pass the Closure as an rvalue
to Post*Task?

  -Scott

>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sky, 2017-03-21 17:54:00]

On Tue, Mar 21, 2017 at 8:03 AM,  <mmenke@chromium.org> wrote:
> On 2017/03/21 14:56:30, sky wrote:
>> Question for you on this:
>>
>> > With the pass-by-const-ref manner as the old code does, we can't avoid
>> > leaving a reference to the callback object on the original thread. That
>> > is, the callback object may be destroyed either on the target thread or
>> > the original thread. That's problematic when a non-thread-safe object is
>> > bound to the callback.
>>
>> If the callback object needs to be destroyed on a particular thread
>> shouldn't
>> the code calling PostTask ensure that happens?
>>
>> In looking at this patch I'm having a hard time knowing when pass-by-value
>> should be used vs const-ref.
>
> With the old API, the code can't easily ensure that happens - the callback
> owns
> a ref, and if you pass the callback by ref, you can't release all references
> to
> the callback on the current thread until after the call to PostTask. With
> this
> API call, you can pass ownership of the callback to PostTask, so avoid that
> issue.

With the new api do you *always* have to pass the Closure as an rvalue
to Post*Task?

  -Scott

>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alex clarke (OOO till 29th), 2017-03-21 18:51:01]

third_party/WebKit/Source/platform/scheduler/ LGTM

[vmpstr, 2017-03-21 19:45:07]

cc lgtm

[gab, 2017-03-21 19:59:31]

On 2017/03/21 17:54:00, sky wrote:
> On Tue, Mar 21, 2017 at 8:03 AM, 
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org> wrote:
> > On 2017/03/21 14:56:30, sky wrote:
> >> Question for you on this:
> >>
> >> > With the pass-by-const-ref manner as the old code does, we can't avoid
> >> > leaving a reference to the callback object on the original thread. That
> >> > is, the callback object may be destroyed either on the target thread or
> >> > the original thread. That's problematic when a non-thread-safe object is
> >> > bound to the callback.
> >>
> >> If the callback object needs to be destroyed on a particular thread
> >> shouldn't
> >> the code calling PostTask ensure that happens?
> >>
> >> In looking at this patch I'm having a hard time knowing when pass-by-value
> >> should be used vs const-ref.
> >
> > With the old API, the code can't easily ensure that happens - the callback
> > owns
> > a ref, and if you pass the callback by ref, you can't release all references
> > to
> > the callback on the current thread until after the call to PostTask. With
> > this
> > API call, you can pass ownership of the callback to PostTask, so avoid that
> > issue.
> 
> With the new api do you *always* have to pass the Closure as an rvalue
> to Post*Task?

@sky: you don't but if you do you save a ref-count bump. Otherwise it's the same
as before (except that now the refbump happens on the calling side instead of
the receiving side).

This is identical to our recommendation to pass scoped_refptr by value instead
of const& (const& guarantees a refbump while passing by-value avoids it if the
caller provides an r-value).

> 
>   -Scott
> 
> >
> > https://codereview.chromium.org/2726523002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
>

[gab, 2017-03-21 20:02:29]

On 2017/03/21 19:59:31, gab wrote:
> On 2017/03/21 17:54:00, sky wrote:
> > On Tue, Mar 21, 2017 at 8:03 AM, 
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org>
wrote:
> > > On 2017/03/21 14:56:30, sky wrote:
> > >> Question for you on this:
> > >>
> > >> > With the pass-by-const-ref manner as the old code does, we can't avoid
> > >> > leaving a reference to the callback object on the original thread. That
> > >> > is, the callback object may be destroyed either on the target thread or
> > >> > the original thread. That's problematic when a non-thread-safe object
is
> > >> > bound to the callback.
> > >>
> > >> If the callback object needs to be destroyed on a particular thread
> > >> shouldn't
> > >> the code calling PostTask ensure that happens?
> > >>
> > >> In looking at this patch I'm having a hard time knowing when
pass-by-value
> > >> should be used vs const-ref.
> > >
> > > With the old API, the code can't easily ensure that happens - the callback
> > > owns
> > > a ref, and if you pass the callback by ref, you can't release all
references
> > > to
> > > the callback on the current thread until after the call to PostTask. With
> > > this
> > > API call, you can pass ownership of the callback to PostTask, so avoid
that
> > > issue.
> > 
> > With the new api do you *always* have to pass the Closure as an rvalue
> > to Post*Task?
> 
> @sky: you don't but if you do you save a ref-count bump. Otherwise it's the
same
> as before (except that now the refbump happens on the calling side instead of
> the receiving side).
> 
> This is identical to our recommendation to pass scoped_refptr by value instead
> of const& (const& guarantees a refbump while passing by-value avoids it if the
> caller provides an r-value).

Long-term though, this is a step towards making PostTask() take a OnceClosure
instead of a Closure at which point the caller will also be forced to provide an
r-value. So this CL makes it at least equivalent -- sometimes better where
caller already handed an r-value -- and is a step towards making it always
better.

[sky, 2017-03-21 22:57:12]

On 2017/03/21 20:02:29, gab wrote:
> On 2017/03/21 19:59:31, gab wrote:
> > On 2017/03/21 17:54:00, sky wrote:
> > > On Tue, Mar 21, 2017 at 8:03 AM, 
> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org>
> wrote:
> > > > On 2017/03/21 14:56:30, sky wrote:
> > > >> Question for you on this:
> > > >>
> > > >> > With the pass-by-const-ref manner as the old code does, we can't
avoid
> > > >> > leaving a reference to the callback object on the original thread.
That
> > > >> > is, the callback object may be destroyed either on the target thread
or
> > > >> > the original thread. That's problematic when a non-thread-safe object
> is
> > > >> > bound to the callback.
> > > >>
> > > >> If the callback object needs to be destroyed on a particular thread
> > > >> shouldn't
> > > >> the code calling PostTask ensure that happens?
> > > >>
> > > >> In looking at this patch I'm having a hard time knowing when
> pass-by-value
> > > >> should be used vs const-ref.
> > > >
> > > > With the old API, the code can't easily ensure that happens - the
callback
> > > > owns
> > > > a ref, and if you pass the callback by ref, you can't release all
> references
> > > > to
> > > > the callback on the current thread until after the call to PostTask.
With
> > > > this
> > > > API call, you can pass ownership of the callback to PostTask, so avoid
> that
> > > > issue.
> > > 
> > > With the new api do you *always* have to pass the Closure as an rvalue
> > > to Post*Task?
> > 
> > @sky: you don't but if you do you save a ref-count bump. Otherwise it's the
> same
> > as before (except that now the refbump happens on the calling side instead
of
> > the receiving side).
> > 
> > This is identical to our recommendation to pass scoped_refptr by value
instead
> > of const& (const& guarantees a refbump while passing by-value avoids it if
the
> > caller provides an r-value).
> 
> Long-term though, this is a step towards making PostTask() take a OnceClosure
> instead of a Closure at which point the caller will also be forced to provide
an
> r-value. So this CL makes it at least equivalent -- sometimes better where
> caller already handed an r-value -- and is a step towards making it always
> better.


Thanks for helping me understand this. Is part of this change making callback no
longer support copy semantics?

[gab, 2017-03-21 23:03:23]

On 2017/03/21 22:57:12, sky wrote:
> On 2017/03/21 20:02:29, gab wrote:
> > On 2017/03/21 19:59:31, gab wrote:
> > > On 2017/03/21 17:54:00, sky wrote:
> > > > On Tue, Mar 21, 2017 at 8:03 AM, 
> > > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org>
> > wrote:
> > > > > On 2017/03/21 14:56:30, sky wrote:
> > > > >> Question for you on this:
> > > > >>
> > > > >> > With the pass-by-const-ref manner as the old code does, we can't
> avoid
> > > > >> > leaving a reference to the callback object on the original thread.
> That
> > > > >> > is, the callback object may be destroyed either on the target
thread
> or
> > > > >> > the original thread. That's problematic when a non-thread-safe
object
> > is
> > > > >> > bound to the callback.
> > > > >>
> > > > >> If the callback object needs to be destroyed on a particular thread
> > > > >> shouldn't
> > > > >> the code calling PostTask ensure that happens?
> > > > >>
> > > > >> In looking at this patch I'm having a hard time knowing when
> > pass-by-value
> > > > >> should be used vs const-ref.
> > > > >
> > > > > With the old API, the code can't easily ensure that happens - the
> callback
> > > > > owns
> > > > > a ref, and if you pass the callback by ref, you can't release all
> > references
> > > > > to
> > > > > the callback on the current thread until after the call to PostTask.
> With
> > > > > this
> > > > > API call, you can pass ownership of the callback to PostTask, so avoid
> > that
> > > > > issue.
> > > > 
> > > > With the new api do you *always* have to pass the Closure as an rvalue
> > > > to Post*Task?
> > > 
> > > @sky: you don't but if you do you save a ref-count bump. Otherwise it's
the
> > same
> > > as before (except that now the refbump happens on the calling side instead
> of
> > > the receiving side).
> > > 
> > > This is identical to our recommendation to pass scoped_refptr by value
> instead
> > > of const& (const& guarantees a refbump while passing by-value avoids it if
> the
> > > caller provides an r-value).
> > 
> > Long-term though, this is a step towards making PostTask() take a
OnceClosure
> > instead of a Closure at which point the caller will also be forced to
provide
> an
> > r-value. So this CL makes it at least equivalent -- sometimes better where
> > caller already handed an r-value -- and is a step towards making it always
> > better.
> 
> 
> Thanks for helping me understand this. Is part of this change making callback
no
> longer support copy semantics?

This CL sure doesn't. I don't otherwise precisely recall the full long-term
plans (tzik@ can reply when he comes online if you want more details) but I'm
pretty sure copying callbacks will still have to be allowed.

[sky, 2017-03-22 00:07:06]

That means it's easy to call the functions with a move, right? In that
case is it worth having two sets, one that takes a copy and one a ref?
Maybe with distinct names?

  -Scott

On Tue, Mar 21, 2017 at 4:03 PM,  <gab@chromium.org> wrote:
> On 2017/03/21 22:57:12, sky wrote:
>> On 2017/03/21 20:02:29, gab wrote:
>> > On 2017/03/21 19:59:31, gab wrote:
>> > > On 2017/03/21 17:54:00, sky wrote:
>> > > > On Tue, Mar 21, 2017 at 8:03 AM,
>> > >
>> > >
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org&gt;
>> > wrote:
>> > > > > On 2017/03/21 14:56:30, sky wrote:
>> > > > >> Question for you on this:
>> > > > >>
>> > > > >> > With the pass-by-const-ref manner as the old code does, we
>> > > > >> > can't
>> avoid
>> > > > >> > leaving a reference to the callback object on the original
>> > > > >> > thread.
>> That
>> > > > >> > is, the callback object may be destroyed either on the target
> thread
>> or
>> > > > >> > the original thread. That's problematic when a non-thread-safe
> object
>> > is
>> > > > >> > bound to the callback.
>> > > > >>
>> > > > >> If the callback object needs to be destroyed on a particular
>> > > > >> thread
>> > > > >> shouldn't
>> > > > >> the code calling PostTask ensure that happens?
>> > > > >>
>> > > > >> In looking at this patch I'm having a hard time knowing when
>> > pass-by-value
>> > > > >> should be used vs const-ref.
>> > > > >
>> > > > > With the old API, the code can't easily ensure that happens - the
>> callback
>> > > > > owns
>> > > > > a ref, and if you pass the callback by ref, you can't release all
>> > references
>> > > > > to
>> > > > > the callback on the current thread until after the call to
>> > > > > PostTask.
>> With
>> > > > > this
>> > > > > API call, you can pass ownership of the callback to PostTask, so
>> > > > > avoid
>> > that
>> > > > > issue.
>> > > >
>> > > > With the new api do you *always* have to pass the Closure as an
>> > > > rvalue
>> > > > to Post*Task?
>> > >
>> > > @sky: you don't but if you do you save a ref-count bump. Otherwise
>> > > it's
> the
>> > same
>> > > as before (except that now the refbump happens on the calling side
>> > > instead
>> of
>> > > the receiving side).
>> > >
>> > > This is identical to our recommendation to pass scoped_refptr by value
>> instead
>> > > of const& (const& guarantees a refbump while passing by-value avoids
>> > > it if
>> the
>> > > caller provides an r-value).
>> >
>> > Long-term though, this is a step towards making PostTask() take a
> OnceClosure
>> > instead of a Closure at which point the caller will also be forced to
> provide
>> an
>> > r-value. So this CL makes it at least equivalent -- sometimes better
>> > where
>> > caller already handed an r-value -- and is a step towards making it
>> > always
>> > better.
>>
>>
>> Thanks for helping me understand this. Is part of this change making
>> callback
> no
>> longer support copy semantics?
>
> This CL sure doesn't. I don't otherwise precisely recall the full long-term
> plans (tzik@ can reply when he comes online if you want more details) but
> I'm
> pretty sure copying callbacks will still have to be allowed.
>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sky, 2017-03-22 00:07:06]

That means it's easy to call the functions with a move, right? In that
case is it worth having two sets, one that takes a copy and one a ref?
Maybe with distinct names?

  -Scott

On Tue, Mar 21, 2017 at 4:03 PM,  <gab@chromium.org> wrote:
> On 2017/03/21 22:57:12, sky wrote:
>> On 2017/03/21 20:02:29, gab wrote:
>> > On 2017/03/21 19:59:31, gab wrote:
>> > > On 2017/03/21 17:54:00, sky wrote:
>> > > > On Tue, Mar 21, 2017 at 8:03 AM,
>> > >
>> > >
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org&gt;
>> > wrote:
>> > > > > On 2017/03/21 14:56:30, sky wrote:
>> > > > >> Question for you on this:
>> > > > >>
>> > > > >> > With the pass-by-const-ref manner as the old code does, we
>> > > > >> > can't
>> avoid
>> > > > >> > leaving a reference to the callback object on the original
>> > > > >> > thread.
>> That
>> > > > >> > is, the callback object may be destroyed either on the target
> thread
>> or
>> > > > >> > the original thread. That's problematic when a non-thread-safe
> object
>> > is
>> > > > >> > bound to the callback.
>> > > > >>
>> > > > >> If the callback object needs to be destroyed on a particular
>> > > > >> thread
>> > > > >> shouldn't
>> > > > >> the code calling PostTask ensure that happens?
>> > > > >>
>> > > > >> In looking at this patch I'm having a hard time knowing when
>> > pass-by-value
>> > > > >> should be used vs const-ref.
>> > > > >
>> > > > > With the old API, the code can't easily ensure that happens - the
>> callback
>> > > > > owns
>> > > > > a ref, and if you pass the callback by ref, you can't release all
>> > references
>> > > > > to
>> > > > > the callback on the current thread until after the call to
>> > > > > PostTask.
>> With
>> > > > > this
>> > > > > API call, you can pass ownership of the callback to PostTask, so
>> > > > > avoid
>> > that
>> > > > > issue.
>> > > >
>> > > > With the new api do you *always* have to pass the Closure as an
>> > > > rvalue
>> > > > to Post*Task?
>> > >
>> > > @sky: you don't but if you do you save a ref-count bump. Otherwise
>> > > it's
> the
>> > same
>> > > as before (except that now the refbump happens on the calling side
>> > > instead
>> of
>> > > the receiving side).
>> > >
>> > > This is identical to our recommendation to pass scoped_refptr by value
>> instead
>> > > of const& (const& guarantees a refbump while passing by-value avoids
>> > > it if
>> the
>> > > caller provides an r-value).
>> >
>> > Long-term though, this is a step towards making PostTask() take a
> OnceClosure
>> > instead of a Closure at which point the caller will also be forced to
> provide
>> an
>> > r-value. So this CL makes it at least equivalent -- sometimes better
>> > where
>> > caller already handed an r-value -- and is a step towards making it
>> > always
>> > better.
>>
>>
>> Thanks for helping me understand this. Is part of this change making
>> callback
> no
>> longer support copy semantics?
>
> This CL sure doesn't. I don't otherwise precisely recall the full long-term
> plans (tzik@ can reply when he comes online if you want more details) but
> I'm
> pretty sure copying callbacks will still have to be allowed.
>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tzik, 2017-03-22 03:31:39]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is less controversial part of http://crrev.com/2637843002.

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-22 03:43:49]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-22 03:51:15]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

Pass the Callback parameter of TaskRunner::PostTask() by value instead
of const ref, and move it to the target thread without churning the ref
count.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-22 07:38:54]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-22 08:03:37]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 08:03:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-22 08:17:29]

On 2017/03/21 12:10:07, kinuko wrote:
> So this doesn't still stop new code to leave a ref / make a copy but the
> intention is to land an incremental change before we actually turn these to
use
> OnceCallback, is that the case?
> 
> lgtm

Updated the description to mention that.
Right, this will reduce the chance to leave the ref on the original thread, but
the caller can leave a ref. And, yes this is a preparation of the following
OnceCallback support.

[tzik, 2017-03-22 08:18:17]

On 2017/03/21 14:55:12, oshima wrote:
> chromeos/ lgtm
> 
> Can you add/file a bug? I know you explained in the description, but for the
CL
> this large, it's better to have a bug (including answer to questions above).

Sure, I filed a bug for the series of CLs, and added BUG= line.

[tzik, 2017-03-22 08:51:19]

On 2017/03/21 23:03:23, gab wrote:
> On 2017/03/21 22:57:12, sky wrote:
> > On 2017/03/21 20:02:29, gab wrote:
> > > On 2017/03/21 19:59:31, gab wrote:
> > > > On 2017/03/21 17:54:00, sky wrote:
> > > > > On Tue, Mar 21, 2017 at 8:03 AM, 
> > > > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org>
> > > wrote:
> > > > > > On 2017/03/21 14:56:30, sky wrote:
> > > > > >> Question for you on this:
> > > > > >>
> > > > > >> > With the pass-by-const-ref manner as the old code does, we can't
> > avoid
> > > > > >> > leaving a reference to the callback object on the original
thread.
> > That
> > > > > >> > is, the callback object may be destroyed either on the target
> thread
> > or
> > > > > >> > the original thread. That's problematic when a non-thread-safe
> object
> > > is
> > > > > >> > bound to the callback.
> > > > > >>
> > > > > >> If the callback object needs to be destroyed on a particular thread
> > > > > >> shouldn't
> > > > > >> the code calling PostTask ensure that happens?
> > > > > >>
> > > > > >> In looking at this patch I'm having a hard time knowing when
> > > pass-by-value
> > > > > >> should be used vs const-ref.
> > > > > >
> > > > > > With the old API, the code can't easily ensure that happens - the
> > callback
> > > > > > owns
> > > > > > a ref, and if you pass the callback by ref, you can't release all
> > > references
> > > > > > to
> > > > > > the callback on the current thread until after the call to PostTask.
> > With
> > > > > > this
> > > > > > API call, you can pass ownership of the callback to PostTask, so
avoid
> > > that
> > > > > > issue.
> > > > > 
> > > > > With the new api do you *always* have to pass the Closure as an rvalue
> > > > > to Post*Task?
> > > > 
> > > > @sky: you don't but if you do you save a ref-count bump. Otherwise it's
> the
> > > same
> > > > as before (except that now the refbump happens on the calling side
instead
> > of
> > > > the receiving side).
> > > > 
> > > > This is identical to our recommendation to pass scoped_refptr by value
> > instead
> > > > of const& (const& guarantees a refbump while passing by-value avoids it
if
> > the
> > > > caller provides an r-value).
> > > 
> > > Long-term though, this is a step towards making PostTask() take a
> OnceClosure
> > > instead of a Closure at which point the caller will also be forced to
> provide
> > an
> > > r-value. So this CL makes it at least equivalent -- sometimes better where
> > > caller already handed an r-value -- and is a step towards making it always
> > > better.
> > 
> > 
> > Thanks for helping me understand this. Is part of this change making
callback
> no
> > longer support copy semantics?
> 
> This CL sure doesn't. I don't otherwise precisely recall the full long-term
> plans (tzik@ can reply when he comes online if you want more details) but I'm
> pretty sure copying callbacks will still have to be allowed.

I'd recommend to use OnceCallback in most case, and would force to use rvalue
for them.
But, right, we'll not remove copyable callback, and existing code should stay
valid.

[tzik, 2017-03-22 08:52:03]

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/2726523002/diff/140001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:1023:
base::ResetAndReturn(&task.task).Run();
On 2017/03/21 05:43:20, tzik wrote:
> On 2017/03/15 19:13:42, gab wrote:
> > What's the difference between this and std::move(task.task).Run()? The
> > documentation on base::ResetAndReturn doesn't make it obvious why this
should
> be
> > preferred, should it? Should the documentation be updated?
> 
> std::move(cb).Run() for Callback and OnceCallback behaves differently until
> https://codereview.chromium.org/2753953003/ lands.
> It doesn't clear |cb| if it's Callback, and does clear it if it's
OnceCallback.

And it landed. Updated the CL to replace ResetAndReturn with std::move.

https://codereview.chromium.org/2726523002/diff/160001/remoting/client/plugin...
File remoting/client/plugin/pepper_main_thread_task_runner.cc (right):

https://codereview.chromium.org/2726523002/diff/160001/remoting/client/plugin...
remoting/client/plugin/pepper_main_thread_task_runner.cc:60:
base::ResetAndReturn(&task).Run();
On 2017/03/21 16:43:36, Wez wrote:
> Why do you need ResetAndReturn() here?

# Converted it to std::move().

This is a preparation for base::OnceCallback, which can run only on rvalue. It's
unneeded for base::Closure, but this will make a following CL smaller.

[tzik, 2017-03-22 09:04:02]

On 2017/03/22 00:07:06, sky wrote:
> That means it's easy to call the functions with a move, right? In that
> case is it worth having two sets, one that takes a copy and one a ref?
> Maybe with distinct names?

Do you mean we may move an object accidentally to PostTask?
Since pass-by-move happens just on a temporary objects and explicit std::move()
or std::forward(), I think this doesn't make error prone situation.

> 
>   -Scott
> 
> On Tue, Mar 21, 2017 at 4:03 PM,  <mailto:gab@chromium.org> wrote:
> > On 2017/03/21 22:57:12, sky wrote:
> >> On 2017/03/21 20:02:29, gab wrote:
> >> > On 2017/03/21 19:59:31, gab wrote:
> >> > > On 2017/03/21 17:54:00, sky wrote:
> >> > > > On Tue, Mar 21, 2017 at 8:03 AM,
> >> > >
> >> > >
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mmenke@chromium.org&gt;
> >> > wrote:
> >> > > > > On 2017/03/21 14:56:30, sky wrote:
> >> > > > >> Question for you on this:
> >> > > > >>
> >> > > > >> > With the pass-by-const-ref manner as the old code does, we
> >> > > > >> > can't
> >> avoid
> >> > > > >> > leaving a reference to the callback object on the original
> >> > > > >> > thread.
> >> That
> >> > > > >> > is, the callback object may be destroyed either on the target
> > thread
> >> or
> >> > > > >> > the original thread. That's problematic when a non-thread-safe
> > object
> >> > is
> >> > > > >> > bound to the callback.
> >> > > > >>
> >> > > > >> If the callback object needs to be destroyed on a particular
> >> > > > >> thread
> >> > > > >> shouldn't
> >> > > > >> the code calling PostTask ensure that happens?
> >> > > > >>
> >> > > > >> In looking at this patch I'm having a hard time knowing when
> >> > pass-by-value
> >> > > > >> should be used vs const-ref.
> >> > > > >
> >> > > > > With the old API, the code can't easily ensure that happens - the
> >> callback
> >> > > > > owns
> >> > > > > a ref, and if you pass the callback by ref, you can't release all
> >> > references
> >> > > > > to
> >> > > > > the callback on the current thread until after the call to
> >> > > > > PostTask.
> >> With
> >> > > > > this
> >> > > > > API call, you can pass ownership of the callback to PostTask, so
> >> > > > > avoid
> >> > that
> >> > > > > issue.
> >> > > >
> >> > > > With the new api do you *always* have to pass the Closure as an
> >> > > > rvalue
> >> > > > to Post*Task?
> >> > >
> >> > > @sky: you don't but if you do you save a ref-count bump. Otherwise
> >> > > it's
> > the
> >> > same
> >> > > as before (except that now the refbump happens on the calling side
> >> > > instead
> >> of
> >> > > the receiving side).
> >> > >
> >> > > This is identical to our recommendation to pass scoped_refptr by value
> >> instead
> >> > > of const& (const& guarantees a refbump while passing by-value avoids
> >> > > it if
> >> the
> >> > > caller provides an r-value).
> >> >
> >> > Long-term though, this is a step towards making PostTask() take a
> > OnceClosure
> >> > instead of a Closure at which point the caller will also be forced to
> > provide
> >> an
> >> > r-value. So this CL makes it at least equivalent -- sometimes better
> >> > where
> >> > caller already handed an r-value -- and is a step towards making it
> >> > always
> >> > better.
> >>
> >>
> >> Thanks for helping me understand this. Is part of this change making
> >> callback
> > no
> >> longer support copy semantics?
> >
> > This CL sure doesn't. I don't otherwise precisely recall the full long-term
> > plans (tzik@ can reply when he comes online if you want more details) but
> > I'm
> > pretty sure copying callbacks will still have to be allowed.
> >
> > https://codereview.chromium.org/2726523002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.
>

[commit-bot: I haz the power, 2017-03-22 10:33:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 10:33:23]

Dry run: This issue passed the CQ dry run.

[gab, 2017-03-22 16:04:39]

On 2017/03/22 09:04:02, tzik wrote:
> On 2017/03/22 00:07:06, sky wrote:
> > That means it's easy to call the functions with a move, right? In that
> > case is it worth having two sets, one that takes a copy and one a ref?
> > Maybe with distinct names?
> 
> Do you mean we may move an object accidentally to PostTask?
> Since pass-by-move happens just on a temporary objects and explicit
std::move()
> or std::forward(), I think this doesn't make error prone situation.

@sky: or did you mean "it's to call the functions *without* a move"?

It indeed is for now but this CL makes that no worse:

 - with const&: receiver always has to take a new ref.
 - by-value: receiver moves ref and caller may or may not move his ref (if he
does this CL makes it better, if he doesn't it's status quo -- since most
callsites invoke base::Bind() inline, resulting in an r-value, this CL should
strictly reduce the number of unnecessary ref bumps already)

Long-term as we migrate further towards base::OnceClosure, we could decide to
make base::PostTask only take OnceClosures and force callers that explicitly
want to pass a copy of an existing Closure to make that copy explicitly: getting
rid of the concern of "easily forgetting a move".

tzik@ is in TOK but feel free to ping me directly if you have more questions so
we can resolve this today.

[sky, 2017-03-22 17:34:12]

Thanks again for your patience.

I'm probably missing something, but why do have code like:

std::move(task).Run()?
Isn't task.Run() the same thing?

https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
File chrome/browser/after_startup_task_utils.h (right):

https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
callback_forward?

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/after_s...
File chrome/browser/after_startup_task_utils.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/after_s...
chrome/browser/after_startup_task_utils.cc:64:
std::move(queued_task->task).Run();
Similar comment about needing std::move here.

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/after_s...
File chrome/browser/after_startup_task_utils_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/after_s...
chrome/browser/after_startup_task_utils_unittest.cc:64: std::move(task).Run();
?

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:43:
std::move(task).Run();
?

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/mock_user_manager.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/mock_user_manager.cc:19:
std::move(task).Run();
?

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_unittest.cc:139:
std::move(tasks_.front().second).Run();
?

https://codereview.chromium.org/2726523002/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/tests/bind_task_runner_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/bind_task_runner_unittest.cc:69:
std::move(task).Run();
Is the std::move(task) really necessary?

https://codereview.chromium.org/2726523002/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/tests/bind_task_runner_unittest.cc:97:
std::move(task).Run();
Similar comment about the need for std::move(task) here.

[gab, 2017-03-22 17:46:45]

On 2017/03/22 17:34:12, sky wrote:
> Thanks again for your patience.
> 
> I'm probably missing something, but why do have code like:
> 
> std::move(task).Run()?
> Isn't task.Run() the same thing?

base::OnceClosure::Run() requires to be invoked on an
r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74

I agree that this syntax is ugly but I'm not sure there's a better alternative
(other option is to allow base::Callback::Run to be invoked on non r-value and
destroy its state on first invocation but I think that would make things more
complicated to implement and less safe for the callers).

https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
File chrome/browser/after_startup_task_utils.h (right):

https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
On 2017/03/22 17:34:11, sky wrote:
> callback_forward?

Need full type to pass by value.

[sky, 2017-03-22 17:50:37]

On Wed, Mar 22, 2017 at 10:46 AM,  <gab@chromium.org> wrote:
> On 2017/03/22 17:34:12, sky wrote:
>> Thanks again for your patience.
>>
>> I'm probably missing something, but why do have code like:
>>
>> std::move(task).Run()?
>> Isn't task.Run() the same thing?
>
> base::OnceClosure::Run() requires to be invoked on an
>
r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74
>
> I agree that this syntax is ugly but I'm not sure there's a better
> alternative
> (other option is to allow base::Callback::Run to be invoked on non r-value
> and
> destroy its state on first invocation but I think that would make things
> more
> complicated to implement and less safe for the callers).

Oh, that is awful and I guarantee going to cause headaches. I think I
prefer destroying state on first invocation.

  -Scott
>
>
>
https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
> File chrome/browser/after_startup_task_utils.h (right):
>
>
https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
> chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
> On 2017/03/22 17:34:11, sky wrote:
>> callback_forward?
>
> Need full type to pass by value.
>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sky, 2017-03-22 17:50:37]

On Wed, Mar 22, 2017 at 10:46 AM,  <gab@chromium.org> wrote:
> On 2017/03/22 17:34:12, sky wrote:
>> Thanks again for your patience.
>>
>> I'm probably missing something, but why do have code like:
>>
>> std::move(task).Run()?
>> Isn't task.Run() the same thing?
>
> base::OnceClosure::Run() requires to be invoked on an
>
r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74
>
> I agree that this syntax is ugly but I'm not sure there's a better
> alternative
> (other option is to allow base::Callback::Run to be invoked on non r-value
> and
> destroy its state on first invocation but I think that would make things
> more
> complicated to implement and less safe for the callers).

Oh, that is awful and I guarantee going to cause headaches. I think I
prefer destroying state on first invocation.

  -Scott
>
>
>
https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
> File chrome/browser/after_startup_task_utils.h (right):
>
>
https://codereview.chromium.org/2726523002/diff/160001/chrome/browser/after_s...
> chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
> On 2017/03/22 17:34:11, sky wrote:
>> callback_forward?
>
> Need full type to pass by value.
>
> https://codereview.chromium.org/2726523002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-22 17:56:43]

On Wed, Mar 22, 2017 at 1:50 PM, Scott Violet <sky@chromium.org> wrote:

> On Wed, Mar 22, 2017 at 10:46 AM,  <gab@chromium.org> wrote:
> > On 2017/03/22 17:34:12, sky wrote:
> >> Thanks again for your patience.
> >>
> >> I'm probably missing something, but why do have code like:
> >>
> >> std::move(task).Run()?
> >> Isn't task.Run() the same thing?
> >
> > base::OnceClosure::Run() requires to be invoked on an
> > r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=
> fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74
> >
> > I agree that this syntax is ugly but I'm not sure there's a better
> > alternative
> > (other option is to allow base::Callback::Run to be invoked on non
> r-value
> > and
> > destroy its state on first invocation but I think that would make things
> > more
> > complicated to implement and less safe for the callers).
>
> Oh, that is awful and I guarantee going to cause headaches. I think I
> prefer destroying state on first invocation.
>

It's how google internal callbacks as well as future STL things will work.
The good news is if you don't move the thing you're running a method on it
won't compile cuz it requires an rvalue. What headaches do you envision?


>
>   -Scott
> >
> >
> > https://codereview.chromium.org/2726523002/diff/160001/
> chrome/browser/after_startup_task_utils.h
> > File chrome/browser/after_startup_task_utils.h (right):
> >
> > https://codereview.chromium.org/2726523002/diff/160001/
> chrome/browser/after_startup_task_utils.h#newcode9
> > chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
> > On 2017/03/22 17:34:11, sky wrote:
> >> callback_forward?
> >
> > Need full type to pass by value.
> >
> > https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-22 17:56:43]

On Wed, Mar 22, 2017 at 1:50 PM, Scott Violet <sky@chromium.org> wrote:

> On Wed, Mar 22, 2017 at 10:46 AM,  <gab@chromium.org> wrote:
> > On 2017/03/22 17:34:12, sky wrote:
> >> Thanks again for your patience.
> >>
> >> I'm probably missing something, but why do have code like:
> >>
> >> std::move(task).Run()?
> >> Isn't task.Run() the same thing?
> >
> > base::OnceClosure::Run() requires to be invoked on an
> > r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=
> fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74
> >
> > I agree that this syntax is ugly but I'm not sure there's a better
> > alternative
> > (other option is to allow base::Callback::Run to be invoked on non
> r-value
> > and
> > destroy its state on first invocation but I think that would make things
> > more
> > complicated to implement and less safe for the callers).
>
> Oh, that is awful and I guarantee going to cause headaches. I think I
> prefer destroying state on first invocation.
>

It's how google internal callbacks as well as future STL things will work.
The good news is if you don't move the thing you're running a method on it
won't compile cuz it requires an rvalue. What headaches do you envision?


>
>   -Scott
> >
> >
> > https://codereview.chromium.org/2726523002/diff/160001/
> chrome/browser/after_startup_task_utils.h
> > File chrome/browser/after_startup_task_utils.h (right):
> >
> > https://codereview.chromium.org/2726523002/diff/160001/
> chrome/browser/after_startup_task_utils.h#newcode9
> > chrome/browser/after_startup_task_utils.h:9: #include "base/callback.h"
> > On 2017/03/22 17:34:11, sky wrote:
> >> callback_forward?
> >
> > Need full type to pass by value.
> >
> > https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[gab, 2017-03-22 18:00:58]

On 2017/03/22 17:56:43, danakj wrote:
> On Wed, Mar 22, 2017 at 1:50 PM, Scott Violet
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=sky@chromium.org> wrote:
> 
> > On Wed, Mar 22, 2017 at 10:46 AM, 
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> > > On 2017/03/22 17:34:12, sky wrote:
> > >> Thanks again for your patience.
> > >>
> > >> I'm probably missing something, but why do have code like:
> > >>
> > >> std::move(task).Run()?
> > >> Isn't task.Run() the same thing?
> > >
> > > base::OnceClosure::Run() requires to be invoked on an
> > > r-value:https://cs.chromium.org/chromium/src/base/callback.h?rcl=
> > fa29809ee94916e79cd5e25060cd03ed3b1bb46f&l=74
> > >
> > > I agree that this syntax is ugly but I'm not sure there's a better
> > > alternative
> > > (other option is to allow base::Callback::Run to be invoked on non
> > r-value
> > > and
> > > destroy its state on first invocation but I think that would make things
> > > more
> > > complicated to implement and less safe for the callers).
> >
> > Oh, that is awful and I guarantee going to cause headaches. I think I
> > prefer destroying state on first invocation.
> >
> 
> It's how google internal callbacks as well as future STL things will work.
> The good news is if you don't move the thing you're running a method on it
> won't compile cuz it requires an rvalue. What headaches do you envision?

Right and the compile error isn't cryptic either (hits the aforementioned
static_assert:
https://cs.chromium.org/chromium/src/base/callback.h?q=std::move%5C(.*%5C)%5C...).

FWIW, this isn't brand new in this CL either, we already have ~100 such
invocations in trunk:
https://cs.chromium.org/search/?q=std::move%5C(.*%5C)%5C.Run%5C(%5C)&sq=packa...

[ccameron, 2017-03-22 18:21:01]

ui/accelerated_widget_mac lgtm

[brettw, 2017-03-22 19:10:32]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-03-22 19:10:34]

I'm not wild about the Run syntax, but I do think it's better to use the
language to check this kind of thing at compile time than have run time checks
that something is done only once. So I'm OK with the concept of this change. I
didn't check the details.

[sky, 2017-03-22 20:22:03]

Ok, LGTM. Thanks again for taking the time to explain it.

[tzik, 2017-03-23 12:52:18]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 12:52:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-23 13:31:15]

tzik@chromium.org changed reviewers:
+ engedy@chromium.org, shrike@chromium.org, xiyuan@chromium.org

[tzik, 2017-03-23 13:31:17]

Thanks gab and dana for driving the discussion!

Adding other owners who I forgot to ask them review. PTAL to:
shrike: //components/memory_pressure
engedy: //components/subresource_filter
xiyuan: //components/fake_user_manager

[engedy, 2017-03-23 14:09:26]

//components/subresource_filter LGTM % nit, thanks!

https://codereview.chromium.org/2726523002/diff/200001/components/subresource...
File
components/subresource_filter/content/browser/content_ruleset_service_unittest.cc
(right):

https://codereview.chromium.org/2726523002/diff/200001/components/subresource...
components/subresource_filter/content/browser/content_ruleset_service_unittest.cc:16:
#include "base/callback_helpers.h"
nit: I think this isn't actually used here.

[commit-bot: I haz the power, 2017-03-23 14:49:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 14:49:23]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-23 15:19:11]

user_manager lgtm

[tzik, 2017-03-24 05:54:15]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 05:54:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 07:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 07:51:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tzik, 2017-03-24 09:25:44]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[tzik, 2017-03-24 09:25:55]

https://codereview.chromium.org/2726523002/diff/200001/components/subresource...
File
components/subresource_filter/content/browser/content_ruleset_service_unittest.cc
(right):

https://codereview.chromium.org/2726523002/diff/200001/components/subresource...
components/subresource_filter/content/browser/content_ruleset_service_unittest.cc:16:
#include "base/callback_helpers.h"
On 2017/03/23 14:09:26, engedy wrote:
> nit: I think this isn't actually used here.

Removed. Right, it was needed by ResetAndReturn in the previous CL and is no
longer needed.

[commit-bot: I haz the power, 2017-03-24 09:25:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 09:33:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 09:34:00]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2017-03-24 09:46:15]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 09:46:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 11:17:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 11:17:59]

Dry run: This issue passed the CQ dry run.

[gab, 2017-03-24 15:56:08]

Just peeked at remaining files lacking reviews:

components/memory_pressure/memory_pressure_monitor_unittest.cc
remoting/
ios/

lgtm % comment, feel free to TBR remaining owners to expedite this change.

https://codereview.chromium.org/2726523002/diff/220001/components/memory_pres...
File components/memory_pressure/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/220001/components/memory_pres...
components/memory_pressure/memory_pressure_monitor_unittest.cc:46: return
PostDelayedTask(from_here, &task, delay);
Passing a pointer to a local variable to a mock method is unexpected IMO.

Why can't we move it? The only EXPECT_CALL entry below that cares about
PostDelayedTask() doesn't verify what's passed in anyways, we could even drop
the param and it would work I think.

[tzik, 2017-03-28 04:41:51]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 04:42:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-28 04:48:54]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 04:49:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-28 05:59:15]

https://codereview.chromium.org/2726523002/diff/220001/components/memory_pres...
File components/memory_pressure/memory_pressure_monitor_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/220001/components/memory_pres...
components/memory_pressure/memory_pressure_monitor_unittest.cc:46: return
PostDelayedTask(from_here, &task, delay);
On 2017/03/24 15:56:08, gab wrote:
> Passing a pointer to a local variable to a mock method is unexpected IMO.
> 
> Why can't we move it? The only EXPECT_CALL entry below that cares about
> PostDelayedTask() doesn't verify what's passed in anyways, we could even drop
> the param and it would work I think.

Removed the Closure parameter.

This couldn't move since MOCK_METHODs don't support move only types. I think
passing the pointer of a local variable was safe as far as the handler doesn't
save the pointee, as I do in startup_task_runner_unittest.cc too.

[tzik, 2017-03-28 06:11:41]

On 2017/03/24 15:56:08, gab wrote:
> Just peeked at remaining files lacking reviews:
> 
> components/memory_pressure/memory_pressure_monitor_unittest.cc
> remoting/
> ios/
> 
> lgtm % comment, feel free to TBR remaining owners to expedite this change.
> 

Let me ping and wait one more day before TBRing.

shrike, wez, sdefresne: Ping. Could you PTAL to //components/memory_pressure,
//remoting, //ios respectively?

[Wez, 2017-03-28 06:40:22]

remoting/ LGTM

[commit-bot: I haz the power, 2017-03-28 06:49:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 06:49:26]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-03-28 08:46:50]

ios/ lgtm

[danakj, 2017-03-28 15:09:48]

On Tue, Mar 28, 2017 at 1:59 AM, <tzik@chromium.org> wrote:

>
> https://codereview.chromium.org/2726523002/diff/220001/
> components/memory_pressure/memory_pressure_monitor_unittest.cc
> File components/memory_pressure/memory_pressure_monitor_unittest.cc
> (right):
>
> https://codereview.chromium.org/2726523002/diff/220001/
> components/memory_pressure/memory_pressure_monitor_unittest.cc#newcode46
> components/memory_pressure/memory_pressure_monitor_unittest.cc:46:
> return PostDelayedTask(from_here, &task, delay);
> On 2017/03/24 15:56:08, gab wrote:
> > Passing a pointer to a local variable to a mock method is unexpected
> IMO.
> >
> > Why can't we move it? The only EXPECT_CALL entry below that cares
> about
> > PostDelayedTask() doesn't verify what's passed in anyways, we could
> even drop
> > the param and it would work I think.
>
> Removed the Closure parameter.
>
> This couldn't move since MOCK_METHODs don't support move only types. I
> think passing the pointer of a local variable was safe as far as the
> handler doesn't save the pointee, as I do in
> startup_task_runner_unittest.cc too.
>

You can deal with mock methods like this:
https://cs.chromium.org/chromium/src/cc/output/gl_renderer_unittest.cc?rcl=92...
Then you can still have the same move-only APIs.


>
> https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-28 15:10:39]

On Tue, Mar 28, 2017 at 1:59 AM, <tzik@chromium.org> wrote:

>
> https://codereview.chromium.org/2726523002/diff/220001/
> components/memory_pressure/memory_pressure_monitor_unittest.cc
> File components/memory_pressure/memory_pressure_monitor_unittest.cc
> (right):
>
> https://codereview.chromium.org/2726523002/diff/220001/
> components/memory_pressure/memory_pressure_monitor_unittest.cc#newcode46
> components/memory_pressure/memory_pressure_monitor_unittest.cc:46:
> return PostDelayedTask(from_here, &task, delay);
> On 2017/03/24 15:56:08, gab wrote:
> > Passing a pointer to a local variable to a mock method is unexpected
> IMO.
> >
> > Why can't we move it? The only EXPECT_CALL entry below that cares
> about
> > PostDelayedTask() doesn't verify what's passed in anyways, we could
> even drop
> > the param and it would work I think.
>
> Removed the Closure parameter.
>
> This couldn't move since MOCK_METHODs don't support move only types. I
> think passing the pointer of a local variable was safe as far as the
> handler doesn't save the pointee, as I do in
> startup_task_runner_unittest.cc too.
>

You can deal with mock methods like this:
https://cs.chromium.org/chromium/src/cc/output/gl_renderer_unittest.cc?rcl=92...
Then you can still have the same move-only APIs.


>
> https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2017-03-28 15:29:52]

https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
File content/browser/startup_task_runner_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
content/browser/startup_task_runner_unittest.cc:35: bool
SaveTaskArg(base::Closure* arg) {
Ouch, that was already an ugly hack and now it's even worse...

How about capturing |task| in PostNonNestableDelayedTask below beforing invoking
the mock (and not have the mock even take a Closure parameter if it doesn't care
about it)?

Also, I don't see a content/ reviewer on this CL? Had anyone looked at this?

[gab, 2017-03-28 15:32:25]

On 2017/03/28 15:10:39, danakj wrote:
> On Tue, Mar 28, 2017 at 1:59 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tzik@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2726523002/diff/220001/
> > components/memory_pressure/memory_pressure_monitor_unittest.cc
> > File components/memory_pressure/memory_pressure_monitor_unittest.cc
> > (right):
> >
> > https://codereview.chromium.org/2726523002/diff/220001/
> > components/memory_pressure/memory_pressure_monitor_unittest.cc#newcode46
> > components/memory_pressure/memory_pressure_monitor_unittest.cc:46:
> > return PostDelayedTask(from_here, &task, delay);
> > On 2017/03/24 15:56:08, gab wrote:
> > > Passing a pointer to a local variable to a mock method is unexpected
> > IMO.
> > >
> > > Why can't we move it? The only EXPECT_CALL entry below that cares
> > about
> > > PostDelayedTask() doesn't verify what's passed in anyways, we could
> > even drop
> > > the param and it would work I think.
> >
> > Removed the Closure parameter.
> >
> > This couldn't move since MOCK_METHODs don't support move only types. I
> > think passing the pointer of a local variable was safe as far as the
> > handler doesn't save the pointee, as I do in
> > startup_task_runner_unittest.cc too.
> >
> 
> You can deal with mock methods like this:
>
https://cs.chromium.org/chromium/src/cc/output/gl_renderer_unittest.cc?rcl=92...
> Then you can still have the same move-only APIs.

Ah that's good to know, but in this case the mock methods weren't even consuming
the Closure (and aren't overrides so aren't forced to take it), might as well
not pass it..?

> 
> 
> >
> > https://codereview.chromium.org/2726523002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[danakj, 2017-03-28 15:37:48]

On Tue, Mar 28, 2017 at 11:32 AM, <gab@chromium.org> wrote:

> On 2017/03/28 15:10:39, danakj wrote:
> > On Tue, Mar 28, 2017 at 1:59 AM,
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tzik@chromium.org&gt;
> wrote:
> >
> > >
> > > https://codereview.chromium.org/2726523002/diff/220001/
> > > components/memory_pressure/memory_pressure_monitor_unittest.cc
> > > File components/memory_pressure/memory_pressure_monitor_unittest.cc
> > > (right):
> > >
> > > https://codereview.chromium.org/2726523002/diff/220001/
> > > components/memory_pressure/memory_pressure_monitor_unittest.
> cc#newcode46
> > > components/memory_pressure/memory_pressure_monitor_unittest.cc:46:
> > > return PostDelayedTask(from_here, &task, delay);
> > > On 2017/03/24 15:56:08, gab wrote:
> > > > Passing a pointer to a local variable to a mock method is unexpected
> > > IMO.
> > > >
> > > > Why can't we move it? The only EXPECT_CALL entry below that cares
> > > about
> > > > PostDelayedTask() doesn't verify what's passed in anyways, we could
> > > even drop
> > > > the param and it would work I think.
> > >
> > > Removed the Closure parameter.
> > >
> > > This couldn't move since MOCK_METHODs don't support move only types. I
> > > think passing the pointer of a local variable was safe as far as the
> > > handler doesn't save the pointee, as I do in
> > > startup_task_runner_unittest.cc too.
> > >
> >
> > You can deal with mock methods like this:
> >
> https://cs.chromium.org/chromium/src/cc/output/gl_renderer_
> unittest.cc?rcl=92255dae474d7290f2fed43c03eeaea1258918d6&l=1538
> > Then you can still have the same move-only APIs.
>
> Ah that's good to know, but in this case the mock methods weren't even
> consuming
> the Closure (and aren't overrides so aren't forced to take it), might as
> well
> not pass it..?
>

Oh, ya sure :)


> >
> >
> > >
> > > https://codereview.chromium.org/2726523002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-
> reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-28 15:38:34]

On Tue, Mar 28, 2017 at 11:32 AM, <gab@chromium.org> wrote:

> On 2017/03/28 15:10:39, danakj wrote:
> > On Tue, Mar 28, 2017 at 1:59 AM,
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=tzik@chromium.org&gt;
> wrote:
> >
> > >
> > > https://codereview.chromium.org/2726523002/diff/220001/
> > > components/memory_pressure/memory_pressure_monitor_unittest.cc
> > > File components/memory_pressure/memory_pressure_monitor_unittest.cc
> > > (right):
> > >
> > > https://codereview.chromium.org/2726523002/diff/220001/
> > > components/memory_pressure/memory_pressure_monitor_unittest.
> cc#newcode46
> > > components/memory_pressure/memory_pressure_monitor_unittest.cc:46:
> > > return PostDelayedTask(from_here, &task, delay);
> > > On 2017/03/24 15:56:08, gab wrote:
> > > > Passing a pointer to a local variable to a mock method is unexpected
> > > IMO.
> > > >
> > > > Why can't we move it? The only EXPECT_CALL entry below that cares
> > > about
> > > > PostDelayedTask() doesn't verify what's passed in anyways, we could
> > > even drop
> > > > the param and it would work I think.
> > >
> > > Removed the Closure parameter.
> > >
> > > This couldn't move since MOCK_METHODs don't support move only types. I
> > > think passing the pointer of a local variable was safe as far as the
> > > handler doesn't save the pointee, as I do in
> > > startup_task_runner_unittest.cc too.
> > >
> >
> > You can deal with mock methods like this:
> >
> https://cs.chromium.org/chromium/src/cc/output/gl_renderer_
> unittest.cc?rcl=92255dae474d7290f2fed43c03eeaea1258918d6&l=1538
> > Then you can still have the same move-only APIs.
>
> Ah that's good to know, but in this case the mock methods weren't even
> consuming
> the Closure (and aren't overrides so aren't forced to take it), might as
> well
> not pass it..?
>

Oh, ya sure :)


> >
> >
> > >
> > > https://codereview.chromium.org/2726523002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-
> reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2726523002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tzik, 2017-03-28 17:31:58]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 17:32:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-03-28 17:34:21]

https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
File content/browser/startup_task_runner_unittest.cc (right):

https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
content/browser/startup_task_runner_unittest.cc:35: bool
SaveTaskArg(base::Closure* arg) {
On 2017/03/28 15:29:51, gab wrote:
> Ouch, that was already an ugly hack and now it's even worse...
> 
> How about capturing |task| in PostNonNestableDelayedTask below beforing
invoking
> the mock (and not have the mock even take a Closure parameter if it doesn't
care
> about it)?
> 
> Also, I don't see a content/ reviewer on this CL? Had anyone looked at this?

Done.

Yes, //content is covered by kinuko's review.

[commit-bot: I haz the power, 2017-03-28 17:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 17:46:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tzik, 2017-03-28 18:26:40]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:27:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 18:47:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 18:47:40]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tzik, 2017-03-28 18:51:55]

The CQ bit was checked by tzik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:52:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 19:09:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 19:09:41]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[gab, 2017-03-28 19:36:30]

On 2017/03/28 17:34:21, tzik wrote:
>
https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
> File content/browser/startup_task_runner_unittest.cc (right):
> 
>
https://codereview.chromium.org/2726523002/diff/260001/content/browser/startu...
> content/browser/startup_task_runner_unittest.cc:35: bool
> SaveTaskArg(base::Closure* arg) {
> On 2017/03/28 15:29:51, gab wrote:
> > Ouch, that was already an ugly hack and now it's even worse...
> > 
> > How about capturing |task| in PostNonNestableDelayedTask below beforing
> invoking
> > the mock (and not have the mock even take a Closure parameter if it doesn't
> care
> > about it)?
> > 
> > Also, I don't see a content/ reviewer on this CL? Had anyone looked at this?
> 
> Done.

Thanks, lgtm++!

> 
> Yes, //content is covered by kinuko's review.

Oops, missed that :)

[tzik, 2017-03-29 04:12:33]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[tzik, 2017-03-29 04:16:47]

TBRing shrike@. PTAL to //components/memory_pressure later.

[tzik, 2017-03-29 04:17:02]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-03-29 04:17:03]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
vmpstr@chromium.org, oshima@chromium.org, kinuko@chromium.org,
alexclarke@chromium.org, xhwang@chromium.org, mmenke@chromium.org,
ccameron@chromium.org, sky@chromium.org, xiyuan@chromium.org,
engedy@chromium.org, wez@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2726523002/#ps280001
(title: "erase Closure*")

[commit-bot: I haz the power, 2017-03-29 04:17:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 05:28:19]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1490761022275180,
"parent_rev": "4cc1dcc212f8b3ae78d6aaa3f13aa62116d8632e", "commit_rev":
"070c8ffb44d04075bd7400691a2325cb6233558a"}

[commit-bot: I haz the power, 2017-03-29 05:29:46]

Description was changed from

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2726523002
Cr-Commit-Position: refs/heads/master@{#460288}
Committed:
https://chromium.googlesource.com/chromium/src/+/070c8ffb44d04075bd7400691a23...
==========

[commit-bot: I haz the power, 2017-03-29 05:29:50]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/070c8ffb44d04075bd7400691a23...

[shrike, 2017-03-29 16:12:37]

On 2017/03/29 04:16:47, tzik wrote:
> TBRing shrike@. PTAL to //components/memory_pressure later.

It's only the memory_pressure_monitor_unittest.cc changes, correct?

[gab, 2017-03-29 16:36:27]

On 2017/03/29 16:12:37, shrike wrote:
> On 2017/03/29 04:16:47, tzik wrote:
> > TBRing shrike@. PTAL to //components/memory_pressure later.
> 
> It's only the memory_pressure_monitor_unittest.cc changes, correct?

Yes.

[shrike, 2017-03-29 18:54:40]

On 2017/03/29 16:36:27, gab wrote:
> Yes.

OK, lgtm.

Please pardon my ignorance, but how does the underscore work/what does it do in
PostDelayedTask(_, delay)? I've never seen that construct before.

[gab, 2017-03-29 19:12:10]

On 2017/03/29 18:54:40, shrike wrote:
> On 2017/03/29 16:36:27, gab wrote:
> > Yes.
> 
> OK, lgtm.
> 
> Please pardon my ignorance, but how does the underscore work/what does it do
in
> PostDelayedTask(_, delay)? I've never seen that construct before.

https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...