Introduce ThreadTaskRunnerHandle::OverrideForTesting and TestMockTimeTaskRunner::ScopedContext.

Introduce ThreadTaskRunnerHandle::OverrideForTesting and TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFefJmNCAAJ

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh, RecentTabHelperTest, AffiliatedMatchHelperTest, and SyncStoppedReporterTest
  - Now mocks main thread runner directly, removes need for test-only SetTaskRunner() methods :).
   (having a custom TaskRunner was causing things like observers to post to the wrong task runner per this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an implementation detail anyways:
    draft @ https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit

BUG=587199
Review-Url: https://codereview.chromium.org/2657013002
Cr-Commit-Position: refs/heads/master@{#454127}
Committed: https://chromium.googlesource.com/chromium/src/+/a6f723282862820eed7df3a375a3f922996d846e

[gab, 2017-01-26 19:16:21]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 19:17:11]

Dry run: 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, 2017-01-26 21:49:42]

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

[commit-bot: I haz the power, 2017-01-26 21:49:43]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[gab, 2017-02-15 17:44:29]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

Prerequisite to fix HttpServerPropertiesManagerTest
in https://codereview.chromium.org/2491613004/#ps450001

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:44:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 17:45:31]

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

[gab, 2017-02-15 17:48:16]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:48:42]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner id).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:48:53]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner id).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:49:13]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:55:35]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

[gab, 2017-02-15 17:56:06]

gab@chromium.org changed reviewers:
+ danakj@chromium.org, zhongyi@chromium.org

[gab, 2017-02-15 17:56:07]

@dana for //base
@zhongyi (+CC xunjieli) for //net (essentially a follow-up cleanup to
https://codereview.chromium.org/2681383002 which already had solved some of the
issues I ran into :))

PS: I'm not sure bots will pass but net_unittests do locally on top of
https://codereview.chromium.org/2491613004/ and this is the core purpose of this
CL, please review regardless and I'll address any extra quirks that CQ finds.

Thanks!
Gab

https://codereview.chromium.org/2657013002/diff/40001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/40001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:809: url::SchemeHostPort
spdy_server_mail;
Note: for the big blocks of changes below, these are merely indenting by an
extra two spaces for the ScopedContext's scope... not sure why the diff comes
out so badly (reformat I guess :(..).

[commit-bot: I haz the power, 2017-02-15 17:58:54]

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

[commit-bot: I haz the power, 2017-02-15 17:58:55]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-02-15 18:31:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-02-15 18:31:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-15 18:32:53]

Dry run: 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, 2017-02-15 19:24:36]

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

[commit-bot: I haz the power, 2017-02-15 19:24:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-02-15 21:50:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 21:51:21]

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

[gab, 2017-02-15 21:53:56]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.

BUG=587199
==========

[gab, 2017-02-15 21:55:09]

gab@chromium.org changed reviewers:
+ emaxx@chromium.org, zea@chromium.org

[gab, 2017-02-15 21:55:41]

On 2017/02/15 17:56:07, gab wrote:
> @dana for //base
> @zhongyi (+CC xunjieli) for //net (essentially a follow-up cleanup to
> https://codereview.chromium.org/2681383002 which already had solved some of
the
> issues I ran into :))

+zea@ for sync/ test side-effects
+emaxx@ for chrome/browser/chromeos/policy/ test side-effects

> 
> PS: I'm not sure bots will pass but net_unittests do locally on top of
> https://codereview.chromium.org/2491613004/ and this is the core purpose of
this
> CL, please review regardless and I'll address any extra quirks that CQ finds.
> 
> Thanks!
> Gab
> 
>
https://codereview.chromium.org/2657013002/diff/40001/net/http/http_server_pr...
> File net/http/http_server_properties_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2657013002/diff/40001/net/http/http_server_pr...
> net/http/http_server_properties_manager_unittest.cc:809: url::SchemeHostPort
> spdy_server_mail;
> Note: for the big blocks of changes below, these are merely indenting by an
> extra two spaces for the ScopedContext's scope... not sure why the diff comes
> out so badly (reformat I guess :(..).

[Zhongyi Shi, 2017-02-15 22:04:41]

zhongyi@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Zhongyi Shi, 2017-02-15 22:04:42]

Thanks for clean this up! A few nits.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:371: return kUpdateCacheDelay;
These methods are for unittests only right? I am not supportive to add tests
only methods in the production code. Could you try to remove this?

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:186: //
HttpServerPropertiesImpl::ScheduleBrokenAlternateProtocolMappingsExpiration()).
Does any of those tests fail if using FastForwardUntilIdle()?

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:691:
net_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(20));
Could you revert the magic numbers here (20ms)? I believe we want the older
version.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:1224: // Move forward the
task runner short by 20ms.
ditto.

