[net] Update style in PriorityQueue and PrioritizedDispatcher after readability review.

Review URL: https://chromiumcodereview.appspot.com/9924023

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@140133 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 234 insertions, 160 deletions

diff --git a/net/base/prioritized_dispatcher.cc b/net/base/prioritized_dispatcher.cc
index 9b96e3f..44348e6 100644
--- a/net/base/prioritized_dispatcher.cc
+++ b/net/base/prioritized_dispatcher.cc
@@ -99,5 +99,3 @@ bool PrioritizedDispatcher::MaybeDispatchJob(const Handle& handle,
 }
 
 }  // namespace net
-
-
diff --git a/net/base/prioritized_dispatcher.h b/net/base/prioritized_dispatcher.h
index 7e59d4e6..896d2db 100644
--- a/net/base/prioritized_dispatcher.h
+++ b/net/base/prioritized_dispatcher.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef NET_BASE_PRIORITY_DISPATCH_H_
-#define NET_BASE_PRIORITY_DISPATCH_H_
+#ifndef NET_BASE_PRIORITIZED_DISPATCHER_H_
+#define NET_BASE_PRIORITIZED_DISPATCHER_H_
 #pragma once
 
 #include <vector>
@@ -18,10 +18,10 @@ namespace net {
 // jobs. It never revokes a job once started. The job must call OnJobFinished
 // once it finishes in order to dispatch further jobs.
 //
-// All operations are O(p) time for p priority levels. The class is fully
-// reentrant: it is safe to execute any method (incl. destructor) from within
-// Job callbacks. However, this class is NOT thread-safe, which is enforced
-// by the underlying non-thread-safe PriorityQueue.
+// This class is NOT thread-safe which is enforced by the underlying
+// non-thread-safe PriorityQueue. All operations are O(p) time for p priority
+// levels. It is safe to execute any method, including destructor, from within
+// Job::Start.
 //
 class NET_EXPORT_PRIVATE PrioritizedDispatcher {
  public:
@@ -29,10 +29,13 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   typedef PriorityQueue<Job*>::Priority Priority;
 
   // Describes the limits for the number of jobs started by the dispatcher.
-  // For example, |total_jobs| = 30 and |reserved_slots| = { 5, 10, 5 }
-  // allow for at most 30 running jobs in total. If there are already 24 jobs
-  // running, then there can be 6 more jobs started of which at most 1 can be
-  // at priority 1 or 2, but the rest have to be at 2.
+  // For example, |total_jobs| = 30 and |reserved_slots| = { 0, 5, 10, 5 } allow
+  // for at most 30 running jobs in total. Jobs at priority 0 can't use slots
+  // reserved for higher priorities, so they are limited to 10.
+  // If there are already 24 jobs running, then only 6 more jobs can start. No
+  // jobs at priority 1 or below can start. After one more job starts, no jobs
+  // at priority 2 or below can start, since the remaining 5 slots are reserved
+  // for priority 3 or above.
   struct NET_EXPORT_PRIVATE Limits {
     Limits(Priority num_priorities, size_t total_jobs);
     ~Limits();
@@ -50,10 +53,10 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   // deleted after it is dispatched or canceled, or the dispatcher is destroyed.
   class Job {
    public:
-    // Note: PriorityDispatch will never delete a Job.
+    // Note: PrioritizedDispatcher will never delete a Job.
     virtual ~Job() {}
-    // Called when the dispatcher starts the job. Must call OnJobFinished when
-    // done.
+    // Called when the dispatcher starts the job. Once the job finishes, it must
+    // call OnJobFinished.
     virtual void Start() = 0;
   };
 
@@ -62,7 +65,7 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
   typedef PriorityQueue<Job*>::Pointer Handle;
 
   // Creates a dispatcher enforcing |limits| on number of running jobs.
-  PrioritizedDispatcher(const Limits& limits);
+  explicit PrioritizedDispatcher(const Limits& limits);
 
   ~PrioritizedDispatcher();
 
@@ -72,14 +75,15 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
   // Adds |job| with |priority| to the dispatcher. If limits permit, |job| is
   // started immediately. Returns handle to the job or null-handle if the job is
-  // started.
+  // started. The dispatcher does not own |job|, but |job| must live as long as
+  // it is queued in the dispatcher.
   Handle Add(Job* job, Priority priority);
 
   // Removes the job with |handle| from the queue. Invalidates |handle|.
   // Note: a Handle is valid iff the job is in the queue, i.e. has not Started.
   void Cancel(const Handle& handle);
 
-  // Removes and returns the oldest-lowest Job from the queue invalidating any
+  // Cancels and returns the oldest-lowest-priority Job invalidating any
   // handles to it. Returns NULL if the queue is empty.
   Job* EvictOldestLowest();
 
@@ -111,5 +115,4 @@ class NET_EXPORT_PRIVATE PrioritizedDispatcher {
 
 }  // namespace net
 
-#endif  // NET_BASE_PRIORITY_DISPATCH_H_
-
+#endif  // NET_BASE_PRIORITIZED_DISPATCHER_H_
diff --git a/net/base/prioritized_dispatcher_unittest.cc b/net/base/prioritized_dispatcher_unittest.cc
index 6c19b92..e2435ab 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,87 @@ 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) {}
+    TestJob(PrioritizedDispatcher* dispatcher,
+            char tag,
+            Priority priority,
+            std::string* log)
+        : dispatcher_(dispatcher),
+          tag_(tag),
+          priority_(priority),
+          running_(false),
+          log_(log) {}
+
+    bool running() const {
+      return running_;
+    }
 
-    // MSVS does not accept EXPECT_EQ(this, ...) so wrap it up.
-    PrioritizedDispatcher::Job* this_job() {
-      return this;
+    const PrioritizedDispatcher::Handle handle() const {
+      return handle_;
     }
 
     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();
+      CHECK(handle_.is_null());
+      CHECK(!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();
+      CHECK(!handle_.is_null());
+      CHECK(!running_);
+      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();
+      CHECK(!handle_.is_null());
+      CHECK(!running_);
+      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_);
+      CHECK(running_);
       running_ = false;
-      test_->log_.append(1u, '.');
+      log_->append(1u, '.');
 
-      dispatch().OnJobFinished();
+      dispatcher_->OnJobFinished();
     }
 
     // PriorityDispatch::Job interface
@@ -106,42 +117,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_); }
+    PrioritizedDispatcher* dispatcher_;
 
-    PrioritizedDispatcherTest* test_;
-
-    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_;
 };
 
@@ -155,9 +166,13 @@ TEST_F(PrioritizedDispatcherTest, AddAFIFO) {
   TestJob* job_c = AddJob('c', IDLE);
   TestJob* job_d = AddJob('d', IDLE);
 
+  ASSERT_TRUE(job_a->running());
   job_a->Finish();
+  ASSERT_TRUE(job_b->running());
   job_b->Finish();
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
+  ASSERT_TRUE(job_d->running());
   job_d->Finish();
 
   Expect("a.b.c.d.");
@@ -173,10 +188,15 @@ TEST_F(PrioritizedDispatcherTest, AddPriority) {
   TestJob* job_d = AddJob('d', HIGHEST);
   TestJob* job_e = AddJob('e', MEDIUM);
 
+  ASSERT_TRUE(job_a->running());
   job_a->Finish();
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
+  ASSERT_TRUE(job_d->running());
   job_d->Finish();
+  ASSERT_TRUE(job_b->running());
   job_b->Finish();
+  ASSERT_TRUE(job_e->running());
   job_e->Finish();
 
   Expect("a.c.d.b.e.");
@@ -199,17 +219,27 @@ 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());
-
-  job_a->Finish();  // Releases h.
-  job_b->Finish();
+  EXPECT_EQ(5u, dispatcher_->num_running_jobs());
+  EXPECT_EQ(3u, dispatcher_->num_queued_jobs());
+
+  ASSERT_TRUE(job_a->running());
+  ASSERT_TRUE(job_b->running());
+  ASSERT_TRUE(job_d->running());
+  ASSERT_TRUE(job_f->running());
+  ASSERT_TRUE(job_g->running());
+  // 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();
+  ASSERT_TRUE(job_e->running());
+  ASSERT_TRUE(job_h->running());
+  // h, e are running.
+  job_e->Finish();  // Releases c.
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
+  job_h->Finish();
 
   Expect("abdfg.h...e..c..");
 }
@@ -223,12 +253,18 @@ TEST_F(PrioritizedDispatcherTest, ChangePriority) {
   TestJob* job_c = AddJob('c', HIGHEST);
   TestJob* job_d = AddJob('d', HIGHEST);
 
+  ASSERT_FALSE(job_b->running());
+  ASSERT_FALSE(job_c->running());
   job_b->ChangePriority(HIGHEST);
   job_c->ChangePriority(MEDIUM);
 
+  ASSERT_TRUE(job_a->running());
   job_a->Finish();
+  ASSERT_TRUE(job_d->running());
   job_d->Finish();
+  ASSERT_TRUE(job_b->running());
   job_b->Finish();
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
 
   Expect("a.d.b.c.");
@@ -244,11 +280,16 @@ TEST_F(PrioritizedDispatcherTest, Cancel) {
   TestJob* job_d = AddJob('d', IDLE);
   TestJob* job_e = AddJob('e', IDLE);
 
+  ASSERT_FALSE(job_b->running());
+  ASSERT_FALSE(job_d->running());
   job_b->Cancel();
   job_d->Cancel();
 
+  ASSERT_TRUE(job_a->running());
   job_a->Finish();
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
+  ASSERT_TRUE(job_e->running());
   job_e->Finish();
 
   Expect("a.c.e.");
@@ -264,17 +305,59 @@ 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());
 
+  ASSERT_TRUE(job_a->running());
   job_a->Finish();
+  ASSERT_TRUE(job_c->running());
   job_c->Finish();
+  ASSERT_TRUE(job_e->running());
   job_e->Finish();
 
   Expect("a.c.e.");
 }
 
+TEST_F(PrioritizedDispatcherTest, EvictFromEmpty) {
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+  Prepare(limits);
+  EXPECT_TRUE(dispatcher_->EvictOldestLowest() == NULL);
+}
+
+#if GTEST_HAS_DEATH_TEST && !defined(NDEBUG)
+TEST_F(PrioritizedDispatcherTest, CancelNull) {
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+  Prepare(limits);
+  EXPECT_DEBUG_DEATH(dispatcher_->Cancel(PrioritizedDispatcher::Handle()), "");
+}
+
+TEST_F(PrioritizedDispatcherTest, CancelMissing) {
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+  Prepare(limits);
+  AddJob('a', IDLE);
+  TestJob* job_b = AddJob('b', IDLE);
+  PrioritizedDispatcher::Handle handle = job_b->handle();
+  ASSERT_FALSE(handle.is_null());
+  dispatcher_->Cancel(handle);
+  EXPECT_DEBUG_DEATH(dispatcher_->Cancel(handle), "");
+}
+
+TEST_F(PrioritizedDispatcherTest, CancelIncompatible) {
+  PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+  Prepare(limits);
+  AddJob('a', IDLE);
+  TestJob* job_b = AddJob('b', IDLE);
+  PrioritizedDispatcher::Handle handle = job_b->handle();
+  ASSERT_FALSE(handle.is_null());
+
+  // New dispatcher.
+  Prepare(limits);
+  AddJob('a', IDLE);
+  AddJob('b', IDLE);
+  EXPECT_DEBUG_DEATH(dispatcher_->Cancel(handle), "");
+}
+#endif  // GTEST_HAS_DEATH_TEST && !defined(NDEBUG)
+
 }  // namespace
 
 }  // namespace net
