[Fixit Dec-2012] Refactor first_run, very few things should depend on whether the First Run senti...

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 3c4168a35127b1b3802a85cdd36ac8482d0b578e..e42076dc82a044a39b964a5b85cd7561a89934c8 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -488,8 +488,7 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       profile_(NULL),
       run_message_loop_(true),
       notify_result_(ProcessSingleton::PROCESS_NONE),
-      is_first_run_(false),
-      first_run_ui_bypass_(false),
+      do_first_run_tasks_(false),
       local_state_(NULL),
       restart_last_session_(false) {
   // If we're running tests (ui_task is non-null).
@@ -690,13 +689,14 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
 #endif
 
   // Android's first run is done in Java instead of native.
+  bool is_first_run = false;
 #if !defined(OS_ANDROID)
   process_singleton_.reset(new ProcessSingleton(user_data_dir_));
   // Ensure ProcessSingleton won't process messages too early. It will be
   // unlocked in PostBrowserStart().
   process_singleton_->Lock(NULL);
 
-  is_first_run_ =
+  is_first_run =
       (first_run::IsChromeFirstRun() ||
           parsed_command_line().HasSwitch(switches::kFirstRun)) &&
       !ProfileManager::IsImportProcess(parsed_command_line());
@@ -732,7 +732,7 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
 
   local_state_ = InitializeLocalState(local_state_task_runner,
                                       parsed_command_line(),
-                                      is_first_run_);
+                                      is_first_run);
 
   // These members must be initialized before returning from this function.
   master_prefs_.reset(new first_run::MasterPrefs);
@@ -798,29 +798,36 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   // On first run, we need to process the predictor preferences before the
   // browser's profile_manager object is created, but after ResourceBundle
   // is initialized.
-  first_run_ui_bypass_ = false;  // True to skip first run UI.
-  if (is_first_run_) {
+  // True to do first run tasks. This should be prefered to is_first_run
+  // unless the desire is actually to know whether this is really first run
+  // (i.e. even if --no-first-run is passed).
+  do_first_run_tasks_ = false;
+  if (is_first_run) {
     first_run::ProcessMasterPreferencesResult pmp_result =
         first_run::ProcessMasterPreferences(user_data_dir_,
                                             master_prefs_.get());
     if (pmp_result == first_run::EULA_EXIT_NOW)
       return chrome::RESULT_CODE_EULA_REFUSED;
 
-    first_run_ui_bypass_ = (pmp_result == first_run::SKIP_FIRST_RUN);
+    do_first_run_tasks_ =
+        (pmp_result != first_run::SKIP_FIRST_RUN_TASKS ||
+         parsed_command_line().HasSwitch(switches::kFirstRun));
 
     AddFirstRunNewTabs(browser_creator_.get(), master_prefs_->new_tabs);
 
     // If we are running in App mode, we do not want to show the importer
     // (first run) UI.
-    if (!first_run_ui_bypass_ &&
+    if (do_first_run_tasks_ &&
         (parsed_command_line().HasSwitch(switches::kApp) ||
-         parsed_command_line().HasSwitch(switches::kAppId) ||
-         parsed_command_line().HasSwitch(switches::kNoFirstRun)))
-      first_run_ui_bypass_ = true;
+         parsed_command_line().HasSwitch(switches::kAppId))) {
+      do_first_run_tasks_ = false;
+    }
 
     // Create Sentinel if no-first-run argument is passed in.
-    if (parsed_command_line().HasSwitch(switches::kNoFirstRun))
+    if (parsed_command_line().HasSwitch(switches::kNoFirstRun)) {
+      do_first_run_tasks_ = false;
       first_run::CreateSentinel();
+    }
   }
 #endif
 
