Remove HasAlternateProtocol().

Remove HasAlternateProtocol().

If GetAlternateProtocol() is called but there is no alternate protocol, it
now returns one with UNINITIALIZED_ALTERNATE_PROTOCOL.

See https://crrev.com/665083009 for the context of this change and how draft
future changes might rely on this CL.

BUG=392575
Committed: https://crrev.com/3c255331e787370ef99f38319ce2d812a89f5b82
Cr-Commit-Position: refs/heads/master@{#314428}

[Bence, 2015-02-02 23:39:13]

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

[Bence, 2015-02-02 23:39:13]

Ryan, PTAL.  Thanks.

[Ryan Hamilton, 2015-02-03 00:07:01]

The main change looks reasonable, but a couple of high-level comments:
* If callers no longer need to call HasAlternateProtocol, please audit existing
callers to remove this now redundant calls.
* I'm not sure that I understand the need to change the semantics of
HasAlternateProtocol with respect to broken. If callers no longer need to call
HasAlternateProtocol, should we get rid of the method? It's confusing to me that
HasAlternateProtocol would return false, but Get would return something other
than UNINITIALIZED. Perhaps I'm thinking about this the wrong way?

[Bence, 2015-02-03 16:05:45]

PTAL.

On 2015/02/03 00:07:01, Ryan Hamilton wrote:
> The main change looks reasonable, but a couple of high-level comments:
> * If callers no longer need to call HasAlternateProtocol, please audit
existing
> callers to remove this now redundant calls.

You are right, I forgot about a couple of those.  Done.

> * I'm not sure that I understand the need to change the semantics of
> HasAlternateProtocol with respect to broken.

After implementing multiple alternate protocols (or alternative services) per
server, imagine the case that for a given server the first alternate protocol is
broken, the second one is not.  We want GetAlternateProtocol to return the
second one.  This change is warming up for it.

Or would we want GetAlternateProtocol to return the first non-broken alternate
protocol if there is one, and the first broken one otherwise?  Why?

Maybe I can implement IsAlternateProtocolBroken(server, alternate_protocol) for
testing only.  That feels a little bit of an overkill.

If you want, I can revert the change with respect to brokenness, and we can do
that on a separate CL.  In that case, this CL would focus on not having to call
HasAlternateProtocol (or removing it).  Let me know.

> If callers no longer need to call
> HasAlternateProtocol, should we get rid of the method?

It's a convenience method used in tests only.  I would be glad to get rid of it.
See Patch Set 4 for what it would be like, and let me know if you prefer that.

> It's confusing to me that
> HasAlternateProtocol would return false, but Get would return something other
> than UNINITIALIZED. Perhaps I'm thinking about this the wrong way?

This would be moot with the removal of HasAlternateProtocol, but you are right,
it was definitely not my intention. (I don't actually think it was the case.)

[Ryan Hamilton, 2015-02-03 18:40:14]

On 2015/02/03 16:05:45, Bence wrote:
> PTAL.
> 
> On 2015/02/03 00:07:01, Ryan Hamilton wrote:
> > The main change looks reasonable, but a couple of high-level comments:
> > * If callers no longer need to call HasAlternateProtocol, please audit
> existing
> > callers to remove this now redundant calls.
> 
> You are right, I forgot about a couple of those.  Done.
> 
> > * I'm not sure that I understand the need to change the semantics of
> > HasAlternateProtocol with respect to broken.
> 
> After implementing multiple alternate protocols (or alternative services) per
> server, imagine the case that for a given server the first alternate protocol
is
> broken, the second one is not.  We want GetAlternateProtocol to return the
> second one.  This change is warming up for it.
> 
> Or would we want GetAlternateProtocol to return the first non-broken alternate
> protocol if there is one, and the first broken one otherwise?  Why?

Ah, I see. I would expect that in the "many alternatives" world,
GetAlternateProtocol will return all of them, not just one. I don't think that
HttpServerProperties will necessarily be in the best position to decide which is
the right alternative, if many match. So if it returns, say, a list then the
caller can loop over the options and decide what it wants to do.

> Maybe I can implement IsAlternateProtocolBroken(server, alternate_protocol)
for
> testing only.  That feels a little bit of an overkill.
> 
> If you want, I can revert the change with respect to brokenness, and we can do
> that on a separate CL.  In that case, this CL would focus on not having to
call
> HasAlternateProtocol (or removing it).  Let me know.

That might be a good idea to make forward progress.

> > If callers no longer need to call
> > HasAlternateProtocol, should we get rid of the method?
> 
> It's a convenience method used in tests only.  I would be glad to get rid of
it.
> See Patch Set 4 for what it would be like, and let me know if you prefer that.

Nice. I think this seems like a good cleanup. 

> > It's confusing to me that
> > HasAlternateProtocol would return false, but Get would return something
other
> > than UNINITIALIZED. Perhaps I'm thinking about this the wrong way?
> 
> This would be moot with the removal of HasAlternateProtocol, but you are
right,
> it was definitely not my intention. (I don't actually think it was the case.)

Cool, let's nuke it.

[Bence, 2015-02-03 18:54:49]

On 2015/02/03 18:40:14, Ryan Hamilton wrote:
> Ah, I see. I would expect that in the "many alternatives" world,
> GetAlternateProtocol will return all of them, not just one. I don't think that
> HttpServerProperties will necessarily be in the best position to decide which
is
> the right alternative, if many match. So if it returns, say, a list then the
> caller can loop over the options and decide what it wants to do.

Okay, sounds like a plan.  Now currently GetAlternateProtocol only returns an
alternate protocol if its probability is at least the threshold.  Do we want, in
the "many alternatives" world, to return a list of alternate protocols with
probability at least the threshold too?  That is, do we want to go through the
internally stored vector of structs, and copy them one by one, conditionally on
the threshold, into a vector that we return?  I am worried about performance,
especially compared to returning the member vector by reference.

[Ryan Hamilton, 2015-02-03 19:25:04]

On 2015/02/03 18:54:49, Bence wrote:
> On 2015/02/03 18:40:14, Ryan Hamilton wrote:
> > Ah, I see. I would expect that in the "many alternatives" world,
> > GetAlternateProtocol will return all of them, not just one. I don't think
that
> > HttpServerProperties will necessarily be in the best position to decide
which
> is
> > the right alternative, if many match. So if it returns, say, a list then the
> > caller can loop over the options and decide what it wants to do.
> 
> Okay, sounds like a plan.  Now currently GetAlternateProtocol only returns an
> alternate protocol if its probability is at least the threshold.  Do we want,
in
> the "many alternatives" world, to return a list of alternate protocols with
> probability at least the threshold too?  That is, do we want to go through the
> internally stored vector of structs, and copy them one by one, conditionally
on
> the threshold, into a vector that we return?  I am worried about performance,
> especially compared to returning the member vector by reference.

If we wanted to optimize this, I could imagine we could keep two vectors, one
with all the alternatives and one with just those alternatives above the
threshold. We could return a reference to the second. How's that sound?

[Bence, 2015-02-03 20:43:21]

PTAL

https://codereview.chromium.org/874603004/diff/80001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (left):

https://codereview.chromium.org/874603004/diff/80001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:261: if (it !=
alternate_protocol_map_.end())
Note that previously GetAlternateProtocol() was able to return an alternate
protocol with probability lower than the threshold, provided that
|g_forced_alternate_protocol| was not nullptr.  This is now fixed.

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (left):

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:93: if (existing_alternate.is_broken)
Note that SetAlternateProtocol is not willing to overwrite a broken alternate
protocol anyway.

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl.cc (left):

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
net/http/http_stream_factory_impl.cc:179: AlternateProtocolInfo(0, 
UNINITIALIZED_ALTERNATE_PROTOCOL, 0);
Default constructor added, making this line redundant.

[Ryan Hamilton, 2015-02-03 22:29:03]

This is lookin' good!

Can you make a small change to the CL description to make it clear that
"uninitialized" means UNINITIALIZED_ALTERNATE_PROTOCOL, not a chuck of
uninitialized memory :>

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl.cc (left):

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
net/http/http_stream_factory_impl.cc:179: AlternateProtocolInfo(0, 
UNINITIALIZED_ALTERNATE_PROTOCOL, 0);
On 2015/02/03 20:43:21, Bence wrote:
> Default constructor added, making this line redundant.

I'm not seeing the new constructor. That being said, if we can get away without
a default constructor, I tend to prefer that since it makes it very obvious what
is being created.

https://codereview.chromium.org/874603004/diff/60002/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/874603004/diff/60002/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:246: it->second.probability >=
alternate_protocol_probability_threshold_)
This check is required because it used to be part of HasAlternateProtocol(),
right?

[Bence, 2015-02-03 22:52:37]

PTAL.  Answers to your questions below.  Thanks.

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl.cc (left):

https://codereview.chromium.org/874603004/diff/80001/net/http/http_stream_fac...
net/http/http_stream_factory_impl.cc:179: AlternateProtocolInfo(0, 
UNINITIALIZED_ALTERNATE_PROTOCOL, 0);
You are right, it has been introduced by https://crrev.com/871753006 yesterday,
so it hasn't been picked up by codesearch yet.

https://codereview.chromium.org/874603004/diff/60002/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/874603004/diff/60002/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:246: it->second.probability >=
alternate_protocol_probability_threshold_)
On 2015/02/03 22:29:03, Ryan Hamilton wrote:
> This check is required because it used to be part of HasAlternateProtocol(),
> right?

Yes, that is indeed the case.

[Ryan Hamilton, 2015-02-03 23:02:37]

lgtm

Thanks!

[Bence, 2015-02-03 23:07:05]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-02-03 23:07:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874603004/60002

[commit-bot: I haz the power, 2015-02-03 23:12:43]

Committed patchset #6 (id:60002)

[commit-bot: I haz the power, 2015-02-03 23:13:37]

Patchset 6 (id:??) landed as
https://crrev.com/3c255331e787370ef99f38319ce2d812a89f5b82
Cr-Commit-Position: refs/heads/master@{#314428}