ABANDONED Handle multiple AlternateProtocols for each HostPortPair.

ABANDONED Handle multiple AlternateProtocols for each HostPortPair.

This CL is close to being done.  However, it is currently way too large to be
checked in.  I'll start carving out parts of it and landing them as individual
CLs.

BUG=392575

[Bence, 2014-10-29 21:58:49]

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

[Bence, 2014-10-29 21:59:32]

Ryan: PTAL.  Thanks.

[Bence, 2014-11-03 16:05:37]

Ryan: ping.

[Ryan Hamilton, 2014-11-03 18:14:19]

On 2014/11/03 16:05:37, Bence wrote:
> Ryan: ping.

Sorry for the delay. I took a vacation day on Friday. Reviewing now.

[Ryan Hamilton, 2014-11-03 19:00:31]

A few comments. I'd like to give it a more thorough review but this should
suffice for now.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:101: return port == other.port;
I'm surprised that this only checks the port. Is this method actually used?

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:177: virtual AlternateProtocols
GetAlternateProtocol(
Since this returns multiple protocols, should the method be named
GetAlternateProtocols()?

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:177: virtual AlternateProtocols
GetAlternateProtocol(
Seems like it might be a good idea for this to return a const ref since this
struct is (potentially bigish).

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:190: const AlternateProtocolInfo&
broken_alternate_protocol) = 0;
Does this only need to take the protocol, or does it also need to take the
host:port for the alternative as well? (Same goes for below)

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:53: // persisted.
I think you might want to move this to the second for loop.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:262: return AlternateProtocols(/* size =
*/ 1,
I'm familiar wit this convention of "naming" literals via comments, but I
usually see it without whitespace:

/*size=*/

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:263: /* val = */
*g_forced_alternate_protocol);
Since the variable already has a readable name, I don't think you need the
"val=" comment.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl_unittest.cc:297: }
Unless it's critical to scope these two lines, prefer using a different local
variable name to using this enclosing block.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl_unittest.cc:405: }
Do you feel like you have sufficient test coverage for the new semantics of
HttpServerProperties? I would have expected to have seen some new tests added.

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory.cc
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory.cc:94: host_port, port, protocol, probability);
Does this need to Clear() first? Otherwise it seems like it will simply append?

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:177: using_quic_ = true;
If alternate_protocol_ now contains the alternate protocol, do we still need
using_quic_? (Maybe we do because of the QUIC proxy case?)

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:280: AlternateProtocolInfo
alternate_protocol_;
I think this should probably be initialized in the constructor.

[Bence, 2014-11-03 19:08:41]

Patchset #2 (id:20001) has been deleted

[Bence, 2014-11-03 22:32:26]

Ryan: PTAL.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:101: return port == other.port;
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> I'm surprised that this only checks the port. Is this method actually used? 

Yes, I use it in AlternateProtocols to identify duplicates.  Once we start
saving host in AlternateProtocolInfo, this method will compare that too. 
(Currently host is implied to be the same as the first one.)

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:177: virtual AlternateProtocols
GetAlternateProtocol(
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Seems like it might be a good idea for this to return a const ref since this
> struct is (potentially bigish). 

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:177: virtual AlternateProtocols
GetAlternateProtocol(
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Since this returns multiple protocols, should the method be named
> GetAlternateProtocols()?

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:190: const AlternateProtocolInfo&
broken_alternate_protocol) = 0;
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Does this only need to take the protocol, or does it also need to take the
> host:port for the alternative as well? (Same goes for below)

Since a server might advertise multiple alternate protocols (services) using the
same protocol but different ports (and hosts), we need the entire
AlternateProtocolInfo structure except for the probability to know which one to
set as broken/query if was broken/confirm as not broken/remove.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:53: // persisted.
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> I think you might want to move this to the second for loop.

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:262: return AlternateProtocols(/* size =
*/ 1,
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> I'm familiar wit this convention of "naming" literals via comments, but I
> usually see it without whitespace:
> 
> /*size=*/

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:263: /* val = */
*g_forced_alternate_protocol);
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Since the variable already has a readable name, I don't think you need the
> "val=" comment.

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl_unittest.cc:297: }
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Unless it's critical to scope these two lines, prefer using a different local
> variable name to using this enclosing block.

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl_unittest.cc:405: }
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Do you feel like you have sufficient test coverage for the new semantics of
> HttpServerProperties? I would have expected to have seen some new tests added.

