[Remote suggestions] Refactor the scheduler

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

 // Copyright 2016 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 "chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
 
 #include <utility>
 
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
@@ skipping 21 matching lines @@
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h"
 #include "components/ntp_snippets/category_rankers/category_ranker.h"
 #include "components/ntp_snippets/content_suggestions_service.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.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_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"
 #include "components/safe_json/safe_json_parser.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/translate/core/browser/language_model.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 34 matching lines @@
 using image_fetcher::ImageFetcherImpl;
 using ntp_snippets::BookmarkSuggestionsProvider;
 using ntp_snippets::CategoryRanker;
 using ntp_snippets::ContentSuggestionsService;
 using ntp_snippets::ForeignSessionsSuggestionsProvider;
 using ntp_snippets::GetFetchEndpoint;
 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;
 using translate::LanguageModel;
 
 // For now, ContentSuggestionsService must only be instantiated on Android.
 // See also crbug.com/688366.
 #if defined(OS_ANDROID)
 #define CONTENT_SUGGESTIONS_ENABLED 1
 #else
@@ skipping 99 matching lines @@
       service->user_classifier());
   auto provider = base::MakeUnique<RemoteSuggestionsProviderImpl>(
       service, pref_service, g_browser_process->GetApplicationLocale(),
       service->category_ranker(), std::move(suggestions_fetcher),
       base::MakeUnique<ImageFetcherImpl>(base::MakeUnique<ImageDecoderImpl>(),
                                          request_context.get()),
       base::MakeUnique<RemoteSuggestionsDatabase>(database_dir, task_runner),
       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,
                                      ContentSuggestionsService* service,
                                      PrefService* pref_service) {
   std::unique_ptr<TabDelegateSyncAdapter> sync_adapter =
       base::MakeUnique<TabDelegateSyncAdapter>(sync_service);
   auto provider = base::MakeUnique<ForeignSessionsSuggestionsProvider>(
       service, std::move(sync_adapter), pref_service);
   service->RegisterProvider(std::move(provider));
@@ skipping 122 matching lines @@
           ntp_snippets::kForeignSessionsSuggestionsFeature)) {
     RegisterForeignSessionsProvider(sync_service, service, pref_service);
   }
 
   return service;
 
 #else
   return nullptr;
 #endif  // CONTENT_SUGGESTIONS_ENABLED
 }