[TaskScheduler]: Cleanup Task.

This CL removes redundant members in base::internal::Task:
- delay: Redundant with PendingTask::delayed_run_time
- sequenced_time: Redundant with PendingTask::queue_time (unused when sequenced_time is used).

This clean up makes Task easier to read, and slightly more lightweight.

Change-Id: I7ed16bd3458d61f98d717f2f3dd83124f9f0121f
Reviewed-on: https://chromium-review.googlesource.com/c/1448977Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629969}

base/pending_task.h

@@ -46,8 +46,12 @@ struct BASE_EXPORT PendingTask {
   // The time when the task should be run.
   base::TimeTicks delayed_run_time;
 
-  // The time at which the task was queued. Only set if the task was posted to
-  // the SequenceManager with SetAddQueueTimeToTasks(true).
+  // The time at which the task was queued. For SequenceManager tasks and
+  // TaskScheduler non-delayed tasks, this happens at post time. For
+  // TaskScheduler delayed tasks, this happens some time after the task's delay
+  // has expired. This is not set for SequenceManager tasks if
+  // SetAddQueueTimeToTasks(true) wasn't call. This defaults to a null TimeTicks
+  // if the task hasn't been inserted in a sequence yet.
   TimeTicks queue_time;
 
   // Chain of up-to-four symbols of the parent tasks which led to this one being

base/task/task_scheduler/sequence.cc

@@ -47,8 +47,8 @@ bool Sequence::Transaction::PushTask(Task task) {
   // Use CHECK instead of DCHECK to crash earlier. See http://crbug.com/711167
   // for details.
   CHECK(task.task);
-  DCHECK(task.sequenced_time.is_null());
-  task.sequenced_time = base::TimeTicks::Now();
+  DCHECK(task.queue_time.is_null());
+  task.queue_time = base::TimeTicks::Now();
 
   task.task = sequence_->traits_.shutdown_behavior() ==
                       TaskShutdownBehavior::BLOCK_SHUTDOWN
@@ -79,11 +79,9 @@ SequenceSortKey Sequence::Transaction::GetSortKey() const {
   DCHECK(!IsEmpty());
 
   // Save the sequenced time of the next task in the sequence.
-  base::TimeTicks next_task_sequenced_time =
-      sequence_->queue_.front().sequenced_time;
+  base::TimeTicks next_task_queue_time = sequence_->queue_.front().queue_time;
 
-  return SequenceSortKey(sequence_->traits_.priority(),
-                         next_task_sequenced_time);
+  return SequenceSortKey(sequence_->traits_.priority(), next_task_queue_time);
 }
 
 bool Sequence::Transaction::IsEmpty() const {

base/task/task_scheduler/sequence_unittest.cc

@@ -61,19 +61,19 @@ TEST(TaskSchedulerSequenceTest, PushTakeRemove) {
   // Take the task in front of the sequence. It should be task A.
   Optional<Task> task = sequence_transaction.TakeTask();
   ExpectMockTask(&mock_task_a, &task.value());
-  EXPECT_FALSE(task->sequenced_time.is_null());
+  EXPECT_FALSE(task->queue_time.is_null());
 
   // Remove the empty slot. Task B should now be in front.
   EXPECT_FALSE(sequence_transaction.Pop());
   task = sequence_transaction.TakeTask();
   ExpectMockTask(&mock_task_b, &task.value());
-  EXPECT_FALSE(task->sequenced_time.is_null());
+  EXPECT_FALSE(task->queue_time.is_null());
 
   // Remove the empty slot. Task C should now be in front.
   EXPECT_FALSE(sequence_transaction.Pop());
   task = sequence_transaction.TakeTask();
   ExpectMockTask(&mock_task_c, &task.value());
-  EXPECT_FALSE(task->sequenced_time.is_null());
+  EXPECT_FALSE(task->queue_time.is_null());
 
   // Remove the empty slot.
   EXPECT_FALSE(sequence_transaction.Pop());
@@ -84,13 +84,13 @@ TEST(TaskSchedulerSequenceTest, PushTakeRemove) {
   // Task D should be in front.
   task = sequence_transaction.TakeTask();
   ExpectMockTask(&mock_task_d, &task.value());
-  EXPECT_FALSE(task->sequenced_time.is_null());
+  EXPECT_FALSE(task->queue_time.is_null());
 
   // Remove the empty slot. Task E should now be in front.
   EXPECT_FALSE(sequence_transaction.Pop());
   task = sequence_transaction.TakeTask();
   ExpectMockTask(&mock_task_e, &task.value());
-  EXPECT_FALSE(task->sequenced_time.is_null());
+  EXPECT_FALSE(task->queue_time.is_null());
 
   // Remove the empty slot. The sequence should now be empty.
   EXPECT_TRUE(sequence_transaction.Pop());
@@ -116,7 +116,7 @@ TEST(TaskSchedulerSequenceTest, GetSortKeyBestEffort) {
 
   // Verify the sort key.
   EXPECT_EQ(TaskPriority::BEST_EFFORT, best_effort_sort_key.priority());
-  EXPECT_EQ(take_best_effort_task->sequenced_time,
+  EXPECT_EQ(take_best_effort_task->queue_time,
             best_effort_sort_key.next_task_sequenced_time());
 
   // Pop for correctness.
@@ -144,7 +144,7 @@ TEST(TaskSchedulerSequenceTest, GetSortKeyForeground) {
 
   // Verify the sort key.
   EXPECT_EQ(TaskPriority::USER_VISIBLE, foreground_sort_key.priority());
-  EXPECT_EQ(take_foreground_task->sequenced_time,
+  EXPECT_EQ(take_foreground_task->queue_time,
             foreground_sort_key.next_task_sequenced_time());
 
   // Pop for correctness.

base/task/task_scheduler/task.cc

@@ -23,8 +23,7 @@ Task::Task(const Location& posted_from, OnceClosure task, TimeDelta delay)
     : PendingTask(posted_from,
                   std::move(task),
                   delay.is_zero() ? TimeTicks() : TimeTicks::Now() + delay,
-                  Nestable::kNonNestable),
-      delay(delay) {
+                  Nestable::kNonNestable) {
   // TaskScheduler doesn't use |sequence_num| but tracing (toplevel.flow) relies
   // on it being unique. While this subtle dependency is a bit overreaching,
   // TaskScheduler is the only task system that doesn't use |sequence_num| and
@@ -38,8 +37,6 @@ Task::Task(const Location& posted_from, OnceClosure task, TimeDelta delay)
 // this case.
 Task::Task(Task&& other) noexcept
     : PendingTask(std::move(other)),
-      delay(other.delay),
-      sequenced_time(other.sequenced_time),
       sequenced_task_runner_ref(std::move(other.sequenced_task_runner_ref)),
       single_thread_task_runner_ref(
           std::move(other.single_thread_task_runner_ref)) {}

base/task/task_scheduler/task.h

@@ -37,15 +37,6 @@ struct BASE_EXPORT Task : public PendingTask {
 
   Task& operator=(Task&& other);
 
-  // The delay that must expire before the task runs.
-  TimeDelta delay;
-
-  // The time at which the task was inserted in its sequence. For an undelayed
-  // task, this happens at post time. For a delayed task, this happens some
-  // time after the task's delay has expired. If the task hasn't been inserted
-  // in a sequence yet, this defaults to a null TimeTicks.
-  TimeTicks sequenced_time;
-
   // A reference to the SequencedTaskRunner or SingleThreadTaskRunner that
   // posted this task, if any. Used to set ThreadTaskRunnerHandle and/or
   // SequencedTaskRunnerHandle while the task is running.

base/task/task_scheduler/task_tracker.cc

@@ -436,8 +436,8 @@ bool TaskTracker::WillPostTask(Task* task,
   DCHECK(task);
   DCHECK(task->task);
 
-  if (!BeforePostTask(GetEffectiveShutdownBehavior(shutdown_behavior,
-                                                   !task->delay.is_zero())))
+  if (!BeforePostTask(GetEffectiveShutdownBehavior(
+          shutdown_behavior, !task->delayed_run_time.is_null())))
     return false;
 
   if (task->delayed_run_time.is_null())
@@ -498,7 +498,7 @@ scoped_refptr<Sequence> TaskTracker::RunAndPopNextTask(
 
   const TaskShutdownBehavior effective_shutdown_behavior =
       GetEffectiveShutdownBehavior(sequence->shutdown_behavior(),
-                                   !task->delay.is_zero());
+                                   !task->delayed_run_time.is_null());
 
   const bool can_run_task = BeforeRunTask(effective_shutdown_behavior);
 
@@ -588,7 +588,7 @@ void TaskTracker::RunOrSkipTask(Task task,
                                 bool can_run_task) {
   DCHECK(sequence);
   RecordLatencyHistogram(LatencyHistogramType::TASK_LATENCY, traits,
-                         task.sequenced_time);
+                         task.queue_time);
 
   const bool previous_singleton_allowed =
       ThreadRestrictions::SetSingletonAllowed(