[Remote suggestions] Refactor the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc

 // Copyright 2015 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.
 
 #include "components/ntp_snippets/remote/remote_suggestions_provider_impl.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 362 matching lines @@
   }
 
   void SetDecodedImage(const gfx::Image& image) { decoded_image_ = image; }
 
  private:
   gfx::Image decoded_image_;
 };
 
 class MockScheduler : public RemoteSuggestionsScheduler {
  public:
+  MOCK_METHOD1(SetProvider, void(RemoteSuggestionsProvider* provider));
   MOCK_METHOD0(OnProviderActivated, void());
   MOCK_METHOD0(OnProviderDeactivated, void());
   MOCK_METHOD0(OnSuggestionsCleared, void());
   MOCK_METHOD0(OnHistoryCleared, void());
+  MOCK_METHOD0(AcquireQuotaForInteractiveFetch, bool());
+  MOCK_METHOD1(OnInteractiveFetchFinished, void(Status fetch_status));
   MOCK_METHOD0(OnBrowserForegrounded, void());
   MOCK_METHOD0(OnBrowserColdStart, void());
   MOCK_METHOD0(OnNTPOpened, void());
   MOCK_METHOD0(OnPersistentSchedulerWakeUp, void());
   MOCK_METHOD0(RescheduleFetching, void());
 };
 
 }  // namespace
 
 class RemoteSuggestionsProviderImplTest : public ::testing::Test {
  public:
   RemoteSuggestionsProviderImplTest()
       : params_manager_(ntp_snippets::kStudyName,
                         {{"content_suggestions_backend",
                           kTestContentSuggestionsServerEndpoint}},
                         {ntp_snippets::kArticleSuggestionsFeature.name}),
         fake_url_fetcher_factory_(
             /*default_factory=*/&failing_url_fetcher_factory_),
         test_url_(kTestContentSuggestionsServerWithAPIKey),
         category_ranker_(base::MakeUnique<ConstantCategoryRanker>()),
         user_classifier_(/*pref_service=*/nullptr,
                          base::MakeUnique<base::DefaultClock>()),
         suggestions_fetcher_(nullptr),
         image_fetcher_(nullptr),
+        scheduler_(base::MakeUnique<NiceMock<MockScheduler>>()),
         database_(nullptr) {
     RemoteSuggestionsProviderImpl::RegisterProfilePrefs(
         utils_.pref_service()->registry());
     RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry());
 
     EXPECT_TRUE(database_dir_.CreateUniqueTempDir());
   }
 
   ~RemoteSuggestionsProviderImplTest() override {
     // We need to run the message loop after deleting the database, because
@@ skipping 32 matching lines @@
     EXPECT_CALL(*image_fetcher, SetImageFetcherDelegate(_));
     ON_CALL(*image_fetcher, GetImageDecoder())
         .WillByDefault(Return(&image_decoder_));
     EXPECT_FALSE(observer_);
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     auto database = base::MakeUnique<RemoteSuggestionsDatabase>(
         database_dir_.GetPath(), task_runner);
     database_ = database.get();
     return base::MakeUnique<RemoteSuggestionsProviderImpl>(
         observer_.get(), utils_.pref_service(), "fr", category_ranker_.get(),
-        std::move(suggestions_fetcher), std::move(image_fetcher),
-        std::move(database),
+        scheduler_.get(), std::move(suggestions_fetcher),
+        std::move(image_fetcher), std::move(database),
         base::MakeUnique<RemoteSuggestionsStatusService>(
             utils_.fake_signin_manager(), utils_.pref_service()));
   }
 
   void WaitForSuggestionsProviderInitialization(
       RemoteSuggestionsProviderImpl* service,
       bool set_empty_response) {
     EXPECT_EQ(RemoteSuggestionsProviderImpl::State::NOT_INITED,
               service->state_);
 
@@ skipping 10 matching lines @@
   }
 
   void ResetSuggestionsProvider(
       std::unique_ptr<RemoteSuggestionsProviderImpl>* service,
       bool set_empty_response) {
     service->reset();
     observer_.reset();
     *service = MakeSuggestionsProvider(set_empty_response);
   }
 
