Update NetworkTimeQueries field trial config

Update NetworkTimeQueries field trial config

Also, this updates the SSLErrorHandler browser tests to use their own
NetworkTimeTracker, instead of the global one. The global one can be
influenced by the field trial config (e.g. might perform a query on startup
before the test has a chance to disable background queries). So the tests
must use their own NetworkTimeTracker to ensure that it behaves as
desired.

BUG=

Review-Url: https://codereview.chromium.org/2653443002
Cr-Commit-Position: refs/heads/master@{#445589}
Committed: https://chromium.googlesource.com/chromium/src/+/53a47b410298f7c18fb35684eea749dd7c6396e5

[estark, 2017-01-22 19:48:29]

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

[estark, 2017-01-22 19:49:06]

estark@chromium.org changed reviewers:
+ jwd@chromium.org

[Alexei Svitkine (slow), 2017-01-23 15:43:53]

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

[Alexei Svitkine (slow), 2017-01-23 15:43:54]

lgtm

[estark, 2017-01-23 16:09:45]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2017-01-23 16:10:00]

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-23 16:21:11]

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

[commit-bot: I haz the power, 2017-01-23 16:21:12]

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...)

[estark, 2017-01-23 18:29:29]

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

[commit-bot: I haz the power, 2017-01-23 18:30:10]

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

[estark, 2017-01-23 18:31:14]

Description was changed from

==========
Update NetworkTimeQueries field trial config

BUG=
==========

to

==========
Update NetworkTimeQueries field trial config

Also, this updates the SSLErrorHandler browser tests to use their own
NetworkTimeTracker, instead of the global one. The global one can be
influenced by the field trial config (e.g. might perform a query on startup
before the test has a chance to disable background queries). So the tests
must use their own NetworkTimeTracker to ensure that it behaves as
desired.

BUG=
==========

[estark, 2017-01-23 18:33:14]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2017-01-23 18:33:14]

meacer, can you please look at ssl_browser_tests.cc? In the field trial config,
I'm enabling both background and on-demand queries, but in the browser tests I
want just on-demand queries and no queries on startup, so that the tracker
behaves more predictably. So I modified the browser tests to create their own
tracker, instead of using the global one which might perform a query on startup
(before the test has a chance to modify the Finch config).

[meacer, 2017-01-23 18:38:45]

On 2017/01/23 18:33:14, estark wrote:
> meacer, can you please look at ssl_browser_tests.cc? In the field trial
config,
> I'm enabling both background and on-demand queries, but in the browser tests I
> want just on-demand queries and no queries on startup, so that the tracker
> behaves more predictably. So I modified the browser tests to create their own
> tracker, instead of using the global one which might perform a query on
startup
> (before the test has a chance to modify the Finch config).

Is the problem that SetupOnMainThread runs after the network service is
initiated? I'm surprised that there is no way to set finch parameters before the
time service is constructed. For example, would enabling/disabling the feature
in SetUpCommandLine work? Then you can perhaps use a ScopedFeatureList in the
test to control finch?

[commit-bot: I haz the power, 2017-01-23 19:23:54]

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

[commit-bot: I haz the power, 2017-01-23 19:23:55]

Dry run: This issue passed the CQ dry run.

[estark, 2017-01-23 21:01:38]

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

[commit-bot: I haz the power, 2017-01-23 21:02:58]

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

[estark, 2017-01-23 21:05:17]

On 2017/01/23 18:38:45, Mustafa Emre Acer wrote:
> On 2017/01/23 18:33:14, estark wrote:
> > meacer, can you please look at ssl_browser_tests.cc? In the field trial
> config,
> > I'm enabling both background and on-demand queries, but in the browser tests
I
> > want just on-demand queries and no queries on startup, so that the tracker
> > behaves more predictably. So I modified the browser tests to create their
own
> > tracker, instead of using the global one which might perform a query on
> startup
> > (before the test has a chance to modify the Finch config).
> 
> Is the problem that SetupOnMainThread runs after the network service is
> initiated? I'm surprised that there is no way to set finch parameters before
the
> time service is constructed. For example, would enabling/disabling the feature
> in SetUpCommandLine work? Then you can perhaps use a ScopedFeatureList in the
> test to control finch?

