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

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 4140e1d6e35ae5dd9ed1ad236a66779aee6d4fcf..f88476840c8487c022b0d562b6b2aef7aed99d5c 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -45,7 +45,7 @@ HttpServerPropertiesImpl::~HttpServerPropertiesImpl() {
 }
 
 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)
@@ -53,8 +53,7 @@ void HttpServerPropertiesImpl::SetSpdyServers(
 
   // 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);
   }
 
@@ -72,7 +71,7 @@ void HttpServerPropertiesImpl::SetSpdyServers(
 }
 
 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) {
@@ -83,22 +82,12 @@ void HttpServerPropertiesImpl::SetAlternativeServiceServers(
         "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);
@@ -133,28 +122,20 @@ void HttpServerPropertiesImpl::SetAlternativeServiceServers(
   }
 }
 
-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);
-  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);
+  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 =
-           new_server_network_stats_map.rbegin();
-       it != new_server_network_stats_map.rend(); ++it) {
+           server_network_stats_map->rbegin();
+       it != 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);
@@ -163,38 +144,31 @@ void HttpServerPropertiesImpl::SetServerNetworkStats(
 }
 
 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);
-  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);
+  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 = temp_map.rbegin();
-       it != temp_map.rend(); ++it) {
+  for (QuicServerInfoMap::reverse_iterator it = quic_server_info_map->rbegin();
+       it != quic_server_info_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;
     }
   }