Make SequencedWorkerPool a TaskRunner

Make SequencedWorkerPool a TaskRunner

Add basic tests for TaskRunner implementations, and
instantiate it for SequencedWorkerPool.

BUG=114329, 114327
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124087

[akalin, 2012-02-16 02:33:03]

+brettw for SWP stuff, +willchan for everything else

Note that this depends on https://chromiumcodereview.appspot.com/9347056/ .

[willchan no longer on Chromium, 2012-02-19 06:37:00]

Looks like a lot of activity is happening on the other thread, so I assume I can
wait on this one.

http://codereview.chromium.org/9401032/diff/1/base/threading/sequenced_worker...
File base/threading/sequenced_worker_pool.h (right):

http://codereview.chromium.org/9401032/diff/1/base/threading/sequenced_worker...
base/threading/sequenced_worker_pool.h:139: // Returns a TaskRunner which posts
(unseqenced) tasks on this
unsequenced

[akalin, 2012-02-22 22:39:11]

Updated with changes from 9347056.  This patch is simpler now, since
SequencedWorkerPool can just inherit from TaskRunner directly.

No need to take a look until the other patch set converges.

http://codereview.chromium.org/9401032/diff/1/base/threading/sequenced_worker...
File base/threading/sequenced_worker_pool.h (right):

http://codereview.chromium.org/9401032/diff/1/base/threading/sequenced_worker...
base/threading/sequenced_worker_pool.h:139: // Returns a TaskRunner which posts
(unseqenced) tasks on this
On 2012/02/19 06:37:01, willchan wrote:
> unsequenced

