Integrate throttling logic into AffiliationBackend.

This CL also changes ownership of the TickClock object used by the AffiliationFetchThrottler, so that it is now owned by the AffiliationBackend.

Furthermore, this CL makes the AffiliationBackend no longer rely on using ThreadTaskRunnerHandle to schedule asynchronous operations, but instead take a SingleThreadTaskRunner to use in its constructor. This allows delayed tasks (which are now present) to be posted to the correct task runner in tests too (namely, in AffiliationServiceTests).

BUG=437865

Review URL: https://codereview.chromium.org/1008373003

Cr-Commit-Position: refs/heads/master@{#320887}

components/password_manager/core/browser/affiliation_backend.cc

@@ -9,12 +9,12 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
-#include "base/task_runner.h"
-#include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/clock.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/password_manager/core/browser/affiliation_database.h"
+#include "components/password_manager/core/browser/affiliation_fetch_throttler.h"
 #include "components/password_manager/core/browser/affiliation_fetcher.h"
 #include "components/password_manager/core/browser/facet_manager.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -23,9 +23,13 @@ namespace password_manager {
 
 AffiliationBackend::AffiliationBackend(
     const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
-    scoped_ptr<base::Clock> time_source)
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    scoped_ptr<base::Clock> time_source,
+    scoped_ptr<base::TickClock> time_tick_source)
     : request_context_getter_(request_context_getter),
+      task_runner_(task_runner),
       clock_(time_source.Pass()),
+      tick_clock_(time_tick_source.Pass()),
       weak_ptr_factory_(this) {
   DCHECK_LT(base::Time(), clock_->Now());
 }
