Create base::WorkerPoolJob. Use it for HostResolverImpl and DirectoryLister.

base/worker_pool_job.cc

Index: base/worker_pool_job.cc
diff --git a/base/worker_pool_job.cc b/base/worker_pool_job.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8003340278821c589c116e6a4d16186a97311b4d
--- /dev/null
+++ b/base/worker_pool_job.cc
@@ -0,0 +1,117 @@
+// Copyright (c) 2010 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 "base/worker_pool_job.h"
+
+#include "base/logging.h"
+#include "base/message_loop_proxy.h"
+#include "base/worker_pool.h"
+
+namespace base {
+
+WorkerPoolJob::WorkerPoolJob()
+    : origin_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
+      canceled_(false),
+      state_(NONE) {}
+
+WorkerPoolJob::~WorkerPoolJob() {}
+
+void WorkerPoolJob::StartJob() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(state_, NONE);
+  state_ = RUNNING;
+  bool posted = WorkerPool::PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &WorkerPoolJob::RunJobOnWorkerPool),
+      true /* is_slow */);
+  DCHECK(posted);
+}
+
+void WorkerPoolJob::CancelJob() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  {

[eroman, 2010/12/22 23:25:08]

nit: extra scope isn't really necessary.

[willchan no longer on Chromium, 2010/12/24 00:47:34]

Done.

+    AutoLock auto_lock(lock_);
+    DCHECK_GT(state_, NONE);
+    DCHECK_LT(state_, DONE);
+    DCHECK(!canceled_);
+    canceled_ = true;
+  }
+}
+
+bool WorkerPoolJob::canceled() const {
+  // No thread check since RunJob() implementations may check to see if they've
+  // been canceled so they can bail out early.
+  AutoLock auto_lock(lock_);
+  DCHECK_NE(state_, NONE);
+  return canceled_;
+}
+
+bool WorkerPoolJob::is_running() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  AutoLock auto_lock(lock_);
+  DCHECK_NE(state_, NONE);
+  return state_ == RUNNING;
+}
+
+bool WorkerPoolJob::started() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  AutoLock auto_lock(lock_);
+  return state_ != NONE;
+}
+
+bool WorkerPoolJob::done() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  AutoLock auto_lock(lock_);
+  return state_ > FINISHING;
+}
+
+void WorkerPoolJob::RunJobOnWorkerPool() {
+  DCHECK(!thread_checker_.CalledOnValidThread());
+
+  RunJob();
+
+  // Optimization to avoid posting to origin loop if we're already canceled.
+  bool canceled = false;
+  {
+    AutoLock auto_lock(lock_);
+    canceled = canceled_;
+  }
+
+  if (canceled) {
+    {
+      AutoLock auto_lock(lock_);
+      state_ = CANCELED_ON_WORKER;

[eroman, 2010/12/22 23:25:08]

Can these be simplified? Seems like a bool would mostly work here. My main
readability issue is thing liks "<" comparisons on the state enum, and
acquisition of the locks.

[willchan no longer on Chromium, 2010/12/24 00:47:34]

I think that a single bool would not be able to encompass all the states.  If
the < comparisons are the thing that's bothering you, then I can use a series of
== and != conditions that will be a bit wordier, but perhaps more readable. 
Would that be preferable?

I don't think the enum vs bool issue has anything to do with locks.  Was that an
extra comment that you think it's worth to lose a minor bit of performance for
simpler locking?

+    }
+    return;
+  }
+
+  {
+    AutoLock auto_lock(lock_);

[eroman, 2010/12/22 23:25:08]

Seeing how you use MessageLoopProxy for origin_loop_, what about just getting
rid of the locking alltogether and only read/write cancelled from the origin
loop? The minor optimization of avoiding a posttask when cancellation happens
before end doesn't seem worth the extra code to me.

[willchan no longer on Chromium, 2010/12/24 00:47:34]

Done.

+    state_ = FINISHING;
+  }
+
+  origin_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &WorkerPoolJob::CompleteJobOnOriginLoop));
+}
+
+void WorkerPoolJob::CompleteJobOnOriginLoop() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // No need to lock, since the only relevant writes happen on the worker
+  // thread, and posting a task will invoke a memory barrier, so those writes
+  // are guaranteed to be visible.
+  DCHECK_EQ(state_, FINISHING);
+
+  // No need to lock, since writes only happen on origin loop.
+  if (canceled_) {
+    state_ = DONE;
+    return;
+  }
+
+  CompleteJob();
+
+  state_ = DONE;
+}
+
+}  // namespace base