+  void SetStrictScheduler() {

[Marc Treib, 2017/03/29 11:11:57]

Hm, by itself this doesn't really make sense, as the provider will still have
the old scheduler - in fact, a dangling pointer to it. Can we wrap this in a
MakeSuggestionsProviderWithStrictScheduler() or something like that?

Or alternatively, somehow check that this is not called after the previous
scheduler_ has been passed around?

+    scheduler_ = base::MakeUnique<StrictMock<MockScheduler>>();
+  }
+
   void SetCategoryRanker(std::unique_ptr<CategoryRanker> category_ranker) {
     category_ranker_ = std::move(category_ranker);
   }
 
   ContentSuggestion::ID MakeArticleID(const std::string& id_within_category) {
     return ContentSuggestion::ID(articles_category(), id_within_category);
   }
 
   Category articles_category() {
     return Category::FromKnownCategory(KnownCategories::ARTICLES);
@@ skipping 16 matching lines @@
   FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
   RemoteSuggestionsFetcher* suggestions_fetcher() {
     return suggestions_fetcher_;
   }
   // TODO(tschumann): Make this a strict-mock. We want to avoid unneccesary
   // network requests.
   NiceMock<MockImageFetcher>* image_fetcher() { return image_fetcher_; }
   FakeImageDecoder* image_decoder() { return &image_decoder_; }
   PrefService* pref_service() { return utils_.pref_service(); }
   RemoteSuggestionsDatabase* database() { return database_; }
+  MockScheduler* scheduler() { return scheduler_.get(); }
 
   // Provide the json to be returned by the fake fetcher.
   void SetUpFetchResponse(const std::string& json) {
     fake_url_fetcher_factory_.SetFakeResponse(test_url_, json, net::HTTP_OK,
                                               net::URLRequestStatus::SUCCESS);
   }
 
   // Have the fake fetcher fail due to a HTTP error like a 404.
   void SetUpHttpError() {
     fake_url_fetcher_factory_.SetFakeResponse(test_url_, /*json=*/std::string(),
                                               net::HTTP_NOT_FOUND,
                                               net::URLRequestStatus::SUCCESS);
   }
 
   void LoadFromJSONString(RemoteSuggestionsProviderImpl* service,
                           const std::string& json) {
     SetUpFetchResponse(json);
     service->FetchSuggestions(/*interactive_request=*/true,
                               /*callback=*/nullptr);
     base::RunLoop().RunUntilIdle();
   }
 
   void LoadMoreFromJSONString(RemoteSuggestionsProviderImpl* service,
                               const Category& category,
                               const std::string& json,
                               const std::set<std::string>& known_ids,
                               FetchDoneCallback callback) {
     SetUpFetchResponse(json);
+    EXPECT_CALL(*scheduler(), AcquireQuotaForInteractiveFetch())
+        .WillOnce(Return(true))
+        .RetiresOnSaturation();
     service->Fetch(category, known_ids, callback);
     base::RunLoop().RunUntilIdle();
   }
 
  private:
   variations::testing::VariationParamsManager params_manager_;
   test::RemoteSuggestionsTestUtils utils_;
   base::MessageLoop message_loop_;
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
   // Instantiation of factory automatically sets itself as URLFetcher's factory.
   net::FakeURLFetcherFactory fake_url_fetcher_factory_;
   const GURL test_url_;
   std::unique_ptr<CategoryRanker> category_ranker_;
   UserClassifier user_classifier_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   RemoteSuggestionsFetcher* suggestions_fetcher_;
   NiceMock<MockImageFetcher>* image_fetcher_;
   FakeImageDecoder image_decoder_;
+  std::unique_ptr<MockScheduler> scheduler_;
 
   base::ScopedTempDir database_dir_;
   RemoteSuggestionsDatabase* database_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderImplTest);
 };
 
 TEST_F(RemoteSuggestionsProviderImplTest, Full) {
   std::string json_str(GetTestJson({GetSuggestion()}));
 
@@ skipping 1132 matching lines @@
   service->RefetchInTheBackground(/*callback=*/nullptr);
   base::RunLoop().RunUntilIdle();
   // TODO(jkrcal): Move together with the pref storage into the scheduler.
   EXPECT_EQ(
       simple_test_clock_ptr->Now().ToInternalValue(),
       pref_service()->GetInt64(prefs::kLastSuccessfulBackgroundFetchTime));
   // TODO(markusheintz): Add a test that simulates a browser restart once the
   // scheduler refactoring is done (crbug.com/672434).
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerIfInited) {
-  // Initiate the service so that it is already READY.
-  auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
-  service->SetRemoteSuggestionsScheduler(&scheduler);
-}
-
-TEST_F(RemoteSuggestionsProviderImplTest, DoesNotCallSchedulerIfNotInited) {
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenReady) {
+  SetStrictScheduler();
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called on
-  // registering.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
-}
-
-TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenReady) {
-  auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called when becoming ready.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
   WaitForSuggestionsProviderInitialization(service.get(),
                                            /*set_empty_response=*/true);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerOnError) {
+  SetStrictScheduler();
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called on error.
-  EXPECT_CALL(scheduler, OnProviderDeactivated());
+  EXPECT_CALL(*scheduler(), OnProviderDeactivated());
   service->EnterState(RemoteSuggestionsProviderImpl::State::ERROR_OCCURRED);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenDisabled) {
+  SetStrictScheduler();
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockScheduler> scheduler;
-  // The provider is not initialized yet, no callback should be called yet.
-  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called when becoming disabled. First deactivate and only after
   // that clear the suggestions so that they are not fetched again.
   {
     InSequence s;
-    EXPECT_CALL(scheduler, OnProviderDeactivated());
+    EXPECT_CALL(*scheduler(), OnProviderDeactivated());
     ASSERT_THAT(service->ready(), Eq(false));
-    EXPECT_CALL(scheduler, OnSuggestionsCleared());
+    EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   }
   service->EnterState(RemoteSuggestionsProviderImpl::State::DISABLED);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenHistoryCleared) {
+  SetStrictScheduler();
   // Initiate the service so that it is already READY.
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnHistoryCleared());
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnHistoryCleared());
   service->ClearHistory(GetDefaultCreationTime(), GetDefaultExpirationTime(),
                         base::Callback<bool(const GURL& url)>());
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedIn) {
+  SetStrictScheduler();
   // Initiate the service so that it is already READY.
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnSuggestionsCleared());
-
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedOut) {
+  SetStrictScheduler();
   // Initiate the service so that it is already READY.
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
   auto service = MakeSuggestionsProvider();
-  StrictMock<MockScheduler> scheduler;
-  // The scheduler should be notified of activation of the provider.
-  EXPECT_CALL(scheduler, OnProviderActivated());
+
   // The scheduler should be notified of clearing the history.
-  EXPECT_CALL(scheduler, OnSuggestionsCleared());
-
-  service->SetRemoteSuggestionsScheduler(&scheduler);
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
   service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
 }
 
 }  // namespace ntp_snippets