Add and persist a new field in AlternativeServiceInfo to list QUIC verisons advertised

Add a new field in AlternativeServiceInfo to list QUIC verisons that are advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2901093004
Cr-Commit-Position: refs/heads/master@{#482651}
Committed: https://chromium.googlesource.com/chromium/src/+/e537a0070404ceaae5d8ad6b7659a7cd24469996

[Zhongyi Shi, 2017-05-24 17:29:18]

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

[Ryan Hamilton, 2017-05-24 19:52:54]

Sweet. One question. It looks like the new field is currently being populated in
some places, but not being read. Is that right? If so, I think I'd avoid
populating it anywhere as part of this CL.

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:124: base::Time expiration);
Does anything use this constructor? SetAlternativeServices seems to use the
second, but I didn't look closely enough.

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:126: // QUIC Alternative Service.
I'm not sure these two comments are accurate. I don't think that the
constructors check the protocol in |alternative_service|. So it would be
possible to use either one. Also, It's possible to advertise QUIC support
without specifying the list of QUIC versions which might suggest a QUIC alt-svc
being set with the first version.

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:135: const AlternativeServiceInfo&
alternative_service_info) = default;
(I'm surprised that you don't get a compiler complaint about = default here. In
any case, since the copy constructor moved to the .cc file, can you move the
assignment operator there too?

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:138: // is persisted to disk.
Why not do this now?

[Zhongyi Shi, 2017-05-24 22:44:55]

Ryan: thanks for helping the review. This CL is mainly for review purpose and
will not be used to land changes.

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:124: base::Time expiration);
On 2017/05/24 19:52:53, Ryan Hamilton wrote:
> Does anything use this constructor? SetAlternativeServices seems to use the
> second, but I didn't look closely enough.

SetAlternativeService uses the second, SetAlternativeService*s* takes a
AlternativeServiceInfoVector. 

SpdySession::OnAltSvc() uses this constructor to create AlternativeServicesInfo,
and pushes to a AlternativeServiceInfoVector. 
https://cs.chromium.org/chromium/src/net/spdy/chromium/spdy_session.cc?q=Alte...

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:126: // QUIC Alternative Service.
On 2017/05/24 19:52:53, Ryan Hamilton wrote:
> I'm not sure these two comments are accurate. I don't think that the
> constructors check the protocol in |alternative_service|. So it would be
> possible to use either one. Also, It's possible to advertise QUIC support
> without specifying the list of QUIC versions which might suggest a QUIC
alt-svc
> being set with the first version.

Done.

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:138: // is persisted to disk.
On 2017/05/24 19:52:54, Ryan Hamilton wrote:
> Why not do this now?

Ah, apparently I forgot to give a heads-up on this CL: I didn't plan to land
this CL but for review only. A following CL dependent on this one will be sent
for review which adds the logic to persist Alt Svc Info with versions into disk.

[Ryan Hamilton, 2017-05-24 23:12:12]

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:124: base::Time expiration);
On 2017/05/24 22:44:54, Zhongyi Shi wrote:
> On 2017/05/24 19:52:53, Ryan Hamilton wrote:
> > Does anything use this constructor? SetAlternativeServices seems to use the
> > second, but I didn't look closely enough.
> 
> SetAlternativeService uses the second, SetAlternativeService*s* takes a
> AlternativeServiceInfoVector. 
> 
> SpdySession::OnAltSvc() uses this constructor to create
AlternativeServicesInfo,
> and pushes to a AlternativeServiceInfoVector. 
>
https://cs.chromium.org/chromium/src/net/spdy/chromium/spdy_session.cc?q=Alte...

Can SpdySession create a QUIC Alt-Svc?

https://codereview.chromium.org/2901093004/diff/60001/net/http/http_server_pr...
net/http/http_server_properties.h:138: // is persisted to disk.
On 2017/05/24 22:44:54, Zhongyi Shi wrote:
> On 2017/05/24 19:52:54, Ryan Hamilton wrote:
> > Why not do this now?
> 
> Ah, apparently I forgot to give a heads-up on this CL: I didn't plan to land
> this CL but for review only. A following CL dependent on this one will be sent
> for review which adds the logic to persist Alt Svc Info with versions into
disk.

Heh. OK. Any reason not to land this?

[Zhongyi Shi, 2017-05-25 19:06:19]

Patchset #6 (id:100001) has been deleted

[Zhongyi Shi, 2017-05-25 19:43:30]

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

[commit-bot: I haz the power, 2017-05-25 19:44:33]

Dry run: CQ is trying da patch.

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

[Zhongyi Shi, 2017-05-25 19:44:40]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons advertised
by the server.

No feature change, following CL will persist version list into disk.

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons advertised
by the server.

No feature change. 

BUG=689762
==========

[Zhongyi Shi, 2017-05-25 19:46:28]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons advertised
by the server.

No feature change. 

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons advertised by
the server. By default, this field is filled with the versions supported by
Chrome as Chrome. 

No feature change. 

BUG=689762
==========

[Zhongyi Shi, 2017-05-25 19:48:48]

Ryan: Thanks for helping the review.  As discussed offline, I  have simplify
this CL to only add the version list. The next CL will take a version list for
AlternativeServiceInfo constructor, SetAlternativeServices(write) and
GetAlternativeServices(read) will also respect the advertsied_versions. PTAL!

[commit-bot: I haz the power, 2017-05-25 21:41:38]

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

[commit-bot: I haz the power, 2017-05-25 21:41:39]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-05-25 22:01:46]

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
net/http/http_server_properties.cc:91: advertised_versions =
HttpNetworkSession::Params().quic_supported_versions;
I don't think this is the right thing to do. This is the default list of
version. It is not the set of versions used by the current instance of the
network stack, may have been configured via finch or command line flag.

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
net/http/http_server_properties.h:132: // is persisted to disk.
I'm not sure I understand why we can't do this now.

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
net/http/http_server_properties_impl_unittest.cc:641: 
nit: remove

