Add getters and setters for BrokenAlternativeService's list of broken and recently-broken alt svcs

net/http/broken_alternative_services.h

 // Copyright (c) 2017 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.
 
 #ifndef NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_
 #define NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_
 
 #include <list>
 #include <unordered_map>
 
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 #include "net/http/http_server_properties.h"
 
 namespace base {
 class TickClock;
 }
 
 namespace net {
 
 struct AlternativeServiceHash {
   size_t operator()(const net::AlternativeService& entry) const {
     return entry.protocol ^ std::hash<std::string>()(entry.host) ^ entry.port;
   }
 };
 
+typedef std::list<std::pair<AlternativeService, base::TimeTicks>>
+    BrokenAlternativeServiceList;
+
+typedef std::unordered_map<AlternativeService,
+                           BrokenAlternativeServiceList::iterator,
+                           AlternativeServiceHash>
+    BrokenAlternativeServiceMap;

[Zhongyi Shi, 2017/06/02 18:19:44]

Hrm, if I remembered this correctly, the BrokenAlternativeServiceList is also
defined in the impl.h. Is there any difference? If not, how about move this to
http_server_properties.h?

[wangyix1, 2017/06/05 19:16:25]

Done.

I don't see it defined in http_server_properties_impl.h, but I agree it makes
sense to move the BrokenAlternativeServiceList typedef to
http_server_properties.h

[Zhongyi Shi, 2017/06/06 22:17:49]

Ah.. I was referring to your previous CL which defines the
AlternativeServiceHash, BrokenAlternativeServiceMap, and
BrokenAlternativeServiceList in net/http/broken_alternative_services.h.

Instead of define in multiple files, how about move them to the
http_server_properties.h?  

[wangyix1, 2017/06/09 19:26:41]

Hmm, I figured I would keep AlternativeServiceHash and
BrokenAlternativeServiceMap as private typedefs in BrokenAlternativeServices
since this map type is very specific to the implementation of
BrokenAlternativeServices: it maps alt-svcs to iterators in
broken_alternative_service_list_. I feel that exposing the
BrokenAlternativeServiceMap is unnecessary since it's only used for this very
specific thing. By extension, since the AlternativeServiceHash is only needed
for the BrokenAlternativeServiceMap, I kept that private too. What do you think?

+
 // This class tracks HTTP alternative services that have been marked as broken.
 // The brokenness of an alt-svc will expire after some time according to an
 // exponential back-off formula: each time an alt-svc is marked broken, the
 // expiration delay will be some constant multiple of its previous expiration
 // delay. This prevents broken alt-svcs from being retried too often by the
 // network stack.
 class NET_EXPORT_PRIVATE BrokenAlternativeServices {
  public:
   // Delegate to be used by owner so it can be notified when the brokenness of
   // an AlternativeService expires.
@@ skipping 41 matching lines @@
   // Returns true if MarkAlternativeServiceRecentlyBroken(alternative_service)
   // or MarkAlternativeServiceBroken(alternative_service) has been called and
   // ConfirmAlternativeService(alternative_service) has not been called
   // afterwards (even if brokenness of |alternative_service| has expired).
   bool WasAlternativeServiceRecentlyBroken(
       const AlternativeService& alternative_service);
 
   // Marks |alternative_service| as not broken and not recently broken.
   void ConfirmAlternativeService(const AlternativeService& alternative_service);
 
+  // Adds broken and recently broken alternative services.
+  // |broken_alternative_service_list| must be sorted from earliest to latest
+  // expiration time.
+  // All AlternativeServices in |broken_alternative_service_list| must exist in
+  // |recently_broken_alternative_services|.
+  void AddBrokenAndRecentlyBrokenAlternativeServices(

[Zhongyi Shi, 2017/06/02 18:19:44]

The return type of this method should be a boolean to indicate whether an update
should be scheduled to persist the data in disk. Do you plan to do that in the
follow-up CL?

[wangyix1, 2017/06/05 19:16:25]

Hmm I didn't think of that. I'm assuming it should return true if any
alternative services were added to |broken_...| or |recently_broken_...|?

[Zhongyi Shi, 2017/06/06 22:17:49]

If you go a step further, the return type will be used to determine whether
HttpServerPropertiesManager will need to schedule an update to disk. And that's
why the return type should be a boolean. But you haven't get to the persisting
logic, so it's fine to leave it to that CL. 

[wangyix1, 2017/06/09 19:26:41]

Acknowledged.

[wangyix1, 2017/06/10 01:07:17]

The current code will only schedule an update to disk after reading from disk if
corrupted data was detected while reading from disk. All the setters called in
HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence() do not
return a boolean. I'm not sure why my setter here needs to return a boolean?
This setter here (like those existing setters) will only be used to update the
cache using stuff read from the disk, so I don't see why this setter would cause
HttpServerPropertiesManager to write everything back to disk. The logic for
detecting corrupted prefs is done elsewhere (in
UpdateCacheFromPrefsOnPrefSequence()) and is the only thing that might cause a
disk update to be scheduled after reading from disk.

[wangyix1, 2017/06/12 22:28:45]

Discussed in person. Will rename this function to
SetBrokenAndRecentlyBrokenAlternativeServices() so it's consistent with the
other setters called in UpdateCacheFromPrefsOnNetworkSequence().

+      std::unique_ptr<BrokenAlternativeServiceList>
+          broken_alternative_service_list,
+      std::unique_ptr<RecentlyBrokenAlternativeServices>
+          recently_broken_alternative_services);
+
+  const BrokenAlternativeServiceList& broken_alternative_service_list() const;
+
+  const RecentlyBrokenAlternativeServices&
+  recently_broken_alternative_services() const;
+
  private:
   // TODO (wangyix): modify HttpServerPropertiesImpl unit tests so this
   // friendness is no longer required.
   friend class HttpServerPropertiesImplPeer;
 
-  // A pair containing a broken AlternativeService and the expiration time of
-  // its brokenness.
-  struct BrokenAltSvcExpireInfo {
-    BrokenAltSvcExpireInfo(const AlternativeService& alt_svc,
-                           base::TimeTicks expire)
-        : alternative_service(alt_svc), expiration(expire) {}
-
-    AlternativeService alternative_service;
-    base::TimeTicks expiration;
-  };
-
-  typedef std::list<BrokenAltSvcExpireInfo> BrokenAlternativeServiceList;
-
-  typedef std::unordered_map<AlternativeService,
-                             BrokenAlternativeServiceList::iterator,
-                             AlternativeServiceHash>
-      BrokenAlternativeServiceMap;
-
   // Inserts |alternative_service| and its |expiration| time into
   // |broken_alternative_service_list_| and |broken_alternative_service_map_|.
   // |it| is the position in |broken_alternative_service_list_| where it was
   // inserted.
   bool AddToBrokenAlternativeServiceListAndMap(
       const AlternativeService& alternative_service,
       base::TimeTicks expiration,
       BrokenAlternativeServiceList::iterator* it);
 
   void ExpireBrokenAlternateProtocolMappings();
@@ skipping 15 matching lines @@
   // Used for scheduling the task that expires the brokenness of alternative
   // services.
   base::OneShotTimer expiration_timer_;
 
   base::WeakPtrFactory<BrokenAlternativeServices> weak_ptr_factory_;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_