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

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

This notably changes DirectoryLister to use the worker pool rather than its own joinable thread.  This allows us to avoid joining the thread which would block the IO thread.  It requires leaking ICU locale data on shutdown, so I've switched that to using LeakySingletonTraits.

BUG=65331
TEST=existing

[willchan no longer on Chromium, 2010-12-10 22:14:28]

I haven't commented anything yet.  I'll be doing that today.  I also need some
chromeos reviewers to make sure I'm not borking their DomUIs.  I also need a
reviewer for that FileBrowseHandler thing.

[Paweł Hajdan Jr., 2010-12-11 10:40:18]

Drive-by with a test comment. No need to wait for another review by me.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
base/worker_pool_job_unittest.cc:24: DCHECK(!waiting_for_run_);
This is a test, and DCHECK can make it crashy. How about using
ASSERT_FALSE(waiting_for_run_) instead?

[willchan no longer on Chromium, 2010-12-11 22:44:26]

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
base/worker_pool_job_unittest.cc:24: DCHECK(!waiting_for_run_);
On 2010/12/11 10:40:19, Paweł Hajdan Jr. wrote:
> This is a test, and DCHECK can make it crashy. How about using
> ASSERT_FALSE(waiting_for_run_) instead?

Normally I'd agree.  In this case, this means we've made a pretty fundamental
error in use of the callback.  We're already waiting and we called Wait again. 
That means we're nesting the MessageLoop.  I'd be worried about causing crashes
elsewhere or worse, hanging the test (which would cause long timeouts).  So I'd
rather just crash here.

[willchan no longer on Chromium, 2010-12-12 03:00:53]

[ +johnnyg for DirectoryLister uses in chrome/browser ]

[Paweł Hajdan Jr., 2010-12-13 12:59:41]

On Sat, Dec 11, 2010 at 23:44, <willchan@chromium.org> wrote:

>
>
http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
> File base/worker_pool_job_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job_unittes...
> base/worker_pool_job_unittest.cc:24: DCHECK(!waiting_for_run_);
> On 2010/12/11 10:40:19, Paweł Hajdan Jr. wrote:
>
>> This is a test, and DCHECK can make it crashy. How about using
>> ASSERT_FALSE(waiting_for_run_) instead?
>>
>
> Normally I'd agree.  In this case, this means we've made a pretty
> fundamental error in use of the callback.  We're already waiting and we
> called Wait again.  That means we're nesting the MessageLoop.  I'd be
> worried about causing crashes elsewhere or worse, hanging the test
> (which would cause long timeouts).  So I'd rather just crash here.


OK, that makes sense. Please ignore my earlier comment. You might want to
convert it to CHECK though.

[eroman, 2010-12-22 00:51:46]

