Add a NetworkQualityEstimator to TestURLRequestContext.

Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456
Committed: https://crrev.com/5ce3e1f8fef0f96ae28a09ac0aba2dc49802f9ee
Cr-Commit-Position: refs/heads/master@{#432277}

[mmenke, 2016-11-08 21:32:38]

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

[commit-bot: I haz the power, 2016-11-08 21:33:22]

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, 2016-11-08 21:45:19]

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

[commit-bot: I haz the power, 2016-11-08 21:45:20]

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

[mmenke, 2016-11-08 21:56:52]

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

[commit-bot: I haz the power, 2016-11-08 21:57:25]

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, 2016-11-08 22:17:53]

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

[commit-bot: I haz the power, 2016-11-08 22:17:54]

Dry run: 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...)

[mmenke, 2016-11-09 17:32:59]

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

[commit-bot: I haz the power, 2016-11-09 17:33:21]

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, 2016-11-09 18:05:07]

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

[commit-bot: I haz the power, 2016-11-09 18:05:07]

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

[mmenke, 2016-11-14 20:02:27]

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

[commit-bot: I haz the power, 2016-11-14 20:03:52]

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

[mmenke, 2016-11-14 20:13:20]

Description was changed from

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

BUG=663456
==========

to

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456
==========

[mmenke, 2016-11-14 20:15:01]

mmenke@chromium.org changed reviewers:
+ tbansal@chromium.org

[mmenke, 2016-11-14 20:15:02]

tbansal:  Does this seem reasonable?  The NQE tests aren't fully setting up the
NQE, so I hacked around that in an ugly manner (I could have updated the text
expectations, but I don't understand the code well enough to understand if the
updated expectations would be correct.  Happy to explore that option further).

[mmenke, 2016-11-14 20:16:38]

https://codereview.chromium.org/2484323002/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:341:
content::TestBrowserThreadBundle::IO_MAINLOOP);
These are because the NQE expects a MessageLoop on construction, unlike the rest
of TestURLRequestContext, which only needs one when the first URLRequest is
started.

[tbansal1, 2016-11-14 20:26:01]

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:145:
params.socket_performance_watcher_factory =
Currently, the problem is that in some tests, |params| does not have NQE set.
With this fix, NQE will be set for params, but in some tests, |params| will have
a different NQE than the context.

Would a better solution be for NQE tests to override the |params| too?
Similar to:
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte...
If yes, I can do that for all NQE tests since I have already done it for one
other test. wdyt?

[commit-bot: I haz the power, 2016-11-14 20:29:03]

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

[commit-bot: I haz the power, 2016-11-14 20:29:04]

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_...)
  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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-11-14 20:29:52]

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:145:
params.socket_performance_watcher_factory =
On 2016/11/14 20:26:01, tbansal1 wrote:
> Currently, the problem is that in some tests, |params| does not have NQE set.
> With this fix, NQE will be set for params, but in some tests, |params| will
have
> a different NQE than the context.
> 
> Would a better solution be for NQE tests to override the |params| too?
> Similar to:
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte...
> If yes, I can do that for all NQE tests since I have already done it for one
> other test. wdyt?

A better solution instead of this CL, or in addition to this CL?

The goal of this CL is to make TestURLRequestContext better reflect the main
URLRequestContext in production code, and that includes the NQE.  Even if most
tests using the TestURLRequestContext don't explicitly check the NQE, this will
let them exercise its code, and make sure it doesn't crash or DCHECK under a
greater variety of circumstances than it makes sense to have explicit NQE tests
for.

[mmenke, 2016-11-14 20:31:56]

On 2016/11/14 20:29:52, mmenke wrote:
>
https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
> File net/url_request/url_request_test_util.cc (right):
> 
>
https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
> net/url_request/url_request_test_util.cc:145:
> params.socket_performance_watcher_factory =
> On 2016/11/14 20:26:01, tbansal1 wrote:
> > Currently, the problem is that in some tests, |params| does not have NQE
set.
> > With this fix, NQE will be set for params, but in some tests, |params| will
> have
> > a different NQE than the context.
> > 
> > Would a better solution be for NQE tests to override the |params| too?
> > Similar to:
> >
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte...
> > If yes, I can do that for all NQE tests since I have already done it for one
> > other test. wdyt?
> 
> A better solution instead of this CL, or in addition to this CL?
> 
> The goal of this CL is to make TestURLRequestContext better reflect the main
> URLRequestContext in production code, and that includes the NQE.  Even if most
> tests using the TestURLRequestContext don't explicitly check the NQE, this
will
> let them exercise its code, and make sure it doesn't crash or DCHECK under a
> greater variety of circumstances than it makes sense to have explicit NQE
tests
> for.

Note:  I've fixed the compile error, but uploading a new patch set claims it's
succeeding, but isn't doing anything.

[mmenke, 2016-11-14 20:35:33]

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

[commit-bot: I haz the power, 2016-11-14 20:36:16]

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

[tbansal1, 2016-11-14 20:39:27]

lgtm. Thanks for doing this. If you have problems fixing NQE tests, please let
me know.

https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:176: // TODO(mmenke):  Fix that?
can you reassign these to me. As mentioned elsewhere, I will change the tests so
that they override NQE in the params too.

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:145:
params.socket_performance_watcher_factory =
On 2016/11/14 20:29:52, mmenke wrote:
> On 2016/11/14 20:26:01, tbansal1 wrote:
> > Currently, the problem is that in some tests, |params| does not have NQE
set.
> > With this fix, NQE will be set for params, but in some tests, |params| will
> have
> > a different NQE than the context.
> > 
> > Would a better solution be for NQE tests to override the |params| too?
> > Similar to:
> >
>
https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator_unitte...
> > If yes, I can do that for all NQE tests since I have already done it for one
> > other test. wdyt?
> 
> A better solution instead of this CL, or in addition to this CL?
"In addition to". OK, I will do that after this CL is submitted.
> 
> The goal of this CL is to make TestURLRequestContext better reflect the main
> URLRequestContext in production code, and that includes the NQE.  Even if most
> tests using the TestURLRequestContext don't explicitly check the NQE, this
will
> let them exercise its code, and make sure it doesn't crash or DCHECK under a
> greater variety of circumstances than it makes sense to have explicit NQE
tests
> for.
Got it.

