Update param types of HttpServerPropertiesImpl setters and getters. Fix MRU order when loading

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 <memory>
 #include <utility>
 
@@ skipping 27 matching lines @@
   canonical_suffixes_.push_back(".c.youtube.com");
   canonical_suffixes_.push_back(".googlevideo.com");
   canonical_suffixes_.push_back(".googleusercontent.com");
 }
 
 HttpServerPropertiesImpl::~HttpServerPropertiesImpl() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 }
 
 void HttpServerPropertiesImpl::SetSpdyServers(
-    std::vector<std::string>* spdy_servers,
+    const std::vector<std::string>* spdy_servers,

[Ryan Hamilton, 2017/06/20 18:25:56]

I wonder if this should be a unique_ptr<SpdyServersMap> instead?

[wangyix1, 2017/06/21 18:45:32]

That should work, it would reduce copying and be consistent with the other
setters. The only thing is that it changes the API a little? Instead of having
to set all spdy_servers to be true or false in spdy_servers_map_, it would be
able to set some true and some false. I'll implement this and see what you
think.

Done.

     bool support_spdy) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (!spdy_servers)
     return;
 
   // Add the entries from persisted data.
   SpdyServersMap spdy_servers_map(SpdyServersMap::NO_AUTO_EVICT);
-  for (std::vector<std::string>::reverse_iterator it = spdy_servers->rbegin();
-       it != spdy_servers->rend(); ++it) {
+  for (auto it = spdy_servers->rbegin(); it != spdy_servers->rend(); ++it) {

[Zhongyi Shi, 2017/06/20 22:06:18]

nit: in general, we prefer to specify the explicit type of the iterator so that
the code is more readable. "auto" is cleaner though...

[wangyix1, 2017/06/21 18:45:32]

Done.

     spdy_servers_map.Put(*it, support_spdy);
   }
 
   // |spdy_servers_map| will have the memory cache.
   spdy_servers_map_.Swap(spdy_servers_map);
 
   // Add the entries from the memory cache.
   for (SpdyServersMap::reverse_iterator it = spdy_servers_map.rbegin();
        it != spdy_servers_map.rend(); ++it) {
     // Add the entry if it is not in the cache, otherwise move it to the front
     // of recency list.
     if (spdy_servers_map_.Get(it->first) == spdy_servers_map_.end())
       spdy_servers_map_.Put(it->first, it->second);
   }
 }
 
 void HttpServerPropertiesImpl::SetAlternativeServiceServers(
-    AlternativeServiceMap* alternative_service_map) {
+    std::unique_ptr<AlternativeServiceMap> alternative_service_map) {
   int32_t size_diff =
       alternative_service_map->size() - alternative_service_map_.size();
   if (size_diff > 0) {
     UMA_HISTOGRAM_COUNTS("Net.AlternativeServiceServers.MorePrefsEntries",
                          size_diff);
   } else {
     UMA_HISTOGRAM_COUNTS(
         "Net.AlternativeServiceServers.MoreOrEqualCacheEntries", -size_diff);
   }
 
-  AlternativeServiceMap new_alternative_service_map(
-      AlternativeServiceMap::NO_AUTO_EVICT);
   // Add the entries from persisted data.
-  for (AlternativeServiceMap::reverse_iterator input_it =
-           alternative_service_map->rbegin();
-       input_it != alternative_service_map->rend(); ++input_it) {
-    DCHECK(!input_it->second.empty());
-    new_alternative_service_map.Put(input_it->first, input_it->second);
-  }
-
-  alternative_service_map_.Swap(new_alternative_service_map);
+  alternative_service_map_.Swap(*alternative_service_map);

[Ryan Hamilton, 2017/06/20 18:25:56]

how about just:

alternative_service_map_ = std::move(alternative_service_map);

[wangyix1, 2017/06/21 18:45:32]

MRUCache doesn't allow copying. Furthermore, it's important that
alternative_service_map ends up with the contents of alternative_service_map_,
so it would be good to explicitly use Swap() here, even if the rvalue reference
copy operation might accomplish the same thing.

[Ryan Hamilton, 2017/06/22 02:45:52]

Even though it doesn't support copying, it *could* support move ala
std::unique_ptr (and other move-only classes). But alas it does not :) Thanks!

 
   // 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) {
