Support base::OnceCallback on base::OneShotTimer

This moves task management logic of base::internal::TimerBase to its
subclasses, so that subclasses can hold user tasks as different types
for each.
Specifically, OneShotTimer's task type is converted to OnceClosure after
this CL, and it gets supporting OnceCallback.

Bug: 850247
Change-Id: Ie017c2ed6e0fb1fe44937a497576d16e5c3dab68
Reviewed-on: https://chromium-review.googlesource.com/1127215Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577784}

base/timer/timer.cc

@@ -58,17 +58,10 @@ class BaseTimerTaskInternal {
   DISALLOW_COPY_AND_ASSIGN(BaseTimerTaskInternal);
 };
 
-TimerBase::TimerBase(bool retain_user_task, bool is_repeating)
-    : TimerBase(retain_user_task, is_repeating, nullptr) {}
+TimerBase::TimerBase() : TimerBase(nullptr) {}
 
-TimerBase::TimerBase(bool retain_user_task,
-                     bool is_repeating,
-                     const TickClock* tick_clock)
-    : scheduled_task_(nullptr),
-      is_repeating_(is_repeating),
-      retain_user_task_(retain_user_task),
-      tick_clock_(tick_clock),
-      is_running_(false) {
+TimerBase::TimerBase(const TickClock* tick_clock)
+    : scheduled_task_(nullptr), tick_clock_(tick_clock), is_running_(false) {
   // It is safe for the timer to be created on a different thread/sequence than
   // the one from which the timer APIs are called. The first call to the
   // checker's CalledOnValidSequence() method will re-bind the checker, and
@@ -76,23 +69,15 @@ TimerBase::TimerBase(bool retain_user_task,
   origin_sequence_checker_.DetachFromSequence();
 }
 
-TimerBase::TimerBase(const Location& posted_from,
-                     TimeDelta delay,
-                     const base::Closure& user_task,
-                     bool is_repeating)
-    : TimerBase(posted_from, delay, user_task, is_repeating, nullptr) {}
+TimerBase::TimerBase(const Location& posted_from, TimeDelta delay)
+    : TimerBase(posted_from, delay, nullptr) {}
 
 TimerBase::TimerBase(const Location& posted_from,
                      TimeDelta delay,
-                     const base::Closure& user_task,
-                     bool is_repeating,
                      const TickClock* tick_clock)
     : scheduled_task_(nullptr),
       posted_from_(posted_from),
       delay_(delay),
-      user_task_(user_task),
-      is_repeating_(is_repeating),
-      retain_user_task_(true),
       tick_clock_(tick_clock),
       is_running_(false) {
   // See comment in other constructor.
@@ -101,7 +86,7 @@ TimerBase::TimerBase(const Location& posted_from,
 
 TimerBase::~TimerBase() {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
-  AbandonAndStop();
+  AbandonScheduledTask();
 }
 
 bool TimerBase::IsRunning() const {
@@ -126,14 +111,11 @@ void TimerBase::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) {
   task_runner_.swap(task_runner);
 }
 
-void TimerBase::Start(const Location& posted_from,
-                      TimeDelta delay,
-                      const base::Closure& user_task) {
+void TimerBase::StartInternal(const Location& posted_from, TimeDelta delay) {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
 
   posted_from_ = posted_from;
   delay_ = delay;
-  user_task_ = user_task;
 
   Reset();
 }
@@ -148,15 +130,12 @@ void TimerBase::Stop() {
   // It's safe to destroy or restart Timer on another sequence after Stop().
   origin_sequence_checker_.DetachFromSequence();
 
-  if (!retain_user_task_)
-    user_task_.Reset();
-  // No more member accesses here: |this| could be deleted after freeing
-  // |user_task_|.
+  OnStop();
+  // No more member accesses here: |this| could be deleted after Stop() call.
 }
 
 void TimerBase::Reset() {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
-  DCHECK(!user_task_.is_null());
 
   // If there's no pending task, start one up and return.
   if (!scheduled_task_) {
@@ -201,14 +180,12 @@ void TimerBase::PostNewScheduledTask(TimeDelta delay) {
     // this code racy. https://crbug.com/587199
     GetTaskRunner()->PostDelayedTask(
         posted_from_,
-        base::BindOnce(&BaseTimerTaskInternal::Run,
-                       base::Owned(scheduled_task_)),
-        delay);
+        BindOnce(&BaseTimerTaskInternal::Run, Owned(scheduled_task_)), delay);
     scheduled_run_time_ = desired_run_time_ = Now() + delay;
   } else {
-    GetTaskRunner()->PostTask(posted_from_,
-                              base::BindOnce(&BaseTimerTaskInternal::Run,
-                                             base::Owned(scheduled_task_)));
+    GetTaskRunner()->PostTask(
+        posted_from_,
+        BindOnce(&BaseTimerTaskInternal::Run, Owned(scheduled_task_)));
     scheduled_run_time_ = desired_run_time_ = TimeTicks();
   }
 }
@@ -250,31 +227,114 @@ void TimerBase::RunScheduledTask() {
     }
   }
 
-  // Make a local copy of the task to run. The Stop method will reset the
-  // |user_task_| member if |retain_user_task_| is false.
-  base::Closure task = user_task_;
-
-  if (is_repeating_)
-    PostNewScheduledTask(delay_);
-  else
-    Stop();
-
-  task.Run();
-
+  RunUserTask();
   // No more member accesses here: |this| could be deleted at this point.
 }
 
 }  // namespace internal
 
+OneShotTimer::OneShotTimer() = default;
+OneShotTimer::OneShotTimer(const TickClock* tick_clock)
+    : internal::TimerBase(tick_clock) {}
+OneShotTimer::~OneShotTimer() = default;
+
+void OneShotTimer::Start(const Location& posted_from,
+                         TimeDelta delay,
+                         OnceClosure user_task) {
+  user_task_ = std::move(user_task);
+  StartInternal(posted_from, delay);
+}
+
 void OneShotTimer::FireNow() {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   DCHECK(!task_runner_) << "FireNow() is incompatible with SetTaskRunner()";
   DCHECK(IsRunning());
 
-  OnceClosure task = user_task();
+  RunUserTask();
+}
+
+void OneShotTimer::OnStop() {
+  user_task_.Reset();
+  // No more member accesses here: |this| could be deleted after freeing
+  // |user_task_|.
+}
+
+void OneShotTimer::RunUserTask() {
+  // Make a local copy of the task to run. The Stop method will reset the
+  // |user_task_| member.
+  OnceClosure task = std::move(user_task_);
   Stop();
-  DCHECK(!user_task());
+  DCHECK(task);
   std::move(task).Run();
+  // No more member accesses here: |this| could be deleted at this point.
+}
+
+RepeatingTimer::RepeatingTimer() = default;
+RepeatingTimer::RepeatingTimer(const TickClock* tick_clock)
+    : internal::TimerBase(tick_clock) {}
+RepeatingTimer::~RepeatingTimer() = default;
+
+RepeatingTimer::RepeatingTimer(const Location& posted_from,
+                               TimeDelta delay,
+                               RepeatingClosure user_task)
+    : internal::TimerBase(posted_from, delay),
+      user_task_(std::move(user_task)) {}
+RepeatingTimer::RepeatingTimer(const Location& posted_from,
+                               TimeDelta delay,
+                               RepeatingClosure user_task,
+                               const TickClock* tick_clock)
+    : internal::TimerBase(posted_from, delay, tick_clock),
+      user_task_(std::move(user_task)) {}
+
+void RepeatingTimer::Start(const Location& posted_from,
+                           TimeDelta delay,
+                           RepeatingClosure user_task) {
+  user_task_ = std::move(user_task);
+  StartInternal(posted_from, delay);
+}
+
+void RepeatingTimer::OnStop() {}
+void RepeatingTimer::RunUserTask() {
+  // Make a local copy of the task to run in case the task destroy the timer
+  // instance.
+  RepeatingClosure task = user_task_;
+  PostNewScheduledTask(GetCurrentDelay());
+  task.Run();
+  // No more member accesses here: |this| could be deleted at this point.
+}
+
+RetainingOneShotTimer::RetainingOneShotTimer() = default;
+RetainingOneShotTimer::RetainingOneShotTimer(const TickClock* tick_clock)
+    : internal::TimerBase(tick_clock) {}
+RetainingOneShotTimer::~RetainingOneShotTimer() = default;
+
+RetainingOneShotTimer::RetainingOneShotTimer(const Location& posted_from,
+                                             TimeDelta delay,
+                                             RepeatingClosure user_task)
+    : internal::TimerBase(posted_from, delay),
+      user_task_(std::move(user_task)) {}
+RetainingOneShotTimer::RetainingOneShotTimer(const Location& posted_from,
+                                             TimeDelta delay,
+                                             RepeatingClosure user_task,
+                                             const TickClock* tick_clock)
+    : internal::TimerBase(posted_from, delay, tick_clock),
+      user_task_(std::move(user_task)) {}
+
+void RetainingOneShotTimer::Start(const Location& posted_from,
+                                  TimeDelta delay,
+                                  RepeatingClosure user_task) {
+  user_task_ = std::move(user_task);
+  StartInternal(posted_from, delay);
+}
+
+void RetainingOneShotTimer::OnStop() {}
+void RetainingOneShotTimer::RunUserTask() {
+  // Make a local copy of the task to run in case the task destroys the timer
+  // instance.
+  RepeatingClosure task = user_task_;
+  Stop();
+  task.Run();
+  // No more member accesses here: |this| could be deleted at this point.
 }
 
 }  // namespace base

net/dns/mdns_client_unittest.cc

@@ -381,17 +381,15 @@ class MockTimer : public base::MockOneShotTimer {
 
   void Start(const base::Location& posted_from,
              base::TimeDelta delay,
-             const base::Closure& user_task) override {
-    StartObserver(posted_from, delay, user_task);
-    base::MockOneShotTimer::Start(posted_from, delay, user_task);
+             base::OnceClosure user_task) override {
+    StartObserver(posted_from, delay);
+    base::MockOneShotTimer::Start(posted_from, delay, std::move(user_task));
   }
 
   // StartObserver is invoked when MockTimer::Start() is called.
   // Does not replace the behavior of MockTimer::Start().
-  MOCK_METHOD3(StartObserver,
-               void(const base::Location& posted_from,
-                    base::TimeDelta delay,
-                    const base::Closure& user_task));
+  MOCK_METHOD2(StartObserver,
+               void(const base::Location& posted_from, base::TimeDelta delay));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockTimer);
@@ -565,7 +563,7 @@ TEST_F(MDnsTest, CacheCleanupWithShortTTL) {
   test_client_.reset(new MDnsClientImpl(&clock, base::WrapUnique(timer)));
   test_client_->StartListening(&socket_factory_);
 
-  EXPECT_CALL(*timer, StartObserver(_, _, _)).Times(1);
+  EXPECT_CALL(*timer, StartObserver(_, _)).Times(1);
   EXPECT_CALL(clock, Now())
       .Times(3)
       .WillRepeatedly(Return(start_time))
@@ -608,7 +606,7 @@ TEST_F(MDnsTest, CacheCleanupWithShortTTL) {
       .WillOnce(Return(start_time + base::TimeDelta::FromSeconds(2)))
       .RetiresOnSaturation();
 
-  EXPECT_CALL(*timer, StartObserver(_, base::TimeDelta(), _));
+  EXPECT_CALL(*timer, StartObserver(_, base::TimeDelta()));
 
   timer->Fire();
 }

net/http/bidirectional_stream_unittest.cc

@@ -384,10 +384,11 @@ class MockTimer : public base::MockOneShotTimer {
 
   void Start(const base::Location& posted_from,
              base::TimeDelta delay,
-             const base::Closure& user_task) override {
+             base::OnceClosure user_task) override {
     // Sets a maximum delay, so the timer does not fire unless it is told to.
     base::TimeDelta infinite_delay = base::TimeDelta::Max();
-    base::MockOneShotTimer::Start(posted_from, infinite_delay, user_task);
+    base::MockOneShotTimer::Start(posted_from, infinite_delay,
+                                  std::move(user_task));
   }
 
  private: