Move work signal count tracking from SequencedWorkerPool to the test file

Add OnHasWork() method to TestingObserver.

Make TestingObserver a constructor parameter, so it can
be read outside the lock.

BUG=117469
TEST=


Review URL: http://codereview.chromium.org/9689028

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

3 files changed, 58 insertions, 54 deletions

diff --git a/base/threading/sequenced_worker_pool.cc b/base/threading/sequenced_worker_pool.cc
index d542733..d517d21 100644
--- a/base/threading/sequenced_worker_pool.cc
+++ b/base/threading/sequenced_worker_pool.cc
@@ -72,7 +72,8 @@ class SequencedWorkerPool::Inner {
   // Take a raw pointer to |worker| to avoid cycles (since we're owned
   // by it).
   Inner(SequencedWorkerPool* worker_pool, size_t max_threads,
-        const std::string& thread_name_prefix);
+        const std::string& thread_name_prefix,
+        TestingObserver* observer);
 
   ~Inner();
 
@@ -93,14 +94,12 @@ class SequencedWorkerPool::Inner {
 
   void FlushForTesting();
 
-  void TriggerSpuriousWorkSignalForTesting();
+  void SignalHasWorkForTesting();
 
   int GetWorkSignalCountForTesting() const;
 
   void Shutdown();
 
-  void SetTestingObserver(TestingObserver* observer);
-
   // Runs the worker loop on the background thread.
   void ThreadLoop(Worker* this_worker);
 
@@ -173,9 +172,6 @@ class SequencedWorkerPool::Inner {
   // tasks are posted or shutdown starts.
   ConditionVariable has_work_cv_;
 
-  // Number of times |has_work_| has been signalled.  Used for testing.
-  int has_work_signal_count_;
-
   // Condition variable that is waited on by non-worker threads (in
   // FlushForTesting()) until IsIdle() goes to true.
   ConditionVariable is_idle_cv_;
@@ -228,7 +224,7 @@ class SequencedWorkerPool::Inner {
   // allowed, though we may still be running existing tasks.
   bool shutdown_called_;
 
-  TestingObserver* testing_observer_;
+  TestingObserver* const testing_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(Inner);
 };
@@ -264,12 +260,12 @@ void SequencedWorkerPool::Worker::Run() {
 SequencedWorkerPool::Inner::Inner(
     SequencedWorkerPool* worker_pool,
     size_t max_threads,
-    const std::string& thread_name_prefix)
+    const std::string& thread_name_prefix,
+    TestingObserver* observer)
     : worker_pool_(worker_pool),
       last_sequence_number_(0),
       lock_(),
       has_work_cv_(&lock_),
-      has_work_signal_count_(0),
       is_idle_cv_(&lock_),
       can_shutdown_cv_(&lock_),
       max_threads_(max_threads),
@@ -280,7 +276,7 @@ SequencedWorkerPool::Inner::Inner(
       pending_task_count_(0),
       blocking_shutdown_pending_task_count_(0),
       shutdown_called_(false),
-      testing_observer_(NULL)  {}
+      testing_observer_(observer) {}
 
 SequencedWorkerPool::Inner::~Inner() {
   // You must call Shutdown() before destroying the pool.
@@ -360,15 +356,10 @@ void SequencedWorkerPool::Inner::FlushForTesting() {
     is_idle_cv_.Wait();
 }
 
-void SequencedWorkerPool::Inner::TriggerSpuriousWorkSignalForTesting() {
+void SequencedWorkerPool::Inner::SignalHasWorkForTesting() {
   SignalHasWork();
 }
 
-int SequencedWorkerPool::Inner::GetWorkSignalCountForTesting() const {
-  AutoLock lock(lock_);
-  return has_work_signal_count_;
-}
-
 void SequencedWorkerPool::Inner::Shutdown() {
   // Mark us as terminated and go through and drop all tasks that aren't
   // required to run on shutdown. Since no new tasks will get posted once the
@@ -383,7 +374,7 @@ void SequencedWorkerPool::Inner::Shutdown() {
 
     // Tickle the threads. This will wake up a waiting one so it will know that
     // it can exit, which in turn will wake up any other waiting ones.
-    has_work_cv_.Signal();
+    SignalHasWork();
 
     // There are no pending or running tasks blocking shutdown, we're done.
     if (CanShutdown())
@@ -407,12 +398,6 @@ void SequencedWorkerPool::Inner::Shutdown() {
                       TimeTicks::Now() - shutdown_wait_begin);
 }
 
-void SequencedWorkerPool::Inner::SetTestingObserver(
-    TestingObserver* observer) {
-  AutoLock lock(lock_);
-  testing_observer_ = observer;
-}
-
 void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
   {
     AutoLock lock(lock_);
@@ -435,7 +420,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
           // worker thread. (Technically not required, since we
           // already get a signal for each new task, but it doesn't
           // hurt.)
-          has_work_cv_.Signal();
+          SignalHasWork();
           delete_these_outside_lock.clear();
 
           // Complete thread creation outside the lock if necessary.
@@ -469,7 +454,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
 
   // We noticed we should exit. Wake up the next worker so it knows it should
   // exit as well (because the Shutdown() code only signals once).
-  has_work_cv_.Signal();
+  SignalHasWork();
 
   // Possibly unblock shutdown.
   can_shutdown_cv_.Signal();
@@ -686,9 +671,8 @@ void SequencedWorkerPool::Inner::FinishStartingAdditionalThread(
 
 void SequencedWorkerPool::Inner::SignalHasWork() {
   has_work_cv_.Signal();
-  {
-    AutoLock lock(lock_);
-    ++has_work_signal_count_;
+  if (testing_observer_) {
+    testing_observer_->OnHasWork();
   }
 }
 
@@ -707,7 +691,17 @@ SequencedWorkerPool::SequencedWorkerPool(
     const std::string& thread_name_prefix)
     : constructor_message_loop_(MessageLoopProxy::current()),
       inner_(new Inner(ALLOW_THIS_IN_INITIALIZER_LIST(this),
-                       max_threads, thread_name_prefix)) {
+                       max_threads, thread_name_prefix, NULL)) {
+  DCHECK(constructor_message_loop_.get());
+}
+
+SequencedWorkerPool::SequencedWorkerPool(
+    size_t max_threads,
+    const std::string& thread_name_prefix,
+    TestingObserver* observer)
+    : constructor_message_loop_(MessageLoopProxy::current()),
+      inner_(new Inner(ALLOW_THIS_IN_INITIALIZER_LIST(this),
+                       max_threads, thread_name_prefix, observer)) {
   DCHECK(constructor_message_loop_.get());
 }
 
@@ -799,12 +793,8 @@ void SequencedWorkerPool::FlushForTesting() {
   inner_->FlushForTesting();
 }
 
-void SequencedWorkerPool::TriggerSpuriousWorkSignalForTesting() {
-  inner_->TriggerSpuriousWorkSignalForTesting();
-}
-
-int SequencedWorkerPool::GetWorkSignalCountForTesting() const {
-  return inner_->GetWorkSignalCountForTesting();
+void SequencedWorkerPool::SignalHasWorkForTesting() {
+  inner_->SignalHasWorkForTesting();
 }
 
 void SequencedWorkerPool::Shutdown() {
@@ -812,8 +802,4 @@ void SequencedWorkerPool::Shutdown() {
   inner_->Shutdown();
 }
 
-void SequencedWorkerPool::SetTestingObserver(TestingObserver* observer) {
-  inner_->SetTestingObserver(observer);
-}
-
 }  // namespace base
diff --git a/base/threading/sequenced_worker_pool.h b/base/threading/sequenced_worker_pool.h
index 961a532..ded2b0a 100644
--- a/base/threading/sequenced_worker_pool.h
+++ b/base/threading/sequenced_worker_pool.h
@@ -122,6 +122,7 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
   class TestingObserver {
    public:
     virtual ~TestingObserver() {}
+    virtual void OnHasWork() = 0;
     virtual void WillWaitForShutdown() = 0;
     virtual void OnDestruct() = 0;
   };
@@ -131,6 +132,12 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
   SequencedWorkerPool(size_t max_threads,
                       const std::string& thread_name_prefix);
 
+  // Like above, but with |observer| for testing.  Does not take
+  // ownership of |observer|.
+  SequencedWorkerPool(size_t max_threads,
+                      const std::string& thread_name_prefix,
+                      TestingObserver* observer);
+
   // Returns a unique token that can be used to sequence tasks posted to
   // PostSequencedWorkerTask(). Valid tokens are alwys nonzero.
   SequenceToken GetSequenceToken();
@@ -219,10 +226,7 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
   void FlushForTesting();
 
   // Spuriously signal that there is work to be done.
-  void TriggerSpuriousWorkSignalForTesting();
-
-  // Get the number of times the work signal has been triggered.
-  int GetWorkSignalCountForTesting() const;
+  void SignalHasWorkForTesting();
 
   // Implements the worker pool shutdown. This should be called during app
   // shutdown, and will discard/join with appropriate tasks before returning.
@@ -231,10 +235,6 @@ class BASE_EXPORT SequencedWorkerPool : public TaskRunner {
   // Must be called from the same thread this object was constructed on.
   void Shutdown();
 
-  // Called by tests to set the testing observer. This is NULL by default
-  // and ownership of the pointer is kept with the caller.
-  void SetTestingObserver(TestingObserver* observer);
-
  protected:
   virtual ~SequencedWorkerPool();
 
diff --git a/base/threading/sequenced_worker_pool_unittest.cc b/base/threading/sequenced_worker_pool_unittest.cc
index 7649a6d..e44599f 100644
--- a/base/threading/sequenced_worker_pool_unittest.cc
+++ b/base/threading/sequenced_worker_pool_unittest.cc
@@ -157,9 +157,10 @@ class SequencedWorkerPoolOwner : public SequencedWorkerPool::TestingObserver {
   SequencedWorkerPoolOwner(size_t max_threads,
                            const std::string& thread_name_prefix)
       : constructor_message_loop_(MessageLoop::current()),
-        pool_(new SequencedWorkerPool(max_threads, thread_name_prefix)) {
-    pool_->SetTestingObserver(this);
-  }
+        pool_(new SequencedWorkerPool(
+            max_threads, thread_name_prefix,
+            ALLOW_THIS_IN_INITIALIZER_LIST(this))),
+        has_work_call_count_(0) {}
 
   virtual ~SequencedWorkerPoolOwner() {
     pool_ = NULL;
@@ -176,8 +177,18 @@ class SequencedWorkerPoolOwner : public SequencedWorkerPool::TestingObserver {
     will_wait_for_shutdown_callback_ = callback;
   }
 
+  int has_work_call_count() const {
+    AutoLock lock(has_work_lock_);
+    return has_work_call_count_;
+  }
+
  private:
   // SequencedWorkerPool::TestingObserver implementation.
+  virtual void OnHasWork() OVERRIDE {
+    AutoLock lock(has_work_lock_);
+    ++has_work_call_count_;
+  }
+
   virtual void WillWaitForShutdown() OVERRIDE {
     if (!will_wait_for_shutdown_callback_.is_null()) {
       will_wait_for_shutdown_callback_.Run();
@@ -194,6 +205,9 @@ class SequencedWorkerPoolOwner : public SequencedWorkerPool::TestingObserver {
   scoped_refptr<SequencedWorkerPool> pool_;
   Closure will_wait_for_shutdown_callback_;
 
+  mutable Lock has_work_lock_;
+  int has_work_call_count_;
+
   DISALLOW_COPY_AND_ASSIGN(SequencedWorkerPoolOwner);
 };
 
@@ -253,6 +267,10 @@ class SequencedWorkerPoolTest : public testing::Test {
     tracker()->ClearCompleteSequence();
   }
 
+  int has_work_call_count() const {
+    return pool_owner_.has_work_call_count();
+  }
+
  private:
   MessageLoop message_loop_;
   SequencedWorkerPoolOwner pool_owner_;
@@ -485,11 +503,11 @@ TEST_F(SequencedWorkerPoolTest, ContinueOnShutdown) {
 // triggered. This is a regression test for http://crbug.com/117469.
 TEST_F(SequencedWorkerPoolTest, SpuriousWorkSignal) {
   EnsureAllWorkersCreated();
-  int old_work_signal_count = pool()->GetWorkSignalCountForTesting();
-  pool()->TriggerSpuriousWorkSignalForTesting();
+  int old_has_work_call_count = has_work_call_count();
+  pool()->SignalHasWorkForTesting();
   // This is inherently racy, but can only produce false positives.
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
-  EXPECT_EQ(old_work_signal_count + 1, pool()->GetWorkSignalCountForTesting());
+  EXPECT_EQ(old_has_work_call_count + 1, has_work_call_count());
 }
 
 class SequencedWorkerPoolTaskRunnerTestDelegate {