Modify SetAlternateProtocol and HasAlternateProtocol behavior.

Modify SetAlternateProtocol logging behavior.

Modify SetAlternateProtocol() behavior with respect to logging and histograms, by
ignoring alternate protocol being shadowed by |g_forced_alternate_protocol| or
being inactive due to too high probability threshold.

Also clean up HasAlternateProtocol() and GetAlternateProtocol().

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

[Bence, 2015-01-16 17:39:44]

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

[Bence, 2015-01-16 17:39:44]

Ryan: please confirm that this is the intended behaviour.

FYI I am separating this change from
https://codereview.chromium.org/665083009/diff/230001/net/http/http_server_pr...
(in progress).

[Ryan Hamilton, 2015-01-16 22:42:46]

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

https://codereview.chromium.org/858433004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:295: }
This looks like a good change. I wonder if this should go into a method of some
sort. I suspect we do something similar elsewhere in the code?

[Bence, 2015-01-23 16:45:58]

Ryan: PTAL.  The refactoring you suggest makes sense to me, note, however, the
functional change.  (In fact, I do not know whether it is possible for a host
and its canonical host to both have alternate services at the same time.)  Is
this what we want?

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

https://codereview.chromium.org/858433004/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:295: }
On 2015/01/16 22:42:45, Ryan Hamilton wrote:
> This looks like a good change. I wonder if this should go into a method of
some
> sort. I suspect we do something similar elsewhere in the code?

Done.

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

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:233: it->second.probability >=
alternate_protocol_probability_threshold_;
Note functional change: in case there is an alternate protocol for |server| and
one for its canonical host, previously both got compared to threshold (and
HasAlternateProtocol returned true if either was above); now only the |server|
one is found and compared.

[Ryan Hamilton, 2015-01-26 20:39:51]

This will be a nice cleanup!

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

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:233: it->second.probability >=
alternate_protocol_probability_threshold_;
On 2015/01/23 16:45:58, Bence wrote:
> Note functional change: in case there is an alternate protocol for |server|
and
> one for its canonical host, previously both got compared to threshold (and
> HasAlternateProtocol returned true if either was above); now only the |server|
> one is found and compared.

I might be misunderstanding, but it doesn't look like a behavior change to me.
In the old code, if there is an entry for |server| we do not consider the
canonical host's entry because of the early return on line 232. In the new code,
it seems like the behavior is the same because the value return by the iterator
would be the server's entry in this case. What do you think?

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:255:
GetAlternateProtocolIterator(server);
This looks much cleaner. Nice!

Makes me wonder if we should consider factoring AlternateProtocolMap into a
stand-alone class. Something to think about down the road.

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:276: const AlternateProtocolInfo
existing_alternate = it->second;
I think this change means that the g_force feature doesn't work.

I'm not sure if anyone still uses the g_force codepath so perhaps we should nuke
it?

[Bence, 2015-01-26 21:53:57]

Ryan, PTAL.  Thanks.

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

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:233: it->second.probability >=
alternate_protocol_probability_threshold_;
You are right, my bad.

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:276: const AlternateProtocolInfo
existing_alternate = it->second;
> I think this change means that the g_force feature doesn't work.

What behavior do you expect when there is g_force, it is broken, and a client
calls SetAlternateProtocol?  If it is not broken, but there's also an
AlternateProtocol for |server|?  Should SetAlternateProtocol changes not be
recorded while shadowed by g_force, and then be used after
DisableForcedAlternateProtocol is called?

> I'm not sure if anyone still uses the g_force codepath so perhaps we 
> should nuke it?

I'm all up for nuking it, but what about
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...
 On the other hand, nothing calls DisableForcedAlternateProtocol, we could nuke
that and save some headache.

[Ryan Hamilton, 2015-01-27 00:25:17]

lgtm

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

https://codereview.chromium.org/858433004/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:276: const AlternateProtocolInfo
existing_alternate = it->second;
On 2015/01/26 21:53:57, Bence wrote:
> > I think this change means that the g_force feature doesn't work.
> 
> What behavior do you expect when there is g_force, it is broken, and a client
> calls SetAlternateProtocol?  If it is not broken, but there's also an
> AlternateProtocol for |server|?  Should SetAlternateProtocol changes not be
> recorded while shadowed by g_force, and then be used after
> DisableForcedAlternateProtocol is called?

Good question. All I was observing was that in the old code, if g_force was set,
then HasAlternateProtocol(server) would be true whereas the new check will be
false. I'm happy to not care about this edge case if you don't.
> 
> > I'm not sure if anyone still uses the g_force codepath so perhaps we 
> > should nuke it?
> 
> I'm all up for nuking it, but what about
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...
>  On the other hand, nothing calls DisableForcedAlternateProtocol, we could
nuke
> that and save some headache.

I'll bet that if you landed a histogram which counted how many times this
command line flag was used, it would be 0 :> In fact, this is probably worth
doing (in a different CL) so we can feel comfortable nuking this.

[Bence, 2015-01-27 01:38:44]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-01-27 01:39:29]

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

[commit-bot: I haz the power, 2015-01-27 01:43:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-27 01:44:41]

Patchset 2 (id:??) landed as
https://crrev.com/e5213fce048c62b058e9c3c9b9906cc2d0af182b
Cr-Commit-Position: refs/heads/master@{#313197}