[Nicolas Zea, 2017-02-15 22:08:05]

sync lgtm

[gab, 2017-02-15 22:29:16]

Replies from phone, thanks

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:371: return kUpdateCacheDelay;
On 2017/02/15 22:04:42, Zhongyi Shi wrote:
> These methods are for unittests only right? I am not supportive to add tests
> only methods in the production code. Could you try to remove this?

I think it's cleaner to FastForwardBy the current amount than to always
FastForwardUntilNoTasksRemain() -- though I only did that where it was crucial
in this CL because I already spent to much time on these tests... There are
presubmits in place to ensure *For testing() methods are only called from
*test.cc files.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:186: //
HttpServerPropertiesImpl::ScheduleBrokenAlternateProtocolMappingsExpiration()).
On 2017/02/15 22:04:42, Zhongyi Shi wrote:
> Does any of those tests fail if using FastForwardUntilIdle()? 

Yes, infinite loop per comment above.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:691:
net_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(20));
On 2017/02/15 22:04:42, Zhongyi Shi wrote:
> Could you revert the magic numbers here (20ms)? I believe we want the older
> version.

That I could do. I had done this prior to the recent cleanup (which conflicted
with my local change). I preferred mine because 1) it doesn't use a magic
constant that needs to be in sync with impl and 2) dividing by 2 is wrong if the
delay is not an even number.

[commit-bot: I haz the power, 2017-02-15 23:13:49]

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

[commit-bot: I haz the power, 2017-02-15 23:13:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2017-02-16 02:49:18]

net/ LGTM mod nits.
Thanks for addressing the todo!

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:84: DCHECK(pref_delegate_);
nit: could you add a 
DCHECK(pref_task_runner->RunsTasksOnCurrentThread()) here?

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:92: // This call must run in
the context of |net_task_runner_|.
Just so I understand the api, if we remove the |scoped_context|, will
HttpServerPropertiesManager::InitializeOnNetworkThread() complain about the
thread doesn't match?

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:245: new
TestMockTimeTaskRunner;
Can we initialize this one in the constructor?

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:1233: // exectuted.
nit: executed. (i know it was misspelled in the original code)

[emaxx, 2017-02-16 03:07:38]

lgtm for chrome/browser/chromeos/policy/

[gab, 2017-02-16 16:50:17]

Nits addressed, thanks.

@dana for //base, as discussed in email thread.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:84: DCHECK(pref_delegate_);
On 2017/02/16 02:49:18, xunjieli wrote:
> nit: could you add a 
> DCHECK(pref_task_runner->RunsTasksOnCurrentThread()) here?

Done (though then I guess you might as well store
base::ThreadTaskRunnerHandle::Get() instead of tasking it as a parameter then).
I leave that up to you if you want to improve it further as a follow-up.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:92: // This call must run in
the context of |net_task_runner_|.
On 2017/02/16 02:49:18, xunjieli wrote:
> Just so I understand the api, if we remove the |scoped_context|, will
> HttpServerPropertiesManager::InitializeOnNetworkThread() complain about the
> thread doesn't match?

It doesn't yet but it should (see TODO(gab) I added in this CL on
TestMockTimeTaskRunner::RunsTasksOnCurrentThread() -- will then require more
tweaks to this test and others, e.g. in this CL I only fixed calls that "do"
things and for which it "matters to get the logic right" but many getters
DCHECK(RunsTasksOnCurrentThread()) and aren't scoped by this CL but will
eventually have to be).

This CL now only tasks care of making sure base::ThreadTaskRunnerHandle::Get()
in the scope of the ScopedContext now properly returns |net_task_runner_|
instead of the main message loop (i.e. that's why you needed RunLoop()
previously in a few places).

This will matter even more in https://codereview.chromium.org/2491613004/ as the
base::Timer will run its delayed task on SequencedTaskRunnerHandle::Get() (which
defers to ThreadTaskRunnerHandle::Get() when running on a
SingleThreadTaskRunner) and having that be the wrong underlying task runner was
breaking this test (and a few others).

Long term I want TestMockTimeTaskRunner to provide isolated
ThreadTaskRunnerHandle and RunsTasksOnCurrentThread for its tasks (even though
it shares the main thread) as it's really mimicking multiple threads on the main
thread for test simplicity but they shouldn't behave as though running on "main
thread".

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:245: new
TestMockTimeTaskRunner;
On 2017/02/16 02:49:18, xunjieli wrote:
> Can we initialize this one in the constructor?

I prefer C++11 member initialization when at all possible, it's more readable
IMO than having to go lookup at the constructor.

It's logically equivalent but this way you have all the relevant code in one
place.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:1233: // exectuted.
On 2017/02/16 02:49:18, xunjieli wrote:
> nit: executed. (i know it was misspelled in the original code) 

Done.

[gab, 2017-02-16 16:50:36]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 16:51:08]

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

[xunjieli, 2017-02-16 17:08:30]

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:84: DCHECK(pref_delegate_);
On 2017/02/16 16:50:15, gab wrote:
> On 2017/02/16 02:49:18, xunjieli wrote:
> > nit: could you add a 
> > DCHECK(pref_task_runner->RunsTasksOnCurrentThread()) here?
> 
> Done (though then I guess you might as well store
> base::ThreadTaskRunnerHandle::Get() instead of tasking it as a parameter
then).
> I leave that up to you if you want to improve it further as a follow-up.

Acknowledged. It used to be base::ThreadTaskRunnerHandle::Get() but was changed
in
https://chromium.googlesource.com/chromium/src/+/894658381ab6a524afa5ab08c780...
to facilitate testing.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:92: // This call must run in
the context of |net_task_runner_|.
On 2017/02/16 16:50:15, gab wrote:
> On 2017/02/16 02:49:18, xunjieli wrote:
> > Just so I understand the api, if we remove the |scoped_context|, will
> > HttpServerPropertiesManager::InitializeOnNetworkThread() complain about the
> > thread doesn't match?
> 
> It doesn't yet but it should (see TODO(gab) I added in this CL on
> TestMockTimeTaskRunner::RunsTasksOnCurrentThread() -- will then require more
> tweaks to this test and others, e.g. in this CL I only fixed calls that "do"
> things and for which it "matters to get the logic right" but many getters
> DCHECK(RunsTasksOnCurrentThread()) and aren't scoped by this CL but will
> eventually have to be).
> 
> This CL now only tasks care of making sure base::ThreadTaskRunnerHandle::Get()
> in the scope of the ScopedContext now properly returns |net_task_runner_|
> instead of the main message loop (i.e. that's why you needed RunLoop()
> previously in a few places).
> 
> This will matter even more in https://codereview.chromium.org/2491613004/ as
the
> base::Timer will run its delayed task on SequencedTaskRunnerHandle::Get()
(which
> defers to ThreadTaskRunnerHandle::Get() when running on a
> SingleThreadTaskRunner) and having that be the wrong underlying task runner
was
> breaking this test (and a few others).
> 
> Long term I want TestMockTimeTaskRunner to provide isolated
> ThreadTaskRunnerHandle and RunsTasksOnCurrentThread for its tasks (even though
> it shares the main thread) as it's really mimicking multiple threads on the
main
> thread for test simplicity but they shouldn't behave as though running on
"main
> thread".

Acknowledged. Thanks for the context.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:245: new
TestMockTimeTaskRunner;
On 2017/02/16 16:50:15, gab wrote:
> On 2017/02/16 02:49:18, xunjieli wrote:
> > Can we initialize this one in the constructor?
> 
> I prefer C++11 member initialization when at all possible, it's more readable
> IMO than having to go lookup at the constructor.
> 
> It's logically equivalent but this way you have all the relevant code in one
> place.

Acknowledged.

[commit-bot: I haz the power, 2017-02-16 17:48:45]

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

[commit-bot: I haz the power, 2017-02-16 17:48:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2017-02-16 17:57:24]

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
File base/test/test_mock_time_task_runner.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.cc:127: : task_runner_handle_(scope) {
move()

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.cc:128: scope->RunUntilIdle();
then task_runner_handle_->

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
File base/test/test_mock_time_task_runner.h (right):

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.h:53: // RunsTasksOnCurrentThread, etc.).
This allows to have the test body all in
allows having the test body

or

allows the test body to be

https://codereview.chromium.org/2657013002/diff/100001/base/threading/thread_...
File base/threading/thread_task_runner_handle.h (right):

https://codereview.chromium.org/2657013002/diff/100001/base/threading/thread_...
base/threading/thread_task_runner_handle.h:40: class BASE_EXPORT
NestedForTesting {
Proposal to make the changes less invasive.

1) OverrideForTesting(scoped_refptr<STTR>) method here on
ThreadTaskRunnerHandle. Make that the only method added to TTRH.

2) It returns a unique_ptr<ThreadTaskRunnerHandle::Restore> which in its
destructor calls OverrideForTesting to put back the last thing.

