Do not call SequencedWorkerPool::GetWorkerPoolForCurrentThread() from CollectEnvironmentData().

Do not call SequencedWorkerPool::GetWorkerPoolForCurrentThread() from CollectEnvironmentData().

SequencedWorkerPool::GetWorkerPoolForCurrentThread() returns nullptr
when called from a SequencedWorkerPool task redirected to the
TaskScheduler.

Note: Code archeology reveals that a reviewer asked to use
SequencedWorkerPool::GetWorkerPoolForCurrentThread() instead of
content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()
to avoid a dependance on BrowserThread. I think it's better to
temporarily introduce a dependance on BrowserThread with this CL
than to make SequencedWorkerPool::GetWorkerPoolForCurrentThread()
work from SequencedWorkerPool tasks redirected to TaskScheduler
just for this call site.
https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc#newcode68

BUG=622400
Committed: https://crrev.com/fdb23ec2e44b83f4fc8debbdb43ed5775cb5d92d
Cr-Commit-Position: refs/heads/master@{#422118}

[fdoray, 2016-09-30 13:35:20]

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

[fdoray, 2016-09-30 13:35:20]

PTAL

[grt (UTC plus 2), 2016-09-30 13:55:57]

yeah, okay. :-) i like how this will go away in the future. lgtm.

[fdoray, 2016-09-30 13:57:32]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-09-30 13:57:37]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2016-09-30 14:38:17]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-09-30 14:38:17]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2379913004/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-30 14:38:33]

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-09-30 15:25:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-30 15:31:03]

Description was changed from

==========
Do not call SequencedWorkerPool::GetWorkerPoolForCurrentThread() from
CollectEnvironmentData().

SequencedWorkerPool::GetWorkerPoolForCurrentThread() returns nullptr
when called from a SequencedWorkerPool task redirected to the
TaskScheduler.

Note: Code archeology reveals that a reviewer asked to use
SequencedWorkerPool::GetWorkerPoolForCurrentThread() instead of
content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()
to avoid a dependance on BrowserThread. I think it's better to
temporarily introduce a dependance on BrowserThread with this CL
than to make SequencedWorkerPool::GetWorkerPoolForCurrentThread()
work from SequencedWorkerPool tasks redirected to TaskScheduler
just for this call site.
https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...

BUG=622400
==========

to

==========
Do not call SequencedWorkerPool::GetWorkerPoolForCurrentThread() from
CollectEnvironmentData().

SequencedWorkerPool::GetWorkerPoolForCurrentThread() returns nullptr
when called from a SequencedWorkerPool task redirected to the
TaskScheduler.

Note: Code archeology reveals that a reviewer asked to use
SequencedWorkerPool::GetWorkerPoolForCurrentThread() instead of
content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()
to avoid a dependance on BrowserThread. I think it's better to
temporarily introduce a dependance on BrowserThread with this CL
than to make SequencedWorkerPool::GetWorkerPoolForCurrentThread()
work from SequencedWorkerPool tasks redirected to TaskScheduler
just for this call site.
https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...

BUG=622400

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

[commit-bot: I haz the power, 2016-09-30 15:31:04]

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

[gab, 2016-09-30 15:48:56]

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:73:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
I think such a call site once again argues for something like
TaskScheduler::RunsTasksOnCurrentThread() in the future.

i.e. for people that want to document that their code runs in a pool without
caring about which specific TaskRunner.

Here I could see something like this match the comment on the line above better:

DCHECK(base::SequencedWorkerPool::GetWorkerPoolForCurrentThread() ||
       base::TaskScheduler::RunsTasksOnCurrentThread());

[gab, 2016-09-30 19:21:04]

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:73:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
Thinking about this some more, WDYT of
DCHECK(!base::ThreadTaskRunnerHandle::IsSet()); here? That would also match
"only makes sense in a thread pool" which is really  meant to say "doesn't make
sense in a single-threaded environment"

[gab, 2016-10-03 19:47:34]

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/2379913004/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:73:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2016/09/30 19:21:03, gab wrote:
> Thinking about this some more, WDYT of
> DCHECK(!base::ThreadTaskRunnerHandle::IsSet()); here? That would also match
> "only makes sense in a thread pool" which is really  meant to say "doesn't
make
> sense in a single-threaded environment"

ping ^^?