Oh, SetUpCommandLine works, thanks for the suggestion! I can just set up the
whole thing (the feature and the params) there and not need to use
network_time::FieldTrialTest at all. Which allows me to simplify FieldTrialTest
and make it for unit tests only and remove the ugly
CreateForBrowserTest/CreateForUnitTest thing.

I think setting up everything in SetUpCommandLine is cleaner (and matches
production code path better) than just disabling the feature via command line
and setting it up via ScopedFeatureList later, but let me know what you think.

[meacer, 2017-01-23 21:35:54]

Nice, your added line count is now negative :)

Lgtm.

https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
File components/network_time/network_time_test_utils.cc (right):

https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
components/network_time/network_time_test_utils.cc:98:
field_trial_list_.reset();  // Averts a CHECK fail in constructor below.
nit: Is this still needed?

[commit-bot: I haz the power, 2017-01-23 22:07:45]

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

[commit-bot: I haz the power, 2017-01-23 22:07:46]

Dry run: This issue passed the CQ dry run.

[estark, 2017-01-23 23:42:38]

https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
File components/network_time/network_time_test_utils.cc (right):

https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
components/network_time/network_time_test_utils.cc:98:
field_trial_list_.reset();  // Averts a CHECK fail in constructor below.
On 2017/01/23 21:35:54, Mustafa Emre Acer wrote:
> nit: Is this still needed?

Err. Probably. I'll try removing it and see if the bots complain.

[estark, 2017-01-23 23:42:49]

The CQ bit was checked by estark@chromium.org

[estark, 2017-01-23 23:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2653443002/#ps60001
(title: "remove field_trial_list_ reset... maybe?")

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

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-24 00:08:07]

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

[commit-bot: I haz the power, 2017-01-24 00:08:08]

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

[estark, 2017-01-24 01:45:16]

Patchset #4 (id:60001) has been deleted

[estark, 2017-01-24 01:45:26]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2017-01-24 01:46:26]

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

[estark, 2017-01-24 01:51:12]

On 2017/01/23 23:42:38, estark wrote:
>
https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
> File components/network_time/network_time_test_utils.cc (right):
> 
>
https://codereview.chromium.org/2653443002/diff/40001/components/network_time...
> components/network_time/network_time_test_utils.cc:98:
> field_trial_list_.reset();  // Averts a CHECK fail in constructor below.
> On 2017/01/23 21:35:54, Mustafa Emre Acer wrote:
> > nit: Is this still needed?
> 
> Err. Probably. I'll try removing it and see if the bots complain.

It looks like that is still needed, so I'm landing the previous patchset that
preserves the reset.

However, that line, and the DCHECK that fails, look pretty weird, so I think
we're probably doing something not-quite-right. Filed crbug.com/684216 to
investigate.

[commit-bot: I haz the power, 2017-01-24 01:52:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485222326351920,
"parent_rev": "c2d68eae813e2234e28798e81c423df7946fbe01", "commit_rev":
"53a47b410298f7c18fb35684eea749dd7c6396e5"}

[commit-bot: I haz the power, 2017-01-24 01:53:37]

Description was changed from

==========
Update NetworkTimeQueries field trial config

Also, this updates the SSLErrorHandler browser tests to use their own
NetworkTimeTracker, instead of the global one. The global one can be
influenced by the field trial config (e.g. might perform a query on startup
before the test has a chance to disable background queries). So the tests
must use their own NetworkTimeTracker to ensure that it behaves as
desired.

BUG=
==========

to

==========
Update NetworkTimeQueries field trial config

Also, this updates the SSLErrorHandler browser tests to use their own
NetworkTimeTracker, instead of the global one. The global one can be
influenced by the field trial config (e.g. might perform a query on startup
before the test has a chance to disable background queries). So the tests
must use their own NetworkTimeTracker to ensure that it behaves as
desired.

BUG=

Review-Url: https://codereview.chromium.org/2653443002
Cr-Commit-Position: refs/heads/master@{#445589}
Committed:
https://chromium.googlesource.com/chromium/src/+/53a47b410298f7c18fb35684eea7...
==========

[commit-bot: I haz the power, 2017-01-24 01:53:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/53a47b410298f7c18fb35684eea7...