[Zhongyi Shi, 2017-06-05 19:57:57]

Patchset #10 (id:200001) has been deleted

[Zhongyi Shi, 2017-06-06 21:51:07]

Patchset #12 (id:260001) has been deleted

[Zhongyi Shi, 2017-06-06 22:04:52]

I have added Get/Set logic to persist advertised versions into disk. Unit tests
are also added. PTAL, thanks!

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
net/http/http_server_properties.h:132: // is persisted to disk.
On 2017/05/25 22:01:45, Ryan Hamilton wrote:
> I'm not sure I understand why we can't do this now.

Done.
Ah, we definitely can do this together. I was thinking make split those changes
in smaller CLs. But yeah, all those changes together make more sense.

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/160001/net/http/http_server_p...
net/http/http_server_properties_impl_unittest.cc:641: 
On 2017/05/25 22:01:45, Ryan Hamilton wrote:
> nit: remove

Done.

https://codereview.chromium.org/2901093004/diff/300001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/300001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:9229:
HttpNetworkSession::Params().quic_supported_versions);
Changes introduced in this file are caused by the API changes for
HttpServerProperties::SetAlternativeService.

https://codereview.chromium.org/2901093004/diff/300001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/300001/net/http/http_server_p...
net/http/http_server_properties.h:165: QuicVersionVector advertised_versions_;
We might want to change the AlternativeServiceInfo to a class. I prefer to do
the simple refactoring in a follow-up CL to keep this CL simple.

[Zhongyi Shi, 2017-06-06 22:08:06]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons advertised by
the server. By default, this field is filled with the versions supported by
Chrome as Chrome. 

No feature change. 

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. The list will also be
persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the versions
supported by the current instance of network stack.

BUG=689762
==========

[Zhongyi Shi, 2017-06-06 22:08:18]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. The list will also be
persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the versions
supported by the current instance of network stack.

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the versions
supported by the current instance of network stack.

BUG=689762
==========

[Zhongyi Shi, 2017-06-06 22:08:31]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the versions
supported by the current instance of network stack.

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current instance of network stack.

BUG=689762
==========

[Zhongyi Shi, 2017-06-06 22:08:53]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current instance of network stack.

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current running instance of network stack.

BUG=689762
==========

[Zhongyi Shi, 2017-06-06 23:09:25]

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

[commit-bot: I haz the power, 2017-06-06 23:09:45]

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

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

[commit-bot: I haz the power, 2017-06-06 23:19:15]

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

[Zhongyi Shi, 2017-06-07 07:07:05]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current running instance of network stack.

BUG=689762
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current running instance of network stack.

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[Zhongyi Shi, 2017-06-07 07:07:12]

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

[commit-bot: I haz the power, 2017-06-07 07:07:23]

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-06-07 07:17:15]

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

[commit-bot: I haz the power, 2017-06-07 07:17:17]

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

[Zhongyi Shi, 2017-06-07 19:10:52]

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

[commit-bot: I haz the power, 2017-06-07 19:11:29]

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-06-07 19:20:22]

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

[commit-bot: I haz the power, 2017-06-07 19:20:23]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ryan Hamilton, 2017-06-07 20:52:24]

Looks promising!

https://codereview.chromium.org/2901093004/diff/360001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:768:
QuicVersionVector());
Would QuicSupportedVersions() be a better choice here? (I haven't looked deeper
to know so you're probably doing the right thing already)

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:9244:
HttpNetworkSession::Params().quic_supported_versions);
Hm, I'm not sure about this. Can we get the params from |session| instead? (here
and elsewhere)

Alternatively, since this is an H2 alt-svc, perhaps we should pass in an empty
vector?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.cc:115: return false;
Is this expression necessary? I suspect == will just do the right thing.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:126: base::Time expirationi,
nit: trailing "i"?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:144: expiration == other.expiration &&
EqualAdvertisedVersions(other);
can you just use advertised_versions == other.advertised_versions?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:155: }
This is a struct not a class so the members are public. I think this means you
can skip the getter.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:417: // Also persist to disk if
advertised versions change significantly for
What does "significantly" mean here?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:419: if
(!old.EqualAdvertisedVersions(*new_it)) {
can you just do old.advertised_services == new_it->advertised_services?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl_unittest.cc:434:
HttpNetworkSession::Params().quic_supported_versions);
For tests, I'd just use QuicSupportedVersions()?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:627: return false;
Yay, early return!

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:654: int version_int;
nit: since the loop variable is |value|, I think you can just call this
|version|.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory.cc:55: // in advertised versions list.
This second sentence is confusing to me. How about a single sentence which says:

// Filter supported QUIC versions.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1163: }
I wonder if we can get away without this code. If we don't have this, then we'll
end up creating a job with an empty QUIC version vector, right? And that vector
will (eventually) be passed down to the QuicStreamFactory. I think the
QuicStreamFactory would be a good place to apply this logic.

What do you think?

https://codereview.chromium.org/2901093004/diff/360001/net/quic/chromium/quic...
File net/quic/chromium/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/quic/chromium/quic...
net/quic/chromium/quic_network_transaction_unittest.cc:136: result.append(",");
nit: 

for (QuicVersion version : versions) {
  if (!result.empty())
    result.append(",")
  ...
}

[Bence, 2017-06-08 17:44:50]

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

[Bence, 2017-06-08 17:44:52]