Rendered moot (since this function doesn't exist anymore)

[willchan no longer on Chromium, 2012-02-23 19:39:57]

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.cc
File base/task_runner_unittest.cc (right):

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.c...
base/task_runner_unittest.cc:5: #include "base/task_runner_unittest.h"
I don't like the filename, since it makes one think this is a test of the
TaskRunner interface, whereas it's a template for testing implementations of
this interface. I'd reserve _unittest.cc files for actual tests, and not utility
files. Can you name it slightly different to indicate it's a
utility/template/whatever?

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h
File base/task_runner_unittest.h (right):

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:104: for (int i = 0; i < 20; ++i) {
Add comments explaining what the two loops are doing. It wasn't immediately
obvious we were running task 1 one time, task 2 two times, etc.

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:111: this->delegate_.StopTaskRunner();
I don't know that this is correct. There's no guarantee that when the TaskRunner
is stopped that it needs to execute all the tasks left in its task queue.
Perhaps the test should only call this after it detects that all tasks have
finished running.

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:138: 
We should add TODOs for future tests to verify RunsTaskOnCurrentThread() returns
true for Tasks run on the TaskRunner. And then spawn a PlatformThread that
checks RunsTaskOnCurrentThread() returns false for the TaskRunner on that new
thread.

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:152: DLOG_IF(WARNING, delay_ms > 0) <<
"Ignoring delay of " << delay_ms << " ms";
I suggest NOTREACHED() instead. If this starts happening, we should know about
it and change the consumer or actually implement it.

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
this.
When is this coming? Do not land without it, or use a NOTREACHED() to make sure
no one is calling it.

[akalin, 2012-02-23 22:52:20]

All comments addressed, PTAL

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.cc
File base/task_runner_unittest.cc (right):

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.c...
base/task_runner_unittest.cc:5: #include "base/task_runner_unittest.h"
On 2012/02/23 19:39:57, willchan wrote:
> I don't like the filename, since it makes one think this is a test of the
> TaskRunner interface, whereas it's a template for testing implementations of
> this interface. I'd reserve _unittest.cc files for actual tests, and not
utility
> files. Can you name it slightly different to indicate it's a
> utility/template/whatever?

Done.

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h
File base/task_runner_unittest.h (right):

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:104: for (int i = 0; i < 20; ++i) {
On 2012/02/23 19:39:57, willchan wrote:
> Add comments explaining what the two loops are doing. It wasn't immediately
> obvious we were running task 1 one time, task 2 two times, etc.

Done.

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:111: this->delegate_.StopTaskRunner();
On 2012/02/23 19:39:57, willchan wrote:
> I don't know that this is correct. There's no guarantee that when the
TaskRunner
> is stopped that it needs to execute all the tasks left in its task queue.
> Perhaps the test should only call this after it detects that all tasks have
> finished running.

It should be the job of the delegate class to do the detection that all tasks
have finished running (if needed).  I think that's covered by the comments at
the top re. the delegates.

http://codereview.chromium.org/9401032/diff/10011/base/task_runner_unittest.h...
base/task_runner_unittest.h:138: 
On 2012/02/23 19:39:57, willchan wrote:
> We should add TODOs for future tests to verify RunsTaskOnCurrentThread()
returns
> true for Tasks run on the TaskRunner. And then spawn a PlatformThread that
> checks RunsTaskOnCurrentThread() returns false for the TaskRunner on that new
> thread.

Done.

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:152: DLOG_IF(WARNING, delay_ms > 0) <<
"Ignoring delay of " << delay_ms << " ms";
On 2012/02/23 19:39:57, willchan wrote:
> I suggest NOTREACHED() instead. If this starts happening, we should know about
> it and change the consumer or actually implement it.

Done.  Used DCHECK_EQ instead of NOTREACHED().  Added delegate method to skip
testing delays (since it'll hit the NOTREACHED()).

http://codereview.chromium.org/9401032/diff/10011/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
this.
On 2012/02/23 19:39:57, willchan wrote:
> When is this coming? Do not land without it, or use a NOTREACHED() to make
sure
> no one is calling it.

Use NOTREACHED().  I'll see if I can implement this in an upcoming CL.

[brettw, 2012-02-24 05:25:51]

LGTM

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
this.
Can you clarify what this comment is trying to say? Also, I'm not sure the
NOTREACHED is necessary. According to the docs, the current behavior is valid,
and this is the type of thing that I suspect we won't get to soon. It will be
confusing for somebody to call RunsTasksOnCurrentThread and see an assertion
here.

[akalin, 2012-02-27 19:58:33]

>
http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
> base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
> this.
> Can you clarify what this comment is trying to say? Also, I'm not sure the
> NOTREACHED is necessary. According to the docs, the current behavior is valid,
> and this is the type of thing that I suspect we won't get to soon. It will be
> confusing for somebody to call RunsTasksOnCurrentThread and see an assertion
> here.

I was gonna try to get to it immediately after this patch lands, actually.

But this brings up an overall issue.  The TaskRunner docs allow some behavior
that is useful mainly for test implementations: the delay can be ignored, and
RunsTasksOnCurrentThread can return true if unsure (although this made more
sense when it was named 'MayRunTasksOnCurrentThread').  On the other hand, we
want to warn clients if a real implementation temporarily uses one of these
behaviors.

I think DCHECKing that the delay is 0 is fine, but I'm not sure about
NOTREACHED() on RunsTasksOnCurrentThread, since those are used mainly for
assertions.  Thoughts, willchan?

[willchan no longer on Chromium, 2012-02-28 22:29:53]

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
this.
On 2012/02/24 05:25:51, brettw wrote:
> Can you clarify what this comment is trying to say? Also, I'm not sure the
> NOTREACHED is necessary. According to the docs, the current behavior is valid,
> and this is the type of thing that I suspect we won't get to soon. It will be
> confusing for somebody to call RunsTasksOnCurrentThread and see an assertion
> here.

I think we don't want people to call this function until we support it, since it
won't be doing the correct assertions. I guess there's a tradeoff here between
potentially having some people get confused about hitting a NOTREACHED() (and
then removing their assertion) vs people expecting an assertion to work, and
then having to fix a bunch of assertions later on when we implement this.

I think I prefer the former, since I don't want to place undue burden on the
implementer to fix all call sites later on.

I defer to Brett here.

[akalin, 2012-02-29 00:17:42]

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/9401032/diff/16002/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:157: // TODO(akalin): Add support for
this.
On 2012/02/24 05:25:51, brettw wrote:
> Can you clarify what this comment is trying to say? Also, I'm not sure the
> NOTREACHED is necessary. According to the docs, the current behavior is valid,
> and this is the type of thing that I suspect we won't get to soon. It will be
> confusing for somebody to call RunsTasksOnCurrentThread and see an assertion
> here.

Clarified comment.  I think I agree with willchan here.  I'll check this in now
(since you LGTMed it) and will work on actually implementing that function,
rendering this moot.

[commit-bot: I haz the power, 2012-02-29 00:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9401032/30001

[Nico, 2012-02-29 01:26:55]

http://codereview.chromium.org/9401032/diff/30001/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool_unittest.cc (right):

http://codereview.chromium.org/9401032/diff/30001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:426: // TODO(akalin): Set this
to true once we SequencedWorkerPool
s/we //

[akalin, 2012-02-29 01:32:48]

done.  committing now.

http://codereview.chromium.org/9401032/diff/30001/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool_unittest.cc (right):

http://codereview.chromium.org/9401032/diff/30001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:426: // TODO(akalin): Set this
to true once we SequencedWorkerPool
On 2012/02/29 01:26:55, Nico wrote:
> s/we //

Done.

[akalin, 2012-02-29 05:34:33]

Oops, willchan, I think I forgot to wait for your LGTM since you had comments.
:/

Do you have any more?  If so, I can address them in another CL.

[willchan no longer on Chromium, 2012-02-29 05:41:50]

Sorry, forgot to lgtm.
On Feb 28, 2012 9:34 PM, <akalin@chromium.org> wrote:

> Oops, willchan, I think I forgot to wait for your LGTM since you had
> comments.
> :/
>
> Do you have any more?  If so, I can address them in another CL.
>
>
http://codereview.chromium.**org/9401032/<http://codereview.chromium.org/9401...
>