SHP 4: Change AlternativeServiceMap to use SchemeHostPort as the key. No change to Pref data.

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 <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 87 matching lines @@
   // Add the entries from the memory cache.
   for (AlternativeServiceMap::reverse_iterator input_it =
            new_alternative_service_map.rbegin();
        input_it != new_alternative_service_map.rend(); ++input_it) {
     if (alternative_service_map_.Get(input_it->first) ==
         alternative_service_map_.end()) {
       alternative_service_map_.Put(input_it->first, input_it->second);
     }
   }
 
-  // Attempt to find canonical servers.
-  uint16_t canonical_ports[] = {80, 443};
+  // Attempt to find canonical servers. Canonical suffix only apply to HTTPS.

[Ryan Hamilton, 2016/04/18 17:18:40]

Does this represent a behavior change? If so, is this intentional?

[Zhongyi Shi, 2016/04/18 19:26:37]

According to our discussion
(https://codereview.chromium.org/1824903002/diff/60001/net/http/http_server_pr...),
I think we agreed on apply canonical hosts to HTTPS only. 

+  uint16_t canonical_ports = 443;

[Ryan Hamilton, 2016/04/18 17:18:40]

Since this is a single port, you could say _port instead of _ports.
Possibly do const char* canonical_scheme = "https" to be consistent?

[Zhongyi Shi, 2016/04/18 19:26:37]

Done.

   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {
     std::string canonical_suffix = canonical_suffixes_[i];
-    for (size_t j = 0; j < arraysize(canonical_ports); ++j) {
-      HostPortPair canonical_host(canonical_suffix, canonical_ports[j]);
-      // If we already have a valid canonical server, we're done.
-      if (ContainsKey(canonical_host_to_origin_map_, canonical_host) &&
-          (alternative_service_map_.Peek(
-               canonical_host_to_origin_map_[canonical_host]) !=
-           alternative_service_map_.end())) {
-        continue;
-      }
-      // Now attempt to find a server which matches this origin and set it as
-      // canonical.
-      for (AlternativeServiceMap::const_iterator it =
-               alternative_service_map_.begin();
-           it != alternative_service_map_.end(); ++it) {
-        if (base::EndsWith(it->first.host(), canonical_suffixes_[i],
-                           base::CompareCase::INSENSITIVE_ASCII)) {
-          canonical_host_to_origin_map_[canonical_host] = it->first;
-          break;
-        }
+    url::SchemeHostPort canonical_host("https", canonical_suffix,

[Ryan Hamilton, 2016/04/18 17:18:40]

nit s/_host/_server/?

[Zhongyi Shi, 2016/04/18 19:26:37]

Done.

+                                       canonical_ports);
+    // If we already have a valid canonical server, we're done.
+    if (ContainsKey(canonical_host_to_origin_map_, canonical_host) &&
+        (alternative_service_map_.Peek(
+             canonical_host_to_origin_map_[canonical_host]) !=
+         alternative_service_map_.end())) {
+      continue;
+    }
+    // Now attempt to find a server which matches this origin and set it as
+    // canonical.
+    for (AlternativeServiceMap::const_iterator it =
+             alternative_service_map_.begin();
+         it != alternative_service_map_.end(); ++it) {
+      if (base::EndsWith(it->first.host(), canonical_suffixes_[i],
+                         base::CompareCase::INSENSITIVE_ASCII) &&
+          it->first.scheme() == "https") {

[Ryan Hamilton, 2016/04/18 17:18:40]

nit: instead of checking https here, you can use canonical_server.scheme

[Zhongyi Shi, 2016/04/18 19:26:37]

Done.

+        canonical_host_to_origin_map_[canonical_host] = it->first;
+        break;
       }
     }
   }
 }
 
 void HttpServerPropertiesImpl::InitializeSpdySettingsServers(
     SpdySettingsMap* spdy_settings_map) {
   // Add the entries from persisted data.
   SpdySettingsMap new_spdy_settings_map(SpdySettingsMap::NO_AUTO_EVICT);
   for (SpdySettingsMap::reverse_iterator it = spdy_settings_map->rbegin();
@@ skipping 88 matching lines @@
   alternative_service_map_.Clear();
   canonical_host_to_origin_map_.clear();
   spdy_settings_map_.Clear();
   last_quic_address_ = IPAddress();
   server_network_stats_map_.Clear();
   quic_server_info_map_.Clear();
 }
 
 bool HttpServerPropertiesImpl::SupportsRequestPriority(
     const url::SchemeHostPort& server) {
-  HostPortPair host_port_pair(server.host(), server.port());
   DCHECK(CalledOnValidThread());
   if (server.host().empty())
     return false;
 
   if (GetSupportsSpdy(server))
     return true;
   const AlternativeServiceVector alternative_service_vector =
-      GetAlternativeServices(host_port_pair);
+      GetAlternativeServices(server);
   for (const AlternativeService& alternative_service :
        alternative_service_vector) {
     if (alternative_service.protocol == QUIC) {
       return true;
     }
   }
   return false;
 }
 
 bool HttpServerPropertiesImpl::GetSupportsSpdy(
@@ skipping 57 matching lines @@
     std::string canonical_suffix = canonical_suffixes_[i];
     if (base::EndsWith(host, canonical_suffixes_[i],
                        base::CompareCase::INSENSITIVE_ASCII)) {
       return canonical_suffix;
     }
   }
   return std::string();
 }
 
 AlternativeServiceVector HttpServerPropertiesImpl::GetAlternativeServices(
-    const HostPortPair& origin) {
+    const url::SchemeHostPort& origin) {
+  HostPortPair host_port_pair(origin.host(), origin.port());

[Ryan Hamilton, 2016/04/18 17:18:40]

nit: move this down to where it's used.

[Zhongyi Shi, 2016/04/18 19:26:37]

Done.

   // Copy valid alternative services into |valid_alternative_services|.
   AlternativeServiceVector valid_alternative_services;
   const base::Time now = base::Time::Now();
   AlternativeServiceMap::iterator map_it = alternative_service_map_.Get(origin);
   if (map_it != alternative_service_map_.end()) {
     for (AlternativeServiceInfoVector::iterator it = map_it->second.begin();
          it != map_it->second.end();) {
       if (it->expiration < now) {
         it = map_it->second.erase(it);
         continue;
       }
       AlternativeService alternative_service(it->alternative_service);
       if (alternative_service.host.empty()) {
         alternative_service.host = origin.host();
       }
       // If the alternative service is equivalent to the origin (same host, same
       // port, and both TCP), then there is already a Job for it, so do not

[Ryan Hamilton, 2016/04/18 17:18:40]

nit: Can you change the comment to remove the mention of "then there is already
a Job for it"

[Zhongyi Shi, 2016/04/18 19:26:37]

Done.

       // return it here.
-      if (origin.Equals(alternative_service.host_port_pair()) &&
+      if (host_port_pair.Equals(alternative_service.host_port_pair()) &&
           NPN_SPDY_MINIMUM_VERSION <= alternative_service.protocol &&
           alternative_service.protocol <= NPN_SPDY_MAXIMUM_VERSION) {
         ++it;
         continue;
       }
       valid_alternative_services.push_back(alternative_service);
       ++it;
     }
     if (map_it->second.empty()) {
       alternative_service_map_.Erase(map_it);
@@ skipping 30 matching lines @@
     valid_alternative_services.push_back(alternative_service);
     ++it;
   }
   if (map_it->second.empty()) {
     alternative_service_map_.Erase(map_it);
   }
   return valid_alternative_services;
 }
 
 bool HttpServerPropertiesImpl::SetAlternativeService(
-    const HostPortPair& origin,
+    const url::SchemeHostPort& origin,
     const AlternativeService& alternative_service,
     base::Time expiration) {
   return SetAlternativeServices(
       origin,
       AlternativeServiceInfoVector(
           /*size=*/1, AlternativeServiceInfo(alternative_service, expiration)));
 }
 
 bool HttpServerPropertiesImpl::SetAlternativeServices(
-    const HostPortPair& origin,
+    const url::SchemeHostPort& origin,
     const AlternativeServiceInfoVector& alternative_service_info_vector) {
   AlternativeServiceMap::iterator it = alternative_service_map_.Peek(origin);
 
   if (alternative_service_info_vector.empty()) {
     if (it == alternative_service_map_.end()) {
       return false;
     }
     ClearAlternativeServices(origin);
     return true;
   }
@@ skipping 17 matching lines @@
     // TODO(rch): Consider the case where multiple requests are started
     // before the first completes. In this case, only one of the jobs
     // would reach this code, whereas all of them should should have.
     HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_MAPPING_MISSING);
   }
 
   // If this host ends with a canonical suffix, then set it as the
   // canonical host.
   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {
     std::string canonical_suffix = canonical_suffixes_[i];
-    if (base::EndsWith(origin.host(), canonical_suffixes_[i],
+    // canonical suffixes only apply to HTTPS.
+    if (origin.scheme() == "https" &&
+        base::EndsWith(origin.host(), canonical_suffixes_[i],
                        base::CompareCase::INSENSITIVE_ASCII)) {
-      HostPortPair canonical_host(canonical_suffix, origin.port());
+      url::SchemeHostPort canonical_host("https", canonical_suffix,
+                                         origin.port());
       canonical_host_to_origin_map_[canonical_host] = origin;
       break;
     }
   }
 
   return changed;
 }
 
 void HttpServerPropertiesImpl::MarkAlternativeServiceBroken(
     const AlternativeService& alternative_service) {
@@ skipping 44 matching lines @@
 
 void HttpServerPropertiesImpl::ConfirmAlternativeService(
     const AlternativeService& alternative_service) {
   if (alternative_service.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL)
     return;
   broken_alternative_services_.erase(alternative_service);
   recently_broken_alternative_services_.erase(alternative_service);
 }
 
 void HttpServerPropertiesImpl::ClearAlternativeServices(
-    const HostPortPair& origin) {
+    const url::SchemeHostPort& origin) {
   RemoveCanonicalHost(origin);
 
   AlternativeServiceMap::iterator it = alternative_service_map_.Peek(origin);
   if (it == alternative_service_map_.end()) {
     return;
   }
   alternative_service_map_.Erase(it);
 }
 
 const AlternativeServiceMap& HttpServerPropertiesImpl::alternative_service_map()
     const {
   return alternative_service_map_;
 }
 
 scoped_ptr<base::Value>
 HttpServerPropertiesImpl::GetAlternativeServiceInfoAsValue()
     const {
   scoped_ptr<base::ListValue> dict_list(new base::ListValue);
   for (const auto& alternative_service_map_item : alternative_service_map_) {
     scoped_ptr<base::ListValue> alternative_service_list(new base::ListValue);
-    const HostPortPair& host_port_pair = alternative_service_map_item.first;
+    const url::SchemeHostPort& server = alternative_service_map_item.first;
     for (const AlternativeServiceInfo& alternative_service_info :
          alternative_service_map_item.second) {
       std::string alternative_service_string(
           alternative_service_info.ToString());
       AlternativeService alternative_service(
           alternative_service_info.alternative_service);
       if (alternative_service.host.empty()) {
-        alternative_service.host = host_port_pair.host();
+        alternative_service.host = server.host();
       }
       if (IsAlternativeServiceBroken(alternative_service)) {
         alternative_service_string.append(" (broken)");
       }
       alternative_service_list->Append(
           new base::StringValue(alternative_service_string));
     }
     if (alternative_service_list->empty())
       continue;
     scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-    dict->SetString("host_port_pair", host_port_pair.ToString());
+    dict->SetString("server", server.Serialize());
     dict->Set("alternative_service",
               scoped_ptr<base::Value>(std::move(alternative_service_list)));
     dict_list->Append(std::move(dict));
   }
   return std::move(dict_list);
 }
 
 const SettingsMap& HttpServerPropertiesImpl::GetSpdySettings(
     const url::SchemeHostPort& server) {
   SpdySettingsMap::iterator it = spdy_settings_map_.Get(server);
@@ skipping 117 matching lines @@
   for (QuicServerInfoMap::reverse_iterator it = quic_server_info_map_.rbegin();
        it != quic_server_info_map_.rend(); ++it) {
     temp_map.Put(it->first, it->second);
   }
 
   quic_server_info_map_.Swap(temp_map);
 }
 
 AlternativeServiceMap::const_iterator
 HttpServerPropertiesImpl::GetAlternateProtocolIterator(
-    const HostPortPair& server) {
+    const url::SchemeHostPort& server) {
   AlternativeServiceMap::const_iterator it =
       alternative_service_map_.Get(server);
   if (it != alternative_service_map_.end())
     return it;
 
   CanonicalHostMap::const_iterator canonical = GetCanonicalHost(server);
   if (canonical == canonical_host_to_origin_map_.end()) {
     return alternative_service_map_.end();
   }
 
-  const HostPortPair canonical_host_port = canonical->second;
-  it = alternative_service_map_.Get(canonical_host_port);
+  const url::SchemeHostPort canonical_server = canonical->second;
+  it = alternative_service_map_.Get(canonical_server);
   if (it == alternative_service_map_.end()) {
     return alternative_service_map_.end();
   }
 
   for (const AlternativeServiceInfo& alternative_service_info : it->second) {
     AlternativeService alternative_service(
         alternative_service_info.alternative_service);
     if (alternative_service.host.empty()) {
-      alternative_service.host = canonical_host_port.host();
+      alternative_service.host = canonical_server.host();
     }
     if (!IsAlternativeServiceBroken(alternative_service)) {
       return it;
     }
   }
 
-  RemoveCanonicalHost(canonical_host_port);
+  RemoveCanonicalHost(canonical_server);
   return alternative_service_map_.end();
 }
 
 HttpServerPropertiesImpl::CanonicalHostMap::const_iterator
-HttpServerPropertiesImpl::GetCanonicalHost(HostPortPair server) const {
+HttpServerPropertiesImpl::GetCanonicalHost(
+    const url::SchemeHostPort& server) const {
+  if (server.scheme() != "https")
+    return canonical_host_to_origin_map_.end();
+
   for (size_t i = 0; i < canonical_suffixes_.size(); ++i) {
     std::string canonical_suffix = canonical_suffixes_[i];
     if (base::EndsWith(server.host(), canonical_suffixes_[i],
                        base::CompareCase::INSENSITIVE_ASCII)) {
-      HostPortPair canonical_host(canonical_suffix, server.port());
+      url::SchemeHostPort canonical_host("https", canonical_suffix,
+                                         server.port());
       return canonical_host_to_origin_map_.find(canonical_host);
     }
   }
 
   return canonical_host_to_origin_map_.end();
 }
 
 void HttpServerPropertiesImpl::RemoveCanonicalHost(
-    const HostPortPair& server) {
+    const url::SchemeHostPort& server) {
   CanonicalHostMap::const_iterator canonical = GetCanonicalHost(server);
   if (canonical == canonical_host_to_origin_map_.end())
     return;
 
   if (!canonical->second.Equals(server))
     return;
 
   canonical_host_to_origin_map_.erase(canonical->first);
 }
 
@@ skipping 52 matching lines @@
   base::TimeDelta delay = when > now ? when - now : base::TimeDelta();
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(
           &HttpServerPropertiesImpl::ExpireBrokenAlternateProtocolMappings,
           weak_ptr_factory_.GetWeakPtr()),
       delay);
 }
 
 }  // namespace net