Minor drive-by comments.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:1118:
std::unique_ptr<base::ListValue> advertised_versions_list(
Optional: According to
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/evQtLFgfmyM...
also
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=43
the preferred format is:
auto advertised_versions_list = base::MakeUnique<base::ListValue>();

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1155: //
AlternaitverServiceInfo, by default append the list of QUIC versions
s/AlternaitverServiceInfo/AlternativeServiceInfo/

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:352:
session_deps_.proxy_service.reset(new ProxyService(
session_deps_.proxy_service = base::MakeUnique<ProxyService>(

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:354:
base::WrapUnique(new FailingProxyResolverFactory), nullptr));
base::MakeUnique<FailingProxyResolverFactory>()

[Zhongyi Shi, 2017-06-08 22:55:48]

Patchset #20 (id:440001) has been deleted

[Zhongyi Shi, 2017-06-08 23:11:14]

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

[Zhongyi Shi, 2017-06-08 23:11:17]

Thanks for helping the review. I have addressed most of the comments, PTAL!

https://codereview.chromium.org/2901093004/diff/360001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:768:
QuicVersionVector());
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> Would QuicSupportedVersions() be a better choice here? (I haven't looked
deeper
> to know so you're probably doing the right thing already)

Originally, an empty list should work, as the versions will be filled in
QuicStreamFactory later. I think it should be fine to add the versions this
network stack supports from the URLRequestContext. 

+xunjieli@: Helen, does this sound the right thing to do?

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:9244:
HttpNetworkSession::Params().quic_supported_versions);
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> Hm, I'm not sure about this. Can we get the params from |session| instead?
(here
> and elsewhere)
> 
> Alternatively, since this is an H2 alt-svc, perhaps we should pass in an empty
> vector?

Thanks for catching this. This change was added in a previous patch. Yeah, we
should pass in an empty vector as H2 alt svc shouldn't have any QUIC versions
advertised. The reason we didn't get anything exploded in tests is that the
advertised versions is only set in AlternativeServiceInfo when the protocol is
QUIC.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.cc:115: return false;
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> Is this expression necessary? I suspect == will just do the right thing.

Acknowledged. This method is removed in the new patch.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:126: base::Time expirationi,
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> nit: trailing "i"?

Done.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:144: expiration == other.expiration &&
EqualAdvertisedVersions(other);
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> can you just use advertised_versions == other.advertised_versions?

DONE.

I was trying to skip the versions comparison if the protocol is not H2. But now
that I have make advertised_versions_ private, and it's only set when protocol
is QUIC. Yeah, a simple equality check on the versions is safe!

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:155: }
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> This is a struct not a class so the members are public. I think this means you
> can skip the getter.

advertised_versions_ are marked as private (and I planned to change this struct
to class in a follow-up CL). I change the advertised_versions_ by intention so
that when we set it, it is guaranteed to be sorted.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:417: // Also persist to disk if
advertised versions change significantly for
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> What does "significantly" mean here?

Ah, advertised_versions used to be not sorted, and significant change means the
elements in the set changes regardless of the order. Since we have changed the
advertised_versions to be always sorted. The equality check on the versions is
trivial and sufficient enough.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:419: if
(!old.EqualAdvertisedVersions(*new_it)) {
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> can you just do old.advertised_services == new_it->advertised_services?

Done.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_impl_unittest.cc:434:
HttpNetworkSession::Params().quic_supported_versions);
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> For tests, I'd just use QuicSupportedVersions()?

Fixed. We should use an empty version vector actually since we are setting a H2
alt svc.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:654: int version_int;
On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> nit: since the loop variable is |value|, I think you can just call this
> |version|.

Done.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:1118:
std::unique_ptr<base::ListValue> advertised_versions_list(
On 2017/06/08 17:44:51, Bence wrote:
> Optional: According to
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/evQtLFgfmyM...
> also
> https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?l=43
> the preferred format is:
> auto advertised_versions_list = base::MakeUnique<base::ListValue>();

Done.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory.cc:55: // in advertised versions list.
On 2017/06/07 20:52:24, Ryan Hamilton wrote:
> This second sentence is confusing to me. How about a single sentence which
says:
> 
> // Filter supported QUIC versions.

Done.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1155: //
AlternaitverServiceInfo, by default append the list of QUIC versions
On 2017/06/08 17:44:51, Bence wrote:
> s/AlternaitverServiceInfo/AlternativeServiceInfo/

Acknowledged.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:1163: }
On 2017/06/07 20:52:24, Ryan Hamilton wrote:
> I wonder if we can get away without this code. If we don't have this, then
we'll
> end up creating a job with an empty QUIC version vector, right? And that
vector
> will (eventually) be passed down to the QuicStreamFactory. I think the
> QuicStreamFactory would be a good place to apply this logic.
> 
> What do you think?

Ah good idea! I didn't think about this. But yeah, QuicStreamFactory knows
currently supported versions by Chrome, and should be able to figure out what to
do with an empty list of advertised versions.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:352:
session_deps_.proxy_service.reset(new ProxyService(
On 2017/06/08 17:44:51, Bence wrote:
> session_deps_.proxy_service = base::MakeUnique<ProxyService>(

This is a change not related to this CL. Not sure what's up here, but apparently
I had some conflicts when rebase. I will resolve this after a new rebase.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:354:
base::WrapUnique(new FailingProxyResolverFactory), nullptr));
On 2017/06/08 17:44:52, Bence wrote:
> base::MakeUnique<FailingProxyResolverFactory>()

ditto.

https://codereview.chromium.org/2901093004/diff/360001/net/quic/chromium/quic...
File net/quic/chromium/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/360001/net/quic/chromium/quic...
net/quic/chromium/quic_network_transaction_unittest.cc:136: result.append(",");
On 2017/06/07 20:52:24, Ryan Hamilton wrote:
> nit: 
> 
> for (QuicVersion version : versions) {
>   if (!result.empty())
>     result.append(",")
>   ...
> }

Done.

[Zhongyi Shi, 2017-06-08 23:12:55]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

If the list is empty when loaded from disk, by default,
HttpStreamFactoryImpl::JobController will fill this field with the
versions supported by the current running instance of network stack.

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[Zhongyi Shi, 2017-06-08 23:16:23]

If you want to diff with a previous reviewed patch, patch 18 is good to use.
Patch 19 & 20 are basically rebasing with masters.

[Zhongyi Shi, 2017-06-08 23:16:32]

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

[commit-bot: I haz the power, 2017-06-08 23:17:08]

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-06-08 23:27:48]

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