Done.

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory.cc
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory.cc:94: host_port, port, protocol, probability);
On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> Does this need to Clear() first? Otherwise it seems like it will simply
append?

Previous behavior is that if an alternate protocol was known to be broken, we
don't overwrite it.  This can be conveniently mimiced this way.

BTW Alternate protocol and alternate service headers are going to be cumulative,
are they not?

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:177: using_quic_ = true;
On 2014/11/03 19:00:31, Ryan Hamilton wrote:
> If alternate_protocol_ now contains the alternate protocol, do we still need
> using_quic_? (Maybe we do because of the QUIC proxy case?)

I feel like we still need |using_quic_|, otherwise what would we do instead of
the two assignments at
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:280: AlternateProtocolInfo
alternate_protocol_;
On 2014/11/03 19:00:31, Ryan Hamilton wrote:
> I think this should probably be initialized in the constructor.

I specifically added a default constructor to AlternateProtocolInfo that would
set it to UNINITIALIZED_ALTERNATE_PROTOCOL, is that not acceptable?

[Ryan Hamilton, 2014-11-04 18:56:23]

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:101: return port == other.port;
On 2014/11/03 22:32:25, Bence wrote:
> On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> > I'm surprised that this only checks the port. Is this method actually used? 
> 
> Yes, I use it in AlternateProtocols to identify duplicates.  Once we start
> saving host in AlternateProtocolInfo, this method will compare that too. 
> (Currently host is implied to be the same as the first one.)

Ah right! There are still no hosts. Gotcha.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties.h:190: const AlternateProtocolInfo&
broken_alternate_protocol) = 0;
On 2014/11/03 22:32:26, Bence wrote:
> On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> > Does this only need to take the protocol, or does it also need to take the
> > host:port for the alternative as well? (Same goes for below)
> 
> Since a server might advertise multiple alternate protocols (services) using
the
> same protocol but different ports (and hosts), we need the entire
> AlternateProtocolInfo structure except for the probability to know which one
to
> set as broken/query if was broken/confirm as not broken/remove.

Argh! I misread AlternateProtocolInfo as the protocol enum again. Once it
becomes AlternateServiceInfo, I think I'll stop making this mistake.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:288: if
(it->EqualsModuloProbabilityAndProtocol(alternate)) {
I don't think I understand why this is the right tests. If I advertised that I
speak, say, both HTTP/2 and QUIC on port 443 I would expect that both of these
entries would be added. I think the only thing you want to exclude is
probability.

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory.cc
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory.cc:94: host_port, port, protocol, probability);
On 2014/11/03 22:32:26, Bence wrote:
> On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> > Does this need to Clear() first? Otherwise it seems like it will simply
> append?
> 
> Previous behavior is that if an alternate protocol was known to be broken, we
> don't overwrite it.  This can be conveniently mimiced this way.

In the case of non-broken, the old code overwrite the existing entry with the
new entry. With this new code, it seems like we will end up appending, right? I
don't think that's desirable.

> BTW Alternate protocol and alternate service headers are going to be
cumulative,
> are they not?

In the sense that we should union them together? Possibly. Although I was
talking with alyssa and perhaps it makes more sense to use Alt-Svc exclusively
if it exists. And only use Alternate-Protocol if Alt-Svc does not exist.

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:177: using_quic_ = true;
On 2014/11/03 22:32:26, Bence wrote:
> On 2014/11/03 19:00:31, Ryan Hamilton wrote:
> > If alternate_protocol_ now contains the alternate protocol, do we still need
> > using_quic_? (Maybe we do because of the QUIC proxy case?)
> 
> I feel like we still need |using_quic_|, otherwise what would we do instead of
> the two assignments at
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

