Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior

net/http/http_broken_alternative_services_manager.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_HTTP_BROKEN_ALTERNATIVE_SERVICES_MANAGER_H_
+#define NET_HTTP_HTTP_BROKEN_ALTERNATIVE_SERVICES_MANAGER_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 net {
+
+struct AlternativeServiceHash {
+  size_t operator()(const net::AlternativeService& entry) const {
+    return entry.protocol ^ std::hash<std::string>()(entry.host) ^ entry.port;
+  }
+};
+
+class NET_EXPORT HttpBrokenAlternativeServicesManager {

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: This class name is a bit of a mouthful, how 'bout just
"BrokenAlternativeServices".

Please add a simple class comment which describes what it does (you'll probably
want to mention expiration and backoff, for example).

I think this can be NET_EXPORT_PRIVATE instead of NET_EXPORT since I don't think
we'll have consumers outside of net/

[wangyix1, 2017/05/26 01:14:24]

Done.

+ public:
+  // Interface for retrieving the current time as base::TimeTicks
+  class NET_EXPORT Clock {
+   public:
+    virtual ~Clock() {}
+    virtual base::TimeTicks Now() const = 0;
+  };
+
+  class NET_EXPORT DefaultClock : public Clock {

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: You could consider adding static method to clock:

static Clock* GetDefaultClockInstance();

And then hide DefaultClock in the .cc file.

[wangyix1, 2017/05/26 01:14:24]

Done.

+   public:
+    static DefaultClock* GetInstance();
+    DefaultClock() {}
+    ~DefaultClock() override {}
+    base::TimeTicks Now() const override;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(DefaultClock);
+  };
+
+  // Delegate for owner to be notified when the brokenness of an
+  // AlternativeService expires.
+  class NET_EXPORT Delegate {
+   public:
+    virtual void OnExpireBrokenAlternativeService(
+        const AlternativeService& expired_alternative_service) = 0;

[Ryan Hamilton, 2017/05/25 03:03:50]

Should we also have virtual ~Delegate() {}?

[wangyix1, 2017/05/26 01:14:24]

Done.

+  };
+
+  HttpBrokenAlternativeServicesManager(Delegate* delegate, Clock* clock);

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: Comment, please, and note the ownership/lifetime requirements of both
arguments. (Namely, that they both must outlive this.

[wangyix1, 2017/05/26 01:14:24]

Done.

+
+  ~HttpBrokenAlternativeServicesManager();
+
+  void MarkAlternativeServiceBroken(

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: comments for all these methods, please.

[wangyix1, 2017/05/26 01:14:24]

Done.

+      const AlternativeService& alternative_service);
+  void MarkAlternativeServiceRecentlyBroken(
+      const AlternativeService& alternative_service);
+  bool IsAlternativeServiceBroken(
+      const AlternativeService& alternative_service) const;
+  bool WasAlternativeServiceRecentlyBroken(
+      const AlternativeService& alternative_service);
+  void ConfirmAlternativeService(const AlternativeService& alternative_service);
+
+  static base::TimeDelta ComputeBrokenAltSvcExpirationDelayForTest(
+      int broken_count);

[Ryan Hamilton, 2017/05/25 03:03:50]

Can we just put this in a test.cc file instead?

[wangyix1, 2017/05/26 01:14:24]

Done.

+
+ private:
+  friend class HttpServerPropertiesImplPeer;

[Ryan Hamilton, 2017/05/25 03:03:50]

We should see if we can get rid of this friend-ness in a follow up. Perhaps add
a TODO.

[wangyix1, 2017/05/26 01:14:24]

Done.

+
+  struct BrokenAltSvcExpireInfo {

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: comment.

[wangyix1, 2017/05/26 01:14:24]

Done.

+    BrokenAltSvcExpireInfo(const AlternativeService& alt_svc,
+                           base::TimeTicks expire)
+        : alternative_service(alt_svc), expiration(expire) {}
+    AlternativeService alternative_service;

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: newline before

[wangyix1, 2017/05/26 01:14:24]

Done.

+    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();
+  void ScheduleBrokenAlternateProtocolMappingsExpiration();
+
+  Delegate* delegate_;

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: // Unowned.

[wangyix1, 2017/05/26 01:14:24]

Done.

+
+  BrokenAlternativeServiceMap broken_alternative_service_map_;
+  BrokenAlternativeServiceList broken_alternative_service_list_;
+  RecentlyBrokenAlternativeServices recently_broken_alternative_services_;
+
+  Clock* clock_;  // Unowned
+
+  // Used for scheduling the task that expires the brokenness of alternative
+  // services.
+  base::OneShotTimer timer_;

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: how 'bout expiration_timer_?

[wangyix1, 2017/05/26 01:14:24]

Done.

+
+  base::WeakPtrFactory<HttpBrokenAlternativeServicesManager> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(HttpBrokenAlternativeServicesManager);

[Ryan Hamilton, 2017/05/25 03:03:50]

nit: I think the new hotness is using = delete for the copy constructor and
assignment operator.

[wangyix1, 2017/05/26 01:14:24]

Done.

+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_HTTP_BROKEN_ALTERNATIVE_SERVICES_MANAGER_H_