@@ -35,6 +39,11 @@ AffiliationBackend::~AffiliationBackend() {
 
 void AffiliationBackend::Initialize(const base::FilePath& db_path) {
   thread_checker_.reset(new base::ThreadChecker);
+
+  DCHECK(!throttler_);
+  throttler_.reset(
+      new AffiliationFetchThrottler(this, task_runner_, tick_clock_.get()));
+
   cache_.reset(new AffiliationDatabase());
   if (!cache_->Init(db_path)) {
     // TODO(engedy): Implement this. crbug.com/437865.
@@ -99,20 +108,6 @@ FacetManager* AffiliationBackend::GetOrCreateFacetManager(
   return facet_managers_.get(facet_uri);
 }
 
-void AffiliationBackend::SendNetworkRequest() {
-  DCHECK(!fetcher_);
-
-  std::vector<FacetURI> requested_facet_uris;
-  for (const auto& facet_manager_pair : facet_managers_) {
-    if (facet_manager_pair.second->DoesRequireFetch())
-      requested_facet_uris.push_back(facet_manager_pair.first);
-  }
-  DCHECK(!requested_facet_uris.empty());
-  fetcher_.reset(AffiliationFetcher::Create(request_context_getter_.get(),
-                                            requested_facet_uris, this));
-  fetcher_->StartRequest();
-}
-
 void AffiliationBackend::OnSendNotification(const FacetURI& facet_uri) {
   DCHECK(thread_checker_ && thread_checker_->CalledOnValidThread());
 
@@ -132,17 +127,14 @@ bool AffiliationBackend::ReadAffiliationsFromDatabase(
 }
 
 void AffiliationBackend::SignalNeedNetworkRequest() {
-  // TODO(engedy): Add more sophisticated throttling logic. crbug.com/437865.
-  if (fetcher_)
-    return;
-  SendNetworkRequest();
+  throttler_->SignalNetworkRequestNeeded();
 }
 
 void AffiliationBackend::RequestNotificationAtTime(const FacetURI& facet_uri,
                                                    base::Time time) {
   // TODO(engedy): Avoid spamming the task runner; only ever schedule the first
   // callback. crbug.com/437865.
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+  task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&AffiliationBackend::OnSendNotification,
                             weak_ptr_factory_.GetWeakPtr(), facet_uri),
       time - clock_->Now());
@@ -151,7 +143,9 @@ void AffiliationBackend::RequestNotificationAtTime(const FacetURI& facet_uri,
 void AffiliationBackend::OnFetchSucceeded(
     scoped_ptr<AffiliationFetcherDelegate::Result> result) {
   DCHECK(thread_checker_ && thread_checker_->CalledOnValidThread());
+
   fetcher_.reset();
+  throttler_->InformOfNetworkRequestComplete(true);
 
   for (const AffiliatedFacets& affiliated_facets : *result) {
     AffiliatedFacetsWithUpdateTime affiliation;
@@ -182,7 +176,7 @@ void AffiliationBackend::OnFetchSucceeded(
   // requests came in while the current fetch was in flight.
   for (const auto& facet_manager_pair : facet_managers_) {
     if (facet_manager_pair.second->DoesRequireFetch()) {
-      SendNetworkRequest();
+      throttler_->SignalNetworkRequestNeeded();
       return;
     }
   }
@@ -191,15 +185,46 @@ void AffiliationBackend::OnFetchSucceeded(
 void AffiliationBackend::OnFetchFailed() {
   DCHECK(thread_checker_ && thread_checker_->CalledOnValidThread());
 
-  // TODO(engedy): Implement this. crbug.com/437865.
-  NOTIMPLEMENTED();
+  fetcher_.reset();
+  throttler_->InformOfNetworkRequestComplete(false);
+
+  // Trigger a retry if a fetch is still needed.
+  for (const auto& facet_manager_pair : facet_managers_) {
+    if (facet_manager_pair.second->DoesRequireFetch()) {
+      throttler_->SignalNetworkRequestNeeded();
+      return;
+    }
+  }
 }
 
 void AffiliationBackend::OnMalformedResponse() {
   DCHECK(thread_checker_ && thread_checker_->CalledOnValidThread());
 
-  // TODO(engedy): Implement this. crbug.com/437865.
-  NOTIMPLEMENTED();
+  // TODO(engedy): Potentially handle this case differently. crbug.com/437865.
+  OnFetchFailed();
+}
+
+bool AffiliationBackend::OnCanSendNetworkRequest() {
+  DCHECK(!fetcher_);
+  std::vector<FacetURI> requested_facet_uris;
+  for (const auto& facet_manager_pair : facet_managers_) {
+    if (facet_manager_pair.second->DoesRequireFetch())
+      requested_facet_uris.push_back(facet_manager_pair.first);
+  }
+
+  // In case a request is no longer needed, return false to indicate this.
+  if (requested_facet_uris.empty())
+    return false;
+
+  fetcher_.reset(AffiliationFetcher::Create(request_context_getter_.get(),
+                                            requested_facet_uris, this));
+  fetcher_->StartRequest();
+  return true;
+}
+
+void AffiliationBackend::SetThrottlerForTesting(
+    scoped_ptr<AffiliationFetchThrottler> throttler) {
+  throttler_ = throttler.Pass();
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/affiliation_backend.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h"
 #include "components/password_manager/core/browser/affiliation_fetcher_delegate.h"
 #include "components/password_manager/core/browser/affiliation_service.h"
 #include "components/password_manager/core/browser/affiliation_utils.h"
@@ -20,7 +21,9 @@
 namespace base {
 class Clock;
 class FilePath;
+class SingleThreadTaskRunner;
 class ThreadChecker;
+class TickClock;
 class Time;
 }  // namespace base
 
@@ -32,6 +35,7 @@ namespace password_manager {
 
 class AffiliationDatabase;
 class AffiliationFetcher;
+class AffiliationFetchThrottler;
 class FacetManager;
 
 // The AffiliationBackend is the part of the AffiliationService that lives on a
@@ -43,15 +47,18 @@ class FacetManager;
 // and then transfer it to the background thread for the rest of its life.
 // Initialize() must be called already on the final (background) thread.
 class AffiliationBackend : public FacetManagerHost,
-                           public AffiliationFetcherDelegate {
+                           public AffiliationFetcherDelegate,
+                           public AffiliationFetchThrottlerDelegate {
  public:
   // Constructs an instance that will use |request_context_getter| for all
-  // network requests, and will rely on |time_source| to tell the current time,
-  // which is expected to always be no less than the Unix epoch.
+  // network requests, use |task_runner| for asynchronous tasks, and will rely
+  // on |time_source| and |time_tick_source| to tell the current time/ticks.
   // Construction is very cheap, expensive steps are deferred to Initialize().
   AffiliationBackend(
       const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
-      scoped_ptr<base::Clock> time_source);
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+      scoped_ptr<base::Clock> time_source,
+      scoped_ptr<base::TickClock> time_tick_source);
   ~AffiliationBackend() override;
 
   // Performs the I/O-heavy part of initialization. The database used to cache
@@ -77,10 +84,6 @@ class AffiliationBackend : public FacetManagerHost,
   // storing it into |facet_managers_| if it did not exist.
   FacetManager* GetOrCreateFacetManager(const FacetURI& facet_uri);
 
-  // Collects facet URIs that require fetching and issues a network request
-  // against the Affiliation API to fetch corresponding affiliation information.
-  void SendNetworkRequest();
-
   // Scheduled by RequestNotificationAtTime() to be called back at times when a
   // FacetManager needs to be notified.
   void OnSendNotification(const FacetURI& facet_uri);
@@ -99,20 +102,28 @@ class AffiliationBackend : public FacetManagerHost,
   void OnFetchFailed() override;
   void OnMalformedResponse() override;
 
-  // Used only for testing.
+  // AffiliationFetchThrottlerDelegate:
+  bool OnCanSendNetworkRequest() override;
+
+  // Returns the number of in-memory FacetManagers. Used only for testing.
   size_t facet_manager_count_for_testing() { return facet_managers_.size(); }
 
+  // To be called after Initialize() to use |throttler| instead of the default
+  // one. Used only for testing.
+  void SetThrottlerForTesting(scoped_ptr<AffiliationFetchThrottler> throttler);
+
   // Created in Initialize(), and ensures that all subsequent methods are called
   // on the same thread.
   scoped_ptr<base::ThreadChecker> thread_checker_;
 
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
-
-  // Will always return a Now() that is strictly greater than the NULL time.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   scoped_ptr<base::Clock> clock_;
+  scoped_ptr<base::TickClock> tick_clock_;
 
   scoped_ptr<AffiliationDatabase> cache_;
   scoped_ptr<AffiliationFetcher> fetcher_;
+  scoped_ptr<AffiliationFetchThrottler> throttler_;
 
   // Contains a FacetManager for each facet URI that need ongoing attention. To
   // save memory, managers are discarded as soon as they become redundant.

components/password_manager/core/browser/affiliation_fetch_throttler.cc

@@ -75,15 +75,14 @@ const int64_t AffiliationFetchThrottler::kGracePeriodAfterReconnectMs =
 AffiliationFetchThrottler::AffiliationFetchThrottler(
     AffiliationFetchThrottlerDelegate* delegate,
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    scoped_ptr<base::TickClock> tick_clock)
+    base::TickClock* tick_clock)
     : delegate_(delegate),
       task_runner_(task_runner),
+      tick_clock_(tick_clock),
       state_(IDLE),
       has_network_connectivity_(false),
       is_fetch_scheduled_(false),
-      tick_clock_(tick_clock.Pass()),
-      exponential_backoff_(
-          new BackoffEntryImpl(&kBackoffPolicy, tick_clock_.get())),
+      exponential_backoff_(new BackoffEntryImpl(&kBackoffPolicy, tick_clock_)),
       weak_ptr_factory_(this) {
   DCHECK(delegate);
   // Start observing before querying the current connectivity state, so that if

components/password_manager/core/browser/affiliation_fetch_throttler.h

@@ -51,11 +51,11 @@ class AffiliationFetchThrottler
  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(). The
-  // |delegate| should outlive the throttler.
+  // |delegate| and |tick_clock| should outlive the throttler.
   AffiliationFetchThrottler(
       AffiliationFetchThrottlerDelegate* delegate,
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-      scoped_ptr<base::TickClock> tick_clock);
+      base::TickClock* tick_clock);
   ~AffiliationFetchThrottler() override;
 
   // Signals to the throttling logic that a network request is needed, and that
@@ -66,11 +66,14 @@ class AffiliationFetchThrottler
   // needed or while a request is in flight. To signal that another request will
   // be needed right away after the current one, call this method after calling
   // InformOfNetworkRequestComplete().
-  void SignalNetworkRequestNeeded();
+  virtual void SignalNetworkRequestNeeded();
 
   // Informs the back-off logic that the in-flight network request has been
   // completed, either with |success| or not.
-  void InformOfNetworkRequestComplete(bool success);
+  virtual void InformOfNetworkRequestComplete(bool success);
+
+ protected:
+  AffiliationFetchThrottlerDelegate* delegate_;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(AffiliationFetchThrottlerTest, FailedRequests);
@@ -112,12 +115,11 @@ class AffiliationFetchThrottler
   void OnConnectionTypeChanged(
       net::NetworkChangeNotifier::ConnectionType type) override;
 
-  AffiliationFetchThrottlerDelegate* delegate_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  base::TickClock* tick_clock_;
   State state_;
   bool has_network_connectivity_;
   bool is_fetch_scheduled_;
-  scoped_ptr<base::TickClock> tick_clock_;
   scoped_ptr<net::BackoffEntry> exponential_backoff_;
 
   base::WeakPtrFactory<AffiliationFetchThrottler> weak_ptr_factory_;

components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc

@@ -27,9 +27,9 @@ namespace {
 class MockAffiliationFetchThrottlerDelegate
     : public AffiliationFetchThrottlerDelegate {
  public:
-  explicit MockAffiliationFetchThrottlerDelegate(
-      scoped_ptr<base::TickClock> tick_clock)
-      : tick_clock_(tick_clock.Pass()),
+  // The |tick_clock| should outlive this instance.
+  explicit MockAffiliationFetchThrottlerDelegate(base::TickClock* tick_clock)
+      : tick_clock_(tick_clock),
         emulated_return_value_(true),
         can_send_count_(0u) {}
   ~MockAffiliationFetchThrottlerDelegate() override {
@@ -49,7 +49,7 @@ class MockAffiliationFetchThrottlerDelegate
   }
 
  private:
-  scoped_ptr<base::TickClock> tick_clock_;
+  base::TickClock* tick_clock_;
   bool emulated_return_value_;
   size_t can_send_count_;
   base::TimeTicks last_can_send_time_;
@@ -64,12 +64,13 @@ class AffiliationFetchThrottlerTest : public testing::Test {
   AffiliationFetchThrottlerTest()
       : network_change_notifier_(net::NetworkChangeNotifier::CreateMock()),
         task_runner_(new base::TestMockTimeTaskRunner),
-        mock_delegate_(task_runner_->GetMockTickClock()) {}
+        mock_tick_clock_(task_runner_->GetMockTickClock()),
+        mock_delegate_(mock_tick_clock_.get()) {}
   ~AffiliationFetchThrottlerTest() override {}
 
   scoped_ptr<AffiliationFetchThrottler> CreateThrottler() {
     return make_scoped_ptr(new AffiliationFetchThrottler(
-        &mock_delegate_, task_runner_, task_runner_->GetMockTickClock()));
+        &mock_delegate_, task_runner_, mock_tick_clock_.get()));
   }
 
   void SimulateHasNetworkConnectivity(bool has_connectivity) {
@@ -125,6 +126,7 @@ class AffiliationFetchThrottlerTest : public testing::Test {
   base::MessageLoop message_loop_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+  scoped_ptr<base::TickClock> mock_tick_clock_;
   MockAffiliationFetchThrottlerDelegate mock_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AffiliationFetchThrottlerTest);

components/password_manager/core/browser/affiliation_service.cc

@@ -11,6 +11,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
+#include "base/time/default_tick_clock.h"
 #include "components/password_manager/core/browser/affiliation_backend.h"
 #include "net/url_request/url_request_context_getter.h"
 
@@ -36,8 +37,12 @@ void AffiliationService::Initialize(
     const base::FilePath& db_path) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!backend_);
-  backend_ = new AffiliationBackend(request_context_getter,
-                                    make_scoped_ptr(new base::DefaultClock));
+  backend_ =
+      new AffiliationBackend(request_context_getter, backend_task_runner_,
+                             make_scoped_ptr(new base::DefaultClock),
+                             make_scoped_ptr(new base::DefaultTickClock));
+
+  scoped_ptr<base::TickClock> tick_clock(new base::DefaultTickClock);
   backend_task_runner_->PostTask(
       FROM_HERE, base::Bind(&AffiliationBackend::Initialize,
                             base::Unretained(backend_), db_path));

components/password_manager/core/browser/affiliation_service_unittest.cc

@@ -43,8 +43,8 @@ class AffiliationServiceTest : public testing::Test {
  public:
   AffiliationServiceTest()
       : main_task_runner_(new base::TestSimpleTaskRunner),
-        background_task_runner_(new base::TestMockTimeTaskRunner),
-        main_task_runner_handle_(main_task_runner_) {}
+        main_task_runner_handle_(main_task_runner_),
+        background_task_runner_(new base::TestMockTimeTaskRunner) {}
   ~AffiliationServiceTest() override {}
 
  protected:
@@ -86,11 +86,12 @@ class AffiliationServiceTest : public testing::Test {
     background_task_runner_->RunUntilIdle();
   }
 
-  ScopedFakeAffiliationAPI fake_affiliation_api_;
-  MockAffiliationConsumer mock_consumer_;
   scoped_refptr<base::TestSimpleTaskRunner> main_task_runner_;
-  scoped_refptr<base::TestMockTimeTaskRunner> background_task_runner_;
   base::ThreadTaskRunnerHandle main_task_runner_handle_;
+  scoped_refptr<base::TestMockTimeTaskRunner> background_task_runner_;
+
+  ScopedFakeAffiliationAPI fake_affiliation_api_;
+  MockAffiliationConsumer mock_consumer_;
 
   scoped_ptr<AffiliationService> service_;
 
@@ -141,11 +142,10 @@ TEST_F(AffiliationServiceTest, GetAffiliations) {
 TEST_F(AffiliationServiceTest, ShutdownWhileTasksArePosted) {
   service()->GetAffiliations(FacetURI::FromCanonicalSpec(kTestFacetURIAlpha1),
                              false, mock_consumer()->GetResultCallback());
-  DestroyService();
+  EXPECT_TRUE(background_task_runner()->HasPendingTask());
 
+  DestroyService();
   background_task_runner()->RunUntilIdle();
-  ASSERT_TRUE(fake_affiliation_api()->HasPendingRequest());
-  fake_affiliation_api()->IgnoreNextRequest();
 
   mock_consumer()->ExpectFailure();
   main_task_runner()->RunUntilIdle();

components/password_manager/core/browser/fake_affiliation_api.cc

@@ -60,6 +60,14 @@ void ScopedFakeAffiliationAPI::ServeNextRequest() {
   fetcher->SimulateSuccess(fake_response.Pass());
 }
 
+void ScopedFakeAffiliationAPI::FailNextRequest() {
+  if (!fake_fetcher_factory_.has_pending_fetchers())
+    return;
+
+  FakeAffiliationFetcher* fetcher = fake_fetcher_factory_.PopNextFetcher();
+  fetcher->SimulateFailure();
+}
+
 void ScopedFakeAffiliationAPI::IgnoreNextRequest() {
   if (!fake_fetcher_factory_.has_pending_fetchers())
     return;

components/password_manager/core/browser/fake_affiliation_api.h

@@ -35,6 +35,9 @@ class ScopedFakeAffiliationAPI {
   // with success.
   void ServeNextRequest();
 
+  // Completes the next pending fetch, if any, with failure.
+  void FailNextRequest();
+
   // Ignores the next pending request, if any, without completing it.
   void IgnoreNextRequest();