@@ -882,10 +889,6 @@ void ChromeBrowserMainParts::PreMainMessageLoopRun() {
 //   ... additional setup, including CreateProfile()
 //   PostProfileInit()
 //   ... additional setup
-//   PreInteractiveFirstRunInit()
-//   ... first_run::AutoImport()
-//   PostInteractiveFirstRunInit()
-//   ... additional setup
 //   PreBrowserStart()
 //   ... browser_creator_->Start (OR parameters().ui_task->Run())
 //   PostBrowserStart()
@@ -901,16 +904,6 @@ void ChromeBrowserMainParts::PostProfileInit() {
     chrome_extra_parts_[i]->PostProfileInit();
 }
 
-void ChromeBrowserMainParts::PreInteractiveFirstRunInit() {
-  for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
-    chrome_extra_parts_[i]->PreInteractiveFirstRunInit();
-}
-
-void ChromeBrowserMainParts::PostInteractiveFirstRunInit() {
-  for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
-    chrome_extra_parts_[i]->PostInteractiveFirstRunInit();
-}
-
 void ChromeBrowserMainParts::PreBrowserStart() {
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PreBrowserStart();
@@ -1120,7 +1113,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 
   // Profile creation ----------------------------------------------------------
 
-  if (is_first_run_) {
+  if (do_first_run_tasks_) {
     // Warn the ProfileManager that an import process will run, possibly
     // locking the WebDataService directory of the next Profile created.
     browser_process_->profile_manager()->SetWillImport();
@@ -1190,31 +1183,13 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // Note that this be done _after_ the PrefService is initialized and all
   // preferences are registered, since some of the code that the importer
   // touches reads preferences.
-  if (is_first_run_) {
-    PreInteractiveFirstRunInit();
-
-    if (!first_run_ui_bypass_ ||
-        parsed_command_line().HasSwitch(switches::kFirstRunForceImport)) {
-      first_run::AutoImport(profile_,
-                            master_prefs_->homepage_defined,
-                            master_prefs_->do_import_items,
-                            master_prefs_->dont_import_items,
-                            process_singleton_.get());
-      first_run::DoFirstRunTasks(profile_, master_prefs_->make_chrome_default);
-#if defined(OS_POSIX) && !defined(OS_CHROMEOS)
-      // TODO(thakis): Look into moving this POSIX-specific section to
-      // ChromeBrowserMainPartsPosix::PostInteractiveFirstRunInit().
-
-      // On Windows, the download is tagged with enable/disable stats so there
-      // is no need for this code.
-
-      // If stats reporting was turned on by the first run dialog then toggle
-      // the pref.
-      if (GoogleUpdateSettings::GetCollectStatsConsent())
-        local_state_->SetBoolean(prefs::kMetricsReportingEnabled, true);
-#endif  // OS_POSIX && !OS_CHROMEOS
-    }  // if (!first_run_ui_bypass_)
-    PostInteractiveFirstRunInit();
+  if (do_first_run_tasks_) {
+    first_run::AutoImport(profile_,
+                          master_prefs_->homepage_defined,
+                          master_prefs_->do_import_items,
+                          master_prefs_->dont_import_items,
+                          process_singleton_.get());
+    first_run::DoPostImportTasks(profile_, master_prefs_->make_chrome_default);

[Nico, 2012/12/21 21:44:22]

This makes the windows code not run when --no-first-run is passed in, while it
was run in that case before. Is this intentional? I remember you on an earlier
review saying that you want that to run even with the flag.

Also, a comment "// This can open the frist run consent dialog" would probably
be good.

[gab, 2012/12/21 22:10:35]

Yes this is intentional (and one of the intents of this CL), we need that
--no-first-run actually mean that no first run tasks will occur.
Which dialog, the only dialog that can pop from reading the code is a consent
dialog for the metrics on Linux (added a comment in first_run.h).

[Nico, 2012/12/21 22:13:05]

Yes, that dialog.

I'd add the comment here, not in the .h. It's not obvious from reading the code
here, and most people won't look at the .h in parallel I think.

[gab, 2012/12/21 22:21:37]

Ok added the comment here as well (left it on the header in case someone wants
to full definition of the method).

 
     browser_process_->profile_manager()->OnImportFinished(profile_);
 
@@ -1224,7 +1199,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     } else {
       browser_creator_->set_is_default_browser_dialog_suppressed(true);
     }
-  }  // if (is_first_run_)
+  }  // if (do_first_run_tasks_)
 #endif  // !defined(OS_ANDROID)
 
 #if defined(OS_WIN)
@@ -1258,9 +1233,10 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // file thread to be run sometime later. If this is the first run we record
   // the installation event.
   PrefService* pref_service = profile_->GetPrefs();
-  int ping_delay = is_first_run_ ? master_prefs_->ping_delay :
+  int ping_delay = do_first_run_tasks_ ? master_prefs_->ping_delay :
       pref_service->GetInteger(first_run::GetPingDelayPrefName().c_str());
-  RLZTracker::InitRlzFromProfileDelayed(profile_, is_first_run_, ping_delay);
+  RLZTracker::InitRlzFromProfileDelayed(profile_, do_first_run_tasks_,
+                                        ping_delay);
 #endif  // defined(ENABLE_RLZ) && !defined(OS_CHROMEOS)
 
   // Configure modules that need access to resources.
@@ -1356,7 +1332,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
 #if defined(OS_WIN)
   // We check this here because if the profile is OTR (chromeos possibility)
   // it won't still be accessible after browser is destroyed.
-  record_search_engine_ = is_first_run_ && !profile_->IsOffTheRecord();
+  record_search_engine_ = do_first_run_tasks_ && !profile_->IsOffTheRecord();
 #endif
 
   // Create the instance of the cloud print proxy service so that it can launch