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

net/http/broken_alternative_services.h

Index: net/http/broken_alternative_services.h
diff --git a/net/http/broken_alternative_services.h b/net/http/broken_alternative_services.h
index 6a51222ee3fba09bd3b6bdfa3a889f79251d2b62..7f50e9cd3e865850c6875d900576625f0f3eb01e 100644
--- a/net/http/broken_alternative_services.h
+++ b/net/http/broken_alternative_services.h
@@ -24,6 +24,14 @@ struct AlternativeServiceHash {
   }
 };
 
+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
@@ -85,29 +93,27 @@ class NET_EXPORT_PRIVATE BrokenAlternativeServices {
   // 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