Change the interface of GetAlternativeServicesFor, always return the best Alt-Svc entry.

Change the interface of GetAlternativeServicesFor to GetAlternativeService, always return the best Alt-Svc entry from a list

BUG=570098
Committed: https://crrev.com/32569c6744b9dc3589fb67f9e25857ba138ed76d
Cr-Commit-Position: refs/heads/master@{#368258}

[Zhongyi Shi, 2015-12-18 01:50:53]

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

[Zhongyi Shi, 2015-12-18 01:50:53]

Unit tests will be online tomorrow. Commments are not polished up yet. Just send
this out to get early feedbacks! Thanks!

[Zhongyi Shi, 2015-12-18 01:51:38]

Description was changed from

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc from a list

BUG=570098
==========

to

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc entry from a list

BUG=570098
==========

[Ryan Hamilton, 2015-12-18 17:52:51]

Nice! I had forgotten about the need to call CanPool on the existing session.
Thanks for remembering.

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

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:242:
ApplyHostMappingRules(request_info.url, &destination).host();
Are you sure this is correct? It may well be, but it's not obvious to me.

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:245: preferred_alternative_service =
alternative_service;
I'm not sure that I understand the dance here with preferred vs candidate. Can
you simplify this by just returning at this point?

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:720: }
I'm a bit fan of early return instead of nesting, so I would do:

if (active_session_.empty()) 
  return false;
SessionMap::iterator it = active_sessions_.find(server_id);
if (it = active_sessions_.end())
  return false;
QuicChromiumClientSession* session = it->second;
return session->CanPool(origin_host.as_string(), privacy_mode);

(Actually, since I think find() will work with empty containers, I would
probably just omit the empty() check.)

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:151: // Returns true if a matching session
already exists.
I might say something like:

// Returns true if there is an existing session to |server_id| which can be used
for requests to |origin_host|.

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:153: PrivacyMode privacy_mode,
Your call, but I would make this take a QuicServerId

[Zhongyi Shi, 2015-12-18 17:57:23]

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

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:245: preferred_alternative_service =
alternative_service;
On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> I'm not sure that I understand the dance here with preferred vs candidate. Can
> you simplify this by just returning at this point?

Candidate is the first entry of the list. While preferred is the QUIC Alt-Svc
that has a active session already. 

The reason that I don't do an early return: we need to go thru the whole list to
identify whether QUIC should be marked broken for this origin (domain
reliability). Does that make sense?

[Ryan Hamilton, 2015-12-18 19:52:59]

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

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:245: preferred_alternative_service =
alternative_service;
On 2015/12/18 17:57:23, Zhongyi Shi wrote:
> On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> > I'm not sure that I understand the dance here with preferred vs candidate.
Can
> > you simplify this by just returning at this point?
> 
> Candidate is the first entry of the list. While preferred is the QUIC Alt-Svc
> that has a active session already. 
> 
> The reason that I don't do an early return: we need to go thru the whole list
to
> identify whether QUIC should be marked broken for this origin (domain
> reliability). Does that make sense?

Are you specifically thinking about the call to delegate->OnQuicBroken()? If I'm
reading that correctly, it only happens if all QUIC Alt-Svcs are broken. But if
we're able to call CanPool() then we've already set quic_all_broken to false,
right? (And if we can actually use that QUIC Alt-Svc it seems like QUIC is by
definition not broken?)

[Zhongyi Shi, 2015-12-18 20:32:15]

On 2015/12/18 19:52:59, Ryan Hamilton wrote:
>
https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
> File net/http/http_stream_factory_impl.cc (right):
> 
>
https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
> net/http/http_stream_factory_impl.cc:245: preferred_alternative_service =
> alternative_service;
> On 2015/12/18 17:57:23, Zhongyi Shi wrote:
> > On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> > > I'm not sure that I understand the dance here with preferred vs candidate.
> Can
> > > you simplify this by just returning at this point?
> > 
> > Candidate is the first entry of the list. While preferred is the QUIC
Alt-Svc
> > that has a active session already. 
> > 
> > The reason that I don't do an early return: we need to go thru the whole
list
> to
> > identify whether QUIC should be marked broken for this origin (domain
> > reliability). Does that make sense?
> 
> Are you specifically thinking about the call to delegate->OnQuicBroken()? If
I'm
> reading that correctly, it only happens if all QUIC Alt-Svcs are broken. But
if
> we're able to call CanPool() then we've already set quic_all_broken to false,
> right? (And if we can actually use that QUIC Alt-Svc it seems like QUIC is by
> definition not broken?)

Good point! If we are able to find a QUIC Alt-Svc that has a active quic
connection for it, we already marked QUIC as non-broken. Early return should be
fine here. Thanks for pointing it out. I will change it in this cl. Also will
upload the other cl that defer the selection to job. But does that sounds like
doing the selection here is more cheap here? We will avoid going thru the whole
alt-svc list for one round, also get early return from the preconnect streams
rather than bothering wait until the DoStart in the job?

[Zhongyi Shi, 2015-12-18 21:54:42]

Patchset #4 (id:60001) has been deleted

[Zhongyi Shi, 2015-12-18 21:59:30]

Thanks for the feedbacks. Changed two methods with early return. Also move the
initialization of server_ and original_url_ from the Job::DoStart() to
constructor so that job won't defer HttpStreamFactoryImpl to apply host mapping
rules to calculate the original_url.

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

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:242:
ApplyHostMappingRules(request_info.url, &destination).host();
On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> Are you sure this is correct? It may well be, but it's not obvious to me.

I still leave the origin_host calculation here, but removed the one in
HttpStreamFactoryImpl::Job::DoStart() by passing server and origin_url to the
cxtor of Job so as to avoid calling HttpStreamFactoryImpl::ApplyHostMappingRules
from there.

https://codereview.chromium.org/1540463003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:245: preferred_alternative_service =
alternative_service;
On 2015/12/18 19:52:59, Ryan Hamilton wrote:
> On 2015/12/18 17:57:23, Zhongyi Shi wrote:
> > On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> > > I'm not sure that I understand the dance here with preferred vs candidate.
> Can
> > > you simplify this by just returning at this point?
> > 
> > Candidate is the first entry of the list. While preferred is the QUIC
Alt-Svc
> > that has a active session already. 
> > 
> > The reason that I don't do an early return: we need to go thru the whole
list
> to
> > identify whether QUIC should be marked broken for this origin (domain
> > reliability). Does that make sense?
> 
> Are you specifically thinking about the call to delegate->OnQuicBroken()? If
I'm
> reading that correctly, it only happens if all QUIC Alt-Svcs are broken. But
if
> we're able to call CanPool() then we've already set quic_all_broken to false,
> right? (And if we can actually use that QUIC Alt-Svc it seems like QUIC is by
> definition not broken?)
Ah, thanks for pointing this out! If we are able to find a QUIC Alt-Svc that has
a active quic connection for it, we already marked QUIC as non-broken. Early
return should be fine here. Thanks for pointing it out. I will change it in this
cl. Also will upload the other cl that defer the selection to job. But does that
sounds like doing the selection here is more cheap here? We will avoid going
thru the whole alt-svc list for one round, also get early return from the
preconnect streams rather than bothering wait until the DoStart in the job?

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:151: // Returns true if a matching session
already exists.
On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> I might say something like:
> 
> // Returns true if there is an existing session to |server_id| which can be
used
> for requests to |origin_host|.

Done.

https://codereview.chromium.org/1540463003/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:153: PrivacyMode privacy_mode,
On 2015/12/18 17:52:51, Ryan Hamilton wrote:
> Your call, but I would make this take a QuicServerId

Done.

[Ryan Hamilton, 2015-12-18 22:56:29]

Looking good. Almost there.

