Disable HTTP/2 Alternative Services.

Disable HTTP/2 Alternative Services.

* Temporarily disable HTTP/2 Alternative Services,
  see https://crbug.com/706974#c5 for motivation.
* Rename test-only enable_http2_alternative_service_with_different_host
  to enable_http2_alternative_service and change function accordingly.

BUG=706974
Review-Url: https://codereview.chromium.org/2821463002
Cr-Commit-Position: refs/heads/master@{#464916}
Committed: https://chromium.googlesource.com/chromium/src/+/a86731ee4f39b32af514750aac3dffa44408268e

[Bence, 2017-04-13 15:05:51]

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

[commit-bot: I haz the power, 2017-04-13 15:06:38]

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-13 15:15:19]

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

[commit-bot: I haz the power, 2017-04-13 15:15:20]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bence, 2017-04-13 15:28:16]

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

[commit-bot: I haz the power, 2017-04-13 15:29: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-04-13 16:42:11]

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

[commit-bot: I haz the power, 2017-04-13 16:42:11]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-04-13 16:47:12]

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

[Bence, 2017-04-13 16:47:12]

Ryan: PTAL.
xunjieli: FYI.

[Ryan Hamilton, 2017-04-13 21:53:45]

lgtm, but I don't understand one change.

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:958: }
I don't understand why we want to move this here instead of keeping it down in
the loop in GetAlternativeServiceForInternal. Can you say more?

[Bence, 2017-04-14 01:10:15]

bnc@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Bence, 2017-04-14 01:10:16]

Sergey: PTAL at jingle/
Ryan: PTAL at net/

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:958: }
On 2017/04/13 21:53:45, Ryan Hamilton wrote:
> I don't understand why we want to move this here instead of keeping it down in
> the loop in GetAlternativeServiceForInternal. Can you say more?

Sure.  If it's in GetAlternativeServiceForInternal(), then that never returns an
HTTP/2 alternative service (unless in tests), so
Net.AlternativeServiceTypeForRequest never logs them, so we won't know if they
are advertised in the wild.

[Ryan Hamilton, 2017-04-14 02:49:13]

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:958: }
On 2017/04/14 01:10:16, Bence wrote:
> On 2017/04/13 21:53:45, Ryan Hamilton wrote:
> > I don't understand why we want to move this here instead of keeping it down
in
> > the loop in GetAlternativeServiceForInternal. Can you say more?
> 
> Sure.  If it's in GetAlternativeServiceForInternal(), then that never returns
an
> HTTP/2 alternative service (unless in tests), so
> Net.AlternativeServiceTypeForRequest never logs them, so we won't know if they
> are advertised in the wild.

In that case, perhaps we need a new histogram for rejected AlternativeServices?
I ask because I know that there are server operators planning to advertise both
QUIC and remote H2 so I'd like to make sure that when we encounter those servers
that we speak one of the alternatives.

[Bence, 2017-04-14 18:10:25]

Description was changed from

==========
Disable HTTP/2 Alternative Services.

* Temporarily disable HTTP/2 Alternative Services,
  see https://crbug.com/706974#c5 for motivation.
* Rename test-only enable_http2_alternative_service_with_different_host
  to enable_http2_alternative_service and change function accordingly.
* Move disabling logic to
  HttpStreamFactoryImpl::Job::Controller::GetAlternativeServiceFor() from
  GetAlternativeServiceForInternal() to make sure that a remote HTTP/2
  alternative service is logged in Net.AlternativeServiceTypeForRequest
  even though it is not used.  This has a side effect of disabling QUIC
  when there is a remote HTTP/2 and a QUIC alt-svc entry in this order,
  which probably never happens in the wild.

BUG=706974
==========

to

==========
Disable HTTP/2 Alternative Services.

* Temporarily disable HTTP/2 Alternative Services,
  see https://crbug.com/706974#c5 for motivation.
* Rename test-only enable_http2_alternative_service_with_different_host
  to enable_http2_alternative_service and change function accordingly.

BUG=706974
==========

[Sergey Ulanov, 2017-04-14 18:20:16]

lgtm

[Bence, 2017-04-14 18:20:24]

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

[commit-bot: I haz the power, 2017-04-14 18:20:50]

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-14 19:19:16]

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

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

Dry run: This issue passed the CQ dry run.

[Bence, 2017-04-17 11:29:32]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-04-17 11:29:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2821463002/#ps60001
(title: "Revert change in histogram logic.")

[commit-bot: I haz the power, 2017-04-17 11:29:44]

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

[Bence, 2017-04-17 11:29:46]

Thank you both for the review.

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2821463002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:958: }
Oops, I wasn't aware of that, sorry.  Let me just revert this part of the change
to make sure that QUIC is used if it is advertised.

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

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492428572885650,
"parent_rev": "6648e7bc747aab9feddecea425dab113924d94cb", "commit_rev":
"a86731ee4f39b32af514750aac3dffa44408268e"}

[commit-bot: I haz the power, 2017-04-17 12:31:41]

Description was changed from

==========
Disable HTTP/2 Alternative Services.

* Temporarily disable HTTP/2 Alternative Services,
  see https://crbug.com/706974#c5 for motivation.
* Rename test-only enable_http2_alternative_service_with_different_host
  to enable_http2_alternative_service and change function accordingly.

BUG=706974
==========

to

==========
Disable HTTP/2 Alternative Services.

* Temporarily disable HTTP/2 Alternative Services,
  see https://crbug.com/706974#c5 for motivation.
* Rename test-only enable_http2_alternative_service_with_different_host
  to enable_http2_alternative_service and change function accordingly.

BUG=706974

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

[commit-bot: I haz the power, 2017-04-17 12:31:42]

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