Makes sense.

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:280: AlternateProtocolInfo
alternate_protocol_;
On 2014/11/03 22:32:26, Bence wrote:
> On 2014/11/03 19:00:31, Ryan Hamilton wrote:
> > I think this should probably be initialized in the constructor.
> 
> I specifically added a default constructor to AlternateProtocolInfo that would
> set it to UNINITIALIZED_ALTERNATE_PROTOCOL, is that not acceptable?

Ah, good point. For some reason I was thinking it was simply an enum.

https://codereview.chromium.org/665083009/diff/80001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/665083009/diff/80001/net/http/http_server_pro...
net/http/http_server_properties.h:84: : port(),
protocol(UNINITIALIZED_ALTERNATE_PROTOCOL), probability() {}
Can you explicitly specify 0 for the numeric initializers?

[Bence, 2014-11-04 20:09:17]

Patchset #5 (id:100001) has been deleted

[Bence, 2014-11-04 21:55:50]

Ryan: PTAL.

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:288: if
(it->EqualsModuloProbabilityAndProtocol(alternate)) {
On 2014/11/04 18:56:22, Ryan Hamilton wrote:
> I don't think I understand why this is the right tests. If I advertised that I
> speak, say, both HTTP/2 and QUIC on port 443 I would expect that both of these
> entries would be added. I think the only thing you want to exclude is
> probability.

Darn, you are right.  The problem with the current design is that if an
alternate protocol is broken, we overwrite the protocol with
ALTERNATE_PROTOCOL_BROKEN, so later we'll have no idea if it was the HTTP/2 or
the QUIC one that was broken.  Like if I test for protocol equality here, the
ALTERNATE_PROTOCOL_BROKEN case six lines below will never get executed.

I propose that I remove |ALTERNATE_PROTOCOL_BROKEN| from |enum
AlternateProtocol| and add |bool broken_| to |struct AlternateProtocolInfo|,
what do you think?

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory.cc
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory.cc:94: host_port, port, protocol, probability);
On 2014/11/04 18:56:22, Ryan Hamilton wrote:
> On 2014/11/03 22:32:26, Bence wrote:
> > On 2014/11/03 19:00:30, Ryan Hamilton wrote:
> > > Does this need to Clear() first? Otherwise it seems like it will simply
> > append?
> > 
> > Previous behavior is that if an alternate protocol was known to be broken,
we
> > don't overwrite it.  This can be conveniently mimiced this way.
> 
> In the case of non-broken, the old code overwrite the existing entry with the
> new entry. With this new code, it seems like we will end up appending, right?
I
> don't think that's desirable.
> 
> > BTW Alternate protocol and alternate service headers are going to be
> cumulative,
> > are they not?
> 
> In the sense that we should union them together? Possibly. Although I was
> talking with alyssa and perhaps it makes more sense to use Alt-Svc exclusively
> if it exists. And only use Alternate-Protocol if Alt-Svc does not exist.

Done.  FYI I had to create a new method in HttpServerProperties*, because
GetAlternateProtocol fails if there is none, and HasAlternateProtocol returns
false if they are all below the probability threshold.

https://codereview.chromium.org/665083009/diff/80001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/665083009/diff/80001/net/http/http_server_pro...
net/http/http_server_properties.h:84: : port(),
protocol(UNINITIALIZED_ALTERNATE_PROTOCOL), probability() {}
On 2014/11/04 18:56:22, Ryan Hamilton wrote:
> Can you explicitly specify 0 for the numeric initializers?

Done.

