Domain Reliability: More security review.

components/domain_reliability/dispatcher.cc

 // Copyright 2014 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.
 
 #include "components/domain_reliability/dispatcher.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/timer/timer.h"
 #include "components/domain_reliability/util.h"
 
 namespace domain_reliability {
 
+struct DomainReliabilityDispatcher::Task {
+  Task(const base::Closure& closure,
+       scoped_ptr<MockableTime::Timer> timer,
+       base::TimeDelta min_delay,
+       base::TimeDelta max_delay);
+  ~Task();
+
+  base::Closure closure;
+  scoped_ptr<MockableTime::Timer> timer;
+  base::TimeDelta min_delay;
+  base::TimeDelta max_delay;
+  bool eligible;
+};
+
 DomainReliabilityDispatcher::DomainReliabilityDispatcher(MockableTime* time)
     : time_(time) {}
 
 DomainReliabilityDispatcher::~DomainReliabilityDispatcher() {
   // TODO(ttuttle): STLElementDeleter?
   STLDeleteElements(&tasks_);
 }
 
 void DomainReliabilityDispatcher::ScheduleTask(
     const base::Closure& closure,
@@ skipping 34 matching lines @@
 DomainReliabilityDispatcher::Task::Task(const base::Closure& closure,
                                         scoped_ptr<MockableTime::Timer> timer,
                                         base::TimeDelta min_delay,
                                         base::TimeDelta max_delay)
     : closure(closure),
       timer(timer.Pass()),
       min_delay(min_delay),
       max_delay(max_delay),
       eligible(false) {}
 
 DomainReliabilityDispatcher::Task::~Task() {}

[Ryan Sleevi, 2014/04/30 23:58:11]

Seems like you should move 73-83 up to 28, to match ordering.

[Deprecated (see juliatuttle), 2014/05/02 19:23:40]

Done.

 
 void DomainReliabilityDispatcher::MakeTaskWaiting(Task* task) {
   DCHECK(task);
   DCHECK(!task->eligible);
   DCHECK(!task->timer->IsRunning());
-  task->timer->Start(
-      FROM_HERE,
-      task->min_delay,
-      base::Bind(
-          &DomainReliabilityDispatcher::MakeTaskEligible,
-          base::Unretained(this),
-          task));
+  task->timer->Start(FROM_HERE,
+                     task->min_delay,
+                     base::Bind(&DomainReliabilityDispatcher::MakeTaskEligible,
+                                base::Unretained(this),
+                                task));
 }
 
 void
 DomainReliabilityDispatcher::MakeTaskEligible(Task* task) {
   DCHECK(task);
   DCHECK(!task->eligible);
   task->eligible = true;
   eligible_tasks_.insert(task);
-  task->timer->Start(
-      FROM_HERE,
-      task->max_delay - task->min_delay,
-      base::Bind(
-          &DomainReliabilityDispatcher::RunAndDeleteTask,
-          base::Unretained(this),
-          task));
+  task->timer->Start(FROM_HERE,
+                     task->max_delay - task->min_delay,
+                     base::Bind(&DomainReliabilityDispatcher::RunAndDeleteTask,
+                                base::Unretained(this),
+                                task));

[Ryan Sleevi, 2014/04/30 23:58:11]

DESIGN: Not something for now, but I'm wondering if there is a better way to
aggregate these timers. MessageLoop is fairly efficient in using a scan and mark
here, and it seems like as long as the timer was the lowest possible timeout,
you could implement it as such.

Then again, this seems like it's re-implementing a lot of the existing
MessageLoop task pump stuff. Why wouldn't PostDelayedTask suffice here?

[Deprecated (see juliatuttle), 2014/05/02 19:23:40]

I added two layers of indirection:
MockableTime: Lets me test time-based interactions in unittests.
Dispatcher: Lets me schedule tasks with a range of times instead of a single
time.

 }
 
 void DomainReliabilityDispatcher::RunAndDeleteTask(Task* task) {
   DCHECK(task);
   DCHECK(!task->closure.is_null());
   task->closure.Run();
   if (task->eligible)
     eligible_tasks_.erase(task);
   tasks_.erase(task);
   delete task;
 }
 
 }  // namespace domain_reliability