Remove obsolete QUIC disabling code.

Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss during the handshake. Also remove the "k of n" tracking and per-port disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Committed: https://crrev.com/703d0e023465c06a8309c24aa9f0713ee53a1482
Cr-Original-Commit-Position: refs/heads/master@{#417677}
Cr-Commit-Position: refs/heads/master@{#418309}

[Ryan Hamilton, 2016-09-07 22:12:23]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet
loss during the handshake. Also remove the "k of n" tracking.
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet
loss during the handshake. Also remove the "k of n" tracking.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Ryan Hamilton, 2016-09-07 22:25:38]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet
loss during the handshake. Also remove the "k of n" tracking.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Ryan Hamilton, 2016-09-07 22:28:01]

Cleanup

[Ryan Hamilton, 2016-09-07 22:28:50]

Cleanup

[Ryan Hamilton, 2016-09-07 22:29:56]

rch@chromium.org changed reviewers:
+ ckrasic@chromium.org, eroman@chromium.org, ianswett@chromium.org

[Ryan Hamilton, 2016-09-07 22:29:56]

eroman: chrome/browser/resources/net_internals/quic_view.html
ckrasic: PTAL
ianswett: FYI

[Buck, 2016-09-07 22:54:16]

lgtm

https://codereview.chromium.org/2318053004/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_connection_logger.h (left):

https://codereview.chromium.org/2318053004/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_connection_logger.h:87: // Returns connection's overall
packet loss rate in fraction.
Not a big deal, but not obvious to me why only removing the comment here.

[Ryan Hamilton, 2016-09-07 23:02:37]

https://codereview.chromium.org/2318053004/diff/40001/net/quic/chromium/quic_...
File net/quic/chromium/quic_connection_logger.h (left):

https://codereview.chromium.org/2318053004/diff/40001/net/quic/chromium/quic_...
net/quic/chromium/quic_connection_logger.h:87: // Returns connection's overall
packet loss rate in fraction.
On 2016/09/07 22:54:16, Buck wrote:
> Not a big deal, but not obvious to me why only removing the comment here.

Thanks. I added it back. I accidentally removed it when I thought I could remove
this whole method. When I realized that I couldn't, I apparently forgot to
restore the comment.

[ianswett, 2016-09-08 19:37:02]

Thanks for the cleanup.  One question on the enum.

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (left):

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:65: enum QuicDisabledReason {
Are we going to want this enum again in the near future?

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.h:550: bool
disable_quic_on_timeout_with_open_streams_;
Were you intending to keep this?  You removed the enum.

[Ryan Hamilton, 2016-09-08 19:42:02]

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_chromium_client_session.h (left):

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_chromium_client_session.h:65: enum QuicDisabledReason {
On 2016/09/08 19:37:02, ianswett wrote:
> Are we going to want this enum again in the near future?

I don't think so. But if we need it, we can add it back.

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.h (right):

https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.h:550: bool
disable_quic_on_timeout_with_open_streams_;
On 2016/09/08 19:37:02, ianswett wrote:
> Were you intending to keep this?  You removed the enum.

Yes, I want this. This is true if we want to try doing the disabling. It's set
via a field trial so we can measure performance. Sound reasonable? (It's used in
the .cc file)

[ianswett, 2016-09-08 19:44:43]

On 2016/09/08 19:42:02, Ryan Hamilton wrote:
>
https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
> File net/quic/chromium/quic_chromium_client_session.h (left):
> 
>
https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
> net/quic/chromium/quic_chromium_client_session.h:65: enum QuicDisabledReason {
> On 2016/09/08 19:37:02, ianswett wrote:
> > Are we going to want this enum again in the near future?
> 
> I don't think so. But if we need it, we can add it back.
> 
>
https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
> File net/quic/chromium/quic_stream_factory.h (right):
> 
>
https://codereview.chromium.org/2318053004/diff/60001/net/quic/chromium/quic_...
> net/quic/chromium/quic_stream_factory.h:550: bool
> disable_quic_on_timeout_with_open_streams_;
> On 2016/09/08 19:37:02, ianswett wrote:
> > Were you intending to keep this?  You removed the enum.
> 
> Yes, I want this. This is true if we want to try doing the disabling. It's set
> via a field trial so we can measure performance. Sound reasonable? (It's used
in
> the .cc file)

SGTM.

[ianswett, 2016-09-08 19:45:11]

lgtm

[eroman, 2016-09-09 18:33:07]

lgtm

[Ryan Hamilton, 2016-09-09 18:43:13]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-09-09 18:43:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ckrasic@chromium.org
Link to the patchset: https://codereview.chromium.org/2318053004/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2016-09-09 18:43:46]

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

[commit-bot: I haz the power, 2016-09-09 20:03:33]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-09-09 20:03:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-09 20:04:50]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

[commit-bot: I haz the power, 2016-09-09 20:04:52]

Patchset 4 (id:??) landed as
https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}

[dmurph, 2016-09-09 20:36:09]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2327923002/ by dmurph@chromium.org.

The reason for reverting is: test build broken on Android
BUG=645623.

[the real yoland, 2016-09-09 20:40:32]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2320313003/ by yolandyan@chromium.org.

The reason for reverting is: Failure:
https://build.chromium.org/p/chromium/builders/Android/builds/61986
which caused the chromium tree to be closed.

[findit-for-me, 2016-09-09 21:23:29]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 417677 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Ryan Hamilton, 2016-09-09 22:06:15]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

[Ryan Hamilton, 2016-09-12 16:38:45]

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

[Ryan Hamilton, 2016-09-12 16:38:46]

