[Remote suggestions] Move all decisions to fetch to the scheduler

components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
index 84349c862695b8acc838a367cc09420ede36a22a..c63cd6ec114705e4d33f620c0d549a9b8c4e72b8 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
@@ -39,6 +39,7 @@
 #include "components/ntp_snippets/remote/remote_suggestion.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "components/ntp_snippets/remote/remote_suggestions_fetcher.h"
+#include "components/ntp_snippets/remote/remote_suggestions_scheduler.h"
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/testing_pref_service.h"
@@ -362,6 +363,19 @@ class FakeImageDecoder : public image_fetcher::ImageDecoder {
   gfx::Image decoded_image_;
 };
 
+class MockScheduler : public RemoteSuggestionsScheduler {
+ public:
+  MOCK_METHOD0(OnProviderActivated, void());
+  MOCK_METHOD0(OnProviderInactivated, void());
+  MOCK_METHOD0(OnSuggestionsCleared, void());
+  MOCK_METHOD0(OnHistoryCleared, void());
+  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 {
@@ -691,7 +705,6 @@ TEST_F(RemoteSuggestionsProviderImplTest, AddRemoteCategoriesToCategoryRanker) {
           .AddCategory({GetSuggestionN(1)}, /*remote_category_id=*/13)
           .AddCategory({GetSuggestionN(2)}, /*remote_category_id=*/12)
           .Build();
-  SetUpFetchResponse(json_str);
   {
     // The order of categories is determined by the order in which they are
     // added. Thus, the latter is tested here.
@@ -704,6 +717,7 @@ TEST_F(RemoteSuggestionsProviderImplTest, AddRemoteCategoriesToCategoryRanker) {
                 AppendCategoryIfNecessary(Category::FromRemoteCategory(12)));
   }
   auto service = MakeSuggestionsProvider(/*set_empty_response=*/false);
+  LoadFromJSONString(service.get(), json_str);

[tschumann, 2017/02/22 20:46:04]

So, this change is needed because we don't do the fetch on start-up anymore?

[jkrcal, 2017/02/23 12:20:13]

Exactly.

 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, PersistCategoryInfos) {
@@ -759,8 +773,8 @@ TEST_F(RemoteSuggestionsProviderImplTest, PersistRemoteCategoryOrder) {
           .AddCategory({GetSuggestionN(1)}, /*remote_category_id=*/13)
           .AddCategory({GetSuggestionN(2)}, /*remote_category_id=*/12)
           .Build();
-  SetUpFetchResponse(json_str);
   auto service = MakeSuggestionsProvider(/*set_empty_response=*/false);
+  LoadFromJSONString(service.get(), json_str);
 
   // We manually recreate the service to simulate Chrome restart and enforce a
   // mock ranker. The response is cleared to ensure that the order is not
@@ -1466,26 +1480,6 @@ TEST_F(RemoteSuggestionsProviderImplTest, LogNumArticlesHistogram) {
   EXPECT_THAT(
       tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"),
       ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
-
-  // There is only a single, dismissed suggestion in the database, so recreating
-  // the service will require us to re-fetch.
-  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 4);
-  ResetSuggestionsProvider(&service, /*set_empty_response=*/true);
-  EXPECT_EQ(observer().StatusForCategory(articles_category()),
-            CategoryStatus::AVAILABLE);
-  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 5);
-  EXPECT_THAT(
-      tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"),
-      ElementsAre(base::Bucket(/*min=*/1, /*count=*/2)));
-
-  // But if there's a non-dismissed suggestion in the database, recreating it
-  // shouldn't trigger a fetch.
-  LoadFromJSONString(
-      service.get(),
-      GetTestJson({GetSuggestionWithUrl("http://not-dismissed.com")}));
-  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6);
-  ResetSuggestionsProvider(&service, /*set_empty_response=*/true);
-  tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6);
 }
 
 TEST_F(RemoteSuggestionsProviderImplTest, DismissShouldRespectAllKnownUrls) {
@@ -1633,31 +1627,6 @@ TEST_F(RemoteSuggestionsProviderImplTest,
               Eq(CategoryStatus::AVAILABLE));
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest,
-       SuggestionsFetchedOnSignInAndSignOut) {
-  auto service = MakeSuggestionsProvider();
-  EXPECT_THAT(service->GetSuggestionsForTesting(articles_category()),
-              IsEmpty());
-
-  // |MakeSuggestionsProvider()| creates a service where user is signed in
-  // already,
-  // so we start by signing out.
-  SetUpFetchResponse(GetTestJson({GetSuggestionN(1)}));
-  service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
-                           RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_THAT(service->GetSuggestionsForTesting(articles_category()),
-              SizeIs(1));
-
-  // Sign in to check a transition from signed out to signed in.
-  SetUpFetchResponse(GetTestJson({GetSuggestionN(1), GetSuggestionN(2)}));
-  service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
-                           RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_THAT(service->GetSuggestionsForTesting(articles_category()),
-              SizeIs(2));
-}
-
 TEST_F(RemoteSuggestionsProviderImplTest, ShouldClearOrphanedImagesOnRestart) {
   auto service = MakeSuggestionsProvider();
 
@@ -1738,86 +1707,97 @@ TEST_F(RemoteSuggestionsProviderImplTest,
   // scheduler refactoring is done (crbug.com/672434).
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest, CallsProviderStatusCallbackIfReady) {
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerIfInited) {
   // Initiate the service so that it is already READY.
   auto service = MakeSuggestionsProvider();
-
-  StrictMock<MockFunction<void(RemoteSuggestionsProvider::ProviderStatus)>>
-      status_callback;
-  // The callback should be called on registering.
-  EXPECT_CALL(status_callback,
-              Call(RemoteSuggestionsProvider::ProviderStatus::ACTIVE));
-  service->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::Bind(&MockFunction<void(
-                         RemoteSuggestionsProvider::ProviderStatus)>::Call,
-                     base::Unretained(&status_callback))));
+  StrictMock<MockScheduler> scheduler;
+  // The scheduler should be notified of activation of the provider.
+  EXPECT_CALL(scheduler, OnProviderActivated());
+  service->SetRemoteSuggestionsScheduler(&scheduler);
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest,
-       DoesNotCallProviderStatusCallbackIfNotInited) {
+TEST_F(RemoteSuggestionsProviderImplTest, DoesNotCallSchedulerIfNotInited) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-
-  StrictMock<MockFunction<void(RemoteSuggestionsProvider::ProviderStatus)>>
-      status_callback;
+  StrictMock<MockScheduler> scheduler;
   // The provider is not initialized yet, no callback should be called on
   // registering.
-  service->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::Bind(&MockFunction<void(
-                         RemoteSuggestionsProvider::ProviderStatus)>::Call,
-                     base::Unretained(&status_callback))));
+  service->SetRemoteSuggestionsScheduler(&scheduler);
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest,
-       CallsProviderStatusCallbackWhenReady) {
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenReady) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockFunction<void(RemoteSuggestionsProvider::ProviderStatus)>>
-      status_callback;
-  service->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::Bind(&MockFunction<void(
-                         RemoteSuggestionsProvider::ProviderStatus)>::Call,
-                     base::Unretained(&status_callback))));
+  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(status_callback,
-              Call(RemoteSuggestionsProvider::ProviderStatus::ACTIVE));
+  EXPECT_CALL(scheduler, OnProviderActivated());
   WaitForSuggestionsProviderInitialization(service.get(),
                                            /*set_empty_response=*/true);
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest, CallsProviderStatusCallbackOnError) {
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerOnError) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockFunction<void(RemoteSuggestionsProvider::ProviderStatus)>>
-      status_callback;
-  service->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::Bind(&MockFunction<void(
-                         RemoteSuggestionsProvider::ProviderStatus)>::Call,
-                     base::Unretained(&status_callback))));
+  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(status_callback,
-              Call(RemoteSuggestionsProvider::ProviderStatus::INACTIVE));
+  EXPECT_CALL(scheduler, OnProviderInactivated());
   service->EnterState(RemoteSuggestionsProviderImpl::State::ERROR_OCCURRED);
 }
 