[Ryan Hamilton, 2014-11-04 22:13:17]

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:288: if
(it->EqualsModuloProbabilityAndProtocol(alternate)) {
On 2014/11/04 21:55:50, Bence wrote:
> On 2014/11/04 18:56:22, Ryan Hamilton wrote:
> > I don't think I understand why this is the right tests. If I advertised that
I
> > speak, say, both HTTP/2 and QUIC on port 443 I would expect that both of
these
> > entries would be added. I think the only thing you want to exclude is
> > probability.
> 
> Darn, you are right.  The problem with the current design is that if an
> alternate protocol is broken, we overwrite the protocol with
> ALTERNATE_PROTOCOL_BROKEN, so later we'll have no idea if it was the HTTP/2 or
> the QUIC one that was broken.  Like if I test for protocol equality here, the
> ALTERNATE_PROTOCOL_BROKEN case six lines below will never get executed.
> 
> I propose that I remove |ALTERNATE_PROTOCOL_BROKEN| from |enum
> AlternateProtocol| and add |bool broken_| to |struct AlternateProtocolInfo|,
> what do you think?

s/broken_/is_broken_/ but yeah, this sounds great. (I think we may have
discussed something like this over the VC a while back.

It might be a good idea to make this change as a small stand-alone change which
you then rebase your current CL on top of. Two small CLs is typically easier to
review than one larger CL.

[Bence, 2014-11-05 15:34:49]

> s/broken_/is_broken_/ but yeah, this sounds great. (I think we may have
> discussed something like this over the VC a while back.
> 
> It might be a good idea to make this change as a small stand-alone change
which
> you then rebase your current CL on top of. Two small CLs is typically easier
to
> review than one larger CL.

Excellent idea.  See https://crrev.com/701163002.

[Bence, 2015-01-16 18:20:02]

Patchset #12 (id:250001) has been deleted

[Bence, 2015-01-30 21:24:50]

Hi Ryan, FYI this is the large draft CL.  I'll break it up to smaller pieces to
be reviewed and landed individually.

https://codereview.chromium.org/665083009/diff/140001/net/http/http_server_pr...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/665083009/diff/140001/net/http/http_server_pr...
net/http/http_server_properties.cc:111: std::string output{};
I don't think this format is currently allowed.

https://codereview.chromium.org/665083009/diff/230001/net/http/http_server_pr...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/665083009/diff/230001/net/http/http_server_pr...
net/http/http_server_properties_impl.cc:310: if
(it->EqualsModuloProbabilityAndProtocol(alternate)) {
This needs to be EqualsModuloProbability.  We couldn't check for protocol
equality while brokenness was encoded in that, but since
https://crrev.com/701163002, we should.

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:501: // ...or alternate_protocol.
Do we need to be able to load old settings, or can we just forget about them?

[Ryan Hamilton, 2015-01-30 21:50:19]

Just an FYI

https://codereview.chromium.org/665083009/diff/140001/net/http/http_server_pr...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/665083009/diff/140001/net/http/http_server_pr...
net/http/http_server_properties.cc:111: std::string output{};
On 2015/01/30 21:24:50, Bence wrote:
> I don't think this format is currently allowed.

the {}s? What's different from simply doing std::string output;?

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:501: // ...or alternate_protocol.
On 2015/01/30 21:24:50, Bence wrote:
> Do we need to be able to load old settings, or can we just forget about them?

We definitely want to support old settings. It looks like this block is pretty
similar to the inner block above. Consider factoring out a helper method to call
in both codepaths.

[Bence, 2015-02-06 19:59:40]

Hello Ryan,

This CL is now in a pretty good shape, lot of it we reviewed and landed
separately so that it's not as long as it once was.  HttpServerPropertiesImpl
stores a vector<AlternateProtocolInfo> for each server.  In the current draft,
GetAlternateProtocol() implements the logic that chooses one that is not broken
and above the threshold.  However, I think you proposed that
GetAlternateProtocol should in fact be GetAlternateProtocols(), returning a
vector<AlternateProtocolInfo>, and this logic of picking a non-broken alternate
protocol should reside elsewhere.  I feel it's time to talk about where it would
be.  Maybe in HttpStreamFactoryImpl::GetAlternateProtocolRequestFor()?

Also, I need a copy of alternate_protocol in
QuicStreamFactory::ProcessGoingAwaySession(), where we do the dance of marking
the alternate protocol broken, removing it, and re-adding it.  I plumbed it
through in Patch Set 26, this is just a prototype.  Here we grab a copy in
QuicStreamFactory::Create().  This is not new:
WasAlternateProtocolRecentlyBroken() has been being called from here for a long
time, and passed along to Job::Job as was_alternate_protocol_recently_broken. 
(Now GetAlternateProtocol needs to be called anyway, since
WasAlternateProtocolRecentlyBroken() now also take an AlternateProtocolInfo
argument.)  I'm just hanging an AlternateProtocolInfo right next to
was_alternate_protocol_recently_broken, and then in Job::DoConnect(), pass it on
to QuicClientSession, which stores it until it's used in
QuicStreamFactory::ProcessGoingAway().  Of course, if you know a better way, let
me know.

Now the question still remains: where should I put the logic which picks the
first (or favorit) non-broken alternate protocol for a given host, if not in
GetAlternateProtocol()?  This logic will be called from (or reside in)
HttpStreamFactoryImpl::GetAlternateProtocolRequestFor(), and will also have to
be called from QuicStreamFactory::Create().

Bence

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/665083009/diff/340001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:501: // ...or alternate_protocol.
On 2015/01/30 21:50:19, Ryan Hamilton wrote:
> On 2015/01/30 21:24:50, Bence wrote:
> > Do we need to be able to load old settings, or can we just forget about
them?
> 
> We definitely want to support old settings. It looks like this block is pretty
> similar to the inner block above. Consider factoring out a helper method to
call
> in both codepaths.

Done.

[Bence, 2015-02-26 20:53:46]

Ryan: PTAL.  GetAlternateProtocols now returns a vector.  The only thing that
remains is pick the right alternate protocol in QuicStreamFactory::Create() and
in HttpStreamFactoryImpl::Job::DoInitConnection().  (Note that neither calls
GetAlternateProtocol right now, some more massage is gonna be needed.)

[Ryan Hamilton, 2015-02-27 18:20:48]

A few comments. I think your design doc is very helpful!

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory.cc:90: host_port, static_cast<uint16>(port),
protocol, probability);
I don't think this is quite right. Let's say that initially we have:
  Alternate-Protocol: QUIC:443
But then we see:
  Alternate-Protocol: npn-spdy:80
(This example is lame, but bear with me :>). I think that as written, this code
will end up with *both* mappings active, but I think we only want one. What do
you think?

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

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:174: AlternateProtocolInfo
HttpStreamFactoryImpl::GetAlternateProtocolRequestFor(
As discussed yesterday, I think this needs to return all the alternates. It
should probably filter the list in the same way that it currently exclude some
alternate protocols.

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:193: alternate !=
alternate_protocols.end(); ++alternate) {
Can you use a C++ range based for loop here?

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:170: AlternateProtocolInfo alternate) {
As discussed yesterday, this should probably take the full list of alternatives
so that it can check active QUIC and SPDY sessions for existing entries.

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

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:716: }
There's a lot going on here, I think it might make sense to put this into a
helper method.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:746: const AlternateProtocolInfo
alternate_protocol(server_id.port(), QUIC, 0.0);
It seems like this should be done *in* WasAlternateProtocol broken, since from
the perspective of the QuicStreamFactory, there is only one alternate protocol
we're talking about. You could consider renaming the method to be
WasQuicRecentlyBroken, if you wish.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1346: // protocol for the server.
I don't understand this comment.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1351: for (alternate =
alternate_protocols.begin();
range-base for loop, please.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1359: }
As per the design doc you wrote up, I think this code needs to be reassessed. We
know the host:port of the server we're talking to an we know that we're speaking
QUIC. We want to (possibly) mark QUIC as broken to this server. But it looks
like we're going to end up checking the origin->server map looking for an origin
with this server's host:port pair. I think this might not be what we want to do?

(The current code may well be confused in this respect. In current practice the
host:port of the origin is always the host:port of the alternate, so the current
code may well work by accident not design)

[Bence, 2015-02-27 19:31:41]

Ryan, FYI.  I feel like we have to sort out broken semantics before landing
this.

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory.cc:90: host_port, static_cast<uint16>(port),
protocol, probability);
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> I don't think this is quite right. Let's say that initially we have:
>   Alternate-Protocol: QUIC:443
> But then we see:
>   Alternate-Protocol: npn-spdy:80
> (This example is lame, but bear with me :>). I think that as written, this
code
> will end up with *both* mappings active, but I think we only want one. What do
> you think?

You are right.  So we will have another method for parsing alternative services,
right?  And we want both ProcessAlternateProtocols and ProcessAlternateServices
to overwrite existing settings, right?

Done.

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

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:174: AlternateProtocolInfo
HttpStreamFactoryImpl::GetAlternateProtocolRequestFor(
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> As discussed yesterday, I think this needs to return all the alternates. It
> should probably filter the list in the same way that it currently exclude some
> alternate protocols.

Absolutely.  Now even after this CL we can only have one alternate protocol for
each server, can I do that in a separate CL?

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:193: alternate !=
alternate_protocols.end(); ++alternate) {
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> Can you use a C++ range based for loop here?

Done.

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/665083009/diff/580001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:170: AlternateProtocolInfo alternate) {
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> As discussed yesterday, this should probably take the full list of
alternatives
> so that it can check active QUIC and SPDY sessions for existing entries.

Acknowledged.

By the way, what guarantees that Job::DoInitConnection is executed _after_
MarkAsAlternate is called?  (Otherwise the logic we are planting there will not
work as intended!)

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

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:716: }
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> There's a lot going on here, I think it might make sense to put this into a
> helper method.

Can I do that later?  A lot of things will change soon.  For example, we will
not be fishing for the alternative service here, but construct one from
host_port_pair.  Also, I don't like messing around with
HttpServerProperties::alternate_protocol_map_ outside the class.  So the method
you're proposing will really have to live in HttpServerProperties.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:746: const AlternateProtocolInfo
alternate_protocol(server_id.port(), QUIC, 0.0);
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> It seems like this should be done *in* WasAlternateProtocol broken, since from
> the perspective of the QuicStreamFactory, there is only one alternate protocol
> we're talking about. You could consider renaming the method to be
> WasQuicRecentlyBroken, if you wish.

I think this just underlines the issue that we need to change
WasAlternateProtocolRecentlyBroken semantics to operate on the alternative
service, and not on the origin.  (Heck, we might not even know the origin in
this method.)

I like the WasQuicRecentlyBroken idea.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1346: // protocol for the server.
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> I don't understand this comment.

I am substituting new lines 1350--1359 for old lines 1327--1330, this is what
I'm explaining here.

Hey, this all have to be scratched, since we will not know what server (origin)
was.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1351: for (alternate =
alternate_protocols.begin();
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> range-base for loop, please.

Note that in this case I couldn't do it, because range-based loops declare the
running variable in loop scope only, I couldn't do the check on line 1357.

https://codereview.chromium.org/665083009/diff/580001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1359: }
On 2015/02/27 18:20:48, Ryan Hamilton wrote:
> As per the design doc you wrote up, I think this code needs to be reassessed.
We
> know the host:port of the server we're talking to an we know that we're
speaking
> QUIC. We want to (possibly) mark QUIC as broken to this server. But it looks
> like we're going to end up checking the origin->server map looking for an
origin
> with this server's host:port pair. I think this might not be what we want to
do?
> 
> (The current code may well be confused in this respect. In current practice
the
> host:port of the origin is always the host:port of the alternate, so the
current
> code may well work by accident not design)

Absolutely.  Let's revisit this once we work out the SetBroken/WasBroken
semantics.

[Ryan Hamilton, 2015-04-01 03:03:28]

Removing myself as a reviewer. Add back when you're ready to land. (Or feel free
to do that in a different CL).

[Ryan Hamilton, 2015-04-01 03:07:44]

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

[Bence, 2015-07-08 05:27:24]

Abandoning CL in favor of https://crrev.com/1216703002.