Modify SetAlternateProtocol and HasAlternateProtocol behavior.

net/http/http_server_properties_impl.cc

Index: net/http/http_server_properties_impl.cc
diff --git a/net/http/http_server_properties_impl.cc b/net/http/http_server_properties_impl.cc
index a8bc04a685980d1f5adb6d1ec778cdbc58f77968..d048e98871acba04071ccae2c0c81d51d494bc26 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -227,17 +227,10 @@ bool HttpServerPropertiesImpl::HasAlternateProtocol(
     const HostPortPair& server) {
   if (g_forced_alternate_protocol)
     return true;
-  AlternateProtocolMap::const_iterator it = alternate_protocol_map_.Get(server);
-  if (it != alternate_protocol_map_.end())
-    return it->second.probability >= alternate_protocol_probability_threshold_;
-
-  auto canonical = GetCanonicalHost(server);
-  if (canonical == canonical_host_to_origin_map_.end() ||
-      canonical->second.Equals(server)) {
-    return false;
-  }
-
-  return HasAlternateProtocol(canonical->second);
+  AlternateProtocolMap::const_iterator it =
+      GetAlternateProtocolIterator(server);
+  return it != alternate_protocol_map_.end() &&
+         it->second.probability >= alternate_protocol_probability_threshold_;

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

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]

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?

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

You are right, my bad.

 }
 
 std::string HttpServerPropertiesImpl::GetCanonicalSuffix(
@@ -258,16 +251,11 @@ HttpServerPropertiesImpl::GetAlternateProtocol(
     const HostPortPair& server) {
   DCHECK(HasAlternateProtocol(server));
 
-  // First check the map.
-  AlternateProtocolMap::iterator it = alternate_protocol_map_.Get(server);
+  AlternateProtocolMap::const_iterator it =
+      GetAlternateProtocolIterator(server);

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

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.

   if (it != alternate_protocol_map_.end())
     return it->second;
 
-  // Next check the canonical host.
-  CanonicalHostMap::const_iterator canonical_host = GetCanonicalHost(server);
-  if (canonical_host != canonical_host_to_origin_map_.end())
-    return alternate_protocol_map_.Get(canonical_host->second)->second;
-
   // We must be forcing an alternate.
   DCHECK(g_forced_alternate_protocol);
   return *g_forced_alternate_protocol;
@@ -282,9 +270,10 @@ void HttpServerPropertiesImpl::SetAlternateProtocol(
   AlternateProtocolInfo alternate(alternate_port,
                                   alternate_protocol,
                                   alternate_probability);
-  if (HasAlternateProtocol(server)) {
-    const AlternateProtocolInfo existing_alternate =
-        GetAlternateProtocol(server);
+  AlternateProtocolMap::const_iterator it =
+      GetAlternateProtocolIterator(server);
+  if (it != alternate_protocol_map_.end()) {
+    const AlternateProtocolInfo existing_alternate = it->second;

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

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]

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 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]

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

 
     if (existing_alternate.is_broken) {
       DVLOG(1) << "Ignore alternate protocol since it's known to be broken.";
@@ -478,6 +467,20 @@ void HttpServerPropertiesImpl::SetAlternateProtocolProbabilityThreshold(
   alternate_protocol_probability_threshold_ = threshold;
 }
 
+AlternateProtocolMap::const_iterator
+HttpServerPropertiesImpl::GetAlternateProtocolIterator(
+    const HostPortPair& server) {
+  AlternateProtocolMap::const_iterator it = alternate_protocol_map_.Get(server);
+  if (it != alternate_protocol_map_.end())
+    return it;
+
+  CanonicalHostMap::const_iterator canonical = GetCanonicalHost(server);
+  if (canonical != canonical_host_to_origin_map_.end())
+    return alternate_protocol_map_.Get(canonical->second);
+
+  return alternate_protocol_map_.end();
+}
+
 HttpServerPropertiesImpl::CanonicalHostMap::const_iterator
 HttpServerPropertiesImpl::GetCanonicalHost(HostPortPair server) const {
   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {