TaskScheduler: Change Sequence::PeekTask to Sequence::TakeTask.

TaskScheduler: Change Sequence::PeekTask to Sequence::TakeTask.

Sequence::TakeTask() transfers ownership of the Task in the front slot of
the Sequence to the caller. This makes the front slot empty. The empty
front slot can be removed with Sequence::RemoveFrontSlot().

Benefits of transfering ownership of a Task to code that runs it:
- The code that runs (and owns) the Task can modify it. This is
  important if we want to use OnceClosure in TaskScheduler.
- It is now clear that a single thread has a pointer to a given
  Task at a time (before, multiple threads calling PeekTask on
  the same Sequence could theoretically get pointers to the same
  Task).

BUG=553459
Committed: https://crrev.com/0c97aa3cb8c7710bdbc7d22c260a4d871f41e264
Cr-Commit-Position: refs/heads/master@{#424719}

[fdoray, 2016-10-07 19:21:35]

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

[fdoray, 2016-10-07 19:21:36]

PTAL

[gab, 2016-10-07 19:51:08]

Awesome :-) -- I guess we'll keep the modification of TaskTracker to fix
shutdown ordering for a follow-up CL?

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.cc:32: if (queue_.empty() || !queue_.front())
I don't think we should allow calling TakeTask() when front slot is empty, that
seems like misuse. i.e. When TakeTask() is called there should only be a single
owner and they shouldn't call TakeTask twice without Pop().

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:45: // Sequence contained no slot before this
operation.
I still prefer to refer to the Sequence as being "empty" instead of "containing
no slot" (otherwise wording of meta-comment of this class also has to be
reworded)

To me it's clear that empty means that the underlying vector is empty. Could
even clarify that in meta comment and then just use "empty" elsewhere.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:51: // Task, the front slot of the Sequence is
empty immediately after this
, the front slot of the Sequence will be empty until RemoveFrontSlot() is
invoked.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:57: TaskTraits GetFrontTaskTraits() const;
PeekTaskTraits()?

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:63: bool RemoveFrontSlot();
I prefer Pop() here, documentation can highlight what it means and why it's void
(well bool).

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

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:82: EXPECT_NE(Action::RUN,
action_);
Add a comment around why this constructor is not suitable for tasks to be ran.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:109:
tracker_->RunTask(std::move(owned_task_), SequenceToken::Create());
#include <utility>

[fdoray, 2016-10-07 20:30:31]

Ordering of task destruction and shutdown bookkeeping will be fixed in a
separate CL.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.cc (right):

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.cc:32: if (queue_.empty() || !queue_.front())
On 2016/10/07 19:51:08, gab wrote:
> I don't think we should allow calling TakeTask() when front slot is empty,
that
> seems like misuse. i.e. When TakeTask() is called there should only be a
single
> owner and they shouldn't call TakeTask twice without Pop().

Done.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:45: // Sequence contained no slot before this
operation.
On 2016/10/07 19:51:08, gab wrote:
> I still prefer to refer to the Sequence as being "empty" instead of
"containing
> no slot" (otherwise wording of meta-comment of this class also has to be
> reworded)
> 
> To me it's clear that empty means that the underlying vector is empty. Could
> even clarify that in meta comment and then just use "empty" elsewhere.

Done.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:51: // Task, the front slot of the Sequence is
empty immediately after this
On 2016/10/07 19:51:08, gab wrote:
> , the front slot of the Sequence will be empty until RemoveFrontSlot() is
> invoked.

Done.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:57: TaskTraits GetFrontTaskTraits() const;
On 2016/10/07 19:51:08, gab wrote:
> PeekTaskTraits()?

Done.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:63: bool RemoveFrontSlot();
On 2016/10/07 19:51:08, gab wrote:
> I prefer Pop() here, documentation can highlight what it means and why it's
void
> (well bool).

Done.

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

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:82: EXPECT_NE(Action::RUN,
action_);
On 2016/10/07 19:51:08, gab wrote:
> Add a comment around why this constructor is not suitable for tasks to be ran.

Done.

https://codereview.chromium.org/2399213005/diff/20001/base/task_scheduler/tas...
base/task_scheduler/task_tracker_unittest.cc:109:
tracker_->RunTask(std::move(owned_task_), SequenceToken::Create());
On 2016/10/07 19:51:08, gab wrote:
> #include <utility>

Done.

[robliao, 2016-10-07 20:57:12]