http://codereview.chromium.org/5710002/diff/7001/base/i18n/file_util_icu.cc
File base/i18n/file_util_icu.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/i18n/file_util_icu.cc#n...
base/i18n/file_util_icu.cc:85: LeakySingletonTraits<LocaleAwareComparator>
>::get();
What is this change for?

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:25: WorkerPool::PostTask(
PostTask can fail. Please handle that case (at the very least by crashing).

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:75: AutoLock auto_lock(lock_);
This optimization doesn't seem worthwhile. I don't think it would be common at
all to post a task, and then cancel it so quickly that it didn't even get a
chance to run yet.

IMO we are better off just acquiring the lock once when done

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:111: origin_loop_->PostTask(
This is racy.

Access to the origin loop MUST be protected within a lock. Since it is possible
for Cancel() to be called from the origin loop right before we call PostTask.

And Cancel() is basically called to prevent the completion from reaching the
origin loop (since we may be deleting it).

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:33: template <typename JobType>
Why is this templatized? 

Since this is already a base class we should be able to access the protected
members regardless of the derived type.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:53: explicit WorkerPoolJob(bool is_slow);
I think we should get rid of this bool.

No consumer of the workerpool ever specifies anything other than true. And in
fact the boolean is disregarded everywhere except windows.

It makes sense for this to only ever be true, since by definition we wouldn't be
using the worker pool for "fast" jobs.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:54: virtual ~WorkerPoolJob();
Please stress somewhere that subclasses MUST be prepared to be deleted on either
the origin thread, or the worker thread.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:64: // is called.  it's fundamentally a race.  When
cancellation is detected (may
it's --> It's

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:65: // be on the WorkerPool thread or the origin thread),
OnCanceled() is invoked.
Perhaps a more succinct description is to say that we are guaranteed
CompleteJob() will never be called after CancelJob() has been called.

Seems like this isn't currently true, but see my rant below about simplifying
the cancellation semantics.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:108: virtual void RunJob() = 0;
Listing pure virtual methods under private section seems weird to me. I would
put them under at least "protected".

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:116: virtual void OnCanceled() {}
Please, no.

First of all, I don't like default virtual method implementations, they can
cause problems.

Secondly, I don't think it makes sense to have a cancellation callback.

The *client* is the one calling Cancel(), as a way to tell the job to not notify
them of completion. It seems overly complicated to have this funky OnCancelled()
callback that can be called from either the worker thread, or the origin loop.

If the client cares about running other stuff on cancellation, they can very
well just call another method right after calling Cancel(). But I think it
breaks the cancellation pattern to be calling other functions in response to it.

[willchan no longer on Chromium, 2010-12-22 02:04:00]

I've addressed most comments other than the cancellation one which requires me
to rethink the tests.

http://codereview.chromium.org/5710002/diff/7001/base/i18n/file_util_icu.cc
File base/i18n/file_util_icu.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/i18n/file_util_icu.cc#n...
base/i18n/file_util_icu.cc:85: LeakySingletonTraits<LocaleAwareComparator>
>::get();
On 2010/12/22 00:51:47, eroman wrote:
> What is this change for?

This is explained in the changelist description.  This data is used on the
worker thread, so I did this to avoid shutdown crashes.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:25: WorkerPool::PostTask(
On 2010/12/22 00:51:47, eroman wrote:
> PostTask can fail. Please handle that case (at the very least by crashing).

Done.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:75: AutoLock auto_lock(lock_);
On 2010/12/22 00:51:47, eroman wrote:
> This optimization doesn't seem worthwhile. I don't think it would be common at
> all to post a task, and then cancel it so quickly that it didn't even get a
> chance to run yet.
> 
> IMO we are better off just acquiring the lock once when done

OK.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:111: origin_loop_->PostTask(
On 2010/12/22 00:51:47, eroman wrote:
> This is racy.
> 
> Access to the origin loop MUST be protected within a lock. Since it is
possible
> for Cancel() to be called from the origin loop right before we call PostTask.
> 
> And Cancel() is basically called to prevent the completion from reaching the
> origin loop (since we may be deleting it).

I don't believe it's racy.  |origin_loop_| is a MessageLoopProxy, so it won't
crash on the PostTask() even if CancelJob() is called. 
CompleteJobOnOriginLoop() will check |canceled_| before calling the
CompleteJob() private virtual.  So cancellation seems to me like it'll still
work.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:33: template <typename JobType>
On 2010/12/22 00:51:47, eroman wrote:
> Why is this templatized? 
> 
> Since this is already a base class we should be able to access the protected
> members regardless of the derived type.

The job() accessor needs the templated param to control the return type.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:53: explicit WorkerPoolJob(bool is_slow);
On 2010/12/22 00:51:47, eroman wrote:
> I think we should get rid of this bool.
> 
> No consumer of the workerpool ever specifies anything other than true. And in
> fact the boolean is disregarded everywhere except windows.
> 
> It makes sense for this to only ever be true, since by definition we wouldn't
be
> using the worker pool for "fast" jobs.

Done.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:54: virtual ~WorkerPoolJob();
On 2010/12/22 00:51:47, eroman wrote:
> Please stress somewhere that subclasses MUST be prepared to be deleted on
either
> the origin thread, or the worker thread.

Done.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:64: // is called.  it's fundamentally a race.  When
cancellation is detected (may
On 2010/12/22 00:51:47, eroman wrote:
> it's --> It's

Done.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:65: // be on the WorkerPool thread or the origin thread),
OnCanceled() is invoked.
On 2010/12/22 00:51:47, eroman wrote:
> Perhaps a more succinct description is to say that we are guaranteed
> CompleteJob() will never be called after CancelJob() has been called.
> 
> Seems like this isn't currently true, but see my rant below about simplifying
> the cancellation semantics.

I'm going to wait until we settle the cancellation semantics.  It's been awhile
since I looked at this, but I grudgingly added OnCanceled() to make the tests
work.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:108: virtual void RunJob() = 0;
On 2010/12/22 00:51:47, eroman wrote:
> Listing pure virtual methods under private section seems weird to me. I would
> put them under at least "protected".

No, it's more correct to make them private.  That means only the base type can
call the virtual.  protected would allow subtypes to also call the member
function.  Allowing the subtype to call RunJob() would be a bug.

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.h#newco...
base/worker_pool_job.h:116: virtual void OnCanceled() {}
On 2010/12/22 00:51:47, eroman wrote:
> Please, no.
> 
> First of all, I don't like default virtual method implementations, they can
> cause problems.
> 
> Secondly, I don't think it makes sense to have a cancellation callback.
> 
> The *client* is the one calling Cancel(), as a way to tell the job to not
notify
> them of completion. It seems overly complicated to have this funky
OnCancelled()
> callback that can be called from either the worker thread, or the origin loop.
> 
> If the client cares about running other stuff on cancellation, they can very
> well just call another method right after calling Cancel(). But I think it
> breaks the cancellation pattern to be calling other functions in response to
it.

I share your concerns here.  I did it just because it made the tests easy.  I
will look into reworking that.

[willchan no longer on Chromium, 2010-12-22 02:50:38]

Ok, I've updated it to remove cancellation.  It weakened the unit tests a bit,
but that's fine.  We also lose some flexibility (I was trying to allow
preserving some bad semantic behavior of the DirectoryLister, but screw it,
we'll just forget about joining the DirectoryLister worker job).  This is
definitely cleaner.

[eroman, 2010-12-22 04:58:48]

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/7001/base/worker_pool_job.cc#newc...
base/worker_pool_job.cc:111: origin_loop_->PostTask(
On 2010/12/22 02:04:01, willchan wrote:
> On 2010/12/22 00:51:47, eroman wrote:
> > This is racy.
> > 
> > Access to the origin loop MUST be protected within a lock. Since it is
> possible
> > for Cancel() to be called from the origin loop right before we call
PostTask.
> > 
> > And Cancel() is basically called to prevent the completion from reaching the
> > origin loop (since we may be deleting it).
> 
> I don't believe it's racy.  |origin_loop_| is a MessageLoopProxy, so it won't
> crash on the PostTask() even if CancelJob() is called. 
> CompleteJobOnOriginLoop() will check |canceled_| before calling the
> CompleteJob() private virtual.  So cancellation seems to me like it'll still
> work.

Oh, my bad! I didn't realize origin_loop_ was a MessageLoopProxy (when I read
through this I assumed it was a MessageLoop)

[eroman, 2010-12-22 23:25:07]

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:33: {
nit: extra scope isn't really necessary.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:84: state_ = CANCELED_ON_WORKER;
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.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:90: AutoLock auto_lock(lock_);
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.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:15: class MessageLoop;
this is not used.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:38: ~ScopedHandle() {
Can't this simply be job_->CancelJob() ?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:76: bool is_running() const;
This seems like it should be only expose to unit-tests, as I don't think we want
production code to rely on this.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:89: template <typename JobType> friend class
ScopedHandle;
See earlier comment, I don't think we need to friend here if it just calls
CancelJob().

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:109: virtual void CompleteJob() = 0;
I think this would be better named OnJobCompleted(), since it is more of a
delegate method than it is an action method.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:116: bool started() const;
Is this needed other than for unittests?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:16: class TestCallback : public
CallbackRunner<Tuple0> {
Can you just fold this logic into TestJob by having a "WaitUntilCompleted()"
method?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:88: TEST(WorkerPoolJobTest, CancelOnWorker) {
I believe this test may result in a leak report.

Since we don't join on the worker thread, isn't it possible for TestJob to still
be referenced and running on the thread?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:117: MessageLoop::current()->RunAllPending();
The timings are hard to reason about here as well. Seems possible we may still
end up destroying the MessageLoop while there is an outstanding task on it.

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

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:33: {
On 2010/12/22 23:25:08, eroman wrote:
> nit: extra scope isn't really necessary.

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:84: state_ = CANCELED_ON_WORKER;
On 2010/12/22 23:25:08, eroman wrote:
> 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.

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?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:90: AutoLock auto_lock(lock_);
On 2010/12/22 23:25:08, eroman wrote:
> 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.

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:15: class MessageLoop;
On 2010/12/22 23:25:08, eroman wrote:
> this is not used.

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:38: ~ScopedHandle() {
On 2010/12/22 23:25:08, eroman wrote:
> Can't this simply be job_->CancelJob() ?

No.  I am a bit anal about cancellation.  I want people only to call CancelJob()
when they have something to cancel.  ScopedHandle reads into the private state
to see if it needs to cancel or not.  I'm fine with ScopedHandle doing that
since people usually aren't canceling themselves if they're using the handle. 
But if they are doing things manually, then I want them to cancel only once. 
Multiple invocations is usually a sign of a bug or arguably bad design.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:76: bool is_running() const;
On 2010/12/22 23:25:08, eroman wrote:
> This seems like it should be only expose to unit-tests, as I don't think we
want
> production code to rely on this.

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:89: template <typename JobType> friend class
ScopedHandle;
On 2010/12/22 23:25:08, eroman wrote:
> See earlier comment, I don't think we need to friend here if it just calls
> CancelJob().

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:109: virtual void CompleteJob() = 0;
On 2010/12/22 23:25:08, eroman wrote:
> I think this would be better named OnJobCompleted(), since it is more of a
> delegate method than it is an action method.

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:116: bool started() const;
On 2010/12/22 23:25:08, eroman wrote:
> Is this needed other than for unittests?

Removed

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:16: class TestCallback : public
CallbackRunner<Tuple0> {
On 2010/12/22 23:25:08, eroman wrote:
> Can you just fold this logic into TestJob by having a "WaitUntilCompleted()"
> method?

Done.

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:88: TEST(WorkerPoolJobTest, CancelOnWorker) {
On 2010/12/22 23:25:08, eroman wrote:
> I believe this test may result in a leak report.
> 
> Since we don't join on the worker thread, isn't it possible for TestJob to
still
> be referenced and running on the thread?

I do not believe that's how valgrind works.  I think it will look at the stack
variables for the worker thread and see that the object is still referenced. 
Otherwise, wouldn't we have a bunch of leaks for the getaddrinfo() objects on
worker threads?

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:117: MessageLoop::current()->RunAllPending();
On 2010/12/22 23:25:08, eroman wrote:
> The timings are hard to reason about here as well. Seems possible we may still
> end up destroying the MessageLoop while there is an outstanding task on it.

What would be wrong with that?  Under valgrind, the MessageLoop will delete the
outstanding tasks.  What kind of error are you worried about?

[eroman, 2010-12-24 00:54:42]

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:88: TEST(WorkerPoolJobTest, CancelOnWorker) {
On 2010/12/24 00:47:34, willchan wrote:
> On 2010/12/22 23:25:08, eroman wrote:
> > I believe this test may result in a leak report.
> > 
> > Since we don't join on the worker thread, isn't it possible for TestJob to
> still
> > be referenced and running on the thread?
> 
> I do not believe that's how valgrind works.  I think it will look at the stack
> variables for the worker thread and see that the object is still referenced. 
> Otherwise, wouldn't we have a bunch of leaks for the getaddrinfo() objects on
> worker threads?

Outstanding worker threads at shutdown causes leaks in valgrind.
This comes up all the time with UI tests in the context of DNS threads, and we
have particular suppressions for that.

If you can avoid it though, you don't want to tickle this in your own unittests.
See for example crbug.com/52459.

[eroman, 2011-01-05 01:24:52]

Can you re-upload, since some of the files are not readable (have rietevield
errors).

The newly added files LGTM.

(I would like to do a review of the others though, ping me when they are
uploaded).

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/23001/base/worker_pool_job.h#newc...
base/worker_pool_job.h:38: ~ScopedHandle() {
On 2010/12/24 00:47:34, willchan wrote:
> On 2010/12/22 23:25:08, eroman wrote:
> > Can't this simply be job_->CancelJob() ?
> 
> No.  I am a bit anal about cancellation.  I want people only to call
CancelJob()
> when they have something to cancel.  ScopedHandle reads into the private state
> to see if it needs to cancel or not.  I'm fine with ScopedHandle doing that
> since people usually aren't canceling themselves if they're using the handle. 
> But if they are doing things manually, then I want them to cancel only once. 
> Multiple invocations is usually a sign of a bug or arguably bad design.

FWIW we use the pattern of calling Cancel() on already cancelled objects in
other places. Especially in destructors which blanket cancel stuff without
checking if they have already been cancelled.

I am ok with this though.

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc
File base/worker_pool_job.cc (right):

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:22: DCHECK_EQ(state_, NONE);
Can the state DCHECK be more restrictive?

i.e, I don't think we want to transition from RUNNING --> RUNNING.

Also it isn't clear that it would be valid to call StartJob() more than once.

(For example if we wanted to allow calling StartJob() after cancellation, it
should reset canceled_).

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:34: DCHECK_NE(state_, DONE);
Can these two checks be combined into:
DCHECK_EQ(RUNNING, state_)

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:41: // been canceled so they can bail out early.
I don't think this comment is true anymore.

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:58: DCHECK_NE(state_, NONE);
Can these two statnments be combined into:

DCHECK_EQ(RUNNING, state_)

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.cc#new...
base/worker_pool_job.cc:69: state_ = DONE;
nit: I wander if you want to set state_ to DONE before calling OnJobCompleted(),
to catch if we try to cancel/start the job again inside that handler.

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.h
File base/worker_pool_job.h (right):

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job.h#newc...
base/worker_pool_job.h:101: // it may never be invoked.
This wording can be stronger. If CancelJob() was called, then OnJobCompleted()
will definitely not be called (since both run on the origin thread).

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job_unitte...
File base/worker_pool_job_unittest.cc (right):

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
nit: might consider changing these to 2011

http://codereview.chromium.org/5710002/diff/22017/base/worker_pool_job_unitte...
base/worker_pool_job_unittest.cc:18: explicit TestJob()
is explicit needed?