Create FeatureList and PrefService early in Cast.

TaskScheduler requires the FeatureList, so it needs to be moved
from CastBrowserMainParts::PreCreateThreads() to an earlier point,
CastMainDelegate::PostEarlyInitialization().

The creation of the "singleton" FieldTrialList instance also needs
to be moved to CastMainDelegate::PostEarlyInitialization(), since the
FeatureList initialization depends on the FieldTrialList. Its
ownership was transferred from BrowserMainParts to CastMainDelegate.

A new CastFeatureListCreator class holds ownership of the service
while the browser process isn't started.

Bug: 887459
Change-Id: I4bcd60720518807e2089ec99a3f7db3e647203d6
Reviewed-on: https://chromium-review.googlesource.com/c/1308096
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604920}

chromecast/app/cast_main_delegate.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/metrics/field_trial.h"
 #include "base/no_destructor.h"
 #include "base/path_service.h"
 #include "base/posix/global_descriptors.h"
@@ -21,6 +22,7 @@
 #include "chromecast/base/cast_paths.h"
 #include "chromecast/base/chromecast_switches.h"
 #include "chromecast/browser/cast_content_browser_client.h"
+#include "chromecast/browser/cast_feature_list_creator.h"
 #include "chromecast/chromecast_buildflags.h"
 #include "chromecast/common/cast_resource_delegate.h"
 #include "chromecast/common/global_descriptors.h"
