Create base::test::ScopedTaskScheduler.

Create base::test::ScopedTaskScheduler.

ScopedTaskScheduler initializes a TaskScheduler and allows usage of the
base/task_scheduler/post_task.h API within a scope. To make tests
deterministic, the TaskScheduler initialized by ScopedTaskScheduler has
a single thread.

BUG=553459
Committed: https://crrev.com/8602d8a7269695a80f9d796ba7b2d69209f8cbdd
Cr-Commit-Position: refs/heads/master@{#433702}

[fdoray, 2016-11-16 20:17:33]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

[fdoray, 2016-11-16 20:17:33]

PTAL

[robliao, 2016-11-16 21:23:05]

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:40: DCHECK(TaskScheduler::GetInstance());
Maybe we can keep a pointer and check the pointer is still equal to
TaskScheduler::GetInstance() to guard against folks who may initialize a task
scheduler down the line without checking GetInstance().

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
base/threading/sequenced_worker_pool_unittest.cc:290:
TaskScheduler::GetInstance()->JoinForTesting();
Should this test instead use a scoped_ptr<ScopedTaskScheduler>?

[gab, 2016-11-16 21:29:19]

lgtm % name

https://codereview.chromium.org/2511473004/diff/20001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/BUILD.gn#newc...
base/test/BUILD.gn:81: "scoped_task_scheduler.h",
I'd call it TestTaskScheduler to match other such constructs, e.g.,
TestBrowserThreadBundle.

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.h (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.h:14: // base/task_scheduler/post_task.h API
within a scope.
s/a/its/

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
base/threading/sequenced_worker_pool_unittest.cc:290:
TaskScheduler::GetInstance()->JoinForTesting();
On 2016/11/16 21:23:05, robliao wrote:
> Should this test instead use a scoped_ptr<ScopedTaskScheduler>?

Being a scheduler test it should probably still test the full scheduler instead
of a single-threaded one?

[robliao, 2016-11-16 21:31:32]

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
base/threading/sequenced_worker_pool_unittest.cc:290:
TaskScheduler::GetInstance()->JoinForTesting();
On 2016/11/16 21:29:19, gab wrote:
> On 2016/11/16 21:23:05, robliao wrote:
> > Should this test instead use a scoped_ptr<ScopedTaskScheduler>?
> 
> Being a scheduler test it should probably still test the full scheduler
instead
> of a single-threaded one?

Now that I think about it, is there any harm in allowing callers to specify how
many threads they do want? We default to 1 and if more are needed, you specify
that.

This way we can avoid a lot of folks needing to create a single worker pool with
x number of threads.

[gab, 2016-11-16 21:38:15]

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
base/threading/sequenced_worker_pool_unittest.cc:290:
TaskScheduler::GetInstance()->JoinForTesting();
On 2016/11/16 21:31:31, robliao wrote:
> On 2016/11/16 21:29:19, gab wrote:
> > On 2016/11/16 21:23:05, robliao wrote:
> > > Should this test instead use a scoped_ptr<ScopedTaskScheduler>?
> > 
> > Being a scheduler test it should probably still test the full scheduler
> instead
> > of a single-threaded one?
> 
> Now that I think about it, is there any harm in allowing callers to specify
how
> many threads they do want? We default to 1 and if more are needed, you specify
> that.
> 
> This way we can avoid a lot of folks needing to create a single worker pool
with
> x number of threads.

We could definitely have an alternate constructor which allows specifying number
of threads. I'm not convinced we should use test helpers in our own unit tests
though. Testing the full API makes more sense to me here.

[fdoray, 2016-11-16 22:04:45]

robliao@: PTAnL

https://codereview.chromium.org/2511473004/diff/20001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/BUILD.gn#newc...
base/test/BUILD.gn:81: "scoped_task_scheduler.h",
On 2016/11/16 21:29:19, gab wrote:
> I'd call it TestTaskScheduler to match other such constructs, e.g.,
> TestBrowserThreadBundle.

Discussed offline. We're keeping ScopedTaskScheduler because the class sets
global state for a scope. The test:: namespace makes it obvious that it is meant
to be used in tests.

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:40: DCHECK(TaskScheduler::GetInstance());
On 2016/11/16 21:23:05, robliao wrote:
> Maybe we can keep a pointer and check the pointer is still equal to
> TaskScheduler::GetInstance() to guard against folks who may initialize a task
> scheduler down the line without checking GetInstance().

Done.

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.h (right):

https://codereview.chromium.org/2511473004/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.h:14: // base/task_scheduler/post_task.h API
within a scope.
On 2016/11/16 21:29:19, gab wrote:
> s/a/its/

Done.

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://codereview.chromium.org/2511473004/diff/20001/base/threading/sequence...
base/threading/sequenced_worker_pool_unittest.cc:290:
TaskScheduler::GetInstance()->JoinForTesting();
On 2016/11/16 21:38:15, gab wrote:
> On 2016/11/16 21:31:31, robliao wrote:
> > On 2016/11/16 21:29:19, gab wrote:
> > > On 2016/11/16 21:23:05, robliao wrote:
> > > > Should this test instead use a scoped_ptr<ScopedTaskScheduler>?
> > > 
> > > Being a scheduler test it should probably still test the full scheduler
> > instead
> > > of a single-threaded one?
> > 
> > Now that I think about it, is there any harm in allowing callers to specify
> how
> > many threads they do want? We default to 1 and if more are needed, you
specify
> > that.
> > 
> > This way we can avoid a lot of folks needing to create a single worker pool
> with
> > x number of threads.
> 
> We could definitely have an alternate constructor which allows specifying
number
> of threads. I'm not convinced we should use test helpers in our own unit tests
> though. Testing the full API makes more sense to me here.

Discussed offline. Custom thread count is only useful to test the TaskScheduler
itself. Tests that do that should use the production API to instantiate
TaskScheduler.

[robliao, 2016-11-16 22:09:56]

lgtm

[gab, 2016-11-16 22:17:43]

lgtm % nit

https://codereview.chromium.org/2511473004/diff/40001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.h (right):

https://codereview.chromium.org/2511473004/diff/40001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.h:33: TaskScheduler* task_scheduler_ = nullptr;
const TaskScheduler*

[fdoray, 2016-11-16 22:45:45]

fdoray@chromium.org changed reviewers:
+ danakj@chromium.org

[fdoray, 2016-11-16 22:45:46]

danakj@: PTAL

https://codereview.chromium.org/2511473004/diff/40001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.h (right):

https://codereview.chromium.org/2511473004/diff/40001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.h:33: TaskScheduler* task_scheduler_ = nullptr;
On 2016/11/16 22:17:43, gab wrote:
> const TaskScheduler*

Done.

[danakj, 2016-11-17 00:48:01]

LGTM

https://codereview.chromium.org/2511473004/diff/60001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2511473004/diff/60001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:20: size_t GetWorkerPoolIndexForTraits(const
TaskTraits&) {
Not used?

[fdoray, 2016-11-17 13:25:25]

https://codereview.chromium.org/2511473004/diff/60001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2511473004/diff/60001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:20: size_t GetWorkerPoolIndexForTraits(const
TaskTraits&) {
On 2016/11/17 00:48:01, danakj wrote:
> Not used?

Done.

[gab, 2016-11-17 14:39:35]

https://codereview.chromium.org/2511473004/diff/100001/base/test/scoped_task_...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2511473004/diff/100001/base/test/scoped_task_...
base/test/scoped_task_scheduler.cc:18: const TimeDelta kDetachDuration =
base::TimeDelta::Max();
FYI, TimeDelta can be constexpr too (but by comment on other CL I suspect we
don't need this param).

[fdoray, 2016-11-17 19:58:31]

gab@: PTAnL. The CL now uses CreateAndSet*Default*TaskScheduler() instead of
CreateAndSet*Simple*TaskScheduler() - because threads that can be detached can't
be joined.

[gab, 2016-11-17 20:52:23]

lgtm

[fdoray, 2016-11-21 16:35:00]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-21 16:35:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2511473004/#ps120001
(title: "Use CreateAndSetDefaultTaskScheduler")

[commit-bot: I haz the power, 2016-11-21 16:35:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 17:47:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 17:47:14]

Failed to apply patch for base/test/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: base/test/BUILD.gn:77
  error: base/test/BUILD.gn: patch does not apply

Patch:       base/test/BUILD.gn
Index: base/test/BUILD.gn
diff --git a/base/test/BUILD.gn b/base/test/BUILD.gn
index
788494ff6d86e148ff551ee5790511a3b77c542b..bafa596fdb5ba57ee2e88427a3d1d4d911083161
100644
--- a/base/test/BUILD.gn
+++ b/base/test/BUILD.gn
@@ -77,6 +77,8 @@ static_library("test_support") {
     "scoped_locale.h",
     "scoped_path_override.cc",
     "scoped_path_override.h",
+    "scoped_task_scheduler.cc",
+    "scoped_task_scheduler.h",
     "sequenced_task_runner_test_template.cc",
     "sequenced_task_runner_test_template.h",
     "sequenced_worker_pool_owner.cc",

[fdoray, 2016-11-21 20:52:06]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-21 20:52:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, danakj@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2511473004/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-21 20:52:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 21:13:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 21:13:38]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fdoray, 2016-11-21 21:49:14]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-21 21:49:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, danakj@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2511473004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-21 21:50:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 23:53:35]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1479764954129670,
"parent_rev": "cfca1e22844da39e235bd128e92b6eefe5aa5021", "commit_rev":
"9358bbfb0fde167b7ec816ed376ac8bcc1be7f22"}

[commit-bot: I haz the power, 2016-11-21 23:54:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-21 23:56:42]

Description was changed from

==========
Create base::test::ScopedTaskScheduler.

ScopedTaskScheduler initializes a TaskScheduler and allows usage of the
base/task_scheduler/post_task.h API within a scope. To make tests
deterministic, the TaskScheduler initialized by ScopedTaskScheduler has
a single thread.

BUG=553459
==========

to

==========
Create base::test::ScopedTaskScheduler.

ScopedTaskScheduler initializes a TaskScheduler and allows usage of the
base/task_scheduler/post_task.h API within a scope. To make tests
deterministic, the TaskScheduler initialized by ScopedTaskScheduler has
a single thread.

BUG=553459

Committed: https://crrev.com/8602d8a7269695a80f9d796ba7b2d69209f8cbdd
Cr-Commit-Position: refs/heads/master@{#433702}
==========

[commit-bot: I haz the power, 2016-11-21 23:56:46]

Patchset 9 (id:??) landed as
https://crrev.com/8602d8a7269695a80f9d796ba7b2d69209f8cbdd
Cr-Commit-Position: refs/heads/master@{#433702}