+  for (auto input_it = alternative_service_map->rbegin();
+       input_it != 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. Canonical suffix only apply to HTTPS.
   const uint16_t kCanonicalPort = 443;
   const char* kCanonicalScheme = "https";
   for (const std::string& canonical_suffix : canonical_suffixes_) {
@@ skipping 14 matching lines @@
       if (base::EndsWith(it->first.host(), canonical_suffix,
                          base::CompareCase::INSENSITIVE_ASCII) &&
           it->first.scheme() == canonical_server.scheme()) {
         canonical_host_to_origin_map_[canonical_server] = it->first;
         break;
       }
     }
   }
 }
 
-void HttpServerPropertiesImpl::SetSupportsQuic(IPAddress* last_address) {
+void HttpServerPropertiesImpl::SetSupportsQuic(const IPAddress* last_address) {
   if (last_address)
     last_quic_address_ = *last_address;
 }
 
 void HttpServerPropertiesImpl::SetServerNetworkStats(
-    ServerNetworkStatsMap* server_network_stats_map) {
+    std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map) {
   // Add the entries from persisted data.
-  ServerNetworkStatsMap new_server_network_stats_map(
-      ServerNetworkStatsMap::NO_AUTO_EVICT);
+  server_network_stats_map_.Swap(*server_network_stats_map);

[Ryan Hamilton, 2017/06/20 18:25:56]

server_network_stats_map_ = std::move(*server_network_stats_map)

[wangyix1, 2017/06/21 18:45:33]

See comments for SetAlternativeServiceServers().

+
+  // Add the entries from the memory cache.
   for (ServerNetworkStatsMap::reverse_iterator it =
            server_network_stats_map->rbegin();
        it != server_network_stats_map->rend(); ++it) {
-    new_server_network_stats_map.Put(it->first, it->second);
-  }
-
-  server_network_stats_map_.Swap(new_server_network_stats_map);
-
-  // Add the entries from the memory cache.
-  for (ServerNetworkStatsMap::reverse_iterator it =
-           new_server_network_stats_map.rbegin();
-       it != new_server_network_stats_map.rend(); ++it) {
     if (server_network_stats_map_.Get(it->first) ==
         server_network_stats_map_.end()) {
       server_network_stats_map_.Put(it->first, it->second);
     }
   }
 }
 
 void HttpServerPropertiesImpl::SetQuicServerInfoMap(
-    QuicServerInfoMap* quic_server_info_map) {
+    std::unique_ptr<QuicServerInfoMap> quic_server_info_map) {
   // Add the entries from persisted data.
-  QuicServerInfoMap temp_map(QuicServerInfoMap::NO_AUTO_EVICT);
+  quic_server_info_map_.Swap(*quic_server_info_map);

[Ryan Hamilton, 2017/06/20 18:25:56]

quic_server_info_map_ = std::move(*quic_server_info_map);

[wangyix1, 2017/06/21 18:45:32]

See comments for SetAlternativeServiceServers().

+
+  // Add the entries from the memory cache.
   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);
-
-  // Add the entries from the memory cache.
-  for (QuicServerInfoMap::reverse_iterator it = temp_map.rbegin();
-       it != temp_map.rend(); ++it) {
     if (quic_server_info_map_.Get(it->first) == quic_server_info_map_.end()) {
       quic_server_info_map_.Put(it->first, it->second);
     }
   }
 }
 
-void HttpServerPropertiesImpl::GetSpdyServerList(
-    base::ListValue* spdy_server_list,
+void HttpServerPropertiesImpl::GetSpdyServers(
+    std::vector<std::string>* spdy_servers,
     size_t max_size) const {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  DCHECK(spdy_server_list);
-  spdy_server_list->Clear();
+  DCHECK(spdy_servers);
+  spdy_servers->clear();
   size_t count = 0;
   // Get the list of servers (scheme/host/port) that support SPDY.
   for (SpdyServersMap::const_iterator it = spdy_servers_map_.begin();
        it != spdy_servers_map_.end() && count < max_size; ++it) {
-    const std::string spdy_server = it->first;
     if (it->second) {
-      spdy_server_list->AppendString(spdy_server);
+      spdy_servers->push_back(it->first);
       ++count;
     }
   }
 }
 
 void HttpServerPropertiesImpl::Clear() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   spdy_servers_map_.Clear();
   alternative_service_map_.Clear();
   canonical_host_to_origin_map_.clear();
@@ skipping 487 matching lines @@
     if (map_it->second.empty()) {
       RemoveCanonicalHost(map_it->first);
       map_it = alternative_service_map_.Erase(map_it);
       continue;
     }
     ++map_it;
   }
 }
 
 }  // namespace net