3) TTRH doesn't need to change its own implementation at all.

Would that work?

[Zhongyi Shi, 2017-02-16 19:35:25]

Thanks for doing this!

Most of the net changes look good to me, I'll come back once the net_unittests
is fixed.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:371: return kUpdateCacheDelay;
On 2017/02/15 22:29:16, gab wrote:
> On 2017/02/15 22:04:42, Zhongyi Shi wrote:
> > These methods are for unittests only right? I am not supportive to add tests
> > only methods in the production code. Could you try to remove this?
> 
> I think it's cleaner to FastForwardBy the current amount than to always
> FastForwardUntilNoTasksRemain() -- though I only did that where it was crucial
> in this CL because I already spent to much time on these tests... There are
> presubmits in place to ensure *For testing() methods are only called from
> *test.cc files.
> 

Acknowledged.

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:691:
net_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(20));
On 2017/02/15 22:29:16, gab wrote:
> On 2017/02/15 22:04:42, Zhongyi Shi wrote:
> > Could you revert the magic numbers here (20ms)? I believe we want the older
> > version.
> 
> That I could do. I had done this prior to the recent cleanup (which conflicted
> with my local change). I preferred mine because 1) it doesn't use a magic
> constant that needs to be in sync with impl and 2) dividing by 2 is wrong if
the
> delay is not an even number.

Ah, I see, move forward a random delta and verify the task is not executed until
the delay is reached. SGTM!

[gab, 2017-02-16 21:03:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-02-16 21:03:37]

Thanks, found an even cleaner alternative :)

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
File base/test/test_mock_time_task_runner.cc (right):

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.cc:127: : task_runner_handle_(scope) {
On 2017/02/16 17:57:24, danakj wrote:
> move()

Can't because... (next comment)

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.cc:128: scope->RunUntilIdle();
On 2017/02/16 17:57:24, danakj wrote:
> then task_runner_handle_->

Wanted to do that too (and move above) but can't because the
ThreadTaskRunnerHandle downgrades the type from TestMockTimeTaskRunner to
SingleThreadTaskRunner and RunUntilIdle() is no longer available.

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
File base/test/test_mock_time_task_runner.h (right):

https://codereview.chromium.org/2657013002/diff/60001/base/test/test_mock_tim...
base/test/test_mock_time_task_runner.h:53: // RunsTasksOnCurrentThread, etc.).
This allows to have the test body all in
On 2017/02/16 17:57:24, danakj wrote:
> allows having the test body
> 
> or
> 
> allows the test body to be

Done.

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2657013002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:84: DCHECK(pref_delegate_);
On 2017/02/16 17:08:29, xunjieli wrote:
> On 2017/02/16 16:50:15, gab wrote:
> > On 2017/02/16 02:49:18, xunjieli wrote:
> > > nit: could you add a 
> > > DCHECK(pref_task_runner->RunsTasksOnCurrentThread()) here?
> > 
> > Done (though then I guess you might as well store
> > base::ThreadTaskRunnerHandle::Get() instead of tasking it as a parameter
> then).
> > I leave that up to you if you want to improve it further as a follow-up.
> 
> Acknowledged. It used to be base::ThreadTaskRunnerHandle::Get() but was
changed
> in
>
https://chromium.googlesource.com/chromium/src/+/894658381ab6a524afa5ab08c780...
> to facilitate testing. 

Ah well, that's no longer necessary with my addition of the
ScopedMockTimeMessageLoopTaskRunner on the main thread :).

And you could also get rid of the Timer::SetTaskRunner() calls I think because
Start() binds to the task runner it's started from which in your case is always
the right one (as DCHECK'ed and ensured by the ScopedContexts in this CL :)).

https://codereview.chromium.org/2657013002/diff/100001/base/threading/thread_...
File base/threading/thread_task_runner_handle.h (right):

https://codereview.chromium.org/2657013002/diff/100001/base/threading/thread_...
base/threading/thread_task_runner_handle.h:40: class BASE_EXPORT
NestedForTesting {
On 2017/02/16 17:57:24, danakj wrote:
> Proposal to make the changes less invasive.
> 
> 1) OverrideForTesting(scoped_refptr<STTR>) method here on
> ThreadTaskRunnerHandle. Make that the only method added to TTRH.
> 
> 2) It returns a unique_ptr<ThreadTaskRunnerHandle::Restore> which in its
> destructor calls OverrideForTesting to put back the last thing.
> 
> 3) TTRH doesn't need to change its own implementation at all.
> 
> Would that work?

