Move Task Scheduler Initialization From chrome/browser to Content

components/task_scheduler_util/initialization_util.cc

Index: components/task_scheduler_util/initialization_util.cc
diff --git a/components/task_scheduler_util/initialization_util.cc b/components/task_scheduler_util/initialization_util.cc
index 967acef935f784591bcebcbbd17b1c2e6693c5ea..4c68202a21422c5113035d4135d832d757e26367 100644
--- a/components/task_scheduler_util/initialization_util.cc
+++ b/components/task_scheduler_util/initialization_util.cc
@@ -171,23 +171,45 @@ VariationsParamsToBrowserSchedulerWorkerPoolParams(
 }  // namespace
 
 void InitializeDefaultBrowserTaskScheduler() {

[gab, 2016/11/30 22:36:58]

This is still used by iOS I guess?

[robliao, 2016/11/30 22:46:57]

Yep! This goes away when iOS moves, which will be after this CL.

+  std::vector<base::SchedulerWorkerPoolParams> params_vector;
+  base::TaskScheduler::WorkerPoolIndexForTraitsCallback
+      index_to_traits_callback;
+  GetBrowserTaskSchedulerInitParameters(
+      &params_vector, &index_to_traits_callback);
+  base::TaskScheduler::CreateAndSetDefaultTaskScheduler(
+      params_vector, base::Bind(WorkerPoolIndexForTraits));
+
+  MaybePerformRedirectionToTaskScheduler();
+}
+
+void GetBrowserTaskSchedulerInitParameters(
+    std::vector<base::SchedulerWorkerPoolParams>* params_vector,
+    base::TaskScheduler::WorkerPoolIndexForTraitsCallback*
+        index_to_traits_callback) {
   static constexpr char kFieldTrialName[] = "BrowserScheduler";
   std::map<std::string, std::string> variation_params;
   if (!variations::GetVariationParams(kFieldTrialName, &variation_params))
     variation_params = GetDefaultBrowserVariationParams();
 
-  auto params_vector =
+  *params_vector =
       VariationsParamsToBrowserSchedulerWorkerPoolParams(variation_params);
-  if (params_vector.empty()) {
+  if (params_vector->empty()) {
     variation_params = GetDefaultBrowserVariationParams();
-    params_vector =
+    *params_vector =
         VariationsParamsToBrowserSchedulerWorkerPoolParams(variation_params);
-    DCHECK(!params_vector.empty());
+    DCHECK(!params_vector->empty());
   }
-  base::TaskScheduler::CreateAndSetDefaultTaskScheduler(
-      params_vector, base::Bind(WorkerPoolIndexForTraits));
 
+  *index_to_traits_callback = base::Bind(WorkerPoolIndexForTraits);
+}
+
+void MaybePerformRedirectionToTaskScheduler() {
   // TODO(gab): Remove this when http://crbug.com/622400 concludes.
+  static constexpr char kFieldTrialName[] = "BrowserScheduler";
+  std::map<std::string, std::string> variation_params;
+  if (!variations::GetVariationParams(kFieldTrialName, &variation_params))

[gab, 2016/11/30 22:36:58]

I think we should have defaults for individual fields instead of a big default
when we have nothing (i.e. we shouldn't have to specify fields for things that
are enabled by default when we want to override a field that isn't on by default
-- i.e. checked in chromium config should only have to enable redirection, not
re-specify defaults already hardcoded in client)

Basically, I'd like to have what we consider the best client-side defaults be
hardcoded in only one place in our codebase, right now I can think of 3
(content_browser_client.cc, task_scheduler_util.cc and the Chromium config?)

[robliao, 2016/11/30 22:46:57]

I believe this is the nature of how things are structured.

content_browser_client is used as the default implementation for everyone. As
such, it needs to have its own defaults because those dependencies must be
hermetic or live in base.

chrome_content_browser_client is able to use variations, and so it defers to
that. When variations fails, we need to fall back onto something. From
components, we have to be hermetic, and so another set of defaults lives here.

This stuff lives in components so we can share between iOS and everywhere else.

From what I can see at the moment, the only way to have a one-stop-shop is to
put all of this in base, which seems strange.

And if we're going to do the hardcoding, might as well specify all of the
parameters up front so that it's easy to tell what is actually getting used.

Thoughts?

[gab, 2016/11/30 23:10:38]

If we fail to get variations then ChromeContentBrowserClient can defer to
ContentBrowserClient instead of having its own fallback default?

I guess that doesn't work for iOS per lack of content::..? But then iOS probably
wants different defaults anyways so maybe it doesn't matter..?

As for the configs, I really think they should only need to specify the things
they want to override, not all or nothing. Or they become very wordy copies of
local defaults and it also stays very wordy to drive variations from the
cmd-line.

[robliao, 2016/11/30 23:45:21]

The main reason for this is that the failure path:

- If getting the variations fails, use the default.
- If the variations we got is invalid for any reason, use the default.

To avoid bouncing back and forth between content and components on success and
failure, we do all of that work in components.

On configs: I'm not sure what this would look like. The worker pool
configuration set tends to be atomic. We wouldn't want to fall back on failure
for say BackgroundFileIO and then use the config that happened to look good for
Background for example. We wouldn't be able to easily dictate the valid
configurations out there in the event of corruption.

Redirection can plausibly be more piecemeal, but those are already true/false.

+    variation_params = GetDefaultBrowserVariationParams();
+
   const auto sequenced_worker_pool_param =
       variation_params.find("RedirectSequencedWorkerPools");
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(