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

chrome/browser/chrome_browser_main.cc

 // Copyright (c) 2012 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 "chrome/browser/chrome_browser_main.h"
 
 #if defined(TOOLKIT_GTK)
 #include <gtk/gtk.h>
 #endif
 
@@ skipping 471 matching lines @@
       result_code_(content::RESULT_CODE_NORMAL_EXIT),
       startup_watcher_(new StartupTimeBomb()),
       shutdown_watcher_(new ShutdownWatcherHelper()),
       startup_timer_(new performance_monitor::StartupTimer()),
       browser_field_trials_(parameters.command_line),
       record_search_engine_(false),
       translate_manager_(NULL),
       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).
   if (parameters.ui_task)
     browser_defaults::enable_help_app = false;
 
   // Chrome disallows cookies by default. All code paths that want to use
   // cookies need to go through one of Chrome's URLRequestContexts which have
   // a ChromeNetworkDelegate attached that selectively allows cookies again.
   if (!disable_enforcing_cookie_policies_for_tests_)
@@ skipping 179 matching lines @@
 #else
   // Getting the user data dir can fail if the directory isn't
   // creatable, for example; on Windows in code below we bring up a
   // dialog prompting the user to pick a different directory.
   // However, ProcessSingleton needs a real user_data_dir on Mac/Linux,
   // so it's better to fail here than fail mysteriously elsewhere.
   CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir_))
       << "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
 
   FilePath local_state_path;
   CHECK(PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path));
   scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
       JsonPrefStore::GetTaskRunnerForFile(local_state_path,
                                           BrowserThread::GetBlockingPool());
   browser_process_.reset(new BrowserProcessImpl(local_state_task_runner,
                                                 parsed_command_line()));
@@ skipping 13 matching lines @@
   }
 
   if (parsed_command_line().HasSwitch(switches::kProfilingOutputFile)) {
     tracking_objects_.set_output_file_path(
         parsed_command_line().GetSwitchValuePath(
             switches::kProfilingOutputFile));
   }
 
   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);
 
 #if !defined(OS_ANDROID)
   // Android doesn't use StartupBrowserCreator.
   browser_creator_.reset(new StartupBrowserCreator);
   // TODO(yfriedman): Refactor Android to re-use UMABrowsingActivityObserver
   chrome::UMABrowsingActivityObserver::Init();
 #endif
@@ skipping 46 matching lines @@
 
 #if defined(TOOLKIT_GTK)
   g_set_application_name(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME).c_str());
 #endif
 
   // Android does first run in Java instead of native.
 #if !defined(OS_ANDROID)
   // 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
 
   // TODO(viettrungluu): why don't we run this earlier?
   if (!parsed_command_line().HasSwitch(switches::kNoErrorDialogs))
     WarnAboutMinimumSystemRequirements();
 
 #if defined(OS_LINUX) || defined(OS_OPENBSD) || defined(OS_MACOSX)
   // Set the product channel for crash reports.
   child_process_logging::SetChannel(
@@ skipping 41 matching lines @@
     chrome_extra_parts_[i]->PreMainMessageLoopRun();
 }
 
 // PreMainMessageLoopRun calls these extra stages in the following order:
 //  PreMainMessageLoopRunImpl()
 //   ... initial setup, including browser_process_ setup.
 //   PreProfileInit()
 //   ... additional setup, including CreateProfile()
 //   PostProfileInit()
 //   ... additional setup
-//   PreInteractiveFirstRunInit()
-//   ... first_run::AutoImport()
-//   PostInteractiveFirstRunInit()
-//   ... additional setup
 //   PreBrowserStart()
 //   ... browser_creator_->Start (OR parameters().ui_task->Run())
 //   PostBrowserStart()
 
 void ChromeBrowserMainParts::PreProfileInit() {
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PreProfileInit();
 }
 
 void ChromeBrowserMainParts::PostProfileInit() {
   LaunchDevToolsHandlerIfNeeded(profile(), parsed_command_line());
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     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();
 #if !defined(OS_ANDROID)
   gpu_util::InstallBrowserMonitor();
 #endif
 }
 
 void ChromeBrowserMainParts::PostBrowserStart() {
 #if !defined(OS_ANDROID)
@@ skipping 189 matching lines @@
         ShellIntegration::SetAsDefaultBrowser();
     }
 #else
     // We don't support retention experiments on Mac or Linux.
     return content::RESULT_CODE_NORMAL_EXIT;
 #endif  // defined(OS_WIN)
   }
 
   // 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();
   }
 
   profile_ = CreateProfile(parameters(), user_data_dir_, parsed_command_line());
   if (!profile_)
     return content::RESULT_CODE_NORMAL_EXIT;
 
 #if defined(ENABLE_BACKGROUND)
@@ skipping 49 matching lines @@
   // blacklist decisions.
   if (g_browser_process->gl_string_manager())
     g_browser_process->gl_string_manager()->Initialize();
 
 #if !defined(OS_ANDROID)
   // 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 (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_);
 
     if (!master_prefs_->suppress_first_run_default_browser_prompt) {
       browser_creator_->set_show_main_browser_window(
           !chrome::ShowFirstRunDefaultBrowserPrompt(profile_));
     } 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)
   // Sets things up so that if we crash from this point on, a dialog will
   // popup asking the user to restart chrome. It is done this late to avoid
   // testing against a bunch of special cases that are taken care early on.
   ChromeBrowserMainPartsWin::PrepareRestartOnCrashEnviroment(
       parsed_command_line());
 
   // Registers Chrome with the Windows Restart Manager, which will restore the
@@ skipping 13 matching lines @@
         base::Bind(&NetworkProfileBubble::CheckNetworkProfile,
                    profile_->GetPath()));
   }
 #endif  // OS_WIN
 
 #if defined(ENABLE_RLZ) && !defined(OS_CHROMEOS)
   // Init the RLZ library. This just binds the dll and schedules a task on the
   // 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)
 
   // Configure modules that need access to resources.
   net::NetModule::SetResourceProvider(chrome_common_net::NetResourceProvider);
 
   // In unittest mode, this will do nothing.  In normal mode, this will create
   // the global IntranetRedirectDetector instance, which will promptly go to
   // sleep for seven seconds (to avoid slowing startup), and wake up afterwards
   // to see if it should do anything else.
@@ skipping 76 matching lines @@
   // ThreadWatcher takes over or when browser is shutdown or when
   // startup_watcher_ is deleted.
   startup_watcher_->Arm(base::TimeDelta::FromSeconds(300));
 
   // Start watching for a hang.
   MetricsService::LogNeedForCleanShutdown();
 
 #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
   // the service process if needed. This is needed because the service process
   // might have shutdown because an update was available.
   // TODO(torne): this should maybe be done with
   // ProfileKeyedServiceFactory::ServiceIsCreatedWithProfile() instead?
 #if !defined(OS_ANDROID)
   CloudPrintProxyServiceFactory::GetForProfile(profile_);
 #endif
@@ skipping 311 matching lines @@
     if (base::win::GetVersion() <= base::win::VERSION_XP)
       uma_name += "_XP";
 
     uma_name += "_PreRead_";
     uma_name += pre_read_percentage;
     AddPreReadHistogramTime(uma_name.c_str(), time);
   }
 #endif
 #endif
 }