That's what I wanted to do at first, but then hit two problems :
 1) The TLS is holding a TTRH, not a STR.
 2) The Restore class ends up looking essentially like NestedForTesting, at
which point it feels silly to have a static method that is essentially merely
constructing that class and handing it to you in a unique_ptr (you might as well
have it directly allocated on the stack) -- and iterating some more to simplify
having just one piece of TLS you get to NestedForTesting owning TLS like I
did...


But... :)

I forgot about that at first and gave it a second shot and had a realization
that since we only need Restore for its destructor (its constructor is merely to
store state for its destructor), we can package all of that in a
ScopedClosureRunner and then it becomes very neat :)!

And added tests this time around per the complexity rising above a trivial
scoper.

[commit-bot: I haz the power, 2017-02-16 21:05:13]

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

[gab, 2017-02-16 21:14:37]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:15:24]

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

[gab, 2017-02-16 21:44:05]

On 2017/02/16 19:35:25, Zhongyi Shi wrote:
> Thanks for doing this!
> 
> Most of the net changes look good to me, I'll come back once the net_unittests
> is fixed.

net_unittests is fixed PTAL :)

(I'm still fighting with a few other side tests which have various dependencies
they didn't intend on ThreadTaskRunnerHandle::Get())

[Zhongyi Shi, 2017-02-16 21:59:07]

lgtm!

[commit-bot: I haz the power, 2017-02-16 22:08:47]

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

[commit-bot: I haz the power, 2017-02-16 22:08:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[danakj, 2017-02-16 22:38:25]

Thanks for the tests too, LGTM

[danakj, 2017-02-16 22:38:44]

The CL desc needs to be updated for the new approach

[gab, 2017-02-17 14:15:38]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::NestedForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.

BUG=587199
==========

[gab, 2017-02-17 14:16:11]

On 2017/02/16 22:38:44, danakj wrote:
> The CL desc needs to be updated for the new approach

Thanks, s/NestedForTesting/OverrideForTesting/ :)

[gab, 2017-02-21 15:30:26]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 15:31:08]

Dry run: 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, 2017-02-21 16:29:02]

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

[commit-bot: I haz the power, 2017-02-21 16:29:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-02-24 15:52:58]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 15:53:13]

Dry run: 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, 2017-02-24 16:42:00]

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

[commit-bot: I haz the power, 2017-02-24 16:42:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-02-27 20:42:35]

Patchset #9 (id:180001) has been deleted

[gab, 2017-02-27 20:53:52]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocument*Test and PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.h:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.

BUG=587199
==========

[gab, 2017-02-27 20:54:20]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocument*Test and PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.h:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocument*Test and PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.

BUG=587199
==========

[gab, 2017-02-27 20:56:29]

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

[gab, 2017-02-27 20:56:30]

