Modify SetAlternateProtocol and HasAlternateProtocol behavior.

net/http/http_server_properties_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_server_properties_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 209 matching lines @@
                                                 SSLConfig* ssl_config) {
   if (RequiresHTTP11(server)) {
     ForceHTTP11(ssl_config);
   }
 }
 
 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(
     const HostPortPair& server) {
   // If this host ends with a canonical suffix, then return the canonical
   // suffix.
   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {
     std::string canonical_suffix = canonical_suffixes_[i];
     if (EndsWith(server.host(), canonical_suffixes_[i], false)) {
       return canonical_suffix;
     }
   }
   return std::string();
 }
 
 AlternateProtocolInfo
 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;
 }
 
 void HttpServerPropertiesImpl::SetAlternateProtocol(
     const HostPortPair& server,
     uint16 alternate_port,
     AlternateProtocol alternate_protocol,
     double alternate_probability) {
 
   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.";
       return;
     }
 
     if (!existing_alternate.Equals(alternate)) {
       LOG(WARNING) << "Changing the alternate protocol for: "
                    << server.ToString()
                    << " from [Port: " << existing_alternate.port
@@ skipping 173 matching lines @@
 const ServerNetworkStatsMap&
 HttpServerPropertiesImpl::server_network_stats_map() const {
   return server_network_stats_map_;
 }
 
 void HttpServerPropertiesImpl::SetAlternateProtocolProbabilityThreshold(
     double threshold) {
   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) {
     std::string canonical_suffix = canonical_suffixes_[i];
     if (EndsWith(server.host(), canonical_suffixes_[i], false)) {
       HostPortPair canonical_host(canonical_suffix, server.port());
       return canonical_host_to_origin_map_.find(canonical_host);
     }
   }
 
@@ skipping 37 matching lines @@
   base::TimeDelta delay = when > now ? when - now : base::TimeDelta();
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(
           &HttpServerPropertiesImpl::ExpireBrokenAlternateProtocolMappings,
           weak_ptr_factory_.GetWeakPtr()),
       delay);
 }
 
 }  // namespace net