Merge 3 different ways of obtaining first run state into a single one.

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 48b6a887bddbc3f5ea4aa6b324f4cc7edaabc670..4aa56836338a26d1d5272089213359cf418aa75f 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -553,7 +553,7 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       profile_(NULL),
       run_message_loop_(true),
       notify_result_(ProcessSingleton::PROCESS_NONE),
-      do_first_run_tasks_(false),
+      is_first_run_(false),
       local_state_(NULL),
       restart_last_session_(false) {
   // If we're running tests (ui_task is non-null).
@@ -821,10 +821,6 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
     user_data_dir_ = chrome::GetUserDataDir(parameters());
   }
 
-  // Whether this is First Run. |do_first_run_tasks_| should be prefered to this
-  // unless the desire is actually to know whether this is really First Run
-  // (i.e., even if --no-first-run is passed).
-  bool is_first_run = false;
   // Android's first run is done in Java instead of native.
 #if defined(OS_ANDROID)
   // Force MediaCaptureDevicesDispatcher created on UI thread.
@@ -833,13 +829,7 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   process_singleton_.reset(new ChromeProcessSingleton(
       user_data_dir_, base::Bind(&ProcessSingletonNotificationCallback)));
 
-  bool force_first_run =
-      parsed_command_line().HasSwitch(switches::kForceFirstRun);
-  bool force_skip_first_run_tasks =
-      (!force_first_run &&
-       parsed_command_line().HasSwitch(switches::kNoFirstRun));
-
-  is_first_run = force_first_run || first_run::IsChromeFirstRun();
+  is_first_run_ = first_run::IsChromeFirstRun();

[gab, 2013/07/26 15:41:13]

We could in fact completely get rid of is_first_run_ state and use static getter
first_run::IsChromeFirstRun() everywhere if that's preferable..?

[cpu_(ooo_6.6-7.5), 2013/07/27 21:16:31]

Yes please. You can have a function local is_first_run in case
IsChromeFirstRun() is doing disk IO but my last recollection is that it caches
the value.

[gab, 2013/07/29 17:39:50]

Done, indeed IsChromeFirstRun() caches the value.

 #endif
 
   scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
@@ -877,7 +867,7 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   }
 
   local_state_ = InitializeLocalState(
-      local_state_task_runner.get(), parsed_command_line(), is_first_run);
+      local_state_task_runner.get(), parsed_command_line(), is_first_run_);
 
   // These members must be initialized before returning from this function.
   master_prefs_.reset(new first_run::MasterPrefs);
@@ -966,66 +956,50 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
 #endif
 
   // Android does first run in Java instead of native.
-#if !defined(OS_ANDROID)
+  // Chrome OS has its own out-of-box-experience code.
+#if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)

[gab, 2013/07/26 15:41:13]

This comes from the old (removed in this CL) hidden side-effect in
ProcessMasterPreferences which I moved up here.

I think it's better to explicitly ifdef out things that shouldn't happen on
Chrome OS than to have do_first_run_tasks_ be set to false through some hidden
ifdef.

[cpu_(ooo_6.6-7.5), 2013/07/27 21:16:31]

Sure, It more along the lines of other things in this horrible horrible file.

   // On first run, we need to process the predictor preferences before the
   // browser's profile_manager object is created, but after ResourceBundle
   // is initialized.
-  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;
 