-
diff --git a/net/base/priority_queue.h b/net/base/priority_queue.h
index f60b88c..d11ffcc 100644
--- a/net/base/priority_queue.h
+++ b/net/base/priority_queue.h
@@ -34,7 +34,7 @@ class PriorityQueue : public base::NonThreadSafe {
  private:
   // This section is up-front for Pointer only.
 #if !defined(NDEBUG)
-  typedef std::list<std::pair<size_t, T> > List;
+  typedef std::list<std::pair<unsigned, T> > List;
 #else
   typedef std::list<T> List;
 #endif
@@ -49,10 +49,26 @@ class PriorityQueue : public base::NonThreadSafe {
     // Constructs a null pointer.
     Pointer() : priority_(kNullPriority) {
 #if !defined(NDEBUG)
-      id_ = static_cast<size_t>(-1);
+      id_ = static_cast<unsigned>(-1);
 #endif
     }
 
+    Pointer(const Pointer& p) : priority_(p.priority_), iterator_(p.iterator_) {
+#if !defined(NDEBUG)
+      id_ = p.id_;
+#endif
+    }
+
+    Pointer& operator=(const Pointer& p) {
+      // Self-assignment is benign.
+      priority_ = p.priority_;
+      iterator_ = p.iterator_;
+#if !defined(NDEBUG)
+      id_ = p.id_;
+#endif
+      return *this;
+    }
+
     bool is_null() const { return priority_ == kNullPriority; }
 
     Priority priority() const { return priority_; }
@@ -94,7 +110,7 @@ class PriorityQueue : public base::NonThreadSafe {
 
 #if !defined(NDEBUG)
     // Used by the queue to check if a Pointer is valid.
-    size_t id_;
+    unsigned id_;
 #endif
   };
 
@@ -114,7 +130,7 @@ class PriorityQueue : public base::NonThreadSafe {
     ++size_;
     List& list = lists_[priority];
 #if !defined(NDEBUG)
-    size_t id = next_id_;
+    unsigned id = next_id_;
     valid_ids_.insert(id);
     ++next_id_;
     return Pointer(priority, list.insert(list.end(),
@@ -208,15 +224,16 @@ class PriorityQueue : public base::NonThreadSafe {
   typedef std::vector<List> ListVector;
 
 #if !defined(NDEBUG)
-  size_t next_id_;
-  base::hash_set<size_t> valid_ids_;
+  unsigned next_id_;
+  base::hash_set<unsigned> valid_ids_;
 #endif
 
   ListVector lists_;
   size_t size_;
+
+  DISALLOW_COPY_AND_ASSIGN(PriorityQueue);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_PRIORITY_QUEUE_H_
-
diff --git a/net/base/priority_queue_unittest.cc b/net/base/priority_queue_unittest.cc
index f0f65a6..c8449bb 100644
--- a/net/base/priority_queue_unittest.cc
+++ b/net/base/priority_queue_unittest.cc
@@ -18,115 +18,88 @@ const int kLastMaxOrder[kNumElements] = { 7, 4, 5, 2, 0, 6, 1, 8, 3 };
 const int kFirstMaxOrder[kNumElements] = { 4, 7, 5, 0, 2, 1, 6, 3, 8 };
 const int kLastMinOrder[kNumElements] = { 8, 3, 6, 1, 2, 0, 5, 7, 4 };
 
-void CheckEmpty(PriorityQueue<int>* queue) {
-  EXPECT_EQ(0u, queue->size());
-  EXPECT_TRUE(queue->FirstMin().is_null());
-  EXPECT_TRUE(queue->LastMin().is_null());
-  EXPECT_TRUE(queue->FirstMax().is_null());
-  EXPECT_TRUE(queue->LastMax().is_null());
-}
-
-TEST(PriorityQueueTest, AddAndClear) {
-  PriorityQueue<int> queue(kNumPriorities);
-  PriorityQueue<int>::Pointer pointers[kNumElements];
-
-  CheckEmpty(&queue);
-  for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(i, queue.size());
-    pointers[i] = queue.Insert(static_cast<int>(i), kPriorities[i]);
+class PriorityQueueTest : public testing::Test {
+ protected:
+  PriorityQueueTest() : queue_(kNumPriorities) {}
+
+  virtual void SetUp() OVERRIDE {
+    CheckEmpty();
+    for (size_t i = 0; i < kNumElements; ++i) {
+      EXPECT_EQ(i, queue_.size());
+      pointers_[i] = queue_.Insert(static_cast<int>(i), kPriorities[i]);
+    }
+    EXPECT_EQ(kNumElements, queue_.size());
   }
-  EXPECT_EQ(kNumElements, queue.size());
 
-  for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(kPriorities[i], pointers[i].priority());
-    EXPECT_EQ(static_cast<int>(i), pointers[i].value());
+  void CheckEmpty() {
+    EXPECT_EQ(0u, queue_.size());
+    EXPECT_TRUE(queue_.FirstMin().is_null());
+    EXPECT_TRUE(queue_.LastMin().is_null());
+    EXPECT_TRUE(queue_.FirstMax().is_null());
+    EXPECT_TRUE(queue_.LastMax().is_null());
   }
 
-  queue.Clear();
-  CheckEmpty(&queue);
-}
-
-TEST(PriorityQueueTest, FirstMinOrder) {
-  PriorityQueue<int> queue(kNumPriorities);
-  PriorityQueue<int>::Pointer pointers[kNumElements];
+  PriorityQueue<int> queue_;
+  PriorityQueue<int>::Pointer pointers_[kNumElements];
+};
 
+TEST_F(PriorityQueueTest, AddAndClear) {
   for (size_t i = 0; i < kNumElements; ++i) {
-    pointers[i] = queue.Insert(static_cast<int>(i), kPriorities[i]);
+    EXPECT_EQ(kPriorities[i], pointers_[i].priority());
+    EXPECT_EQ(static_cast<int>(i), pointers_[i].value());
   }
+  queue_.Clear();
+  CheckEmpty();
+}
 
+TEST_F(PriorityQueueTest, FirstMinOrder) {
   for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(kNumElements - i, queue.size());
+    EXPECT_EQ(kNumElements - i, queue_.size());
     // Also check Equals.
-    EXPECT_TRUE(queue.FirstMin().Equals(pointers[kFirstMinOrder[i]]));
-    EXPECT_EQ(kFirstMinOrder[i], queue.FirstMin().value());
-    queue.Erase(queue.FirstMin());
+    EXPECT_TRUE(queue_.FirstMin().Equals(pointers_[kFirstMinOrder[i]]));
+    EXPECT_EQ(kFirstMinOrder[i], queue_.FirstMin().value());
+    queue_.Erase(queue_.FirstMin());
   }
-  CheckEmpty(&queue);
+  CheckEmpty();
 }
 
-TEST(PriorityQueueTest, LastMinOrder) {
-  PriorityQueue<int> queue(kNumPriorities);
-
-  for (size_t i = 0; i < kNumElements; ++i) {
-    queue.Insert(static_cast<int>(i), kPriorities[i]);
-  }
-
+TEST_F(PriorityQueueTest, LastMinOrder) {
   for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(kLastMinOrder[i], queue.LastMin().value());
-    queue.Erase(queue.LastMin());
+    EXPECT_EQ(kLastMinOrder[i], queue_.LastMin().value());
+    queue_.Erase(queue_.LastMin());
   }
-  CheckEmpty(&queue);
+  CheckEmpty();
 }
 
-TEST(PriorityQueueTest, FirstMaxOrder) {
-  PriorityQueue<int> queue(kNumPriorities);
-
+TEST_F(PriorityQueueTest, FirstMaxOrder) {
   for (size_t i = 0; i < kNumElements; ++i) {
-    queue.Insert(static_cast<int>(i), kPriorities[i]);
+    EXPECT_EQ(kFirstMaxOrder[i], queue_.FirstMax().value());
+    queue_.Erase(queue_.FirstMax());
   }
-
-  for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(kFirstMaxOrder[i], queue.FirstMax().value());
-    queue.Erase(queue.FirstMax());
-  }
-  CheckEmpty(&queue);
+  CheckEmpty();
 }
 
-TEST(PriorityQueueTest, LastMaxOrder) {
-  PriorityQueue<int> queue(kNumPriorities);
-
-  for (size_t i = 0; i < kNumElements; ++i) {
-    queue.Insert(static_cast<int>(i), kPriorities[i]);
-  }
-
+TEST_F(PriorityQueueTest, LastMaxOrder) {
   for (size_t i = 0; i < kNumElements; ++i) {
-    EXPECT_EQ(kLastMaxOrder[i], queue.LastMax().value());
-    queue.Erase(queue.LastMax());
+    EXPECT_EQ(kLastMaxOrder[i], queue_.LastMax().value());
+    queue_.Erase(queue_.LastMax());
   }
-  CheckEmpty(&queue);
+  CheckEmpty();
 }
 
-TEST(PriorityQueueTest, EraseFromMiddle) {
-  PriorityQueue<int> queue(kNumPriorities);
-  PriorityQueue<int>::Pointer pointers[kNumElements];
-
-  for (size_t i = 0; i < kNumElements; ++i) {
-    pointers[i] = queue.Insert(static_cast<int>(i), kPriorities[i]);
-  }
-
-  queue.Erase(pointers[2]);
-  queue.Erase(pointers[3]);
+TEST_F(PriorityQueueTest, EraseFromMiddle) {
+  queue_.Erase(pointers_[2]);
+  queue_.Erase(pointers_[3]);
 
   int expected_order[] = { 8, 1, 6, 0, 5, 4, 7 };
 
   for (size_t i = 0; i < arraysize(expected_order); ++i) {
-    EXPECT_EQ(expected_order[i], queue.FirstMin().value());
-    queue.Erase(queue.FirstMin());
+    EXPECT_EQ(expected_order[i], queue_.FirstMin().value());
+    queue_.Erase(queue_.FirstMin());
   }
-  CheckEmpty(&queue);
+  CheckEmpty();
 }
 
 }  // namespace
 
 }  // namespace net
-