Approach looks reasonable.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:59: virtual void DidRunTask(TaskPriority
task_priority,
Since we're no longer passing the task in, maybe a rename is in order.
DidRunTaskWithPriority?

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_unittest.cc (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_unittest.cc:239: // When this is true, the
next method called on this delegate should be
Instead: Whether the next method called on this delegate should be DidRunTask()

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:25: // A Sequence holds tasks that must be
executed in posting order.
A Sequence holds slots each containing up to a single task that must be executed
in posting order.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:27: // PushTask() adds a task in a new slot at
the end of the Sequence. TakeTask()
This duplicates the method comments below. I would remove this.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:56: // caller. The front slot of the Sequence
will be empty until Pop() is
Might be clearer to say the "front slot of the Sequence will be nullptr and
remain until Pop()"

[gab, 2016-10-07 21:10:34]

lgtm

[fdoray, 2016-10-11 12:29:19]

robliao@: PTAnL

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:59: virtual void DidRunTask(TaskPriority
task_priority,
On 2016/10/07 20:57:11, robliao wrote:
> Since we're no longer passing the task in, maybe a rename is in order.
> DidRunTaskWithPriority?

Done.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_unittest.cc (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_unittest.cc:239: // When this is true, the
next method called on this delegate should be
On 2016/10/07 20:57:11, robliao wrote:
> Instead: Whether the next method called on this delegate should be
DidRunTask()

Done.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
File base/task_scheduler/sequence.h (right):

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:25: // A Sequence holds tasks that must be
executed in posting order.
On 2016/10/07 20:57:11, robliao wrote:
> A Sequence holds slots each containing up to a single task that must be
executed
> in posting order.

Done.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:27: // PushTask() adds a task in a new slot at
the end of the Sequence. TakeTask()
On 2016/10/07 20:57:12, robliao wrote:
> This duplicates the method comments below. I would remove this.

Done.

https://codereview.chromium.org/2399213005/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence.h:56: // caller. The front slot of the Sequence
will be empty until Pop() is
On 2016/10/07 20:57:12, robliao wrote:
> Might be clearer to say the "front slot of the Sequence will be nullptr and
> remain until Pop()"

Done.

[robliao, 2016-10-11 17:12:43]

lgtm

[fdoray, 2016-10-11 17:14:03]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-10-11 17:14:04]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2399213005/#ps80001
(title: "CR robliao #5")

[commit-bot: I haz the power, 2016-10-11 17:14:39]

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, 2016-10-11 17:24:52]

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

[commit-bot: I haz the power, 2016-10-11 17:24:53]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fdoray, 2016-10-11 18:36:03]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-11 18:36:22]

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, 2016-10-11 18:47:56]

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

[commit-bot: I haz the power, 2016-10-11 18:47:57]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fdoray, 2016-10-11 19:40:30]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-11 19:40:59]

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, 2016-10-11 19:53:55]

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

[commit-bot: I haz the power, 2016-10-11 19:53:56]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fdoray, 2016-10-12 12:15:05]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-12 12:15:23]

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, 2016-10-12 12:21:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-12 12:24:07]

Description was changed from

==========
TaskScheduler: Change Sequence::PeekTask to Sequence::TakeTask.

Sequence::TakeTask() transfers ownership of the Task in the front slot of
the Sequence to the caller. This makes the front slot empty. The empty
front slot can be removed with Sequence::RemoveFrontSlot().

Benefits of transfering ownership of a Task to code that runs it:
- The code that runs (and owns) the Task can modify it. This is
  important if we want to use OnceClosure in TaskScheduler.
- It is now clear that a single thread has a pointer to a given
  Task at a time (before, multiple threads calling PeekTask on
  the same Sequence could theoretically get pointers to the same
  Task).

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

to

==========
TaskScheduler: Change Sequence::PeekTask to Sequence::TakeTask.

Sequence::TakeTask() transfers ownership of the Task in the front slot of
the Sequence to the caller. This makes the front slot empty. The empty
front slot can be removed with Sequence::RemoveFrontSlot().

Benefits of transfering ownership of a Task to code that runs it:
- The code that runs (and owns) the Task can modify it. This is
  important if we want to use OnceClosure in TaskScheduler.
- It is now clear that a single thread has a pointer to a given
  Task at a time (before, multiple threads calling PeekTask on
  the same Sequence could theoretically get pointers to the same
  Task).

BUG=553459

Committed: https://crrev.com/0c97aa3cb8c7710bdbc7d22c260a4d871f41e264
Cr-Commit-Position: refs/heads/master@{#424719}
==========

[commit-bot: I haz the power, 2016-10-12 12:24:09]

Patchset 5 (id:??) landed as
https://crrev.com/0c97aa3cb8c7710bdbc7d22c260a4d871f41e264
Cr-Commit-Position: refs/heads/master@{#424719}