Readability review

net/base/prioritized_dispatcher_unittest.cc

Index: net/base/prioritized_dispatcher_unittest.cc
diff --git a/net/base/prioritized_dispatcher_unittest.cc b/net/base/prioritized_dispatcher_unittest.cc
index 6c19b92e5780f36f747e41b50ce72458eb0f3e57..d2ca9d5dd6fcc2df0b884332108fcba9ed252e58 100644
--- a/net/base/prioritized_dispatcher_unittest.cc
+++ b/net/base/prioritized_dispatcher_unittest.cc
@@ -6,6 +6,7 @@
 #include <string>
 
 #include "base/compiler_specific.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "net/base/prioritized_dispatcher.h"
@@ -28,77 +29,79 @@ COMPILE_ASSERT(MINIMUM_PRIORITY == 0u &&
 class PrioritizedDispatcherTest : public testing::Test {
  public:
   typedef PrioritizedDispatcher::Priority Priority;
-  // A job that appends |data| to |log_| when started and '.' when finished.
+  // A job that appends |tag| to |log| when started and '.' when finished.
   // This is intended to confirm the execution order of a sequence of jobs added
-  // to the dispatcher.
+  // to the dispatcher. Note that finishing order of jobs does not matter.
   class TestJob : public PrioritizedDispatcher::Job {
    public:
-    TestJob(PrioritizedDispatcherTest* test, char data, Priority priority)
-        : test_(test), data_(data), priority_(priority), running_(false) {}
-
-    // MSVS does not accept EXPECT_EQ(this, ...) so wrap it up.
-    PrioritizedDispatcher::Job* this_job() {
-      return this;
-    }
+    TestJob(PrioritizedDispatcher* dispatcher,
+            char tag,
+            Priority priority,
+            std::string* log)
+        : dispatcher_(dispatcher),
+          tag_(tag),
+          priority_(priority),
+          running_(false),
+          log_(log) {}
 
     void Add() {
-      EXPECT_TRUE(handle_.is_null());
-      EXPECT_FALSE(running_);
-      size_t num_queued = dispatch().num_queued_jobs();
-      size_t num_running = dispatch().num_running_jobs();
+      DCHECK(handle_.is_null());
+      DCHECK(!running_);
+      size_t num_queued = dispatcher_->num_queued_jobs();
+      size_t num_running = dispatcher_->num_running_jobs();
 
-      handle_ = dispatch().Add(this, priority_);
+      handle_ = dispatcher_->Add(this, priority_);
 
       if (handle_.is_null()) {
-        EXPECT_EQ(num_queued, dispatch().num_queued_jobs());
+        EXPECT_EQ(num_queued, dispatcher_->num_queued_jobs());
         EXPECT_TRUE(running_);
-        EXPECT_EQ(num_running + 1, dispatch().num_running_jobs());
+        EXPECT_EQ(num_running + 1, dispatcher_->num_running_jobs());
       } else {
         EXPECT_FALSE(running_);
         EXPECT_EQ(priority_, handle_.priority());
-        EXPECT_EQ(this_job(), handle_.value());
-        EXPECT_EQ(num_running, dispatch().num_running_jobs());
+        EXPECT_EQ(tag_, reinterpret_cast<TestJob*>(handle_.value())->tag_);
+        EXPECT_EQ(num_running, dispatcher_->num_running_jobs());
       }
     }
 
     void ChangePriority(Priority priority) {
       EXPECT_FALSE(running_);
       ASSERT_FALSE(handle_.is_null());
-      size_t num_queued = dispatch().num_queued_jobs();
-      size_t num_running = dispatch().num_running_jobs();
+      size_t num_queued = dispatcher_->num_queued_jobs();
+      size_t num_running = dispatcher_->num_running_jobs();
 
-      handle_ = dispatch().ChangePriority(handle_, priority);
+      handle_ = dispatcher_->ChangePriority(handle_, priority);
 
       if (handle_.is_null()) {
         EXPECT_TRUE(running_);
-        EXPECT_EQ(num_queued - 1, dispatch().num_queued_jobs());
-        EXPECT_EQ(num_running + 1, dispatch().num_running_jobs());
+        EXPECT_EQ(num_queued - 1, dispatcher_->num_queued_jobs());
+        EXPECT_EQ(num_running + 1, dispatcher_->num_running_jobs());
       } else {
         EXPECT_FALSE(running_);
         EXPECT_EQ(priority, handle_.priority());
-        EXPECT_EQ(this_job(), handle_.value());
-        EXPECT_EQ(num_queued, dispatch().num_queued_jobs());
-        EXPECT_EQ(num_running, dispatch().num_running_jobs());
+        EXPECT_EQ(tag_, reinterpret_cast<TestJob*>(handle_.value())->tag_);
+        EXPECT_EQ(num_queued, dispatcher_->num_queued_jobs());
+        EXPECT_EQ(num_running, dispatcher_->num_running_jobs());
       }
     }
 
     void Cancel() {
       EXPECT_FALSE(running_);
       ASSERT_FALSE(handle_.is_null());
-      size_t num_queued = dispatch().num_queued_jobs();
+      size_t num_queued = dispatcher_->num_queued_jobs();
 
-      dispatch().Cancel(handle_);
+      dispatcher_->Cancel(handle_);
 
-      EXPECT_EQ(num_queued - 1, dispatch().num_queued_jobs());
+      EXPECT_EQ(num_queued - 1, dispatcher_->num_queued_jobs());
       handle_ = PrioritizedDispatcher::Handle();
     }
 
     void Finish() {
       EXPECT_TRUE(running_);
       running_ = false;
-      test_->log_.append(1u, '.');
+      log_->append(1u, '.');
 
-      dispatch().OnJobFinished();
+      dispatcher_->OnJobFinished();
     }
 
     // PriorityDispatch::Job interface
@@ -106,42 +109,42 @@ class PrioritizedDispatcherTest : public testing::Test {
       EXPECT_FALSE(running_);
       handle_ = PrioritizedDispatcher::Handle();
       running_ = true;
-      test_->log_.append(1u, data_);
+      log_->append(1u, tag_);
     }
 
    private:
-    PrioritizedDispatcher& dispatch() { return *(test_->dispatch_); }
-
-    PrioritizedDispatcherTest* test_;
+    PrioritizedDispatcher* dispatcher_;
 
-    char data_;
+    char tag_;
     Priority priority_;
 
     PrioritizedDispatcher::Handle handle_;
     bool running_;
+
+    std::string* log_;
   };
 
  protected:
   void Prepare(const PrioritizedDispatcher::Limits& limits) {
-    dispatch_.reset(new PrioritizedDispatcher(limits));
+    dispatcher_.reset(new PrioritizedDispatcher(limits));
   }
 
   TestJob* AddJob(char data, Priority priority) {
-    TestJob* job = new TestJob(this, data, priority);
+    TestJob* job = new TestJob(dispatcher_.get(), data, priority, &log_);
     jobs_.push_back(job);
     job->Add();
     return job;
   }
 
   void Expect(std::string log) {
-    EXPECT_EQ(0u, dispatch_->num_queued_jobs());
-    EXPECT_EQ(0u, dispatch_->num_running_jobs());
+    EXPECT_EQ(0u, dispatcher_->num_queued_jobs());
+    EXPECT_EQ(0u, dispatcher_->num_running_jobs());
     EXPECT_EQ(log, log_);
     log_.clear();
   }
 
   std::string log_;
-  scoped_ptr<PrioritizedDispatcher> dispatch_;
+  scoped_ptr<PrioritizedDispatcher> dispatcher_;
   ScopedVector<TestJob> jobs_;
 };
 
@@ -199,17 +202,19 @@ TEST_F(PrioritizedDispatcherTest, EnforceLimits) {
   TestJob* job_g = AddJob('g', HIGHEST); // Uses reserved slot.
   TestJob* job_h = AddJob('h', HIGHEST); // Must wait.
 
-  EXPECT_EQ(5u, dispatch_->num_running_jobs());
-  EXPECT_EQ(3u, dispatch_->num_queued_jobs());
+  EXPECT_EQ(5u, dispatcher_->num_running_jobs());
+  EXPECT_EQ(3u, dispatcher_->num_queued_jobs());
 
-  job_a->Finish();  // Releases h.
-  job_b->Finish();
+  // a, b, d, f, g are running. Finish them in any order.
+  job_b->Finish();  // Releases h.
+  job_f->Finish();
+  job_a->Finish();
+  job_g->Finish();  // Releases e.
   job_d->Finish();
-  job_f->Finish();  // Releases e.
-  job_g->Finish();
-  job_h->Finish();  // Releases c.
-  job_e->Finish();
+  // h, e are running.
+  job_e->Finish();  // Releases c.
   job_c->Finish();
+  job_h->Finish();
 
   Expect("abdfg.h...e..c..");
 }
@@ -264,8 +269,8 @@ TEST_F(PrioritizedDispatcherTest, Evict) {
   TestJob* job_d = AddJob('d', LOW);
   TestJob* job_e = AddJob('e', HIGHEST);
 
-  EXPECT_EQ(job_b, dispatch_->EvictOldestLowest());
-  EXPECT_EQ(job_d, dispatch_->EvictOldestLowest());
+  EXPECT_EQ(job_b, dispatcher_->EvictOldestLowest());
+  EXPECT_EQ(job_d, dispatcher_->EvictOldestLowest());
 
   job_a->Finish();
   job_c->Finish();
@@ -274,7 +279,14 @@ TEST_F(PrioritizedDispatcherTest, Evict) {
   Expect("a.c.e.");
 }
 
+TEST_F(PrioritizedDispatcherTest, EvictFromEmpty) {
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+  Prepare(limits);
+  EXPECT_TRUE(dispatcher_->EvictOldestLowest() == NULL);
+}
+
 }  // namespace
 
 }  // namespace net
 
+