https://codereview.chromium.org/1540463003/diff/70001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1540463003/diff/70001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:94: GURL original_url =
ApplyHostMappingRules(request_info.url, &server);
nit: origin_url not original_url

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:713: return false;
I think you can remove these two lines...

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:153: bool CanPool(QuicServerId server_id,
How about |CanUseExistingSession| or some such. Because server_id.host might ==
origing_host, in which case it's not pooling.

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:154: PrivacyMode privacy_mode,
I think you can remove this arg.

[Zhongyi Shi, 2015-12-19 00:49:32]

Patchset #6 (id:110001) has been deleted

[Zhongyi Shi, 2015-12-19 01:23:02]

Thanks for the long discussions on where to filter the best Alt-Svc from the
list.

https://codereview.chromium.org/1540463003/diff/70001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1540463003/diff/70001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:94: GURL original_url =
ApplyHostMappingRules(request_info.url, &server);
On 2015/12/18 22:56:29, Ryan Hamilton wrote:
> nit: origin_url not original_url

Done.

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:713: return false;
On 2015/12/18 22:56:29, Ryan Hamilton wrote:
> I think you can remove these two lines...

We have to leave it there. If we remove that, on Android it will crash. See
crbug.com/498823.

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:153: bool CanPool(QuicServerId server_id,
On 2015/12/18 22:56:29, Ryan Hamilton wrote:
> How about |CanUseExistingSession| or some such. Because server_id.host might
==
> origing_host, in which case it's not pooling.

Done.

https://codereview.chromium.org/1540463003/diff/70001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:154: PrivacyMode privacy_mode,
On 2015/12/18 22:56:29, Ryan Hamilton wrote:
> I think you can remove this arg.

Unfortunately, we can't. We still need privacy_mode when call
QuicChromiumClientSession::CanPool though we are passing in the QuicServerId.

[Ryan Hamilton, 2015-12-21 17:37:52]

Ok, I think this looks good. Now it just needs tests...

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:188: AlternativeService
candidate_alternative_service;
nit: I'd probably just drop "candidate_"

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:246:
ApplyHostMappingRules(request_info.url, &destination).host();
I'm not actually sure that this "works". StringPiece is simply a point to an
existing block of memory. Assuming that ApplyHostMappingRules returns a
HostPortPair, that object will be destroyed when this line is finished, and the
address that origin_host points to will be invalid.

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:671:
stream_factory_->ApplyHostMappingRules(request_info_.url, &server_);
I think you can remove this line, right?

[Zhongyi Shi, 2015-12-28 22:43:03]

Patchset #7 (id:150001) has been deleted

[Zhongyi Shi, 2015-12-28 22:43:22]

Updated old test cases to testing whether we are using the most promising
alternative service.

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:246:
ApplyHostMappingRules(request_info.url, &destination).host();
On 2015/12/21 17:37:51, Ryan Hamilton wrote:
> I'm not actually sure that this "works". StringPiece is simply a point to an
> existing block of memory. Assuming that ApplyHostMappingRules returns a
> HostPortPair, that object will be destroyed when this line is finished, and
the
> address that origin_host points to will be invalid.

Done.

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1540463003/diff/130001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:671:
stream_factory_->ApplyHostMappingRules(request_info_.url, &server_);
On 2015/12/21 17:37:51, Ryan Hamilton wrote:
> I think you can remove this line, right?

Done.

[Zhongyi Shi, 2015-12-29 17:15:36]

Change the quic_test_packet_maker to enable offset tracking in
MakeRequest/ResponseHeaderPacket.

[Zhongyi Shi, 2015-12-29 17:24:38]

Patchset #5 (id:90001) has been deleted

[Zhongyi Shi, 2015-12-30 00:27:45]

Aha, finally got the new testcase working with a lot of generalized helper
methods in quic_network_transaction_unittests helper method and methods in
quic_test_packet_maker.

Added a testcase to verify we are using the first alternative service which has
existing quic sessions if we have multiple available. If you want to run diff to
previous patch sets, please diff with path set 7, rather than 8. Patch set 8 is
a full generalized helper methods for future reference if we decide to land all
the generalized helper methods. 

Thanks (C:

[Ryan Hamilton, 2015-12-30 21:51:42]

Great progress!

https://codereview.chromium.org/1540463003/diff/170001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/170001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:823: // Open a session to
mail.example.org:443 using the first entry of the
The Alt-Svc: header seems to be foo.example.com not mail.example.org?

https://codereview.chromium.org/1540463003/diff/170001/net/quic/test_tools/qu...
File net/quic/test_tools/quic_test_packet_maker.h (right):

https://codereview.chromium.org/1540463003/diff/170001/net/quic/test_tools/qu...
net/quic/test_tools/quic_test_packet_maker.h:58: scoped_ptr<QuicEncryptedPacket>
MakeAckPacket(
Can you add comments which explain how the previous method interacts with this
one?

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:544: StaticSocketDataProvider*
hanging_data = new StaticSocketDataProvider();
I think this leaks memory, doesn't it?

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:898: // Open a session to
mail.example.org:443 using the first entry of the
This actually opens a QUIC session to foo.example.com, right?

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:971:
"quic=\"bar.example.com:443\", quic=\"frog.example.com:443\", "
I think each alternative should have a different port, so that we can ensure
that the right alternative was used, via the port checks below.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:982: // No connection pooling.
Open a new session to bar.example.com:443.
Can you expand on "No connection pooling". I think the idea is that the existing
quic session to foo.example.com is no longer a viable alt-svc.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:1024:
SendRequestAndExpectQuicResponseOnPort("hello from foo!", 443);
Overall this test look good. I wonder, though, if we should also have tests in
which the requests are to different origins. For example we could make a request
to www.example.org which makes a QUIC session to www.example.org. Then we could
make a requests to mail.example.org which has www.example.org (and itself)
listed as an alternative.

This is very similar to what you've done, it just happens slightly differently?

We should probably also have a test where a server specifies an alternative
which is not valid. For example if www.example.org said that www.example.com was
an alternative, and the connection to www.example.org did not have an acceptable
cert, then we should create a new session.

[Zhongyi Shi, 2016-01-05 22:21:52]

Patchset #12 (id:290001) has been deleted

[Zhongyi Shi, 2016-01-05 22:22:25]

Patchset #11 (id:270001) has been deleted

[Zhongyi Shi, 2016-01-05 22:54:20]

Add two more test cases as suggested after the "AlwaysExpectCert" patch is
landed.

PTAL, thanks :)

https://codereview.chromium.org/1540463003/diff/170001/net/quic/test_tools/qu...
File net/quic/test_tools/quic_test_packet_maker.h (right):

https://codereview.chromium.org/1540463003/diff/170001/net/quic/test_tools/qu...
net/quic/test_tools/quic_test_packet_maker.h:58: scoped_ptr<QuicEncryptedPacket>
MakeAckPacket(
On 2015/12/30 21:51:41, Ryan Hamilton wrote:
> Can you add comments which explain how the previous method interacts with this
> one?

Done.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:544: StaticSocketDataProvider*
hanging_data = new StaticSocketDataProvider();
On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> I think this leaks memory, doesn't it?

Ahhh, yes. I fix it with passing the hanging_data from the args so that the
lifetime of hanging_data is the same with the tests.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:898: // Open a session to
mail.example.org:443 using the first entry of the
On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> This actually opens a QUIC session to http://foo.example.com, right?

Yes. Comment updated.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:971:
"quic=\"bar.example.com:443\", quic=\"frog.example.com:443\", "
On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> I think each alternative should have a different port, so that we can ensure
> that the right alternative was used, via the port checks below.

Done.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:982: // No connection pooling.
Open a new session to bar.example.com:443.
On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> Can you expand on "No connection pooling". I think the idea is that the
existing
> quic session to http://foo.example.com is no longer a viable alt-svc.

Done.

[Ryan Hamilton, 2016-01-05 23:51:59]

Looking good. just minor comments.

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:544: StaticSocketDataProvider*
hanging_data = new StaticSocketDataProvider();
On 2016/01/05 22:54:20, Zhongyi Shi wrote:
> On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> > I think this leaks memory, doesn't it?
> 
> Ahhh, yes. I fix it with passing the hanging_data from the args so that the
> lifetime of hanging_data is the same with the tests.

Instead of requiring the caller to pass in a new DataProviders, how about making
hanging_data_ a vector and each time this method is called, pushing back onto
that vector?

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:1165: // Adding a valid cert for
docs.example.org.
Perhaps this comment should mention that the cert is valid for *.example.org,
but not mail.example.com?

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:715: // TODO(zhongyi): crbug.com/498823 - delete
active_sessions_.empty() checks.
Can you say more about this?

[Zhongyi Shi, 2016-01-06 02:31:40]

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:544: StaticSocketDataProvider*
hanging_data = new StaticSocketDataProvider();
On 2016/01/05 23:51:58, Ryan Hamilton wrote:
> On 2016/01/05 22:54:20, Zhongyi Shi wrote:
> > On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> > > I think this leaks memory, doesn't it?
> > 
> > Ahhh, yes. I fix it with passing the hanging_data from the args so that the
> > lifetime of hanging_data is the same with the tests.
> 
> Instead of requiring the caller to pass in a new DataProviders, how about
making
> hanging_data_ a vector and each time this method is called, pushing back onto
> that vector?

The StaticSocketDataProvider is DISALLOW_COPY_AND_ASSIGN. So probably better to
keep it as it is?

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:1165: // Adding a valid cert for
docs.example.org.
On 2016/01/05 23:51:58, Ryan Hamilton wrote:
> Perhaps this comment should mention that the cert is valid for *.example.org,
> but not mail.example.com?

Done.

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1540463003/diff/270002/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:715: // TODO(zhongyi): crbug.com/498823 - delete
active_sessions_.empty() checks.
On 2016/01/05 23:51:58, Ryan Hamilton wrote:
> Can you say more about this?

I talked to raman earlier about that bug, this is a temporary fix for the
android crash issue. He said once we finally resolve that issue, we can remove
the empty() checks here.

[Ryan Hamilton, 2016-01-06 17:06:23]

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
File net/quic/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1540463003/diff/250001/net/quic/quic_network_...
net/quic/quic_network_transaction_unittest.cc:544: StaticSocketDataProvider*
hanging_data = new StaticSocketDataProvider();
On 2016/01/06 02:31:40, Zhongyi Shi wrote:
> On 2016/01/05 23:51:58, Ryan Hamilton wrote:
> > On 2016/01/05 22:54:20, Zhongyi Shi wrote:
> > > On 2015/12/30 21:51:42, Ryan Hamilton wrote:
> > > > I think this leaks memory, doesn't it?
> > > 
> > > Ahhh, yes. I fix it with passing the hanging_data from the args so that
the
> > > lifetime of hanging_data is the same with the tests.
> > 
> > Instead of requiring the caller to pass in a new DataProviders, how about
> making
> > hanging_data_ a vector and each time this method is called, pushing back
onto
> > that vector?
> 
> The StaticSocketDataProvider is DISALLOW_COPY_AND_ASSIGN. So probably better
to
> keep it as it is?

That's true, but you can do 

std::vector<scoped_ptr<StaticSocketDataProvider>> hanging_data_;

and then and a new heap-allocated StaticSocketDataProvider.

[Zhongyi Shi, 2016-01-06 19:42:06]

> That's true, but you can do 
> 
> std::vector<scoped_ptr<StaticSocketDataProvider>> hanging_data_;
> 
> and then and a new heap-allocated StaticSocketDataProvider.

I did consider that, but thought it's too complicated comparing to have an
instance on the test stack. But as it's more straightforward to write the
testcases, I change it to vector. PTAL.

[Ryan Hamilton, 2016-01-08 00:19:08]

LGTM!

[Zhongyi Shi, 2016-01-08 00:56:48]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-01-08 00:57:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1540463003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1540463003/360001

[commit-bot: I haz the power, 2016-01-08 02:54:38]

Description was changed from

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc entry from a list

BUG=570098
==========

to

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc entry from a list

BUG=570098
==========

[commit-bot: I haz the power, 2016-01-08 02:54:39]

Committed patchset #14 (id:360001)

[commit-bot: I haz the power, 2016-01-08 02:56:08]

Description was changed from

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc entry from a list

BUG=570098
==========

to

==========
Change the interface of GetAlternativeServicesFor to GetAlternativeService,
always return the best Alt-Svc entry from a list

BUG=570098

Committed: https://crrev.com/32569c6744b9dc3589fb67f9e25857ba138ed76d
Cr-Commit-Position: refs/heads/master@{#368258}
==========

[commit-bot: I haz the power, 2016-01-08 02:56:09]

Patchset 14 (id:??) landed as
https://crrev.com/32569c6744b9dc3589fb67f9e25857ba138ed76d
Cr-Commit-Position: refs/heads/master@{#368258}

[mef, 2016-01-08 16:03:09]

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

[mef, 2016-01-08 16:03:10]

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

https://codereview.chromium.org/1540463003/diff/360001/net/http/http_stream_f...
net/http/http_stream_factory_impl.cc:133: if (alternative_service.protocol !=
UNINITIALIZED_ALTERNATE_PROTOCOL) {
Should this code be also applied to
HttpStreamFactoryImpl::RequestBidirectionalStreamJob above?