-TEST_F(RemoteSuggestionsProviderImplTest,
-       CallsProviderStatusCallbackWhenDisabled) {
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenDisabled) {
   auto service = MakeSuggestionsProviderWithoutInitialization();
-  StrictMock<MockFunction<void(RemoteSuggestionsProvider::ProviderStatus)>>
-      status_callback;
-  service->SetProviderStatusCallback(
-      base::MakeUnique<RemoteSuggestionsProvider::ProviderStatusCallback>(
-          base::Bind(&MockFunction<void(
-                         RemoteSuggestionsProvider::ProviderStatus)>::Call,
-                     base::Unretained(&status_callback))));
+  StrictMock<MockScheduler> scheduler;
+  // The provider is not initialized yet, no callback should be called yet.
+  service->SetRemoteSuggestionsScheduler(&scheduler);
 
   // Should be called when becoming disabled.
-  EXPECT_CALL(status_callback,
-              Call(RemoteSuggestionsProvider::ProviderStatus::INACTIVE));
+  EXPECT_CALL(scheduler, OnProviderInactivated());
+  EXPECT_CALL(scheduler, OnSuggestionsCleared());
   service->EnterState(RemoteSuggestionsProviderImpl::State::DISABLED);
 }
 
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenHistoryCleared) {
+  // 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());
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(scheduler, OnHistoryCleared());
+  service->SetRemoteSuggestionsScheduler(&scheduler);
+  service->ClearHistory(GetDefaultCreationTime(), GetDefaultExpirationTime(),
+                        base::Callback<bool(const GURL& url)>());
+}
+
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedIn) {
+  // 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());
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(scheduler, OnSuggestionsCleared());
+
+  service->SetRemoteSuggestionsScheduler(&scheduler);
+  service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN,
+                           RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT);
+}
+
+TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedOut) {
+  // 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());
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(scheduler, OnSuggestionsCleared());
+
+  service->SetRemoteSuggestionsScheduler(&scheduler);
+  service->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
+                           RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
+}
+
 }  // namespace ntp_snippets