[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 dd7f5fae3b42bea51ec3ee79fbf64aa8907fa59a..138406ef6c1dbfb85493bfef9873ee68126e2e95 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -489,8 +489,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,16 +689,22 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
       << "Must be able to get user data directory!";
 #endif
 
+  bool is_first_run = false;
   // Android's first run is done in Java instead of native.
 #if !defined(OS_ANDROID)
+  // Create Sentinel if no-first-run argument is passed in (note that First Run
+  // might still occur if --force-first-run is also passed in).
+  if (parsed_command_line().HasSwitch(switches::kNoFirstRun))
+    first_run::CreateSentinel();
+
   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() ||

[cpu_(ooo_6.6-7.5), 2012/12/27 23:53:05]

Should first_run::IsChromeFirstRun() be changed as well? the ideal situation
would be for that function to be removed in favor of consulting your new
variable.

[gab, 2012/12/28 20:17:46]

is_first_run will be true if this is actually first run (i.e. sentinel is not
present); this is what first_run::IsChromeFirstRun() is checking and I think
this is correct.

do_first_run_tasks_ will later be true if it is first run AND we don't want to
skip the first run tasks for some other reasons... this is where things changed
as very few things should happen solely because it's actually first run when
skipping the first run tasks as been requested.

-          parsed_command_line().HasSwitch(switches::kFirstRun)) &&
+       parsed_command_line().HasSwitch(switches::kForceFirstRun)) &&
       !ProfileManager::IsImportProcess(parsed_command_line());
 #endif
 
@@ -733,7 +738,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);
@@ -800,29 +805,23 @@ 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_) {
+  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);
 
     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_ &&
-        (parsed_command_line().HasSwitch(switches::kApp) ||
-         parsed_command_line().HasSwitch(switches::kAppId) ||
-         parsed_command_line().HasSwitch(switches::kNoFirstRun)))
-      first_run_ui_bypass_ = true;
-
-    // Create Sentinel if no-first-run argument is passed in.
-    if (parsed_command_line().HasSwitch(switches::kNoFirstRun))
-      first_run::CreateSentinel();

[cpu_(ooo_6.6-7.5), 2012/12/27 23:53:05]

I am a bit confused here, I expected the code 824-825 not to exist according to
the CL description

"2) First Run sentinel is set so that first run never happens for this new
Chrome. "

What I see in the CL is that it just happens earlier. Was this late setting a
bug?

[gab, 2012/12/28 20:17:46]

Well in the description I stated what I need as an end goal; you are right,
point (2) was already working, but I stated it to be clear on why it's still
there.

The problem is that kNoFirstRun didn't also prevent some first run tasks from
happening (i.e. everything that is_first_run_ used to trigger)...

I moved it to the top to drop the sentinel earlier (and not have to worry about
special logic later to check kNoFirstRun again). I realized when giving this
code another look that it is now overly restrictive (it also prevents the things
that also want to know whether "it is really first run" from knowing). I moved
the creation of the beacon back to here so that now:

--no-first-run => Do absolute must do first run initialization (create default
profile preferences and InitializeLocalState()); drop the First Run beacon; and
skip all other first run tasks (as long as it is first run of course; if it
isn't --no-first-run has no effect).

--force-first-run => Do everything first run related no matter what (whether the
beacon is present or --no-first-run is on the command line).

+    if (parsed_command_line().HasSwitch(switches::kApp) ||
+        parsed_command_line().HasSwitch(switches::kAppId)) {
+      do_first_run_tasks_ = false;
+    }
   }
 #endif
 
@@ -884,10 +883,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()
@@ -903,16 +898,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();
@@ -1122,7 +1107,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();
@@ -1192,31 +1177,14 @@ 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());
+    // Note: this can pop the first run consent dialog on linux.
+    first_run::DoPostImportTasks(profile_, master_prefs_->make_chrome_default);
 
     browser_process_->profile_manager()->OnImportFinished(profile_);
 
@@ -1226,7 +1194,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)
@@ -1260,12 +1228,12 @@ 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());
   // Negative ping delay means to send ping immediately after a first search is
   // recorded.
   RLZTracker::InitRlzFromProfileDelayed(
-      profile_, is_first_run_, ping_delay < 0,
+      profile_, do_first_run_tasks_, ping_delay < 0,
       base::TimeDelta::FromMilliseconds(abs(ping_delay)));
 #endif  // defined(ENABLE_RLZ) && !defined(OS_CHROMEOS)
 
@@ -1362,7 +1330,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