Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949513005:
Update param types of HttpServerPropertiesImpl setters and getters. Fix MRU order when loading (Closed)
DescriptionUpdate param types of HttpServerPropertiesImpl setters and getters
Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying.
Fix loading of alternative service map from prefs so it's MRU order instead of reverse MRU.
Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers().
Review-Url: https://codereview.chromium.org/2949513005
Cr-Commit-Position: refs/heads/master@{#482420}
Committed: https://chromium.googlesource.com/chromium/src/+/064e7bf9724307e7d07f0b8852de3a4bee4eb6f6
Patch Set 1 #Patch Set 2 : Fixed minor typos #
Total comments: 29
Patch Set 3 : Changed more HttpServerPropertiesManager and Impl param types #Patch Set 4 : Fixed rch and zhongyi's nits from ps2 #Patch Set 5 : Change SetSpdyServers() to take SpdyServersMap instead of vector<string>, bool #Patch Set 6 : Added missing newline #
Total comments: 9
Patch Set 7 : Fixed rch's nits from ps6 #Patch Set 8 : Rebase #Patch Set 9 : Forgot some if() guards to avoid nullptr dereference #Patch Set 10 : Updated HttpServerPropertiesManagerTest.SingleUpdateForTwoSpdyServerPrefChanges to reflect new load… #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 37 (23 generated)
Description was changed from ========== git cl format Updated HttpServerPropertiesImpl setters to use unique_ptrs to reduce copying in HttpServerPropertiesManager Made SetSpdyServers() vector param a const ptr Changed HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(); returns vector instead of ListValue BUG= ========== to ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). It now returns vector<string> instead of ListValue of strings. ==========
Description was changed from ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). It now returns vector<string> instead of ListValue of strings. ========== to ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers(). ==========
wangyix@chromium.org changed reviewers: + rch@chromium.org, zhongyi@chromium.org
PTAL
Thanks for doing this! https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:48: const std::vector<std::string>* spdy_servers, I wonder if this should be a unique_ptr<SpdyServersMap> instead? https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:86: alternative_service_map_.Swap(*alternative_service_map); how about just: alternative_service_map_ = std::move(alternative_service_map); https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:133: server_network_stats_map_.Swap(*server_network_stats_map); server_network_stats_map_ = std::move(*server_network_stats_map) https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:149: quic_server_info_map_.Swap(*quic_server_info_map); quic_server_info_map_ = std::move(*quic_server_info_map); https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:43: void SetSpdyServers(const std::vector<std::string>* spdy_servers, Can this be a const&? https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:49: void SetSupportsQuic(const IPAddress* last_address); ditto https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.cc:844: spdy_servers.get(), kMaxSupportsSpdyServerHostsToPersist); it's a bit odd that GetSpdyServers takes a vector<string> whereas the type of this is ServerList. I'd recommend making them both the same. Maybe vector<string> is easy enough to use everywhere. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:237: void UpdatePrefsOnPrefThread(std::unique_ptr<ServerList> spdy_servers, Instead of passing a unique_ptr here (where all the other args are raw pointers) and a raw pointer in UpdateCacheFromPrefsOnNetworkSequence (where all the other args are unique_ptrs) would it make sense to flip them?
Thanks for clean this up! https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:56: for (auto it = spdy_servers->rbegin(); it != spdy_servers->rend(); ++it) { nit: in general, we prefer to specify the explicit type of the iterator so that the code is more readable. "auto" is cleaner though... https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl_unittest.cc:157: std::string string_value_g; nit: remove string_value_g https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl_unittest.cc:278: std::string string_value_m; remove unused string_value_* https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:211: const ServerList* spdy_servers, Could this be std::unique_ptr<ServerList>? https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:237: void UpdatePrefsOnPrefThread(std::unique_ptr<ServerList> spdy_servers, On 2017/06/20 18:25:56, Ryan Hamilton wrote: > Instead of passing a unique_ptr here (where all the other args are raw pointers) > and a raw pointer in UpdateCacheFromPrefsOnNetworkSequence (where all the other > args are unique_ptrs) would it make sense to flip them? +1, right now it's a mix of the two. It will be much cleaner to make it consistent.
Patchset #6 (id:100001) has been deleted
PTAL. Per rch's comment, HttpServerPropertiesImpl::SetSpdyServers() has been changed to take a SpdyServersMap instead of a vector<string> and a bool. This requires some minor changes in HttpServerPropertiesManager, but should be okay. This change makes it more consistent with the other setters and reduces copying. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:48: const std::vector<std::string>* spdy_servers, On 2017/06/20 18:25:56, Ryan Hamilton wrote: > I wonder if this should be a unique_ptr<SpdyServersMap> instead? 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. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:56: for (auto it = spdy_servers->rbegin(); it != spdy_servers->rend(); ++it) { On 2017/06/20 22:06:18, Zhongyi Shi wrote: > nit: in general, we prefer to specify the explicit type of the iterator so that > the code is more readable. "auto" is cleaner though... Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:86: alternative_service_map_.Swap(*alternative_service_map); On 2017/06/20 18:25:56, Ryan Hamilton wrote: > how about just: > > alternative_service_map_ = std::move(alternative_service_map); 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. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:133: server_network_stats_map_.Swap(*server_network_stats_map); On 2017/06/20 18:25:56, Ryan Hamilton wrote: > server_network_stats_map_ = std::move(*server_network_stats_map) See comments for SetAlternativeServiceServers(). https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:149: quic_server_info_map_.Swap(*quic_server_info_map); On 2017/06/20 18:25:56, Ryan Hamilton wrote: > quic_server_info_map_ = std::move(*quic_server_info_map); See comments for SetAlternativeServiceServers(). https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:43: void SetSpdyServers(const std::vector<std::string>* spdy_servers, On 2017/06/20 18:25:56, Ryan Hamilton wrote: > Can this be a const&? The unit-test SpdyServerPropertiesTest.Set tests this function with spdy_servers=nullptr. Other than that, there's nothing stopping it from being const&. I think that part of the unit-test can be removed as it doesn't seem too important. https://cs.chromium.org/chromium/src/net/http/http_server_properties_impl_uni... Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:49: void SetSupportsQuic(const IPAddress* last_address); On 2017/06/20 18:25:56, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl_unittest.cc:157: std::string string_value_g; On 2017/06/20 22:06:19, Zhongyi Shi wrote: > nit: remove string_value_g Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl_unittest.cc:278: std::string string_value_m; On 2017/06/20 22:06:18, Zhongyi Shi wrote: > remove unused string_value_* Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.cc:844: spdy_servers.get(), kMaxSupportsSpdyServerHostsToPersist); On 2017/06/20 18:25:56, Ryan Hamilton wrote: > it's a bit odd that GetSpdyServers takes a vector<string> whereas the type of > this is ServerList. I'd recommend making them both the same. Maybe > vector<string> is easy enough to use everywhere. Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:211: const ServerList* spdy_servers, On 2017/06/20 22:06:19, Zhongyi Shi wrote: > Could this be std::unique_ptr<ServerList>? Done. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:237: void UpdatePrefsOnPrefThread(std::unique_ptr<ServerList> spdy_servers, On 2017/06/20 18:25:56, Ryan Hamilton wrote: > Instead of passing a unique_ptr here (where all the other args are raw pointers) > and a raw pointer in UpdateCacheFromPrefsOnNetworkSequence (where all the other > args are unique_ptrs) would it make sense to flip them? Ah, I think I just forgot to update these. For UpdateCacheFromPrefsOnNetworkSequence() and UpdatePrefsOnPrefThread(), I'll make the first 5 args all unique_ptr since ownership is being transferred to them after the thread hop. Done.
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sweet. https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:86: alternative_service_map_.Swap(*alternative_service_map); On 2017/06/21 18:45:32, wangyix1 wrote: > On 2017/06/20 18:25:56, Ryan Hamilton wrote: > > how about just: > > > > alternative_service_map_ = std::move(alternative_service_map); > > 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. 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! https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:43: void SetSpdyServers(const std::vector<std::string>* spdy_servers, On 2017/06/21 18:45:33, wangyix1 wrote: > On 2017/06/20 18:25:56, Ryan Hamilton wrote: > > Can this be a const&? > > The unit-test SpdyServerPropertiesTest.Set tests this function with > spdy_servers=nullptr. Other than that, there's nothing stopping it from being > const&. I think that part of the unit-test can be removed as it doesn't seem too > important. > > https://cs.chromium.org/chromium/src/net/http/http_server_properties_impl_uni... > > Done. > This is now a unique_ptr not a const&. Is that to avoid a copy? If so, SGTM. I notice that |supports_spdy| is also removed. Is that because it was unused before? https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.h:237: void UpdatePrefsOnPrefThread(std::unique_ptr<ServerList> spdy_servers, On 2017/06/21 18:45:33, wangyix1 wrote: > On 2017/06/20 18:25:56, Ryan Hamilton wrote: > > Instead of passing a unique_ptr here (where all the other args are raw > pointers) > > and a raw pointer in UpdateCacheFromPrefsOnNetworkSequence (where all the > other > > args are unique_ptrs) would it make sense to flip them? > > Ah, I think I just forgot to update these. For > UpdateCacheFromPrefsOnNetworkSequence() and UpdatePrefsOnPrefThread(), I'll make > the first 5 args all unique_ptr since ownership is being transferred to them > after the thread hop. > > Done. Sweet! Much cleaner! https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties.h (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties.h:203: // |spdy_servers_map_| has flattened representation of servers I suspect this is cut-paste, but I don't think |spdy_servers_map_| refers to something in this class. Perhaps rephrase? https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:499: it != servers_list->end(); ++it) {*/ Remove? https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:501: it != servers_list->begin();) { This appears to iterate in a different order than before. Does that represent a behavior change?
PTAL https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2949513005/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.h:43: void SetSpdyServers(const std::vector<std::string>* spdy_servers, On 2017/06/22 02:45:52, Ryan Hamilton wrote: > On 2017/06/21 18:45:33, wangyix1 wrote: > > On 2017/06/20 18:25:56, Ryan Hamilton wrote: > > > Can this be a const&? > > > > The unit-test SpdyServerPropertiesTest.Set tests this function with > > spdy_servers=nullptr. Other than that, there's nothing stopping it from being > > const&. I think that part of the unit-test can be removed as it doesn't seem > too > > important. > > > > > https://cs.chromium.org/chromium/src/net/http/http_server_properties_impl_uni... > > > > Done. > > > > This is now a unique_ptr not a const&. Is that to avoid a copy? If so, SGTM. > > I notice that |supports_spdy| is also removed. Is that because it was unused > before? Yes, it's now a unique_ptr<SpdyServersMap> instead of a vector<string> and a bool. This avoids a copy and makes it more consistent with the other HttpServerPropertiesImpl setters that use MRUCache params. However, this change required some other changes to be made in HttpServerPropertiesManager, specifically, the line you commented on where a ListValue is now iterated backwards instead of forwards. I'll reply to that comment to explain more. |supports_spdy| is removed since that information is included SpdyServersMap (which is MRUCache<string, bool>). This does change the API a little: instead of having to set every server in the vector to support or not support spdy, it can now set some servers in the map to support spdy and others to not support spdy. https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties.h (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties.h:203: // |spdy_servers_map_| has flattened representation of servers On 2017/06/22 02:45:52, Ryan Hamilton wrote: > I suspect this is cut-paste, but I don't think |spdy_servers_map_| refers to > something in this class. Perhaps rephrase? Done. https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:499: it != servers_list->end(); ++it) {*/ On 2017/06/22 02:45:52, Ryan Hamilton wrote: > Remove? Done. https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:501: it != servers_list->begin();) { On 2017/06/22 02:45:52, Ryan Hamilton wrote: > This appears to iterate in a different order than before. Does that represent a > behavior change? No behavior change. This change is a result of Impl::SetSpdyServers() taking a SpdyServersMap directly instead of a vector of servers. Before, the code did this: * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated forwards, AddServersData() called on each entry. * AddServersData() appends entry to |spdy_servers|, which is a vector<string> * |spdy_servers| is eventually passed to Impl::SetSpdyServers(). * Impl::SetSpdyServers() internally iterates |spdy_servers| backwards and adds each entry to a SpdyServersMap Now, the code does this: * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated backwards, AddServersData() called on each entry. * AddServersData() adds entry to |spdy_servers_map|, which is an MRUCache. * |spdy_servers_map| is eventually passed to Impl::SetSpdyServers() In summary, the step where a list of servers is iterated backwards to convert it into an MRUCache of servers has moved from Impl::SetSpdyServers() to Manager::UpdateCacheFromPrefsOnPrefSequence()
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm (any idea why the bots are red) https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:501: it != servers_list->begin();) { On 2017/06/22 17:45:09, wangyix1 wrote: > On 2017/06/22 02:45:52, Ryan Hamilton wrote: > > This appears to iterate in a different order than before. Does that represent > a > > behavior change? > > No behavior change. This change is a result of Impl::SetSpdyServers() taking a > SpdyServersMap directly instead of a vector of servers. Before, the code did > this: > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated forwards, > AddServersData() called on each entry. > > * AddServersData() appends entry to |spdy_servers|, which is a vector<string> > > * |spdy_servers| is eventually passed to Impl::SetSpdyServers(). > > * Impl::SetSpdyServers() internally iterates |spdy_servers| backwards and adds > each entry to a SpdyServersMap > > > Now, the code does this: > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated backwards, > AddServersData() called on each entry. > > * AddServersData() adds entry to |spdy_servers_map|, which is an MRUCache. > > * |spdy_servers_map| is eventually passed to Impl::SetSpdyServers() > > > > In summary, the step where a list of servers is iterated backwards to convert it > into an MRUCache of servers has moved from Impl::SetSpdyServers() to > Manager::UpdateCacheFromPrefsOnPrefSequence() > > Ah! Thanks for the great summary.
Actually don't take a look yet, found some issues. https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:501: it != servers_list->begin();) { On 2017/06/22 17:45:09, wangyix1 wrote: > On 2017/06/22 02:45:52, Ryan Hamilton wrote: > > This appears to iterate in a different order than before. Does that represent > a > > behavior change? > > No behavior change. This change is a result of Impl::SetSpdyServers() taking a > SpdyServersMap directly instead of a vector of servers. Before, the code did > this: > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated forwards, > AddServersData() called on each entry. > > * AddServersData() appends entry to |spdy_servers|, which is a vector<string> > > * |spdy_servers| is eventually passed to Impl::SetSpdyServers(). > > * Impl::SetSpdyServers() internally iterates |spdy_servers| backwards and adds > each entry to a SpdyServersMap > > > Now, the code does this: > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated backwards, > AddServersData() called on each entry. > > * AddServersData() adds entry to |spdy_servers_map|, which is an MRUCache. > > * |spdy_servers_map| is eventually passed to Impl::SetSpdyServers() > > > > In summary, the step where a list of servers is iterated backwards to convert it > into an MRUCache of servers has moved from Impl::SetSpdyServers() to > Manager::UpdateCacheFromPrefsOnPrefSequence() > > I take that back, this does have a behavior change. AddServersData() adds stuff to other maps too. Let me look into this first.
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers(). ========== to ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Fix loading of alternative service map from prefs so it's MRU order instead of reverse MRU. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers(). ==========
alternative service map is now loaded from prefs in MRU order instead of reverse MRU order. PTAL https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2949513005/diff/120001/net/http/http_server_p... net/http/http_server_properties_manager.cc:501: it != servers_list->begin();) { On 2017/06/22 22:11:06, wangyix1 wrote: > On 2017/06/22 17:45:09, wangyix1 wrote: > > On 2017/06/22 02:45:52, Ryan Hamilton wrote: > > > This appears to iterate in a different order than before. Does that > represent > > a > > > behavior change? > > > > No behavior change. This change is a result of Impl::SetSpdyServers() taking a > > SpdyServersMap directly instead of a vector of servers. Before, the code did > > this: > > > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated forwards, > > AddServersData() called on each entry. > > > > * AddServersData() appends entry to |spdy_servers|, which is a vector<string> > > > > * |spdy_servers| is eventually passed to Impl::SetSpdyServers(). > > > > * Impl::SetSpdyServers() internally iterates |spdy_servers| backwards and adds > > each entry to a SpdyServersMap > > > > > > Now, the code does this: > > > > * In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated backwards, > > AddServersData() called on each entry. > > > > * AddServersData() adds entry to |spdy_servers_map|, which is an MRUCache. > > > > * |spdy_servers_map| is eventually passed to Impl::SetSpdyServers() > > > > > > > > In summary, the step where a list of servers is iterated backwards to convert > it > > into an MRUCache of servers has moved from Impl::SetSpdyServers() to > > Manager::UpdateCacheFromPrefsOnPrefSequence() > > > > > > I take that back, this does have a behavior change. AddServersData() adds stuff > to other maps too. Let me look into this first. Discussed with rch. This behavior change actually fixes a bug where the alternative service map when loaded from prefs is loaded with reverse MRU order.
LGTM! https://codereview.chromium.org/2949513005/diff/200001/net/http/http_server_p... File net/http/http_server_properties_manager_unittest.cc (left): https://codereview.chromium.org/2949513005/diff/200001/net/http/http_server_p... net/http/http_server_properties_manager_unittest.cc:429: EXPECT_EQ(1234, map_it->second[1].alternative_service().port); Urm, old test indicated that the order has been changed from version 4+. Well, good catch.
The CQ bit was checked by wangyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/2949513005/#ps200001 (title: "Updated HttpServerPropertiesManagerTest.SingleUpdateForTwoSpdyServerPrefChanges to reflect new load…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wangyix@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498509438342300,
"parent_rev": "09fba90138a1c3197d0568fec12ceb75217aedf3", "commit_rev":
"064e7bf9724307e7d07f0b8852de3a4bee4eb6f6"}
Message was sent while issue was closed.
Description was changed from ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Fix loading of alternative service map from prefs so it's MRU order instead of reverse MRU. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers(). ========== to ========== Update param types of HttpServerPropertiesImpl setters and getters Use unique_ptrs in params of HttpServerPropertiesImpl functions SetAlternativeServiceServers(), SetServerNetworkStats(), and SetQuicServerInfoMap() to reduce copying. Fix loading of alternative service map from prefs so it's MRU order instead of reverse MRU. Rename HttpServerPropertiesImpl::GetSpdyServerList() to GetSpdyServers(). Change it to return vector<string> instead of ListValue of strings. These changes make it consistent with SetSpdyServers(). Review-Url: https://codereview.chromium.org/2949513005 Cr-Commit-Position: refs/heads/master@{#482420} Committed: https://chromium.googlesource.com/chromium/src/+/064e7bf9724307e7d07f0b8852de... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/064e7bf9724307e7d07f0b8852de... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
