[NTP::Cleanup] Avoid nested complex function calls

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 b3befc0d38ee143688df8522e50c1136b7af90f5..745e1e46eaadeb6fc0047128b9113f31f9046116 100644
--- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -171,31 +171,36 @@ void RegisterArticleProvider(SigninManagerBase* signin_manager,
               base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   bool is_stable_channel =
       chrome::GetChannel() == version_info::Channel::STABLE;
+  auto suggestions_fetcher = base::MakeUnique<RemoteSuggestionsFetcher>(
+      signin_manager, token_service, request_context, pref_service,
+      language_model, base::Bind(&safe_json::SafeJsonParser::Parse),
+      is_stable_channel ? google_apis::GetAPIKey()
+                        : google_apis::GetNonStableAPIKey(),
+      service->user_classifier());
+  auto image_decoder_for_fetcher = base::MakeUnique<ImageDecoderImpl>();
+  auto image_fetcher = base::MakeUnique<ImageFetcherImpl>(
+      std::move(image_decoder_for_fetcher), request_context.get());
+  auto image_decoder = base::MakeUnique<ImageDecoderImpl>();
+  auto database =
+      base::MakeUnique<RemoteSuggestionsDatabase>(database_dir, task_runner);
+  auto status_service = base::MakeUnique<RemoteSuggestionsStatusService>(
+      signin_manager, pref_service);
   auto provider = base::MakeUnique<RemoteSuggestionsProviderImpl>(
       service, pref_service, g_browser_process->GetApplicationLocale(),
-      service->category_ranker(),
-      base::MakeUnique<RemoteSuggestionsFetcher>(
-          signin_manager, token_service, request_context, pref_service,
-          language_model, base::Bind(&safe_json::SafeJsonParser::Parse),
-          is_stable_channel ? google_apis::GetAPIKey()
-                            : google_apis::GetNonStableAPIKey(),
-          service->user_classifier()),
-      base::MakeUnique<ImageFetcherImpl>(base::MakeUnique<ImageDecoderImpl>(),
-                                         request_context.get()),
-      base::MakeUnique<ImageDecoderImpl>(),
-      base::MakeUnique<RemoteSuggestionsDatabase>(database_dir, task_runner),
-      base::MakeUnique<RemoteSuggestionsStatusService>(signin_manager,
-                                                       pref_service));
+      service->category_ranker(), std::move(suggestions_fetcher),
+      std::move(image_fetcher), std::move(image_decoder), std::move(database),
+      std::move(status_service));

[tschumann, 2017/02/01 11:35:37]

I'm having a hard time following the motivation.
Clearly, moving the fetcher into a separate variable makes the code easier to
read. But that's completely independent of using MakeUnique.
For example, I don't see the benefit of moving the other parameters into
variables (as (1) there's another layer of indirection now, (2) they are still
in scope after the call but moved out, which is not an easy state to reason
about.

[Marc Treib, 2017/02/01 12:33:20]

Maybe put all of this into a scope, so the variables won't exist after the call?

[vitaliii, 2017/02/01 13:15:15]

Done.

1) I inlined MakeUnique calls, which have no arguments;

2) For ArticlesProvider I placed all of them into a scope, so (2) is not
applicable anymore. 

As for (1) I still think that this way it is more readable + this discourages
inlining MakeUnique calls with arguments.

 
   PersistentScheduler* scheduler = nullptr;
 #if defined(OS_ANDROID)
   scheduler = NTPSnippetsLauncher::Get();
 #endif  // OS_ANDROID
 
+  auto scheduling_provider_clock = base::MakeUnique<base::DefaultClock>();
   auto scheduling_provider =
       base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
           service, std::move(provider), scheduler, service->user_classifier(),
-          pref_service, base::MakeUnique<base::DefaultClock>());
+          pref_service, std::move(scheduling_provider_clock));
   service->set_remote_suggestions_provider(scheduling_provider.get());
   service->set_remote_suggestions_scheduler(scheduling_provider.get());
   service->RegisterProvider(std::move(scheduling_provider));
@@ -261,9 +266,10 @@ KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
   HistoryService* history_service = HistoryServiceFactory::GetForProfile(
       profile, ServiceAccessType::EXPLICIT_ACCESS);
   PrefService* pref_service = profile->GetPrefs();
+  auto ranker_clock = base::MakeUnique<base::DefaultClock>();
   std::unique_ptr<CategoryRanker> category_ranker =
-      ntp_snippets::BuildSelectedCategoryRanker(
-          pref_service, base::MakeUnique<base::DefaultClock>());
+      ntp_snippets::BuildSelectedCategoryRanker(pref_service,
+                                                std::move(ranker_clock));
   auto* service =
       new ContentSuggestionsService(State::ENABLED, signin_manager,
           history_service, pref_service, std::move(category_ranker));