TaskScheduler: Make SchedulerWorkerThread own its delegate.

TaskScheduler: Make SchedulerWorkerThread own its delegate.

This will allow us to have one delegate per SchedulerWorkerThread and
to maintain state in that delegate.

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

[fdoray, 2016-04-21 19:34:11]

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

[fdoray, 2016-04-21 19:34:11]

Can you review this CL? Thanks.

[gab, 2016-04-22 19:30:29]

lgtm w/ nits

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:39: WrapUnique(new
SchedulerWorkerThreadDelegateImpl(this)),
This compiles without a fwd-decl of SchedulerWorkerThreadDelegateTestImpl?

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:87: class
SchedulerWorkerThreadDelegateImpl
Name this differently than main impl, e.g. SchedulerWorkerThreadDelegateTestImpl

[danakj, 2016-04-22 20:16:33]

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

[danakj, 2016-04-22 20:16:34]

LGTM

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:87: class
SchedulerWorkerThreadDelegateImpl
On 2016/04/22 19:30:29, gab wrote:
> Name this differently than main impl, e.g.
SchedulerWorkerThreadDelegateTestImpl

nit: not a fan of "test" in class names. Suggest following the naming scheme
here: http://martinfowler.com/articles/mocksArentStubs.html

"FakeSchedulerWorkerThreadDelegate" or "FakeWorkerThreadDelegate" or something
is my suggestion.

Also suggest making this not nested, and expose things on the outer class as
needed for it.

[robliao, 2016-04-22 20:30:48]

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread.cc:18:
SchedulerWorkerThread::CreateWorkerThread(ThreadPriority thread_priority,
While we're here, would Create(*) be sufficient as a name?

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:87: class
SchedulerWorkerThreadDelegateImpl
On 2016/04/22 20:16:33, danakj wrote:
> On 2016/04/22 19:30:29, gab wrote:
> > Name this differently than main impl, e.g.
> SchedulerWorkerThreadDelegateTestImpl
> 
> nit: not a fan of "test" in class names. Suggest following the naming scheme
> here: http://martinfowler.com/articles/mocksArentStubs.html
> 
> "FakeSchedulerWorkerThreadDelegate" or "FakeWorkerThreadDelegate" or something
> is my suggestion.
> 
> Also suggest making this not nested, and expose things on the outer class as
> needed for it.

There is a reasonable precedent in Chrome to name things with Test at the
beginning, so maybe

TestSchedulerWorkerThreadDelegate

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:95: // Without this
|auto_lock|, OnMainEntry() could be called twice without
Nit: Without synchronization

[fdoray, 2016-04-22 22:46:46]

robliao@: Can you take another look? Thanks.

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread.cc:18:
SchedulerWorkerThread::CreateWorkerThread(ThreadPriority thread_priority,
On 2016/04/22 20:30:48, robliao wrote:
> While we're here, would Create(*) be sufficient as a name?

Done. Also renamed SchedulerThreadPool::CreateThreadPool ->
SchedulerThreadPool::Create.

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
File base/task_scheduler/scheduler_worker_thread_unittest.cc (right):

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:39: WrapUnique(new
SchedulerWorkerThreadDelegateImpl(this)),
On 2016/04/22 19:30:29, gab wrote:
> This compiles without a fwd-decl of SchedulerWorkerThreadDelegateTestImpl?

Yes. We can use any type defined in the class in the methods of the class, even
if the definition of the type is after the method.

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:87: class
SchedulerWorkerThreadDelegateImpl
On 2016/04/22 20:30:48, robliao wrote:
> On 2016/04/22 20:16:33, danakj wrote:
> > On 2016/04/22 19:30:29, gab wrote:
> > > Name this differently than main impl, e.g.
> > SchedulerWorkerThreadDelegateTestImpl
> > 
> > nit: not a fan of "test" in class names. Suggest following the naming scheme
> > here: http://martinfowler.com/articles/mocksArentStubs.html
> > 
> > "FakeSchedulerWorkerThreadDelegate" or "FakeWorkerThreadDelegate" or
something
> > is my suggestion.
> > 
> > Also suggest making this not nested, and expose things on the outer class as
> > needed for it.
> 
> There is a reasonable precedent in Chrome to name things with Test at the
> beginning, so maybe
> 
> TestSchedulerWorkerThreadDelegate

Done. Renamed to TestSchedulerWorkerThreadDelegate.

https://codereview.chromium.org/1903103007/diff/1/base/task_scheduler/schedul...
base/task_scheduler/scheduler_worker_thread_unittest.cc:95: // Without this
|auto_lock|, OnMainEntry() could be called twice without
On 2016/04/22 20:30:48, robliao wrote:
> Nit: Without synchronization

Done.

[robliao, 2016-04-22 22:59:55]

lgtm

[fdoray, 2016-04-25 22:50:42]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-25 22:50:43]

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

[commit-bot: I haz the power, 2016-04-25 22:51:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903103007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903103007/80001

[commit-bot: I haz the power, 2016-04-26 01:52:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-26 01:53:36]

Description was changed from

==========
TaskScheduler: Make SchedulerWorkerThread own its delegate.

This will allow us to have one delegate per SchedulerWorkerThread and
to maintain state in that delegate.

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

to

==========
TaskScheduler: Make SchedulerWorkerThread own its delegate.

This will allow us to have one delegate per SchedulerWorkerThread and
to maintain state in that delegate.

BUG=553459

Committed: https://crrev.com/b035703e56aaf11e91c61891c20a178e72fee797
Cr-Commit-Position: refs/heads/master@{#389663}
==========

[commit-bot: I haz the power, 2016-04-26 01:53:37]

Patchset 5 (id:??) landed as
https://crrev.com/b035703e56aaf11e91c61891c20a178e72fee797
Cr-Commit-Position: refs/heads/master@{#389663}