TaskScheduler: Don't use a callback to post a task from TaskTracker.

TaskScheduler: Don't use a callback to post a task from TaskTracker.

Replace the existing TaskTracker::PostTask() method (which runs a
callback when it allows a task to be posted) with a
TaskTracker::WillPostTask() method (which returns true when it allows
a task to be posted). The caller is responsible for posting the task
when the new method returns true.

BUG=553459
Committed: https://crrev.com/7eef862850afcd443ea44fdebeb286b2b35d4ca2
Cr-Commit-Position: refs/heads/master@{#386072}

[fdoray, 2016-04-06 19:32:20]

fdoray@chromium.org changed reviewers:
+ danakj@chromium.org, gab@chromium.org, robliao@chromium.org

[fdoray, 2016-04-06 19:32:20]

danakj@: Can you review this CL? Thanks.

[danakj, 2016-04-06 19:38:30]

LGTM

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_tracker_unittest.cc (right):

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:132: // Post the task.
update comment

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:177: // Post a task.
update comment?

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:181: // Post a BLOCK_SHUTDOWN task
just to block shutdown.
and here, "Post" isn't strictly right.

[robliao, 2016-04-06 19:42:33]

lgtm + comments

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_tracker_unittest.cc (right):

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:132: // Post the task.
On 2016/04/06 19:38:30, danakj wrote:
> update comment

Maybe update the test names too.

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:206: // Post a task.
Same here.

[fdoray, 2016-04-06 20:59:27]

All done. I'll wait until https://codereview.chromium.org/1708773002/ is LGTMed
to land this CL.

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
File base/task_scheduler/task_tracker_unittest.cc (right):

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:132: // Post the task.
On 2016/04/06 19:42:33, robliao wrote:
> On 2016/04/06 19:38:30, danakj wrote:
> > update comment
> 
> Maybe update the test names too.

Done.

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:177: // Post a task.
On 2016/04/06 19:38:30, danakj wrote:
> update comment?

Done.

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:181: // Post a BLOCK_SHUTDOWN task
just to block shutdown.
On 2016/04/06 19:38:30, danakj wrote:
> and here, "Post" isn't strictly right.

Done.

https://codereview.chromium.org/1862113002/diff/1/base/task_scheduler/task_tr...
base/task_scheduler/task_tracker_unittest.cc:206: // Post a task.
On 2016/04/06 19:42:33, robliao wrote:
> Same here.

Done.

[gab, 2016-04-07 17:05:18]

lgtm w/ nits

Not a fan of having half of the API be notification based (tell me when you do
stuff) and the other half be action based (give me the task and I'll run it),
but given it's only an internal API and there is a performance reason to do it
this way (avoids an extra Bind()) it makes sense to go with this approach.

Thanks,
Gab

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:68:
task_annotator.DidQueueTask(kQueueFunctionName, *task);
Not sure how big of a refactor that would be but it really feels like this
should also be WillPostTask or WillQueueTask, it feels wrong to call
DidQueueTask from WillPostTask... (not something for this CL but...).

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:73: void TaskTracker::RunTask(const Task*
task) {
I think I'd asked that on the initial TaskTracker CL and forget the answer, but
shouldn't this take a std::uniqueptr<Task>? Why would the caller want to retain
ownership?

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:39: // this operation is allowed (|task|
should otherwise not be posted).
s/(|task| should otherwise not be posted)/(|task| should be posted
if-and-only-if it is)/

i.e. also highlight that it *must* be posted if this returns true

[fdoray, 2016-04-07 18:05:58]

gab@: Done.

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:68:
task_annotator.DidQueueTask(kQueueFunctionName, *task);
On 2016/04/07 17:05:18, gab wrote:
> Not sure how big of a refactor that would be but it really feels like this
> should also be WillPostTask or WillQueueTask, it feels wrong to call
> DidQueueTask from WillPostTask... (not something for this CL but...).

It's used at 4 other places. Sometimes it's called before the task is queued,
sometimes it's called after... It should be easy to rename it and always call it
before queuing the task.

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:73: void TaskTracker::RunTask(const Task*
task) {
On 2016/04/07 17:05:17, gab wrote:
> I think I'd asked that on the initial TaskTracker CL and forget the answer,
but
> shouldn't this take a std::uniqueptr<Task>? Why would the caller want to
retain
> ownership?

The Task is owned by its Sequence when it runs.

If we wanted to be sure that a Task doesn't run twice, we could have
TaskTracker::RunTask(scoped_refptr<Sequence> sequence) which runs the next task
in |sequence| and pops it (I would prefer no to add a TaskTracker->Sequence
dependency).

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.h (right):

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.h:39: // this operation is allowed (|task|
should otherwise not be posted).
On 2016/04/07 17:05:18, gab wrote:
> s/(|task| should otherwise not be posted)/(|task| should be posted
> if-and-only-if it is)/
> 
> i.e. also highlight that it *must* be posted if this returns true

Done.

[gab, 2016-04-07 18:58:57]

lgtm++

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
File base/task_scheduler/task_tracker.cc (right):

https://codereview.chromium.org/1862113002/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker.cc:73: void TaskTracker::RunTask(const Task*
task) {
On 2016/04/07 18:05:58, fdoray wrote:
> On 2016/04/07 17:05:17, gab wrote:
> > I think I'd asked that on the initial TaskTracker CL and forget the answer,
> but
> > shouldn't this take a std::uniqueptr<Task>? Why would the caller want to
> retain
> > ownership?
> 
> The Task is owned by its Sequence when it runs.
> 
> If we wanted to be sure that a Task doesn't run twice, we could have
> TaskTracker::RunTask(scoped_refptr<Sequence> sequence) which runs the next
task
> in |sequence| and pops it (I would prefer no to add a TaskTracker->Sequence
> dependency).

Ah right, ok let's keep it this way then.

[fdoray, 2016-04-08 01:03:22]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-08 01:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/1862113002/#ps40001
(title: "CR gab #6 (clarify WillPostTask comment)")

[commit-bot: I haz the power, 2016-04-08 01:03:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862113002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862113002/40001

[commit-bot: I haz the power, 2016-04-08 01:06:55]

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

[commit-bot: I haz the power, 2016-04-08 01:06:56]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fdoray, 2016-04-08 01:38:41]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-08 01:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
robliao@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1862113002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-08 01:39:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862113002/60001

[commit-bot: I haz the power, 2016-04-08 01:44:54]

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

[commit-bot: I haz the power, 2016-04-08 01:44:55]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[fdoray, 2016-04-08 14:23:28]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-04-08 14:23:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862113002/60001

[commit-bot: I haz the power, 2016-04-08 14:51:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-08 14:53:20]

Description was changed from

==========
TaskScheduler: Don't use a callback to post a task from TaskTracker.

Replace the existing TaskTracker::PostTask() method (which runs a
callback when it allows a task to be posted) with a
TaskTracker::WillPostTask() method (which returns true when it allows
a task to be posted). The caller is responsible for posting the task
when the new method returns true.

BUG=553459
==========

to

==========
TaskScheduler: Don't use a callback to post a task from TaskTracker.

Replace the existing TaskTracker::PostTask() method (which runs a
callback when it allows a task to be posted) with a
TaskTracker::WillPostTask() method (which returns true when it allows
a task to be posted). The caller is responsible for posting the task
when the new method returns true.

BUG=553459

Committed: https://crrev.com/7eef862850afcd443ea44fdebeb286b2b35d4ca2
Cr-Commit-Position: refs/heads/master@{#386072}
==========

[commit-bot: I haz the power, 2016-04-08 14:53:21]

Patchset 4 (id:??) landed as
https://crrev.com/7eef862850afcd443ea44fdebeb286b2b35d4ca2
Cr-Commit-Position: refs/heads/master@{#386072}