Add flag to override Effective Connection Type

Add flag to override Effective Connection Type

Add flag to override Effective Connection Type (ECT) returned by
Network Quality Estimator (NQE). If ECT is overridden using both the
flag and the field trial, then the value set by the flag takes the
precedence.

BUG=716221
Review-Url: https://codereview.chromium.org/2850003002
Cr-Commit-Position: refs/heads/master@{#469432}
Committed: https://chromium.googlesource.com/chromium/src/+/d3e08ca678225b65a89c91f01329ecc11ef8b554

[tbansal1, 2017-04-29 02:29:54]

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

[commit-bot: I haz the power, 2017-04-29 02:30:06]

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-04-29 03:16:03]

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

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

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

[tbansal1, 2017-05-01 17:21:37]

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

[commit-bot: I haz the power, 2017-05-01 17:22:12]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-01 17:30:24]

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

[commit-bot: I haz the power, 2017-05-01 17:30:49]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-01 17:35:18]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2017-05-01 17:35:35]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-05-01 18:45:19]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2017-05-01 18:47:09]

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

[commit-bot: I haz the power, 2017-05-01 18:48:05]

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-05-01 19:52:29]

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

[commit-bot: I haz the power, 2017-05-01 19:52:30]

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

[tbansal1, 2017-05-01 22:38:36]

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

[commit-bot: I haz the power, 2017-05-01 22:39:22]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-01 22:43:44]

Description was changed from

==========
Add flag for ECT

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

to

==========
Add flag to override Effective Connection Type

Add flag to override Effective Connection Type (ECT) returned by
Network Quality Estimator (NQE). If ECT is overridden using both the
flag and the field trial, then the value set by the flag takes the
precedence.

BUG=716221
==========

[tbansal1, 2017-05-01 22:43:53]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2017-05-01 22:44:01]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2017-05-01 22:44:21]

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

[tbansal1, 2017-05-01 22:44:22]

ryansturm: ptal at *. Thanks.

[RyanSturm, 2017-05-01 23:08:35]

lgtm % a few comments

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:744: // Ensure that all effective connection types
returned by Network Quality
Maybe add a static_assert to make sure kForceEffectiveConnectionTypeChoices has
length net::EFFECTIVE_CONNECTION_TYPE_LAST

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:3046: "Experimental effective connection
type";
All flags are experimental. "Simulate (or Override) effective connection type"
would be better IMO.

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:3050: "estimator.";
Add a terse description of what NQE is and what it controls?

[commit-bot: I haz the power, 2017-05-01 23:17:09]

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

[commit-bot: I haz the power, 2017-05-01 23:17:10]

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

[tbansal1, 2017-05-02 00:01:50]

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

[tbansal1, 2017-05-02 00:01:50]

mmenke: ptal at io_thread.cc and io_thread_unittest.cc.
ishereman: tools/metrics/histograms/histograms.xml
Thanks.

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:744: // Ensure that all effective connection types
returned by Network Quality
On 2017/05/01 23:08:34, Ryan Sturm wrote:
> Maybe add a static_assert to make sure kForceEffectiveConnectionTypeChoices
has
> length net::EFFECTIVE_CONNECTION_TYPE_LAST

Done.

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:3046: "Experimental effective connection
type";
On 2017/05/01 23:08:35, Ryan Sturm wrote:
> All flags are experimental. "Simulate (or Override) effective connection type"
> would be better IMO.

Done.

https://codereview.chromium.org/2850003002/diff/100001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:3050: "estimator.";
On 2017/05/01 23:08:35, Ryan Sturm wrote:
> Add a terse description of what NQE is and what it controls?

added a bit more of text.

[tbansal1, 2017-05-02 00:02:37]

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

[commit-bot: I haz the power, 2017-05-02 00:03:27]

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-05-02 01:07:18]

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

[commit-bot: I haz the power, 2017-05-02 01:07:19]

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

[mmenke, 2017-05-02 16:09:44]

LGTM

https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:575: }
Not going to block on this, but for network stack servification, I suspect we'll
want a C++ API here, rather than sending a bunch of key/value string pairs that
are parsed in net/.

[tbansal1, 2017-05-02 16:36:51]

On 2017/05/02 16:09:44, mmenke wrote:
> LGTM
> 
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> chrome/browser/io_thread.cc:575: }
> Not going to block on this, but for network stack servification, I suspect
we'll
> want a C++ API here, rather than sending a bunch of key/value string pairs
that
> are parsed in net/.