@engedy for test tweaks in components/password_manager/* and
components/subresource_filter/*

@fdoray to double-check diff between PS 8 & 9 in //base (as discussed offline)

Thanks!
Gab

[gab, 2017-02-27 20:57:13]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 20:57:36]

Dry run: 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, 2017-02-27 22:10:36]

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

[commit-bot: I haz the power, 2017-02-27 22:10:37]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[fdoray, 2017-02-28 04:11:21]

lgtm

https://codereview.chromium.org/2657013002/diff/220001/base/test/test_mock_ti...
File base/test/test_mock_time_task_runner.h (right):

https://codereview.chromium.org/2657013002/diff/220001/base/test/test_mock_ti...
base/test/test_mock_time_task_runner.h:77: //      
TestMockTimeTaskRunner::ScopedContext(foo_task_runner_.get());
add variable name

[gab, 2017-02-28 19:09:32]

gab@chromium.org changed reviewers:
+ bshe@chromium.org

[gab, 2017-02-28 19:14:58]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocument*Test and PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and
PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an
implementation detail anyways:
    draft @
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=587199
==========

[gab, 2017-02-28 19:16:03]

+bshe@ for components/wallpaper/* tweaks (CC bruthig per
https://codereview.chromium.org/2679133003)

ping engedy@ for test tweaks in components/password_manager/* and
components/subresource_filter/*

https://codereview.chromium.org/2657013002/diff/220001/base/test/test_mock_ti...
File base/test/test_mock_time_task_runner.h (right):

https://codereview.chromium.org/2657013002/diff/220001/base/test/test_mock_ti...
base/test/test_mock_time_task_runner.h:77: //      
TestMockTimeTaskRunner::ScopedContext(foo_task_runner_.get());
On 2017/02/28 04:11:21, fdoray wrote:
> add variable name

Nice catch, done.

[gab, 2017-02-28 19:16:13]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 19:16:43]

Dry run: 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, 2017-02-28 20:31:05]

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

[commit-bot: I haz the power, 2017-02-28 20:31:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2017-02-28 21:30:07]

LGTM on components/password_manager/ and components/subresource_filter/.

[bshe, 2017-02-28 21:55:20]

On 2017/02/28 21:30:07, engedy (slow) wrote:
> LGTM on components/password_manager/ and components/subresource_filter/.

wallpaper lgtm

[gab, 2017-03-01 01:58:57]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 01:59:44]

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

[gab, 2017-03-01 02:21:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 02:21:52]

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

[gab, 2017-03-01 02:25:28]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh
 - SyncStoppedReporterTest
  - In both cases can now simply spin mock task runner, no need to also
RunLoop() as its
    tasks don't erroneously post back to the main runner per
ThreadTaskRunnerHandle::Get()
    being correct now.
 - AffiliatedMatchHelperTest: impl no longer needs a custom task runner for
testing.
   (having one was causing its observers to post to the wrong task runner per
this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and
PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an
implementation detail anyways:
    draft @
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh, RecentTabHelperTest,
AffiliatedMatchHelperTest, and SyncStoppedReporterTest
  - Now mocks main thread runner directly, removes need for test-only
SetTaskRunner() methods :).
   (having a custom TaskRunner was causing things like observers to post to the
wrong task runner per this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and
PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an
implementation detail anyways:
    draft @
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=587199
==========

[gab, 2017-03-01 02:28:06]

gab@chromium.org changed reviewers:
+ petewil@chromium.org

[gab, 2017-03-01 02:28:08]

@emaxx: further cleanup of chromeos/policy/ :) PTanL

@petewil for chrome/browser/android/offline_pages/ tweaks

Thanks!
Gab

[commit-bot: I haz the power, 2017-03-01 02:48:14]

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

[commit-bot: I haz the power, 2017-03-01 02:48:16]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[gab, 2017-03-01 02:56:15]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 02:56:56]

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

[gab, 2017-03-01 02:59:56]

Patchset #15 (id:320001) has been deleted

[commit-bot: I haz the power, 2017-03-01 03:01:34]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[gab, 2017-03-01 03:02:08]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 03:02:53]

Dry run: 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, 2017-03-01 03:57:12]

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

[commit-bot: I haz the power, 2017-03-01 03:57:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-03-01 04:32:35]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 04:32:52]

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

[gab, 2017-03-01 04:38:41]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 04:38:54]

Dry run: 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, 2017-03-01 05:36:44]

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

[commit-bot: I haz the power, 2017-03-01 05:36:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[gab, 2017-03-01 07:09:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 07:09:45]

Dry run: 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, 2017-03-01 09:03:20]

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

[commit-bot: I haz the power, 2017-03-01 09:03:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2017-03-01 09:03:39]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 09:03:56]

Dry run: 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, 2017-03-01 10:07:19]

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

[commit-bot: I haz the power, 2017-03-01 10:07:21]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-03-01 14:43:53]

LGTM with nits (sorry for not pointing out them earlier)

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc (right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc:100:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
nit: #include "base/single_thread_task_runner.h"?
I don't think we can rely here on base/threading/thread_task_runner_handle.h
including it.

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc
(right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc:71:
mocked_main_runner_->RunUntilIdle();
nit: #include "base/test/test_mock_time_task_runner.h"?

[Pete Williamson, 2017-03-01 18:07:32]

Offline Pages changes LGTM as long as the tests pass (and they appear to be
passing).

[gab, 2017-03-02 00:33:31]

Thanks, @emaxx: reply to nits below. I'll CQ now but happy to discuss further if
you disagree.

Cheers!
Gab

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc (right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc:100:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2017/03/01 14:43:53, emaxx wrote:
> nit: #include "base/single_thread_task_runner.h"?
> I don't think we can rely here on base/threading/thread_task_runner_handle.h
> including it.

By IWYU (include-what-you-use), thread_task_runner_handle.h has to include
single_thread_task_runner.h per returning it by value.

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc
(right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc:71:
mocked_main_runner_->RunUntilIdle();
On 2017/03/01 14:43:53, emaxx wrote:
> nit: #include "base/test/test_mock_time_task_runner.h"?

We had a similar discussion @
https://bugs.chromium.org/p/chromium/issues/detail?id=618793#c6.

Conclusion was that headers that always require another one to be at all useful
should include it and not require its users to include multiple headers every
time.

i.e. jam@: "Stepping back, the purpose of IWYU is to avoid slowing down the
build by including unnecessary headers. In this case, the headers are always
necessary and will always be included by the source files anyways. So I don't
see a negative."

[gab, 2017-03-02 00:33:44]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-02 00:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
xunjieli@chromium.org, danakj@chromium.org, zhongyi@chromium.org,
fdoray@chromium.org, engedy@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2657013002/#ps400001
(title: "fix RecentTabHelperTest crash?")

[commit-bot: I haz the power, 2017-03-02 00:34:23]

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, 2017-03-02 00:46:19]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1488414824532610,
"parent_rev": "c0e297639ffe51b7a3fc75fe96f9a1fb832a0ea1", "commit_rev":
"a6f723282862820eed7df3a375a3f922996d846e"}

[commit-bot: I haz the power, 2017-03-02 00:46:57]

Description was changed from

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh, RecentTabHelperTest,
AffiliatedMatchHelperTest, and SyncStoppedReporterTest
  - Now mocks main thread runner directly, removes need for test-only
SetTaskRunner() methods :).
   (having a custom TaskRunner was causing things like observers to post to the
wrong task runner per this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and
PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an
implementation detail anyways:
    draft @
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=587199
==========

to

==========
Introduce ThreadTaskRunnerHandle::OverrideForTesting and
TestMockTimeTaskRunner::ScopedContext.

Will be used to provide proper ThreadTaskRunnerHandle context in unit
tests when multiple test task runners share the main thread.

TestMockTimeTaskRunner::ScopedContext as discussed @
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/kSIe9p5ZYGM/koFef...

This cleans up HttpServerPropertiesManagerTest to actually run code
in the scope of the task runners the impl expects it to (it would
previously pass because TestMockTimeTaskRunner::RunsOnCurrentThread()
is looser than it should be -- merely checks thread id, not runner context).
This is a prerequisite for https://codereview.chromium.org/2491613004/
as it otherwise breaks per its tasks posting to unexpected tasks runners
(i.e. the current ThreadTaskRunnerHandle() which was the underlying unmocked
MessageLoop in these tests).

Further addressed in those tests:
 - No longer need to use MessageLoop/RunLoop
   - Which removes need for completion Closure on the updates.
 - Exposes timer timings to fast-forward by the right timing instead of
   FastForwardUntilNoTasksRemain() -- test could use some more cleanup in that
   regard but that's a first pass of mandatory sites.

Also addressed in this CL:
 - ServerBackedStateKeysBrokerTest.Refresh, RecentTabHelperTest,
AffiliatedMatchHelperTest, and SyncStoppedReporterTest
  - Now mocks main thread runner directly, removes need for test-only
SetTaskRunner() methods :).
   (having a custom TaskRunner was causing things like observers to post to the
wrong task runner per this CL's
    ThreadTaskRunnerHandle override in its deferred initialization).
 - AsyncDocumentSubresourceFilterTest, WallPaperColorCalculatorTest and
PostAndReplyImplTest
  - Unfortunately those tests were fine but as documented in
thread_task_runner_handle.cc:
    supporting SequencedTaskRunnerHandle overrides from main thread would be
overkill for
    now... so they were tweaked to avoid doing that.
  - Also, upcoming base::test::ScopedTaskEnvironment makes all of this an
implementation detail anyways:
    draft @
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P...

BUG=587199

Review-Url: https://codereview.chromium.org/2657013002
Cr-Commit-Position: refs/heads/master@{#454127}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6f723282862820eed7df3a375a3...
==========

[commit-bot: I haz the power, 2017-03-02 00:46:59]

Committed patchset #19 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/a6f723282862820eed7df3a375a3...

[emaxx, 2017-03-02 02:39:39]

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc (right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc:100:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2017/03/02 00:33:31, gab (slow - travel and perf) wrote:
> On 2017/03/01 14:43:53, emaxx wrote:
> > nit: #include "base/single_thread_task_runner.h"?
> > I don't think we can rely here on base/threading/thread_task_runner_handle.h
> > including it.
> 
> By IWYU (include-what-you-use), thread_task_runner_handle.h has to include
> single_thread_task_runner.h per returning it by value.

I'm not entirely following this argument. ThreadTaskRunnerHandle is returning
SingleThreadTaskRunner not by a value, but inside a smart pointer. Incomplete
types may be used in declarations with STL's smart pointers std::unique_ptr and
std::shared_ptr, and so do is with scoped_refptr - as documented here:
https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=392

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc
(right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc:71:
mocked_main_runner_->RunUntilIdle();
On 2017/03/02 00:33:31, gab (slow - travel and perf) wrote:
> On 2017/03/01 14:43:53, emaxx wrote:
> > nit: #include "base/test/test_mock_time_task_runner.h"?
> 
> We had a similar discussion @
> https://bugs.chromium.org/p/chromium/issues/detail?id=618793#c6.
> 
> Conclusion was that headers that always require another one to be at all
useful
> should include it and not require its users to include multiple headers every
> time.
> 
> i.e. jam@: "Stepping back, the purpose of IWYU is to avoid slowing down the
> build by including unnecessary headers. In this case, the headers are always
> necessary and will always be included by the source files anyways. So I don't
> see a negative."

The definition of the ScopedMockTimeMessageLoopTaskRunner class uses only
pointers to TestMockTimeTaskRunner and scoped_refptr's to it. So technically,
unless I'm missing something, it's not required for
scoped_mock_time_message_loop_task_runner.h to include
test_mock_time_task_runner.h.
Therefore IWYU, in my opinion, still applies here: the header A is not required
to include header B, and not every consumer of A is required to include header B
- only if they want to dereference pointers to corresponding types.

Having said that, I don't have any intention to stand in the way of these
changes. Just trying to understand the reasoning here and the scope of the IWYU
rule.

[gab, 2017-03-02 03:24:44]

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc (right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.cc:100:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2017/03/02 02:39:39, emaxx wrote:
> On 2017/03/02 00:33:31, gab (slow - travel and perf) wrote:
> > On 2017/03/01 14:43:53, emaxx wrote:
> > > nit: #include "base/single_thread_task_runner.h"?
> > > I don't think we can rely here on
base/threading/thread_task_runner_handle.h
> > > including it.
> > 
> > By IWYU (include-what-you-use), thread_task_runner_handle.h has to include
> > single_thread_task_runner.h per returning it by value.
> 
> I'm not entirely following this argument. ThreadTaskRunnerHandle is returning
> SingleThreadTaskRunner not by a value, but inside a smart pointer. Incomplete
> types may be used in declarations with STL's smart pointers std::unique_ptr
and
> std::shared_ptr, and so do is with scoped_refptr - as documented here:
> https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=392

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/0khIRhxDq6E/dis...

(can't fwd-decl the pointer type of a scoped_refptr passed by value -- and the
documented/tested paradigm should be remove because it requires an advanced
fwd-template-decl which is literally never used outside of that test)

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc
(right):

https://codereview.chromium.org/2657013002/diff/400001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/server_backed_state_keys_broker_unittest.cc:71:
mocked_main_runner_->RunUntilIdle();
On 2017/03/02 02:39:39, emaxx wrote:
> On 2017/03/02 00:33:31, gab (slow - travel and perf) wrote:
> > On 2017/03/01 14:43:53, emaxx wrote:
> > > nit: #include "base/test/test_mock_time_task_runner.h"?
> > 
> > We had a similar discussion @
> > https://bugs.chromium.org/p/chromium/issues/detail?id=618793#c6.
> > 
> > Conclusion was that headers that always require another one to be at all
> useful
> > should include it and not require its users to include multiple headers
every
> > time.
> > 
> > i.e. jam@: "Stepping back, the purpose of IWYU is to avoid slowing down the
> > build by including unnecessary headers. In this case, the headers are always
> > necessary and will always be included by the source files anyways. So I
don't
> > see a negative."
> 
> The definition of the ScopedMockTimeMessageLoopTaskRunner class uses only
> pointers to TestMockTimeTaskRunner and scoped_refptr's to it. So technically,
> unless I'm missing something, it's not required for
> scoped_mock_time_message_loop_task_runner.h to include
> test_mock_time_task_runner.h.
> Therefore IWYU, in my opinion, still applies here: the header A is not
required
> to include header B, and not every consumer of A is required to include header
B
> - only if they want to dereference pointers to corresponding types.
> 
> Having said that, I don't have any intention to stand in the way of these
> changes. Just trying to understand the reasoning here and the scope of the
IWYU
> rule.

In pure IWYU sense you're right, but as was concluded in discussion on
aforementioned bug. It makes sense for
scoped_mock_time_message_loop_task_runner.h to provide
test_mock_timer_task_runner.h given it's merely a proxy to it (i.e. it doesn't
make sense to use this mock if you're not going to at least use the underlying
TestMockTimeTaskRunner* once).