[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
index 27ac0a2efee7c070fe71981ad46f0b1da1bf3161..06e1db3f78188b0aab239325e9c8a084828da0ed 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
@@ -32,8 +32,8 @@
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
-#include "components/ntp_snippets/remote/ntp_snippets_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
+#include "components/ntp_snippets/remote/remote_suggestions_hard_scheduler.h"
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/testing_pref_service.h"
@@ -307,7 +307,7 @@ class FailingFakeURLFetcherFactory : public net::URLFetcherFactory {
   }
 };
 
-class MockScheduler : public NTPSnippetsScheduler {
+class MockHardScheduler : public RemoteSuggestionsHardScheduler {
  public:
   MOCK_METHOD2(Schedule,
                bool(base::TimeDelta period_wifi,
@@ -452,7 +452,7 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     return base::MakeUnique<RemoteSuggestionsProvider>(
         observer_.get(), &category_factory_, utils_.pref_service(), "fr",
-        &user_classifier_, &scheduler_, std::move(snippets_fetcher),
+        &user_classifier_, &hard_scheduler_, std::move(snippets_fetcher),
         std::move(image_fetcher), std::move(image_decoder),
         base::MakeUnique<RemoteSuggestionsDatabase>(database_dir_.GetPath(),
                                                     task_runner),
@@ -503,7 +503,7 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
  protected:
   const GURL& test_url() { return test_url_; }
   FakeContentSuggestionsProviderObserver& observer() { return *observer_; }
-  MockScheduler& mock_scheduler() { return scheduler_; }
+  MockHardScheduler& mock_scheduler() { return hard_scheduler_; }
   // TODO(tschumann): Make this a strict-mock. We want to avoid unneccesary
   // network requests.
   NiceMock<MockImageFetcher>* image_fetcher() { return image_fetcher_; }
@@ -543,7 +543,7 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
   const GURL test_url_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
   UserClassifier user_classifier_;
-  NiceMock<MockScheduler> scheduler_;
+  NiceMock<MockHardScheduler> hard_scheduler_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
   CategoryFactory category_factory_;
   NiceMock<MockImageFetcher>* image_fetcher_;
@@ -554,46 +554,6 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderTest);
 };
 
-TEST_F(RemoteSuggestionsProviderTest, ScheduleOnStart) {

[Marc Treib, 2016/12/09 12:25:26]

I think some of the removed tests kinda still apply - what's the new behavior of
RemoteSuggestionsProvider vs the scheduler?
I think injecting a mock scheduler (the full thing, not just the "hard" one)
might be good, then we could test that the provider calls the right things at
the right time.

[jkrcal, 2016/12/19 09:40:23]

Not relevant any more. I've added some more tests for notifying status changes.

-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  auto service = MakeSnippetsService();
-
-  // When we have no snippets are all, loading the service initiates a fetch.
-  EXPECT_EQ("OK", service->snippets_fetcher()->last_status());
-}
-
-TEST_F(RemoteSuggestionsProviderTest, DontRescheduleOnStart) {
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  SetUpFetchResponse(GetTestJson({GetSnippet()}));
-  auto service = MakeSnippetsService(/*set_empty_response=*/false);
-
-  // When recreating the service, we should not get any |Schedule| calls:
-  // The tasks are already scheduled with the correct intervals, so nothing on
-  // initialization, and the service has data from the DB, so no automatic fetch
-  // should happen.
-  Mock::VerifyAndClearExpectations(&mock_scheduler());
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(0);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  ResetSnippetsService(&service);
-}
-
-TEST_F(RemoteSuggestionsProviderTest, RescheduleAfterSuccessfulFetch) {
-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  auto service = MakeSnippetsService();
-
-  // A successful fetch should trigger another |Schedule|.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _));
-  LoadFromJSONString(service.get(), GetTestJson({GetSnippet()}));
-}
-
 TEST_F(RemoteSuggestionsProviderTest, DontRescheduleAfterFailedFetch) {
   // We should get two |Schedule| calls: The first when initialization
   // completes, the second one after the automatic (since the service doesn't
@@ -606,37 +566,6 @@ TEST_F(RemoteSuggestionsProviderTest, DontRescheduleAfterFailedFetch) {
   LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()}));
 }
 
-TEST_F(RemoteSuggestionsProviderTest, IgnoreRescheduleBeforeInit) {
-  // We should get two |Schedule| calls: The first when initialization
-  // completes, the second one after the automatic (since the service doesn't
-  // have any data yet) fetch finishes.
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  // The |RescheduleFetching| call shouldn't do anything (in particular not
-  // result in an |Unschedule|), since the service isn't initialized yet.
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-  auto service = MakeSnippetsServiceWithoutInitialization();
-  service->RescheduleFetching(false);
-  WaitForSnippetsServiceInitialization(service.get(),
-                                       /*set_empty_response=*/true);
-}
-
-TEST_F(RemoteSuggestionsProviderTest, HandleForcedRescheduleBeforeInit) {
-  {
-    InSequence s;
-    // The |RescheduleFetching| call with force=true should result in an
-    // |Unschedule|, since the service isn't initialized yet.
-    EXPECT_CALL(mock_scheduler(), Unschedule()).Times(1);
-    // We should get two |Schedule| calls: The first when initialization
-    // completes, the second one after the automatic (since the service doesn't
-    // have any data yet) fetch finishes.
-    EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  }
-  auto service = MakeSnippetsServiceWithoutInitialization();
-  service->RescheduleFetching(true);
-  WaitForSnippetsServiceInitialization(service.get(),
-                                       /*set_empty_response=*/true);
-}
-
 TEST_F(RemoteSuggestionsProviderTest, RescheduleOnStateChange) {
   {
     InSequence s;
@@ -661,15 +590,6 @@ TEST_F(RemoteSuggestionsProviderTest, RescheduleOnStateChange) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(RemoteSuggestionsProviderTest, DontUnscheduleOnShutdown) {
-  EXPECT_CALL(mock_scheduler(), Schedule(_, _)).Times(2);
-  EXPECT_CALL(mock_scheduler(), Unschedule()).Times(0);
-
-  auto service = MakeSnippetsService();
-
-  service.reset();
-  base::RunLoop().RunUntilIdle();
-}
 
 TEST_F(RemoteSuggestionsProviderTest, Full) {
   std::string json_str(GetTestJson({GetSnippet()}));
@@ -1618,7 +1538,10 @@ TEST_F(RemoteSuggestionsProviderTest, StoreLastSuccessfullBackgroundFetchTime) {
   // Advance the time and check whether the time was updated correctly after the
   // background fetch.
   simple_test_tick_clock_ptr->Advance(TimeDelta::FromHours(1));
-  service->FetchSnippetsInTheBackground();
+
+  // TODO(jkrcal): Move together with the pref storage into the scheduler.
+  static_cast<RemoteSuggestionsScheduler::Updater*>(service.get())
+      ->UpdateRemoteSuggestionsBySchedule();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(
       simple_test_tick_clock_ptr->NowTicks().ToInternalValue(),