OK, one option is to move NQE params to HttpNetworkSession::Params. I believe
that's where QUIC and other net related params are stored. I will track that
work in https://bugs.chromium.org/p/chromium/issues/detail?id=705691.

[mmenke, 2017-05-02 16:40:35]

On 2017/05/02 16:36:51, tbansal1 wrote:
> On 2017/05/02 16:09:44, mmenke wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> > File chrome/browser/io_thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> > chrome/browser/io_thread.cc:575: }
> > Not going to block on this, but for network stack servification, I suspect
> we'll
> > want a C++ API here, rather than sending a bunch of key/value string pairs
> that
> > are parsed in net/.
> 
> OK, one option is to move NQE params to HttpNetworkSession::Params. I believe
> that's where QUIC and other net related params are stored. I will track that
> work in https://bugs.chromium.org/p/chromium/issues/detail?id=705691.

I don't think that's a good idea, since NQE is part of the URLRequestContext,
not just the HttpNetworkSession - HttpNetworkSession::Params are currently just
used for things scoped purely to the session.

[Ilya Sherman, 2017-05-02 21:15:49]

histograms.xml lgtm

[tbansal1, 2017-05-02 21:54:15]

On 2017/05/02 16:40:35, mmenke wrote:
> On 2017/05/02 16:36:51, tbansal1 wrote:
> > On 2017/05/02 16:09:44, mmenke wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> > > File chrome/browser/io_thread.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2850003002/diff/120001/chrome/browser/io_thre...
> > > chrome/browser/io_thread.cc:575: }
> > > Not going to block on this, but for network stack servification, I suspect
> > we'll
> > > want a C++ API here, rather than sending a bunch of key/value string pairs
> > that
> > > are parsed in net/.
> > 
> > OK, one option is to move NQE params to HttpNetworkSession::Params. I
believe
> > that's where QUIC and other net related params are stored. I will track that
> > work in https://bugs.chromium.org/p/chromium/issues/detail?id=705691.
> 
> I don't think that's a good idea, since NQE is part of the URLRequestContext,
> not just the HttpNetworkSession - HttpNetworkSession::Params are currently
just
> used for things scoped purely to the session.

Thanks, I will figure out and send a CL soon. This is also needed to enable
Cronet embedders to set the NQE config params.

[tbansal1, 2017-05-03 23:07:51]

Patchset #3 (id:140001) has been deleted

[tbansal1, 2017-05-03 23:08:10]

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

[commit-bot: I haz the power, 2017-05-03 23:08: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, 2017-05-04 00:19:14]

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

[commit-bot: I haz the power, 2017-05-04 00:19:15]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-04 14:40:15]

mmenke: PTAnL at io_thread-unittest.cc. I made some changes to fix a breaking
test (specifically, the changes delay the creation of the IOthread). Thanks.

[tbansal1, 2017-05-04 17:12:13]

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

[commit-bot: I haz the power, 2017-05-04 17:12:51]

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-05-04 18:43:24]

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

[commit-bot: I haz the power, 2017-05-04 18:43:25]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-04 19:41:45]

On 2017/05/04 14:40:15, tbansal1 wrote:
> mmenke: PTAnL at io_thread-unittest.cc. I made some changes to fix a breaking
> test (specifically, the changes delay the creation of the IOthread). Thanks.

LGTM

[tbansal1, 2017-05-04 19:43:47]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-05-04 19:43:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2850003002/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-04 19:44:17]

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-05-04 19:51:15]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493927027548570,
"parent_rev": "6dbff02c181d5689f8c509b0890733c5c7b9a413", "commit_rev":
"d3e08ca678225b65a89c91f01329ecc11ef8b554"}

[commit-bot: I haz the power, 2017-05-04 19:51:34]

Description was changed from

==========
Add flag to override Effective Connection Type

Add flag to override Effective Connection Type (ECT) returned by
Network Quality Estimator (NQE). If ECT is overridden using both the
flag and the field trial, then the value set by the flag takes the
precedence.

BUG=716221
==========

to

==========
Add flag to override Effective Connection Type

Add flag to override Effective Connection Type (ECT) returned by
Network Quality Estimator (NQE). If ECT is overridden using both the
flag and the field trial, then the value set by the flag takes the
precedence.

BUG=716221

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

[commit-bot: I haz the power, 2017-05-04 19:51:36]

Committed patchset #4 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d3e08ca678225b65a89c91f01329...