Implement throttling logic for fetching affiliation information.

components/password_manager/core/browser/affiliation_fetch_throttler.h

+// Copyright 2014 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 COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_AFFILIATION_FETCH_THROTTLER_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_AFFILIATION_FETCH_THROTTLER_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/backoff_entry.h"

[mmenke, 2015/01/14 19:25:07]

Think we can forward declare this?

[engedy, 2015/01/15 19:16:42]

Done.

+#include "net/base/network_change_notifier.h"
+
+namespace base {
+class TickClock;
+class SingleThreadTaskRunner;
+}
+
+namespace password_manager {
+
+// A delegate to receive a callback once it is okay to issue a network request.

[mmenke, 2015/01/14 19:25:07]

This needs clearer documentation, since it's the first thing one sees when
looking at this file.

[engedy, 2015/01/15 19:16:42]

I have moved the delegate to its own file.

+class AffiliationFetchThrottlerDelegate {

[mmenke, 2015/01/14 19:25:07]

option:  Believe it's currently recommended these go in their own file

[engedy, 2015/01/15 19:16:42]

Done.

+ public:
+  // Will be called once the throttling policy allows issuing a network request,
+  // provided there has been at least one SignalNetworkRequestNeeded() call
+  // since the last request. The implementor should return whether a request was

[mmenke, 2015/01/14 19:25:07]

nit:  implementor -> implemention

Should avoid ambiguity, and implementor can also be mean the person who writes
the implementation.

[engedy, 2015/01/15 19:16:42]

Done.

+  // actually initiated, and if so, should call InformOfNetworkRequestComplete()

[mmenke, 2015/01/14 19:25:07]

nit:  should -> must (Maybe I've been reading too many RFCs)

[engedy, 2015/01/15 19:16:42]

Done.

+  // when the request completes.
+  virtual bool OnCanSendNetworkRequest() = 0;
+
+ protected:
+  virtual ~AffiliationFetchThrottlerDelegate() {}
+};
+
+// Implements the throttling logic that AffiliationBackend will use when it
+// needs to issue requests to fetch affiliation information over the network.
+//
+// Essentially, the logic implements exponential backoff on network and server
+// errors, plus that no requests will be issued in the first place while there
+// is known to be no network connectivity.
+class AffiliationFetchThrottler
+    : public net::NetworkChangeNotifier::ConnectionTypeObserver {

[mmenke, 2015/01/14 19:25:07]

I wonder about whether we really should be using NCN - it's not the most
reliable signal in the world, so this code should also work if it's incorrectly
claiming we're online.  Using it can be an optimization (If we're offline, no
need to do exponential backoff, and could save some effort as well), but should
work without it.

[engedy, 2015/01/15 19:16:42]

Yes, even as a signal that is known to produce false-positives, it would be more
than merely an optimization.

My biggest worry: if the user goes offline for some hours, then, by the time
connectivity is reestablished, the backoff delay will be so large that the first
network request will not be issued until hours after the offline-online
transition. Cached data will likely expire, so dependent functionality will not
work in this time window.

+ public:
+  // Creates an instance that will use |tick_clock| as its tick source, and will
+  // post to |task_runner| to call the |delegate|'s OnSendNetworkRequest().
+  AffiliationFetchThrottler(
+      AffiliationFetchThrottlerDelegate* delegate,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_ptr<base::TickClock> tick_clock);
+  ~AffiliationFetchThrottler() override;
+
+  // Signals to the throttling logic that a network request is needed, and that
+  // OnCanSendNetworkRequest() should be called as soon as the request can be
+  // sent. Calls to this method will be ignored while a request is in flight.
+  void SignalNetworkRequestNeeded();
+
+  // Informs the back-off logic that a network request has been completed, with
+  // either |success| or not.
+  void InformOfNetworkRequestComplete(bool success);
+
+ protected:
+  enum State {
+    IDLE,
+    FETCH_NEEDED,
+    FETCH_IN_FLIGHT
+  };
+
+  // Ensures that OnBackoffDelayExpiredCallback() is scheduled to be called back
+  // once the |exponential_backoff_| delay expires.
+  void EnsureCallbackIsScheduled();
+
+  // Called back when the |exponential_backoff_| delay expires.
+  void OnBackoffDelayExpiredCallback();
+
+  // net::NetworkChangeNotifier::ConnectionTypeObserver:
+  virtual void OnConnectionTypeChanged(

[mmenke, 2015/01/14 19:25:07]

nit: --virtual

[engedy, 2015/01/15 19:16:42]

Done.

+      net::NetworkChangeNotifier::ConnectionType type) override;
+
+ private:
+  AffiliationFetchThrottlerDelegate* delegate_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  State state_;
+  bool has_network_connectivity_;
+  bool is_fetch_scheduled_;
+  scoped_ptr<base::TickClock> tick_clock_;
+  scoped_ptr<net::BackoffEntry> exponential_backoff_;

[mmenke, 2015/01/14 19:25:07]

Need to include scoped_ptr.h

[engedy, 2015/01/15 19:16:42]

Done.

+  base::WeakPtrFactory<AffiliationFetchThrottler> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AffiliationFetchThrottler);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_AFFILIATION_FETCH_THROTTLER_H_