[commit-bot: I haz the power, 2017-06-08 23:27:49]

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

[Ryan Hamilton, 2017-06-08 23:53:16]

I've got more reviewing to do, but one point before I head off to the shuttle.

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/360001/net/http/http_server_p...
net/http/http_server_properties.h:155: }
On 2017/06/08 23:11:16, Zhongyi Shi wrote:
> On 2017/06/07 20:52:23, Ryan Hamilton wrote:
> > This is a struct not a class so the members are public. I think this means
you
> > can skip the getter.
> 
> advertised_versions_ are marked as private (and I planned to change this
struct
> to class in a follow-up CL). I change the advertised_versions_ by intention so
> that when we set it, it is guaranteed to be sorted. 

I think you're going to have to bit the bullet. This can be a class or a struct,
but right now it's kinda neither. If you really want to make this a class, then
you'll probably want to do that first.

[xunjieli, 2017-06-09 16:41:56]

Is there a better alternative to adding a QuicVersionVector to
SetAlternativeService() and AlternativeServiceInfo? 

AlternativeService can advertise any protocol. Can we have a subclass/struct
that represents the idea of a Quic AlterativeService?

Instead of overloading SetAlternativeService(), we can have a
SetQuicAlternativeService(). That will be clear from the callsite on when we
need to pass a QuicVersionVector.
The style guide discourages function overloading if it's ambiguous from call
sites (https://google.github.io/styleguide/cppguide.html#Function_Overloading ).

https://codereview.chromium.org/2901093004/diff/460001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/460001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:773:
advertised_versions = CurrentSupportedVersions();
suggestion for a follow-up:
Consider add a prefix/qualifier to CurrentSupportedVersion(). This static method
is in the global namespace. The name isn't specific enough.
e.g. quic_version::GetSupportedVersions()

https://codereview.chromium.org/2901093004/diff/460001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/460001/net/http/http_server_p...
net/http/http_server_properties.h:127: const QuicVersionVector&
advertised_versions);
AlternativeService can advertise any protocol. It's odd that we stick in a
QuicVersionVector here. Can we have a separate Struct or a subclass?
QuicAlternativeServiceInfo?

[Zhongyi Shi, 2017-06-16 00:24:05]

Helen and Bence: PTAL, thanks!

As discussed offline with xunjieli@, I hided the constructor, and uses named
constructors to create H2- or QUIC-specific Alt Svc. 

To set alternative service, two separate methods are created:
SetHttp2AlternativeService and SetQuicAlternativeService. The latter one takes a
list of quic versions.

https://codereview.chromium.org/2901093004/diff/460001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/460001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:773:
advertised_versions = CurrentSupportedVersions();
On 2017/06/09 16:41:55, xunjieli wrote:
> suggestion for a follow-up:
> Consider add a prefix/qualifier to CurrentSupportedVersion(). This static
method
> is in the global namespace. The name isn't specific enough.
> e.g. quic_version::GetSupportedVersions()

Will do in a follow-up CL. Since this method is in the shared code. I will need
to add this in the internal code base first.

https://codereview.chromium.org/2901093004/diff/460001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/460001/net/http/http_server_p...
net/http/http_server_properties.h:127: const QuicVersionVector&
advertised_versions);
On 2017/06/09 16:41:55, xunjieli wrote:
> AlternativeService can advertise any protocol. It's odd that we stick in a
> QuicVersionVector here. Can we have a separate Struct or a subclass?
> QuicAlternativeServiceInfo?

As discussed offline, I hide the generic constructor, and use named constructors
to create H2/QUIC Alternative Service.