xunjieli: 	components/cronet/, please

[xunjieli, 2016-09-12 16:43:00]

There might be some left in the other two Cronet files.

(1) Could you remove kQuicMaxNumberOfLossyConnections and
kQuicPacketLossThreshold from
components/cronet/url_request_context_config.cc ?

(2) Could you remove 
 .put("max_number_of_lossy_connections", 10)
 .put("packet_loss_threshold", 0.5)
from components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
?


Thanks.

[Ryan Hamilton, 2016-09-12 16:59:49]

On 2016/09/12 16:43:00, xunjieli wrote:
> There might be some left in the other two Cronet files.
> 
> (1) Could you remove kQuicMaxNumberOfLossyConnections and
> kQuicPacketLossThreshold from
> components/cronet/url_request_context_config.cc ?
> 
> (2) Could you remove 
>  .put("max_number_of_lossy_connections", 10)
>  .put("packet_loss_threshold", 0.5)
> from
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
> ?
> 
> 
> Thanks.

Done. Thanks!

[xunjieli, 2016-09-12 17:14:46]

LGTM mod one comment below.

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:157: }
line 145 - line 157 are no longer relevant?

[xunjieli, 2016-09-12 17:17:44]

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:157: }
On 2016/09/12 17:14:46, xunjieli wrote:
> line 145 - line 157 are no longer relevant?

Might be also worth removing
URLRequestContextBuilder::set_quic_max_number_of_lossy_connections
and set_quic_packet_loss_threshold, since these two fields are no longer in the
HttpNetworkSession::Params.

[Ryan Hamilton, 2016-09-12 17:48:21]

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

[commit-bot: I haz the power, 2016-09-12 17:49:05]

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

[Ryan Hamilton, 2016-09-12 17:49:27]

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:157: }
On 2016/09/12 17:17:43, xunjieli wrote:
> On 2016/09/12 17:14:46, xunjieli wrote:
> > line 145 - line 157 are no longer relevant?
> 
> Might be also worth removing
> URLRequestContextBuilder::set_quic_max_number_of_lossy_connections
> and set_quic_packet_loss_threshold, since these two fields are no longer in
the
> HttpNetworkSession::Params.

Good point. Done

https://codereview.chromium.org/2318053004/diff/100001/components/cronet/url_...
components/cronet/url_request_context_config.cc:157: }
On 2016/09/12 17:14:46, xunjieli wrote:
> line 145 - line 157 are no longer relevant?

Done.

[xunjieli, 2016-09-12 17:51:44]

LGTM! Thanks

[Ryan Hamilton, 2016-09-12 19:00:02]

The CQ bit was unchecked by rch@chromium.org

[Ryan Hamilton, 2016-09-13 04:18:07]

On 2016/09/12 17:51:44, xunjieli wrote:
> LGTM! Thanks

Do you happen to know how I can see the ouput from the failing tests?

https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...

[xunjieli, 2016-09-13 13:48:59]

On 2016/09/13 04:18:07, Ryan Hamilton wrote:
> On 2016/09/12 17:51:44, xunjieli wrote:
> > LGTM! Thanks
> 
> Do you happen to know how I can see the ouput from the failing tests?
> 
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...

Sorry I was OOO yesterday afternoon. The bot is red probably due to
crbug.com/637051. Sorry about that. The revert is landed so I kicked off another
run. You can find the logs ("logcat dump") under "gsutil upload" step. The
symbolicated one is "stdio" under "stack_tool_with_logcat_dump" step.

[Ryan Hamilton, 2016-09-13 17:21:52]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-09-13 17:21:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
ianswett@chromium.org, eroman@chromium.org, ckrasic@chromium.org,
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2318053004/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-13 17:22:20]

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

[Ryan Hamilton, 2016-09-13 17:22:38]

On 2016/09/13 13:48:59, xunjieli wrote:
> On 2016/09/13 04:18:07, Ryan Hamilton wrote:
> > On 2016/09/12 17:51:44, xunjieli wrote:
> > > LGTM! Thanks
> > 
> > Do you happen to know how I can see the ouput from the failing tests?
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...
> 
> Sorry I was OOO yesterday afternoon. The bot is red probably due to
> crbug.com/637051. Sorry about that. The revert is landed so I kicked off
another
> run. You can find the logs ("logcat dump") under "gsutil upload" step. The
> symbolicated one is "stdio" under "stack_tool_with_logcat_dump" step.

Ah, great. thanks! Ok, looks like it passed the bot. Trying to land again.
*fingers crossed*

Thanks!

[commit-bot: I haz the power, 2016-09-13 18:35:56]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

[commit-bot: I haz the power, 2016-09-13 18:35:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-13 18:38:42]

Description was changed from

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Cr-Commit-Position: refs/heads/master@{#417677}
==========

to

==========
Remove obsolete QUIC disabling code.

No longer disable QUIC based on public resets post handshake or packet loss
during the handshake. Also remove the "k of n" tracking and per-port
disablement.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1c1aa05eaa6b2a04c51e175ed76b5c6f810ff0aa
Committed: https://crrev.com/703d0e023465c06a8309c24aa9f0713ee53a1482
Cr-Original-Commit-Position: refs/heads/master@{#417677}
Cr-Commit-Position: refs/heads/master@{#418309}
==========

[commit-bot: I haz the power, 2016-09-13 18:38:43]

Patchset 9 (id:??) landed as
https://crrev.com/703d0e023465c06a8309c24aa9f0713ee53a1482
Cr-Commit-Position: refs/heads/master@{#418309}