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

base/task_scheduler/sequence.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_TASK_SCHEDULER_SEQUENCE_H_
 #define BASE_TASK_SCHEDULER_SEQUENCE_H_
 
 #include <stddef.h>
 
 #include <memory>
 #include <queue>
 
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequence_token.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence_sort_key.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_traits.h"
 
 namespace base {
 namespace internal {
 
-// A sequence holds tasks that must be executed in posting order.
+// A Sequence holds tasks that must be executed in posting order.
 //
 // Note: there is a known refcounted-ownership cycle in the Scheduler
 // architecture: Sequence -> Task -> TaskRunner -> Sequence -> ...
 // This is okay so long as the other owners of Sequence (PriorityQueue and
 // SchedulerWorker in alternation and
 // SchedulerWorkerPoolImpl::SchedulerWorkerDelegateImpl::GetWork()
 // temporarily) keep running it (and taking Tasks from it as a result). A
 // dangling reference cycle would only occur should they release their reference
 // to it while it's not empty. In other words, it is only correct for them to
 // release it after PopTask() returns false to indicate it was made empty by
 // that call (in which case the next PushTask() will return true to indicate to
 // the caller that the Sequence should be re-enqueued for execution).
 //
 // This class is thread-safe.
 class BASE_EXPORT Sequence : public RefCountedThreadSafe<Sequence> {
  public:
   Sequence();
 
-  // Adds |task| at the end of the sequence's queue. Returns true if the
-  // sequence was empty before this operation.
+  // Adds |task| in a new slot at the end of the Sequence. Returns true if the
+  // Sequence contained no slot before this operation.

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

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.

[fdoray, 2016/10/07 20:30:30]

Done.

   bool PushTask(std::unique_ptr<Task> task);
 
-  // Returns the task in front of the sequence's queue, if any.
-  const Task* PeekTask() const;
+  // Transfers ownership of the Task in the front slot of the Sequence to the
+  // caller. If this returns nullptr, the front slot of the Sequence was empty
+  // or the Sequence contained no slot before this operation. If this returns a
+  // Task, the front slot of the Sequence is empty immediately after this

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

, the front slot of the Sequence will be empty until RemoveFrontSlot() is
invoked.

[fdoray, 2016/10/07 20:30:30]

Done.

+  // operation.
+  std::unique_ptr<Task> TakeTask();
 
-  // Removes the task in front of the sequence's queue. Returns true if the
-  // sequence is empty after this operation. Cannot be called on an empty
-  // sequence.
-  bool PopTask();
+  // Returns the TaskTraits of the Task in front of the Sequence. Cannot be
+  // called on an empty Sequence or on a Sequence whose front slot is empty.
+  TaskTraits GetFrontTaskTraits() const;

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

PeekTaskTraits()?

[fdoray, 2016/10/07 20:30:30]

Done.

 
-  // Returns a SequenceSortKey representing the priority of the sequence. Cannot
-  // be called on an empty sequence.
+  // Removes the slot in front of the Sequence. The front slot must have been
+  // emptied by TakeTask() before this is called. Cannot be called on a Sequence
+  // that contains no slot. Returns true if the Sequence contains no slot
+  // immediately after this operation.
+  bool RemoveFrontSlot();

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

I prefer Pop() here, documentation can highlight what it means and why it's void
(well bool).

[fdoray, 2016/10/07 20:30:30]

Done.

+
+  // Returns a SequenceSortKey representing the priority of the Sequence. Cannot
+  // be called on an empty Sequence.
   SequenceSortKey GetSortKey() const;
 
   // Returns a token that uniquely identifies this Sequence.
   const SequenceToken& token() const { return token_; }
 
  private:
   friend class RefCountedThreadSafe<Sequence>;
   ~Sequence();
 
   const SequenceToken token_ = SequenceToken::Create();
 
   // Synchronizes access to all members.
   mutable SchedulerLock lock_;
 
   // Queue of tasks to execute.
   std::queue<std::unique_ptr<Task>> queue_;
 
-  // Number of tasks contained in the sequence for each priority.
+  // Number of tasks contained in the Sequence for each priority.
   size_t num_tasks_per_priority_[static_cast<int>(TaskPriority::HIGHEST) + 1] =
       {};
 
   DISALLOW_COPY_AND_ASSIGN(Sequence);
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_SEQUENCE_H_