[Remote suggestions] Refactor the scheduler

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

Index: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
index 4f62bf3db3c943bd6e4e5abbb70b48afbf890cd4..bc681aede5abd731ac92c67e7e4efeb78322a240 100644
--- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -39,8 +39,8 @@
 #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_provider_impl.h"
+#include "components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h"
 #include "components/ntp_snippets/remote/remote_suggestions_status_service.h"
-#include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 #include "components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h"
 #include "components/ntp_snippets/sessions/tab_delegate_sync_adapter.h"
 #include "components/prefs/pref_service.h"
@@ -95,8 +95,8 @@ using ntp_snippets::PersistentScheduler;
 using ntp_snippets::RemoteSuggestionsDatabase;
 using ntp_snippets::RemoteSuggestionsFetcher;
 using ntp_snippets::RemoteSuggestionsProviderImpl;
+using ntp_snippets::RemoteSuggestionsSchedulerImpl;
 using ntp_snippets::RemoteSuggestionsStatusService;
-using ntp_snippets::SchedulingRemoteSuggestionsProvider;
 using ntp_snippets::TabDelegateSyncAdapter;
 using suggestions::ImageDecoderImpl;
 using syncer::SyncService;
@@ -216,21 +216,20 @@ void RegisterArticleProvider(SigninManagerBase* signin_manager,
       base::MakeUnique<RemoteSuggestionsStatusService>(signin_manager,
                                                        pref_service));
 
-  PersistentScheduler* scheduler = nullptr;
+  PersistentScheduler* persistent_scheduler = nullptr;
 #if defined(OS_ANDROID)
-  scheduler = NTPSnippetsLauncher::Get();
+  persistent_scheduler = NTPSnippetsLauncher::Get();
 #endif  // OS_ANDROID
 
-  RemoteSuggestionsProviderImpl* provider_raw = provider.get();
-  auto scheduling_provider =
-      base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
-          service, std::move(provider), scheduler, service->user_classifier(),
-          pref_service, g_browser_process->local_state(),
-          base::MakeUnique<base::DefaultClock>());
-  provider_raw->SetRemoteSuggestionsScheduler(scheduling_provider.get());
-  service->set_remote_suggestions_provider(scheduling_provider.get());
-  service->set_remote_suggestions_scheduler(scheduling_provider.get());
-  service->RegisterProvider(std::move(scheduling_provider));
+  auto scheduler = base::MakeUnique<RemoteSuggestionsSchedulerImpl>(
+      provider.get(), persistent_scheduler, service->user_classifier(),
+      pref_service, g_browser_process->local_state(),
+      base::MakeUnique<base::DefaultClock>());
+  provider->SetRemoteSuggestionsScheduler(scheduler.get());
+  service->set_remote_suggestions_provider(provider.get());
+
+  service->set_remote_suggestions_scheduler(std::move(scheduler));

[Marc Treib, 2017/03/23 16:41:15]

Hm, the order of initialization is a bit convoluted here, since everyone depends
on everyone else.

Since the service now owns the scheduler, I'd prefer if we could pass it there
in the constructor. That means we'd have to pass the provider pointer to the
scheduler later, but that seems preferable overall: The owning pointer is there
right away, and the weak pointer is passed later. WDYT?

[tschumann, 2017/03/23 17:38:27]

Sounds good to me. Ownership is best expressed with constructors.

Related but a different refactoring: It's a bit weird that the provider's
constructor takes an observer. The nice things about observers is usually that
you don't need them at construction time...

[jkrcal, 2017/03/27 10:03:02]

Done.

+  service->RegisterProvider(std::move(provider));
 }
 
 void RegisterForeignSessionsProvider(SyncService* sync_service,