[Bence, 2017-06-16 14:43:40]

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:771:
context_->http_server_properties()->SetQuicAlternativeService(
I believe this should be called with an empty QuicVersionVector, because we have
no information about what versions the server support.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:343:
http_server_properties->SetQuicAlternativeService(
I believe this should also be called with an empty QuicVersionVector, because we
have no information about what versions the server support.

https://codereview.chromium.org/2901093004/diff/520001/components/grpc_suppor...
File components/grpc_support/test/get_stream_engine.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/grpc_suppor...
components/grpc_support/test/get_stream_engine.cc:56:
server_properties_->SetQuicAlternativeService(
I believe this should be called with an empty QuicVersionVector, because we have
no information about what versions the server support.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.cc:87: DCHECK(alternative_service.protocol !=
kProtoQUIC);
DCHECK_EQ(kProtoHTTP2, alternative_service.protocol);

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.cc:97: DCHECK(alternative_service.protocol ==
kProtoQUIC);
DCHECK_EQ(kProtoQUIC, alternative_service.protocol);

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.h:167: if (alternative_service_.protocol !=
kProtoQUIC)
DCHECK_EQ(kProtoQUIC, alternative_service.protocol);

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:391: DCHECK(alternative_service.protocol
!= kProtoQUIC);
Optional: this is not necessary, as CreateHttp2AlternativeServiceInfo already
checks for this.

If you decide to keep it, maybe use instead
DCHECK_EQ(kProtoHTTP2, alternative_service.protocol);

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory.cc:54: // Check if QUIC version is supported.
Filter supported QUIC versions.
I think this whole |match_found| logic should be removed, and all the versions
supported by the server persisted verbatim, even if there is no overlap.  This
way if Chrome supports a QUIC version after an update, it can immediately use
the QUIC advertisement previously made by the server.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1727: // Test that
GetAlternativeServiceInfoFor will include a list of advertised
Since GetAlternativeServiceInfoFor() only has one call site, which immediately
extracts .alternative_service and throws out the rest of the Info struct, maybe
it could be changed back to GetAlternativeServiceFor().  After all, the caller
really should not worry about what versions are supported, only whether there is
overlap with the client-supported versions.

[xunjieli, 2017-06-16 14:43:56]

cronet/ lgtm.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:765: if (params)
{
|params| should be always non-null, right?
Can you make this into a DCHECK?

DCHECK(params);
context_->>http_server_properties()->SetQuicAlternativeService(
  quic_server, alternative_service, base::Time::Max(),
  params->advertised_versions);

[Zhongyi Shi, 2017-06-20 23:23:37]

xunjieli@: thanks for the review.
bnc@: PTAL! Let me know if I could help answer any other questions.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:765: if (params)
{
On 2017/06/16 14:43:56, xunjieli wrote:
> |params| should be always non-null, right?
> Can you make this into a DCHECK?
> 
> DCHECK(params);
> context_->>http_server_properties()->SetQuicAlternativeService(
>   quic_server, alternative_service, base::Time::Max(),
>   params->advertised_versions);
> 

Done.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:771:
context_->http_server_properties()->SetQuicAlternativeService(
On 2017/06/16 14:43:40, Bence wrote:
> I believe this should be called with an empty QuicVersionVector, because we
have
> no information about what versions the server support.

Setting versions here doesn't have behavioral difference comparing to to setting
an empty version list. And it sticks to the API more closely.

When adding support for multiple QUIC versions, we are expecting always have the
advertised versions(and supported) persisted in server properties. The only
exception that version list is missing in server properties for quic servers
happens when we are restarting and updating Chrome from an older version(doesn't
support multi version) to a new version that supports multi version. But in
general, we always want to set the advertised versions when
SetAlternativeService.

There's not much behavior difference in setting an empty version list vs the
versions supported by the current network stack. In the current world, where the
multi version isn't a thing. We always try previously persisted alt svc with the
QUIC version supported by the current running instance of net stack. We never
persist version for alt svc, and if a Chrome update happens, we read the server
properties from the disk, try with a different supported version by the new
running instance of net stack, without realizing the version has been changed.

Additionally, if version list is missing(which happens when updating from a
Chrome not supporting multi version to supporting), we will treat the alt svc as
supporting the current versions the net stack supports in the QuicStreamFactory
layer.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:343:
http_server_properties->SetQuicAlternativeService(
On 2017/06/16 14:43:40, Bence wrote:
> I believe this should also be called with an empty QuicVersionVector, because
we
> have no information about what versions the server support.

Acknowledged.

https://codereview.chromium.org/2901093004/diff/520001/components/grpc_suppor...
File components/grpc_support/test/get_stream_engine.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/grpc_suppor...
components/grpc_support/test/get_stream_engine.cc:56:
server_properties_->SetQuicAlternativeService(
On 2017/06/16 14:43:40, Bence wrote:
> I believe this should be called with an empty QuicVersionVector, because we
have
> no information about what versions the server support.

Acknowledged.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.cc:87: DCHECK(alternative_service.protocol !=
kProtoQUIC);
On 2017/06/16 14:43:40, Bence wrote:
> DCHECK_EQ(kProtoHTTP2, alternative_service.protocol);

Done.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.cc:97: DCHECK(alternative_service.protocol ==
kProtoQUIC);
On 2017/06/16 14:43:40, Bence wrote:
> DCHECK_EQ(kProtoQUIC, alternative_service.protocol);

Done.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.h:167: if (alternative_service_.protocol !=
kProtoQUIC)
On 2017/06/16 14:43:40, Bence wrote:
> DCHECK_EQ(kProtoQUIC, alternative_service.protocol);

Although we are expecting this method only be called when alt svc is using QUIC
protocol, there was no API enforcement on the protocol field. 

I think an early return after check will make sure that we are doing the right
thing regardless of the callsite correctness. DCHECK will only fire in a debug
build. Or should we change this to CHECK()?

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:391: DCHECK(alternative_service.protocol
!= kProtoQUIC);
On 2017/06/16 14:43:40, Bence wrote:
> Optional: this is not necessary, as CreateHttp2AlternativeServiceInfo already
> checks for this.
> 
> If you decide to keep it, maybe use instead
> DCHECK_EQ(kProtoHTTP2, alternative_service.protocol);

Done.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory.cc:54: // Check if QUIC version is supported.
Filter supported QUIC versions.
On 2017/06/16 14:43:40, Bence wrote:
> I think this whole |match_found| logic should be removed, and all the versions
> supported by the server persisted verbatim, even if there is no overlap.  This
> way if Chrome supports a QUIC version after an update, it can immediately use
> the QUIC advertisement previously made by the server.

That's a fair good point. I have considered this before. However, this will be
an additional behavior change. Besides, this might cause us to persist a lot of
of alt svc in disk but not actually use them if we don't check mutual support.
Think about the case that we are currently supporting QUIC_VERSION_37, and
Alt-Svc headers advertise a lot of alt svc servers only supporting
QUIC_VERSION_40, which is a valid version, but won't be supported in the near
future. This won't bring us much gain.

We might consider to persist the full advertised version list if one of the
versions is matching our supported versions. But the gain is also unclear. I
would prefer to consider this as a follow-up idea after multi-version is
implemented. WDYT?

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1727: // Test that
GetAlternativeServiceInfoFor will include a list of advertised
On 2017/06/16 14:43:40, Bence wrote:
> Since GetAlternativeServiceInfoFor() only has one call site, which immediately
> extracts .alternative_service and throws out the rest of the Info struct,
maybe
> it could be changed back to GetAlternativeServiceFor().  After all, the caller
> really should not worry about what versions are supported, only whether there
is
> overlap with the client-supported versions.

Whoops. I probably should give some more context here. I agree with you that
currently GetAlternativeServiceInfoFor doesn't nothing as it drops the version
list. However, in the next CL after this one is landed, the version list will be
taken by the JobController via AlternativeServiceInfo, then passed to
QuicStreamFactory so that a "best" QuicVersion will be picked based on the
advertised versions and the supported versions, and QuicStreamFactory will then
pass the selected version to QuicChomiumClientSession so that we will use
dynamic QUIC versions based on the server advertisement.

[Bence, 2017-06-21 12:39:25]

Just a few more questions, sorry for bringing up so many edge cases.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:771:
context_->http_server_properties()->SetQuicAlternativeService(
On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> On 2017/06/16 14:43:40, Bence wrote:
> > I believe this should be called with an empty QuicVersionVector, because we
> have
> > no information about what versions the server support.
> 
> Setting versions here doesn't have behavioral difference comparing to to
setting
> an empty version list. And it sticks to the API more closely.
> 
> When adding support for multiple QUIC versions, we are expecting always have
the
> advertised versions(and supported) persisted in server properties. The only
> exception that version list is missing in server properties for quic servers
> happens when we are restarting and updating Chrome from an older
version(doesn't
> support multi version) to a new version that supports multi version. But in
> general, we always want to set the advertised versions when
> SetAlternativeService.
> 
> There's not much behavior difference in setting an empty version list vs the
> versions supported by the current network stack. In the current world, where
the
> multi version isn't a thing. We always try previously persisted alt svc with
the
> QUIC version supported by the current running instance of net stack. We never
> persist version for alt svc, and if a Chrome update happens, we read the
server
> properties from the disk, try with a different supported version by the new
> running instance of net stack, without realizing the version has been changed.
> 
> Additionally, if version list is missing(which happens when updating from a
> Chrome not supporting multi version to supporting), we will treat the alt svc
as
> supporting the current versions the net stack supports in the
QuicStreamFactory
> layer.

What happens if one version of Chrome supports QUIC 37, so that is persisted to
disk, then after update, Chrome supports 38 but not 37, so the entry is
disregarded?  Because of this scenario, I still think it would be better to
persist without an empty version vector if we have no information about the
version.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_server_p...
net/http/http_server_properties.h:167: if (alternative_service_.protocol !=
kProtoQUIC)
On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> On 2017/06/16 14:43:40, Bence wrote:
> > DCHECK_EQ(kProtoQUIC, alternative_service.protocol);
> 
> Although we are expecting this method only be called when alt svc is using
QUIC
> protocol, there was no API enforcement on the protocol field. 
> 
> I think an early return after check will make sure that we are doing the right
> thing regardless of the callsite correctness. DCHECK will only fire in a debug
> build. Or should we change this to CHECK()?

Good points.

One option is to leave it as it is now, and I'm fine with that.

The other option is to make it part of the API (by adding a comment in the
header file), and use a DCHECK().  I have a slight preference to this, but early
return is also fine.

I do not think a CHECK() is necessary: it has the same runtime cost as an early
return, but my understanding is that a CHECK() is usually reserved for
preventing security issues/leaks.  Having a DCHECK() and adequate test coverage
should be enough to guarantee that all callers follow the API requirement.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory.cc:54: // Check if QUIC version is supported.
Filter supported QUIC versions.
On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> On 2017/06/16 14:43:40, Bence wrote:
> > I think this whole |match_found| logic should be removed, and all the
versions
> > supported by the server persisted verbatim, even if there is no overlap. 
This
> > way if Chrome supports a QUIC version after an update, it can immediately
use
> > the QUIC advertisement previously made by the server.
> 
> That's a fair good point. I have considered this before. However, this will be
> an additional behavior change. Besides, this might cause us to persist a lot
of
> of alt svc in disk but not actually use them if we don't check mutual support.
> Think about the case that we are currently supporting QUIC_VERSION_37, and
> Alt-Svc headers advertise a lot of alt svc servers only supporting
> QUIC_VERSION_40, which is a valid version, but won't be supported in the near
> future. This won't bring us much gain.
> 
> We might consider to persist the full advertised version list if one of the
> versions is matching our supported versions. But the gain is also unclear. I
> would prefer to consider this as a follow-up idea after multi-version is
> implemented. WDYT? 

Thank you for the explanation.

Given that restarts (for which persistence is relevant) are expected to be much
more frequent than updates (for which persisting currently unsupported versions
would be relevant), also the typical expiration time for AltSvc is shorter than
typical update frequency, I found your argument convincing.  Thank you.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1727: // Test that
GetAlternativeServiceInfoFor will include a list of advertised
On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> On 2017/06/16 14:43:40, Bence wrote:
> > Since GetAlternativeServiceInfoFor() only has one call site, which
immediately
> > extracts .alternative_service and throws out the rest of the Info struct,
> maybe
> > it could be changed back to GetAlternativeServiceFor().  After all, the
caller
> > really should not worry about what versions are supported, only whether
there
> is
> > overlap with the client-supported versions.
> 
> Whoops. I probably should give some more context here. I agree with you that
> currently GetAlternativeServiceInfoFor doesn't nothing as it drops the version
> list. However, in the next CL after this one is landed, the version list will
be
> taken by the JobController via AlternativeServiceInfo, then passed to
> QuicStreamFactory so that a "best" QuicVersion will be picked based on the
> advertised versions and the supported versions, and QuicStreamFactory will
then
> pass the selected version to QuicChomiumClientSession so that we will use
> dynamic QUIC versions based on the server advertisement. 

Oh, I totally forgot about that.  Sorry.

Optionally, GetAlternativeServiceFor() could do the version matching, and return
the single best Alternative Service.  This has an advantage in case there are
two QUIC AltSvc entries for the same origin with different HostPortPairs and
with different versions: GetAlternativeServiceFor() could pick the one with the
version that is supported, but if the version matching logic lives in
QuicStreamFactory, then since GetAlternativeServiceInfoFor() only returns a
single entry, it might not return the best one.  Of course this is an edge case
which should be very rare.

[Bence, 2017-06-21 19:15:53]

LGTM, after some offline discussion.  Thank you for answering all my questions
in detail!

[Zhongyi Shi, 2017-06-21 21:01:33]

bnc@: thanks for all your questions and a thorough review! 

rch@: PTAL, thanks!

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:771:
context_->http_server_properties()->SetQuicAlternativeService(
On 2017/06/21 12:39:25, Bence wrote:
> On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> > On 2017/06/16 14:43:40, Bence wrote:
> > > I believe this should be called with an empty QuicVersionVector, because
we
> > have
> > > no information about what versions the server support.
> > 
> > Setting versions here doesn't have behavioral difference comparing to to
> setting
> > an empty version list. And it sticks to the API more closely.
> > 
> > When adding support for multiple QUIC versions, we are expecting always have
> the
> > advertised versions(and supported) persisted in server properties. The only
> > exception that version list is missing in server properties for quic servers
> > happens when we are restarting and updating Chrome from an older
> version(doesn't
> > support multi version) to a new version that supports multi version. But in
> > general, we always want to set the advertised versions when
> > SetAlternativeService.
> > 
> > There's not much behavior difference in setting an empty version list vs the
> > versions supported by the current network stack. In the current world, where
> the
> > multi version isn't a thing. We always try previously persisted alt svc with
> the
> > QUIC version supported by the current running instance of net stack. We
never
> > persist version for alt svc, and if a Chrome update happens, we read the
> server
> > properties from the disk, try with a different supported version by the new
> > running instance of net stack, without realizing the version has been
changed.
> > 
> > Additionally, if version list is missing(which happens when updating from a
> > Chrome not supporting multi version to supporting), we will treat the alt
svc
> as
> > supporting the current versions the net stack supports in the
> QuicStreamFactory
> > layer.
> 
> What happens if one version of Chrome supports QUIC 37, so that is persisted
to
> disk, then after update, Chrome supports 38 but not 37, so the entry is
> disregarded?  Because of this scenario, I still think it would be better to
> persist without an empty version vector if we have no information about the
> version.

Discussed offline. I have confirmed with xunjieli@: the supported versions will
not change dynamically. It's changed either with a library update or a app
config update (CronetEngine will pick up new configurations). Both require app
restart to take effect, where this method will be invoked again, and those
alternative services will be set again with new versions, thus this will not
break the Alt Svc usage.

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1727: // Test that
GetAlternativeServiceInfoFor will include a list of advertised
On 2017/06/21 12:39:25, Bence wrote:
> On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> > On 2017/06/16 14:43:40, Bence wrote:
> > > Since GetAlternativeServiceInfoFor() only has one call site, which
> immediately
> > > extracts .alternative_service and throws out the rest of the Info struct,
> > maybe
> > > it could be changed back to GetAlternativeServiceFor().  After all, the
> caller
> > > really should not worry about what versions are supported, only whether
> there
> > is
> > > overlap with the client-supported versions.
> > 
> > Whoops. I probably should give some more context here. I agree with you that
> > currently GetAlternativeServiceInfoFor doesn't nothing as it drops the
version
> > list. However, in the next CL after this one is landed, the version list
will
> be
> > taken by the JobController via AlternativeServiceInfo, then passed to
> > QuicStreamFactory so that a "best" QuicVersion will be picked based on the
> > advertised versions and the supported versions, and QuicStreamFactory will
> then
> > pass the selected version to QuicChomiumClientSession so that we will use
> > dynamic QUIC versions based on the server advertisement. 
> 
> Oh, I totally forgot about that.  Sorry.
> 
> Optionally, GetAlternativeServiceFor() could do the version matching, and
return
> the single best Alternative Service.  This has an advantage in case there are
> two QUIC AltSvc entries for the same origin with different HostPortPairs and
> with different versions: GetAlternativeServiceFor() could pick the one with
the
> version that is supported, but if the version matching logic lives in
> QuicStreamFactory, then since GetAlternativeServiceInfoFor() only returns a
> single entry, it might not return the best one.  Of course this is an edge
case
> which should be very rare.

Currently, we are returning a single AlternativerServiceInfo, but with all the
advertised versions. In most cases, if we didn't change the client supported
versions, we know at least one of the advertised versions will be found in the
AltSvcInfo. 

The edge case is that we might support for QUIC_VERSION_37 for a while, and
persisted a AlternativeService with QUIC_VERSION_37. And then we update to
support QUIC_VERSION_39. In the old world, we will ignorantly connect to the
server with QUIC_VERSION_39, which will fail later. In the new world, we will
create a QUIC job and until the QuicStreamFactory found no mutually supported
versions, we will could kill the quic job, mark QUIC broken. This is close to
what we were doing.

[Zhongyi Shi, 2017-06-21 23:42:31]

zhongyi@chromium.org changed reviewers:
+ mef@chromium.org

[Zhongyi Shi, 2017-06-21 23:42:37]

mef@: PTAL at components/grpc_support/*, thanks!

[mef, 2017-06-22 17:59:55]

On 2017/06/21 23:42:37, Zhongyi Shi wrote:
> mef@: PTAL at components/grpc_support/*, thanks!

components/grpc_support/* lgtm

[Zhongyi Shi, 2017-06-22 18:02:57]

zhongyi@chromium.org changed reviewers:
- rch@chromium.org

[Zhongyi Shi, 2017-06-22 18:02:59]

Thanks all for helping the review. Landing now.

[Zhongyi Shi, 2017-06-22 18:03:08]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2017-06-22 18:03:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org, bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/2901093004/#ps560001
(title: "Rebase onto origin/master")

[commit-bot: I haz the power, 2017-06-22 18:03:21]

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-06-22 18:14:04]

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

[commit-bot: I haz the power, 2017-06-22 18:14:06]

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

[Zhongyi Shi, 2017-06-26 22:21:38]

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

[commit-bot: I haz the power, 2017-06-26 22:21:57]

Dry run: CQ is trying da patch.

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

[Zhongyi Shi, 2017-06-26 22:23:24]

Added QUIC version filtering in SpdySession::OnAltSvc after
https://crbug.com/736063 is fixed.

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2901093004/diff/520001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:771:
context_->http_server_properties()->SetQuicAlternativeService(
On 2017/06/21 21:01:32, Zhongyi Shi wrote:
> On 2017/06/21 12:39:25, Bence wrote:
> > On 2017/06/20 23:23:37, Zhongyi Shi wrote:
> > > On 2017/06/16 14:43:40, Bence wrote:
> > > > I believe this should be called with an empty QuicVersionVector, because
> we
> > > have
> > > > no information about what versions the server support.
> > > 
> > > Setting versions here doesn't have behavioral difference comparing to to
> > setting
> > > an empty version list. And it sticks to the API more closely.
> > > 
> > > When adding support for multiple QUIC versions, we are expecting always
have
> > the
> > > advertised versions(and supported) persisted in server properties. The
only
> > > exception that version list is missing in server properties for quic
servers
> > > happens when we are restarting and updating Chrome from an older
> > version(doesn't
> > > support multi version) to a new version that supports multi version. But
in
> > > general, we always want to set the advertised versions when
> > > SetAlternativeService.
> > > 
> > > There's not much behavior difference in setting an empty version list vs
the
> > > versions supported by the current network stack. In the current world,
where
> > the
> > > multi version isn't a thing. We always try previously persisted alt svc
with
> > the
> > > QUIC version supported by the current running instance of net stack. We
> never
> > > persist version for alt svc, and if a Chrome update happens, we read the
> > server
> > > properties from the disk, try with a different supported version by the
new
> > > running instance of net stack, without realizing the version has been
> changed.
> > > 
> > > Additionally, if version list is missing(which happens when updating from
a
> > > Chrome not supporting multi version to supporting), we will treat the alt
> svc
> > as
> > > supporting the current versions the net stack supports in the
> > QuicStreamFactory
> > > layer.
> > 
> > What happens if one version of Chrome supports QUIC 37, so that is persisted
> to
> > disk, then after update, Chrome supports 38 but not 37, so the entry is
> > disregarded?  Because of this scenario, I still think it would be better to
> > persist without an empty version vector if we have no information about the
> > version.
> 
> Discussed offline. I have confirmed with xunjieli@: the supported versions
will
> not change dynamically. It's changed either with a library update or a app
> config update (CronetEngine will pick up new configurations). Both require app
> restart to take effect, where this method will be invoked again, and those
> alternative services will be set again with new versions, thus this will not
> break the Alt Svc usage.

As I realized version advertisement is optional. I felt it would be more
consistent to leave the version field empty. Which will not set up anything in
disk so that the server is kind of in a state supporting ANY QUIC version.

[commit-bot: I haz the power, 2017-06-26 22:24:51]

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

[commit-bot: I haz the power, 2017-06-26 22:24:52]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Zhongyi Shi, 2017-06-26 22:48:31]

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

[commit-bot: I haz the power, 2017-06-26 22:48:55]

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-06-26 23:21:01]

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

[commit-bot: I haz the power, 2017-06-26 23:21:02]

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

[Zhongyi Shi, 2017-06-27 00:57:37]

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

[commit-bot: I haz the power, 2017-06-27 00:57:48]

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-06-27 03:21:21]

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

[commit-bot: I haz the power, 2017-06-27 03:21:22]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2017-06-27 16:44:23]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2017-06-27 16:44:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
mef@chromium.org, xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2901093004/#ps640001
(title: "fix Canonical test")

[commit-bot: I haz the power, 2017-06-27 16:44:33]

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-06-27 16:48:25]

CQ is committing da patch.

Bot data: {"patchset_id": 640001, "attempt_start_ts": 1498581863442180,
"parent_rev": "eb04efa548c78d5f7d427beb4440829598d10f6c", "commit_rev":
"e537a0070404ceaae5d8ad6b7659a7cd24469996"}

[commit-bot: I haz the power, 2017-06-27 16:48:42]

Description was changed from

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add a new field in AlternativeServiceInfo to list QUIC verisons that are
advertised by the server and supported by Chrome. 
The list will also be persisted to disk. 

BUG=689762
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[commit-bot: I haz the power, 2017-06-27 16:48:44]

Committed patchset #29 (id:640001) as
https://chromium.googlesource.com/chromium/src/+/e537a0070404ceaae5d8ad6b7659...