-    // Do first run tasks unless:
-    //  - Explicitly forced not to by |force_skip_first_run_tasks| or
-    //    |pmp_result|.
-    //  - Running in App (or App Launcher) mode, where showing the importer
-    //    (first run) UI is undesired.
-    do_first_run_tasks_ = (
-        !force_skip_first_run_tasks &&
-        pmp_result != first_run::SKIP_FIRST_RUN_TASKS &&
-        !parsed_command_line().HasSwitch(switches::kApp) &&

[gab, 2013/07/26 15:41:13]

We would previously set do_first_run_tasks_ to false if first run was in App
mode of AppLauncher mode.

I think this was previously done to avoid the import UI (now silent in
background -- i.e., AutoImport()) and avoid adding first run tabs...

Now a bunch of things have come under this logic which I don't think belong
here.... besides adding first run tabs, every other first run initialization
should happen even if running App Launcher or App mode first.

[cpu_(ooo_6.6-7.5), 2013/07/27 21:16:31]

In general I agree, but you should fire an email to benwells, it might be
possible to have a machine that has only IE and install a google calculator that
installs chrome under the hood. I don't know if that is ready or if we don't
plan to do that anymore. I guess if it works for a chromeframe only install it
should work with apps. This is just a suggestion.

[gab, 2013/07/29 17:39:50]

[cc'ing benwells here]

Right, so AFAIK this has been put to sleep for now while AppsV2 is brought up,
it will be revisited later.

Ideally we would have --no-first-run and --cancel-first-run. In most cases users
of that flag simply don't want first run now, but don't want to cancel it the
first time Chrome actually runs (i.e. CF, apps, tests, etc.).

We still need --cancel-first-run in some scenarios (i.e., self-destruct where we
don't want the system-level chrome being launched after destroying the
user-level chrome to launch in first run).

For now we only have --no-first-run which actually cancels first run from ever
happening in future runs of chrome and is likely too strong in most scenarios.

It will be very easy after this CL and https://codereview.chromium.org/20743002/
to implement separate --no-first-run and --cancel-first-run if required.

[gab, 2013/07/29 22:22:29]

Here https://codereview.chromium.org/21150006/ :)!

+    if (!parsed_command_line().HasSwitch(switches::kApp) &&
         !parsed_command_line().HasSwitch(switches::kAppId) &&
-        !parsed_command_line().HasSwitch(switches::kShowAppList));
-
-    if (do_first_run_tasks_) {
+        !parsed_command_line().HasSwitch(switches::kShowAppList)) {
       AddFirstRunNewTabs(browser_creator_.get(), master_prefs_->new_tabs);
+    }
 
-      // TODO(macourteau): refactor preferences that are copied from
-      // master_preferences into local_state, as a "local_state" section in
-      // master preferences. If possible, a generic solution would be prefered
-      // over a copy one-by-one of specific preferences. Also see related TODO
-      // in first_run.h.
-
-      // Store the initial VariationsService seed in local state, if it exists
-      // in master prefs.
-      if (!master_prefs_->variations_seed.empty()) {
-        local_state_->SetString(prefs::kVariationsSeed,
-                                master_prefs_->variations_seed);
-        // Set the variation seed date to the current system time. If the user's
-        // clock is incorrect, this may cause some field trial expiry checks to
-        // not do the right thing until the next seed update from the server,
-        // when this value will be updated.
-        local_state_->SetInt64(prefs::kVariationsSeedDate,
-                               base::Time::Now().ToInternalValue());
-      }
-
-      if (!master_prefs_->suppress_default_browser_prompt_for_version.empty()) {
-        local_state_->SetString(
-            prefs::kBrowserSuppressDefaultBrowserPrompt,
-            master_prefs_->suppress_default_browser_prompt_for_version);
-      }
-
-      AppListService::Get()->HandleFirstRun();
+    // TODO(macourteau): refactor preferences that are copied from
+    // master_preferences into local_state, as a "local_state" section in

[cpu_(ooo_6.6-7.5), 2013/07/27 21:16:31]

btw, I pinged him on his 3 bugs (130598, 180304, 230115)

+    // master preferences. If possible, a generic solution would be prefered
+    // over a copy one-by-one of specific preferences. Also see related TODO
+    // in first_run.h.
+
+    // Store the initial VariationsService seed in local state, if it exists
+    // in master prefs.
+    if (!master_prefs_->variations_seed.empty()) {
+      local_state_->SetString(prefs::kVariationsSeed,
+                              master_prefs_->variations_seed);
+      // Set the variation seed date to the current system time. If the user's
+      // clock is incorrect, this may cause some field trial expiry checks to
+      // not do the right thing until the next seed update from the server,
+      // when this value will be updated.
+      local_state_->SetInt64(prefs::kVariationsSeedDate,
+                             base::Time::Now().ToInternalValue());
     }
 
-    if (do_first_run_tasks_ ||
-        parsed_command_line().HasSwitch(switches::kNoFirstRun)) {
-      // Create the First Run sentinel whether first run tasks are executed
-      // or not.
-      first_run::CreateSentinel();

[gab, 2013/07/26 15:41:13]

Now handled inside first_run::IsChromeFirstRun(); no point having browser main
care about setting this internal state of first_run.

Note: first_run::CreateSentinel() is further moved to first_run::internal:: in
https://codereview.chromium.org/20743002/

+    if (!master_prefs_->suppress_default_browser_prompt_for_version.empty()) {
+      local_state_->SetString(
+          prefs::kBrowserSuppressDefaultBrowserPrompt,
+          master_prefs_->suppress_default_browser_prompt_for_version);
     }
+
+    AppListService::Get()->HandleFirstRun();
   }
 #endif
 
@@ -1357,13 +1331,13 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
   // Create an instance of GpuModeManager to watch gpu mode pref change.
   g_browser_process->gpu_mode_manager();
 
-#if !defined(OS_ANDROID)
+#if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
   // Show the First Run UI if this is the first time Chrome has been run on
   // this computer, or we're being compelled to do so by a command line flag.
   // 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 (do_first_run_tasks_) {
+  if (is_first_run_) {
     first_run::AutoImport(profile_,
                           master_prefs_->homepage_defined,
                           master_prefs_->do_import_items,
@@ -1379,8 +1353,8 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     } else {
       browser_creator_->set_is_default_browser_dialog_suppressed(true);
     }
-  }  // if (do_first_run_tasks_)
-#endif  // !defined(OS_ANDROID)
+  }  // if (is_first_run_)
+#endif  // !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 
 #if defined(OS_WIN)
   // Sets things up so that if we crash from this point on, a dialog will
@@ -1413,12 +1387,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 = do_first_run_tasks_ ? master_prefs_->ping_delay :
+  int ping_delay = is_first_run_ ? 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_, do_first_run_tasks_, ping_delay < 0,
+      profile_, is_first_run_, ping_delay < 0,
       base::TimeDelta::FromMilliseconds(abs(ping_delay)));
 #endif  // defined(ENABLE_RLZ) && !defined(OS_CHROMEOS)
 
@@ -1641,7 +1615,7 @@ bool ChromeBrowserMainParts::MainMessageLoopRun(int* result_code) {
   // These should be invoked as close to the start of the browser's
   // UI thread message loop as possible to get a stable measurement
   // across versions.
-  RecordBrowserStartupTime(do_first_run_tasks_);
+  RecordBrowserStartupTime(is_first_run_);
   startup_timer_->SignalStartupComplete(
       performance_monitor::StartupTimer::STARTUP_NORMAL);