[mmenke, 2016-11-14 20:43:02]

https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:176: // TODO(mmenke):  Fix that?
On 2016/11/14 20:39:27, tbansal1 wrote:
> can you reassign these to me. As mentioned elsewhere, I will change the tests
so
> that they override NQE in the params too.

Do you want to do it, or would you rather me do it?  This comment implies the
former, "If you have problems fixing NQE tests, please let me know" implies the
former.

I'm happy to just update expectations according to the errors I get with setting
the NQE in the params as well (I'm reluctant to dig further into the code and
figure if those updates are reasonable, just because I don't think there's much
benefit in me learning the low level details of how NQE works - the higher level
details of how it's hooked into URLRequest may be useful to me in the future,
but not the socket layer stuff).

[tbansal1, 2016-11-14 21:26:59]

On 2016/11/14 20:43:02, mmenke wrote:
>
https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
> File net/nqe/network_quality_estimator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2484323002/diff/60001/net/nqe/network_quality...
> net/nqe/network_quality_estimator_unittest.cc:176: // TODO(mmenke):  Fix that?
> On 2016/11/14 20:39:27, tbansal1 wrote:
> > can you reassign these to me. As mentioned elsewhere, I will change the
tests
> so
> > that they override NQE in the params too.
> 
> Do you want to do it, or would you rather me do it?  This comment implies the
> former, "If you have problems fixing NQE tests, please let me know" implies
the
> former.
I can do it since I am familiar with the low level details of NQE. 

Sorry for the confusion. "If you have problems fixing NQE tests, please let me
know"
was for the current CL in the current state -- If there are NQE test failures
that are time-consuimg to debug, then it might be easier for me to dig into
them.
> 
> I'm happy to just update expectations according to the errors I get with
setting
> the NQE in the params as well (I'm reluctant to dig further into the code and
> figure if those updates are reasonable, just because I don't think there's
much
> benefit in me learning the low level details of how NQE works - the higher
level
> details of how it's hooked into URLRequest may be useful to me in the future,
> but not the socket layer stuff).

[commit-bot: I haz the power, 2016-11-14 21:27:59]

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

[commit-bot: I haz the power, 2016-11-14 21:28:00]

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

[mmenke, 2016-11-14 21:47:06]

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

[commit-bot: I haz the power, 2016-11-14 21:48:31]

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

[mmenke, 2016-11-14 22:18:26]

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

[commit-bot: I haz the power, 2016-11-14 22:19:37]

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, 2016-11-14 22:35:39]

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

[commit-bot: I haz the power, 2016-11-14 22:35:40]

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

[mmenke, 2016-11-14 22:51:52]

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

[commit-bot: I haz the power, 2016-11-14 22:53:12]

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, 2016-11-15 02:15:56]

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

[commit-bot: I haz the power, 2016-11-15 02:15:57]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-15 15:41:13]

mmenke@chromium.org changed reviewers:
+ palmer@chromium.org, tedchoc@chromium.org

[mmenke, 2016-11-15 15:41:13]

palmer:  Please review chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
(TestURLRequestContext must now be created on a thread with a message loop)
tedchoc:  Please review
content/browser/android/url_request_content_job_unittest.cc (Same thing -
message loop must be set up before creating the TestURLRequestContext, which
seems a good idea anyways, as using the context requires a MessageLoop to exist,
anyways.

[mmenke, 2016-11-15 15:42:15]

Oops, let's try that again, making things more legible.

[+palmer]:  Please review
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (TestURLRequestContext
must now be created on a thread with a message loop)

[+tedchoc]:  Please review
content/browser/android/url_request_content_job_unittest.cc (Same thing -
message loop must be set up before creating the  TestURLRequestContext, which
seems a good idea anyways, as using the context requires a MessageLoop to exist,
anyways.

[palmer, 2016-11-15 19:53:04]

lgtm

[Ted C, 2016-11-15 22:26:13]

On 2016/11/15 19:53:04, palmer wrote:
> lgtm

lgtm

[mmenke, 2016-11-15 22:29:04]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-11-15 22:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2484323002/#ps140001
(title: "Update comments, fix Android tests")

[commit-bot: I haz the power, 2016-11-15 22:29:52]

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

[commit-bot: I haz the power, 2016-11-15 22:46:02]

Description was changed from

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456
==========

to

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456
==========

[commit-bot: I haz the power, 2016-11-15 22:46:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-15 22:51:00]

Description was changed from

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456
==========

to

==========
Add a NetworkQualityEstimator to TestURLRequestContext.

This was inspired by a bug in exercised NetworkQualityEstimator NQE codepath,
though this CL would not have caught the issue.

BUG=663456

Committed: https://crrev.com/5ce3e1f8fef0f96ae28a09ac0aba2dc49802f9ee
Cr-Commit-Position: refs/heads/master@{#432277}
==========

[commit-bot: I haz the power, 2016-11-15 22:51:01]

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

[eroman, 2016-11-17 02:12:23]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2506303002/ by eroman@chromium.org.

The reason for reverting is: May be the cause of increase net_unittests flake,
speculatively reverting (see comment #20)

BUG=665686.