TaskScheduler: Record TaskScheduler.NumTasksBeforeDetach.* from OnDetach().

TaskScheduler: Record TaskScheduler.NumTasksBeforeDetach.* from OnDetach().

This CL introduces SchedulerWorker::Delegate::OnDetach() and records
TaskScheduler.NumTasksBeforeDetach.* from this new method.

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

[fdoray, 2016-10-18 19:40:44]

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

[fdoray, 2016-10-18 19:40:45]

PTAL

[gab, 2016-10-19 18:12:23]

Awesome :), lgtm % comments

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:41: // a thread managed by the
SchedulerWorker instance. The methods are never
s/a thread/the thread/ (and remove last sentence -- IMO as-is it confusingly can
be read as the SchedulerWorker instance having many threads but managing
synchronization)

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:750: //  
SchedulerWorker.
How is this condition guaranteed to be met (and how is VerifyThreadIdIsNot()
guaranteed to run) if there isn't a WaitForAllWorkersIdleForTesting() passed
this point?

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_unittest.cc (left):

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_unittest.cc:445:
PlatformThread::Sleep(TimeDelta::FromMilliseconds(50));
Less sleep in tests, yay :)

[fdoray, 2016-10-20 13:45:01]

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

[fdoray, 2016-10-20 13:45:02]

gab@: Thanks

rkaplow@: PTAL at histograms

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker.h (right):

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker.h:41: // a thread managed by the
SchedulerWorker instance. The methods are never
On 2016/10/19 18:12:23, gab wrote:
> s/a thread/the thread/ (and remove last sentence -- IMO as-is it confusingly
can
> be read as the SchedulerWorker instance having many threads but managing
> synchronization)

Done.

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right):

https://codereview.chromium.org/2430633003/diff/20001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:750: //  
SchedulerWorker.
On 2016/10/19 18:12:23, gab wrote:
> How is this condition guaranteed to be met (and how is VerifyThreadIdIsNot()
> guaranteed to run) if there isn't a WaitForAllWorkersIdleForTesting() passed
> this point?

Done.

[rkaplow, 2016-10-20 14:11:15]

lgtm

lg assuming this is just improved wording. If it's a functional change consider
replacing the histogram

[fdoray, 2016-10-20 14:33:33]

On 2016/10/20 14:11:15, rkaplow wrote:
> lgtm
> 
> lg assuming this is just improved wording. If it's a functional change
consider
> replacing the histogram

It's a functional change. Since very few tasks are redirected to TaskScheduler
so far, the values recorded to TaskScheduler histograms are not meaningful. We
will have to filter out early data when we start analyzing TaskScheduler
histograms. Therefore, replacing this histogram is not necessary.

[fdoray, 2016-10-20 14:33:37]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-10-20 14:33:37]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2430633003/#ps40001
(title: "CR gab #3   rebase")

[commit-bot: I haz the power, 2016-10-20 14:33:52]

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

[rkaplow, 2016-10-20 14:34:40]

lgtm

ok sure. Just be careful you're not mixing data.

[commit-bot: I haz the power, 2016-10-20 16:26:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:18:55]

Description was changed from

==========
TaskScheduler: Record TaskScheduler.NumTasksBeforeDetach.* from OnDetach().

This CL introduces SchedulerWorker::Delegate::OnDetach() and records
TaskScheduler.NumTasksBeforeDetach.* from this new method.

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

to

==========
TaskScheduler: Record TaskScheduler.NumTasksBeforeDetach.* from OnDetach().

This CL introduces SchedulerWorker::Delegate::OnDetach() and records
TaskScheduler.NumTasksBeforeDetach.* from this new method.

BUG=553459

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

[commit-bot: I haz the power, 2016-10-21 13:18:56]

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