@@ -196,12 +198,21 @@ void CastMainDelegate::ZygoteForked() {
 #endif  // defined(OS_LINUX)
 
 bool CastMainDelegate::ShouldCreateFeatureList() {
-  // TODO(https://crbug.com/887459): Move the creation of FeatureList from
-  // CastBrowserMainParts::PreCreateThreads() to
-  // CastMainDelegate::PostEarlyInitialization().
   return false;
 }
 
+void CastMainDelegate::PostEarlyInitialization(bool is_running_tests) {
+  DCHECK(cast_feature_list_creator_);
+
+  // The |FieldTrialList| is a dependency of the feature list.
+  field_trial_list_ = std::make_unique<base::FieldTrialList>(nullptr);
+
+  // Initialize the base::FeatureList and the PrefService (which it depends on),
+  // so objects initialized after this point can use features from
+  // base::FeatureList.
+  cast_feature_list_creator_->CreatePrefServiceAndFeatureList();
+}
+
 void CastMainDelegate::InitializeResourceBundle() {
   base::FilePath pak_file;
   CHECK(base::PathService::Get(FILE_CAST_PAK, &pak_file));
@@ -250,7 +261,10 @@ void CastMainDelegate::InitializeResourceBundle() {
 }
 
 content::ContentBrowserClient* CastMainDelegate::CreateContentBrowserClient() {
-  browser_client_ = CastContentBrowserClient::Create();
+  DCHECK(!cast_feature_list_creator_);
+  cast_feature_list_creator_ = std::make_unique<CastFeatureListCreator>();
+  browser_client_ =
+      CastContentBrowserClient::Create(cast_feature_list_creator_.get());
   return browser_client_.get();
 }
 

chromecast/app/cast_main_delegate.h

@@ -12,6 +12,10 @@
 #include "chromecast/common/cast_content_client.h"
 #include "content/public/app/content_main_delegate.h"
 
+namespace base {
+class FieldTrialList;
+}  // namespace base
+
 namespace content {
 class BrowserMainRunner;
 }  // namespace content
@@ -19,6 +23,7 @@ class BrowserMainRunner;
 namespace chromecast {
 
 class CastResourceDelegate;
+class CastFeatureListCreator;
 
 namespace shell {
 
@@ -41,6 +46,7 @@ class CastMainDelegate : public content::ContentMainDelegate {
   void ZygoteForked() override;
 #endif  // defined(OS_LINUX)
   bool ShouldCreateFeatureList() override;
+  void PostEarlyInitialization(bool is_running_tests) override;
   content::ContentBrowserClient* CreateContentBrowserClient() override;
   content::ContentRendererClient* CreateContentRendererClient() override;
   content::ContentUtilityClient* CreateContentUtilityClient() override;
@@ -58,6 +64,14 @@ class CastMainDelegate : public content::ContentMainDelegate {
   std::unique_ptr<content::BrowserMainRunner> browser_runner_;
 #endif  // defined(OS_ANDROID)
 
+  // |field_trial_list_| is a singleton-like that needs to live for as long as
+  // anything uses it. It is accessible through |FieldTrialList| static methods,
+  // but gives no warning if those methods are called without some instance
+  // existing somewhere.
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
+
+  std::unique_ptr<CastFeatureListCreator> cast_feature_list_creator_;
+
   DISALLOW_COPY_AND_ASSIGN(CastMainDelegate);
 };
 

chromecast/browser/BUILD.gn

@@ -33,6 +33,8 @@ cast_source_set("browser") {
     "cast_content_gesture_handler.h",
     "cast_download_manager_delegate.cc",
     "cast_download_manager_delegate.h",
+    "cast_feature_list_creator.cc",
+    "cast_feature_list_creator.h",
     "cast_http_user_agent_settings.cc",
     "cast_http_user_agent_settings.h",
     "cast_media_blocker.cc",

chromecast/browser/cast_browser_main_parts.cc

@@ -10,7 +10,6 @@
 #include <string>
 #include <vector>
 
-#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
@@ -26,23 +25,20 @@
 #include "build/build_config.h"
 #include "cc/base/switches.h"
 #include "chromecast/base/cast_constants.h"
-#include "chromecast/base/cast_features.h"
 #include "chromecast/base/cast_paths.h"
 #include "chromecast/base/chromecast_switches.h"
 #include "chromecast/base/metrics/cast_metrics_helper.h"
 #include "chromecast/base/metrics/grouped_histogram.h"
-#include "chromecast/base/pref_names.h"
 #include "chromecast/base/version.h"
 #include "chromecast/browser/cast_browser_context.h"
 #include "chromecast/browser/cast_browser_process.h"
 #include "chromecast/browser/cast_content_browser_client.h"
+#include "chromecast/browser/cast_feature_list_creator.h"
 #include "chromecast/browser/cast_memory_pressure_monitor.h"
 #include "chromecast/browser/cast_net_log.h"
 #include "chromecast/browser/devtools/remote_debugging_server.h"
 #include "chromecast/browser/media/media_caps_impl.h"
-#include "chromecast/browser/metrics/cast_metrics_prefs.h"
 #include "chromecast/browser/metrics/cast_metrics_service_client.h"
-#include "chromecast/browser/pref_service_helper.h"
 #include "chromecast/browser/tts/tts_controller_impl.h"
 #include "chromecast/browser/tts/tts_platform_stub.h"
 #include "chromecast/browser/url_request_context_factory.h"
@@ -56,8 +52,7 @@
 #include "chromecast/net/connectivity_checker.h"
 #include "chromecast/public/cast_media_shlib.h"
 #include "chromecast/service/cast_service.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/proxy_config/pref_proxy_config_tracker_impl.h"
+#include "components/prefs/pref_service.h"
 #include "components/viz/common/switches.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -318,7 +313,6 @@ CastBrowserMainParts::CastBrowserMainParts(
     CastContentBrowserClient* cast_content_browser_client)
     : BrowserMainParts(),
       cast_browser_process_(new CastBrowserProcess()),
-      field_trial_list_(nullptr),
       parameters_(parameters),
       cast_content_browser_client_(cast_content_browser_client),
       url_request_context_factory_(url_request_context_factory),
@@ -434,25 +428,9 @@ int CastBrowserMainParts::PreCreateThreads() {
     return 1;
 #endif
 
-  scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple());
-  metrics::RegisterPrefs(pref_registry.get());
-  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_registry.get());
   cast_browser_process_->SetPrefService(
-      PrefServiceHelper::CreatePrefService(pref_registry.get()));
-
-  // As soon as the PrefService is set, initialize the base::FeatureList, so
-  // objects initialized after this point can use features from
-  // base::FeatureList.
-  const auto* features_dict =
-      cast_browser_process_->pref_service()->GetDictionary(
-          prefs::kLatestDCSFeatures);
-  const auto* experiment_ids = cast_browser_process_->pref_service()->GetList(
-      prefs::kActiveDCSExperiments);
-  auto* command_line = base::CommandLine::ForCurrentProcess();
-  InitializeFeatureList(
-      *features_dict, *experiment_ids,
-      command_line->GetSwitchValueASCII(switches::kEnableFeatures),
-      command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+      cast_content_browser_client_->GetCastFeatureListCreator()
+          ->TakePrefService());
 
 #if defined(USE_AURA)
   cast_browser_process_->SetCastScreen(std::make_unique<CastScreen>());

chromecast/browser/cast_browser_main_parts.h

@@ -9,7 +9,6 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/metrics/field_trial.h"
 #include "build/build_config.h"
 #include "build/buildflag.h"
 #include "chromecast/chromecast_buildflags.h"
@@ -85,7 +84,6 @@ class CastBrowserMainParts : public content::BrowserMainParts {
 
  private:
   std::unique_ptr<CastBrowserProcess> cast_browser_process_;
-  base::FieldTrialList field_trial_list_;
   const content::MainFunctionParams parameters_;  // For running browser tests.
   // Caches a pointer of the CastContentBrowserClient.
   CastContentBrowserClient* const cast_content_browser_client_ = nullptr;

chromecast/browser/cast_content_browser_client.cc

@@ -163,9 +163,11 @@ void CreateMediaDrmStorage(content::RenderFrameHost* render_frame_host,
 
 }  // namespace
 
-CastContentBrowserClient::CastContentBrowserClient()
+CastContentBrowserClient::CastContentBrowserClient(
+    CastFeatureListCreator* cast_feature_list_creator)
     : cast_browser_main_parts_(nullptr),
-      url_request_context_factory_(new URLRequestContextFactory()) {}
+      url_request_context_factory_(new URLRequestContextFactory()),
+      cast_feature_list_creator_(cast_feature_list_creator) {}
 
 CastContentBrowserClient::~CastContentBrowserClient() {
 #if BUILDFLAG(IS_CAST_USING_CMA_BACKEND)

chromecast/browser/cast_content_browser_client.h

@@ -47,6 +47,7 @@ class X509Certificate;
 namespace chromecast {
 class CastService;
 class CastWindowManager;
+class CastFeatureListCreator;
 class MemoryPressureControllerImpl;
 
 namespace media {
@@ -68,7 +69,8 @@ class CastContentBrowserClient : public content::ContentBrowserClient {
  public:
   // Creates an implementation of CastContentBrowserClient. Platform should
   // link in an implementation as needed.
-  static std::unique_ptr<CastContentBrowserClient> Create();
+  static std::unique_ptr<CastContentBrowserClient> Create(
+      CastFeatureListCreator* cast_feature_list_creator);
 
   ~CastContentBrowserClient() override;
 
@@ -197,13 +199,17 @@ class CastContentBrowserClient : public content::ContentBrowserClient {
   bool ShouldEnableStrictSiteIsolation() override;
   std::vector<std::unique_ptr<content::NavigationThrottle>>
   CreateThrottlesForNavigation(content::NavigationHandle* handle) override;
+  CastFeatureListCreator* GetCastFeatureListCreator() {
+    return cast_feature_list_creator_;
+  }
 
 #if BUILDFLAG(USE_CHROMECAST_CDMS)
   virtual std::unique_ptr<::media::CdmFactory> CreateCdmFactory();
 #endif  // BUILDFLAG(USE_CHROMECAST_CDMS)
 
  protected:
-  CastContentBrowserClient();
+  explicit CastContentBrowserClient(
+      CastFeatureListCreator* cast_feature_list_creator);
 
   URLRequestContextFactory* url_request_context_factory() const {
     return url_request_context_factory_.get();
@@ -258,6 +264,8 @@ class CastContentBrowserClient : public content::ContentBrowserClient {
       resource_dispatcher_host_delegate_;
   std::unique_ptr<media::CmaBackendFactory> cma_backend_factory_;
 
+  CastFeatureListCreator* cast_feature_list_creator_;
+
   DISALLOW_COPY_AND_ASSIGN(CastContentBrowserClient);
 };
 

chromecast/browser/cast_content_browser_client_simple.cc

@@ -12,8 +12,10 @@ namespace chromecast {
 namespace shell {
 
 // static
-std::unique_ptr<CastContentBrowserClient> CastContentBrowserClient::Create() {
-  return base::WrapUnique(new CastContentBrowserClient());
+std::unique_ptr<CastContentBrowserClient> CastContentBrowserClient::Create(
+    CastFeatureListCreator* cast_feature_list_creator) {
+  return base::WrapUnique(
+      new CastContentBrowserClient(cast_feature_list_creator));
 }
 
 }  // namespace shell

chromecast/browser/cast_feature_list_creator.cc

+// Copyright 2018 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 "chromecast/browser/cast_feature_list_creator.h"
+
+#include "base/base_switches.h"
+#include "base/command_line.h"
+#include "chromecast/base/cast_features.h"
+#include "chromecast/base/pref_names.h"
+#include "chromecast/browser/metrics/cast_metrics_prefs.h"
+#include "chromecast/browser/pref_service_helper.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "components/proxy_config/pref_proxy_config_tracker_impl.h"
+
+namespace chromecast {
+
+CastFeatureListCreator::CastFeatureListCreator() {}
+
+CastFeatureListCreator::~CastFeatureListCreator() {}
+
+void CastFeatureListCreator::CreatePrefServiceAndFeatureList() {
+  DCHECK(!pref_service_);
+
+  scoped_refptr<PrefRegistrySimple> pref_registry(new PrefRegistrySimple());
+  metrics::RegisterPrefs(pref_registry.get());
+  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_registry.get());
+  pref_service_ =
+      shell::PrefServiceHelper::CreatePrefService(pref_registry.get());
+
+  const auto* features_dict =
+      pref_service_->GetDictionary(prefs::kLatestDCSFeatures);
+  const auto* experiment_ids =
+      pref_service_->GetList(prefs::kActiveDCSExperiments);
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+  InitializeFeatureList(
+      *features_dict, *experiment_ids,
+      command_line->GetSwitchValueASCII(switches::kEnableFeatures),
+      command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+}
+
+std::unique_ptr<PrefService> CastFeatureListCreator::TakePrefService() {
+  return std::move(pref_service_);
+}
+
+}  // namespace chromecast

chromecast/browser/cast_feature_list_creator.h

+// Copyright 2018 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.
+
+#ifndef CHROMECAST_BROWSER_CAST_FEATURE_LIST_CREATOR_H_
+#define CHROMECAST_BROWSER_CAST_FEATURE_LIST_CREATOR_H_
+
+#include <memory>
+
+class PrefService;
+
+namespace chromecast {
+
+// Creator for the singleton |FeatureList|. Stateful due to having to create and
+// hold a |PrefService| instance until |CastBrowserProcess| takes ownership when
+// the full browser process starts,
+class CastFeatureListCreator {
+ public:
+  CastFeatureListCreator();
+  virtual ~CastFeatureListCreator();
+
+  // Creates the |PrefService| and uses it to initialize |FeatureList|. Retains
+  // ownership of the |PrefService|.
+  void CreatePrefServiceAndFeatureList();
+
+  // Takes ownership of the |PrefService| previously created.
+  std::unique_ptr<PrefService> TakePrefService();
+
+ private:
+  // Holds the |PrefService| until TakePrefService() is called and ownership
+  // is taken away.
+  std::unique_ptr<PrefService> pref_service_;
+};
+
+}  // namespace chromecast
+
+#endif  // CHROMECAST_BROWSER_CAST_FEATURE_LIST_CREATOR_H_