Small cleanup for ChromeBrowserMain:

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
 
 #include <string>
 #include <vector>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/debug/debugger.h"
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/prefs/json_pref_store.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/pref_value_store.h"
@@ skipping 55 matching lines @@
 #include "chrome/browser/prefs/chrome_pref_service_factory.h"
 #include "chrome/browser/prefs/command_line_pref_store.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_proxy_service.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_proxy_service_factory.h"
 #include "chrome/browser/process_singleton.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_render_view_host_observer.h"
 #include "chrome/browser/search_engines/search_engine_type.h"
-#include "chrome/browser/search_engines/template_url.h"
-#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
-#include "chrome/browser/search_engines/template_url_service.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/service/service_process_control.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/three_d_api_observer.h"
 #include "chrome/browser/translate/translate_manager.h"
 #include "chrome/browser/ui/app_list/app_list_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/startup/default_browser_prompt.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
@@ skipping 89 matching lines @@
 #endif
 
 #if defined(ENABLE_RLZ)
 #include "chrome/browser/rlz/rlz.h"
 #endif
 
 #if defined(TOOLKIT_VIEWS)
 #include "ui/views/focus/accelerator_handler.h"
 #endif
 
-#if defined(USE_X11)
-#include "chrome/browser/chrome_browser_main_x11.h"
-#endif
-
 using content::BrowserThread;
 
 namespace {
 
 // This function provides some ways to test crash and assertion handling
 // behavior of the program.
 void HandleTestParameters(const CommandLine& command_line) {
   // This parameter causes an assertion.
   if (command_line.HasSwitch(switches::kBrowserAssertTest)) {
     DCHECK(false);
@@ skipping 334 matching lines @@
 ChromeBrowserMainParts::ChromeBrowserMainParts(
     const content::MainFunctionParams& parameters)
     : parameters_(parameters),
       parsed_command_line_(parameters.command_line),
       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),
       rvh_callback_(base::Bind(&RenderViewHostCreated)),
-      record_search_engine_(false),
       translate_manager_(NULL),
       profile_(NULL),
       run_message_loop_(true),
       notify_result_(ProcessSingleton::PROCESS_NONE),
       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;
@@ skipping 115 matching lines @@
 #if defined(OS_CHROMEOS)
   chromeos::CrosSettings::Get()->GetBoolean(chromeos::kStatsReportingPref,
                                             &enabled);
 #else
   enabled = local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
 #endif  // #if defined(OS_CHROMEOS)
 #endif  // defined(GOOGLE_CHROME_BUILD)
   return enabled;
 }
 
+void ChromeBrowserMainParts::RecordBrowserStartupTime(bool is_first_run) {
+  // Don't record any metrics if UI was displayed before this point e.g.
+  // warning dialogs.
+  if (startup_metric_utils::WasNonBrowserUIDisplayed())
+    return;
+
+// CurrentProcessInfo::CreationTime() is currently only implemented on Mac and
+// Windows.
+#if defined(OS_MACOSX) || defined(OS_WIN)
+  const base::Time* process_creation_time =
+      base::CurrentProcessInfo::CreationTime();
+
+  if (!is_first_run && process_creation_time) {
+    RecordPreReadExperimentTime("Startup.BrowserMessageLoopStartTime",
+        base::Time::Now() - *process_creation_time);
+  }
+#endif  // defined(OS_MACOSX) || defined(OS_WIN)
+
+  // Record collected startup metrics.
+  startup_metric_utils::OnBrowserStartupComplete(is_first_run);
+
+  // Deletes self.
+  new LoadCompleteListener();
+}
+
+// This code is specific to the Windows-only PreReadExperiment field-trial.
+void ChromeBrowserMainParts::RecordPreReadExperimentTime(const char* name,
+                                                         base::TimeDelta time) {
+  DCHECK(name != NULL);
+
+  // This gets called with different histogram names, so we don't want to use
+  // the UMA_HISTOGRAM_CUSTOM_TIMES macro--it uses a static variable, and the
+  // first call wins.
+  AddPreReadHistogramTime(name, time);
+
+#if defined(OS_WIN)
+#if defined(GOOGLE_CHROME_BUILD)
+  // The pre-read experiment is Windows and Google Chrome specific.
+  scoped_ptr<base::Environment> env(base::Environment::Create());
+
+  // Only record the sub-histogram result if the experiment is running
+  // (environment variable is set, and valid).
+  std::string pre_read_percentage;
+  if (env->GetVar(chrome::kPreReadEnvironmentVariable, &pre_read_percentage)) {
+    std::string uma_name(name);
+
+    // We want XP to record a separate histogram, as the loader on XP
+    // is very different from the Vista and Win7 loaders.
+    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
+}
+
 // -----------------------------------------------------------------------------
 // TODO(viettrungluu): move more/rest of BrowserMain() into BrowserMainParts.
 
 #if defined(OS_WIN)
 #define DLLEXPORT __declspec(dllexport)
 
 // We use extern C for the prototype DLLEXPORT to avoid C++ name mangling.
 extern "C" {
 DLLEXPORT void __cdecl RelaunchChromeBrowserWithNewCommandLineIfNeeded();
 }
 
 DLLEXPORT void __cdecl RelaunchChromeBrowserWithNewCommandLineIfNeeded() {
   // Need an instance of AtExitManager to handle singleton creations and
   // deletions.  We need this new instance because, the old instance created
   // in ChromeMain() got destructed when the function returned.
   base::AtExitManager exit_manager;
   upgrade_util::RelaunchChromeBrowserWithNewCommandLineIfNeeded();
 }
 #endif
 
 // content::BrowserMainParts implementation ------------------------------------
 
 void ChromeBrowserMainParts::PreEarlyInitialization() {
-#if defined(USE_X11)
-  SetBrowserX11ErrorHandlersPreEarlyInitialization();
-#endif

[Nico, 2013/07/10 17:32:50]

I would've guessed that this is here to make sure it runs as the first extra
part (and chrome_content_browser_client.cc now adds the extra x11 part as last
component). Then again, it was added in
https://chromiumcodereview.appspot.com/17084014 by oshima and neither that CL
description nor the current code have a comment to that end, so I suppose this
is fine, but +oshima just in case.

[jam, 2013/07/10 17:47:02]

I didn't see any indication that it was put here because of that. if it really
needs to be run before other extra parts, then we can change the order that it's
added

[oshima, 2013/07/10 18:50:37]

SetBrowserX11ErrorHandlersPreEarlyInitialization should be called as early as
possible (but less critical)
and SetBrowserX11ErrorHandlersPostMainMessageLoopStart() should be called as
late as possible (more critical).

in which order X11 parts will be called?

   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreEarlyInitialization");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PreEarlyInitialization();
 }
 
 void ChromeBrowserMainParts::PostEarlyInitialization() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PostEarlyInitialization");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PostEarlyInitialization();
 }
 
 void ChromeBrowserMainParts::ToolkitInitialized() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::ToolkitInitialized");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->ToolkitInitialized();
 }
 
 void ChromeBrowserMainParts::PreMainMessageLoopStart() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreMainMessageLoopStart");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PreMainMessageLoopStart();
 }
 
 void ChromeBrowserMainParts::PostMainMessageLoopStart() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PostMainMessageLoopStart");
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PostMainMessageLoopStart();
-#if defined(USE_X11)
-  SetBrowserX11ErrorHandlersPostMainMessageLoopStart();
-#endif
 }
 
 int ChromeBrowserMainParts::PreCreateThreads() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PreCreateThreads");
   result_code_ = PreCreateThreadsImpl();
   // These members must be initialized before returning from this function.
   DCHECK(master_prefs_.get());
 #if !defined(OS_ANDROID)
   DCHECK(browser_creator_.get());
 #endif
@@ skipping 210 matching lines @@
 
     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();
     }
   }
 #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(
       chrome::VersionInfo::GetVersionStringModifier());
 #endif
 
   // Initialize tracking synchronizer system.
   tracking_synchronizer_ = new chrome_browser_metrics::TrackingSynchronizer();
 
   // Now that all preferences have been registered, set the install date
@@ skipping 424 matching lines @@
 
 #if defined(OS_WIN) && !defined(GOOGLE_CHROME_BUILD)
   if (parsed_command_line().HasSwitch(switches::kDebugPrint)) {
     base::FilePath path =
         parsed_command_line().GetSwitchValuePath(switches::kDebugPrint);
     printing::PrintedDocument::set_debug_dump_path(path);
   }
 #endif
 
   HandleTestParameters(parsed_command_line());
-  RecordBreakpadStatusUMA(browser_process_->metrics_service());
+  browser_process_->metrics_service()->RecordBreakpadHasDebugger(
+      base::debug::BeingDebugged());

[Nico, 2013/07/10 17:32:50]

On Mac and Linux, this used to call
metrics->RecordBreakpadRegistration(IsCrashReporterEnabled()) too. Can that call
just be dropped?

Add BUG=139023 for the Android version.

[jam, 2013/07/10 17:47:02]

not quite. mac and linux used to do this as well, see
line 154
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...

and line 86
https://codereview.chromium.org/18967002/diff/45002/chrome/browser/chrome_bro...

+
 #if defined(ENABLE_LANGUAGE_DETECTION)
   LanguageUsageMetrics::RecordAcceptLanguages(
       profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
   LanguageUsageMetrics::RecordApplicationLanguage(
       browser_process_->GetApplicationLocale());
 #endif
 
   // The extension service may be available at this point. If the command line
   // specifies --uninstall-extension, attempt the uninstall extension startup
   // action.
@@ skipping 11 matching lines @@
   startup_watcher_->Arm(base::TimeDelta::FromSeconds(300));
 
   // On mobile, need for clean shutdown arises only when the application comes
   // to foreground (i.e. MetricsService::OnAppEnterForeground is called).
   // http://crbug.com/179143
 #if !defined(OS_ANDROID)
   // Start watching for a hang.
   MetricsService::LogNeedForCleanShutdown();
 #endif
 
-#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_ = 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
   // BrowserContextKeyedServiceFactory::ServiceIsCreatedWithBrowserContext()
   // instead?
 #if !defined(OS_ANDROID)
   CloudPrintProxyServiceFactory::GetForProfile(profile_);
 #endif
 
@@ skipping 157 matching lines @@
 }
 
 void ChromeBrowserMainParts::PostMainMessageLoopRun() {
   TRACE_EVENT0("startup", "ChromeBrowserMainParts::PostMainMessageLoopRun");
 #if defined(OS_ANDROID)
   // Chrome on Android does not use default MessageLoop. It has its own
   // Android specific MessageLoop
   NOTREACHED();
 #else
 
-#if defined(USE_X11)
-  // Unset the X11 error handlers. The X11 error handlers log the errors using a
-  // |PostTask()| on the message-loop. But since the message-loop is in the
-  // process of terminating, this can cause errors.
-  UnsetBrowserX11ErrorHandlers();
-#endif
-
   // Start watching for jank during shutdown. It gets disarmed when
   // |shutdown_watcher_| object is destructed.
   shutdown_watcher_->Arm(base::TimeDelta::FromSeconds(300));
 
   // Disarm the startup hang detector time bomb if it is still Arm'ed.
   startup_watcher_->Disarm();
 
   for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
     chrome_extra_parts_[i]->PostMainMessageLoopRun();
 
-#if defined(OS_WIN)
-  // Log the search engine chosen on first run. Do this at shutdown, after any
-  // changes are made from the first run bubble link, etc.
-  if (record_search_engine_) {
-    TemplateURLService* url_service =
-        TemplateURLServiceFactory::GetForProfile(profile_);
-    const TemplateURL* default_search_engine =
-        url_service->GetDefaultSearchProvider();
-    // The default engine can be NULL if the administrator has disabled
-    // default search.
-    SearchEngineType search_engine_type =
-        TemplateURLPrepopulateData::GetEngineType(default_search_engine ?
-            default_search_engine->url() : std::string());
-    // Record the search engine chosen.
-    UMA_HISTOGRAM_ENUMERATION("Chrome.SearchSelectExempt", search_engine_type,
-                              SEARCH_ENGINE_MAX);
-  }
-#endif
-
   // Some tests don't set parameters.ui_task, so they started translate
   // language fetch that was never completed so we need to cleanup here
   // otherwise it will be done by the destructor in a wrong thread.
   if (parameters().ui_task == NULL && translate_manager_ != NULL)
     translate_manager_->CleanupPendingUlrFetcher();
 
   if (notify_result_ == ProcessSingleton::PROCESS_NONE)
     process_singleton_->Cleanup();
 
   // Stop all tasks that might run on WatchDogThread.
@@ skipping 31 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif
 #endif
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }
-
-// Misc ------------------------------------------------------------------------
-
-void RecordBrowserStartupTime(bool is_first_run) {
-  // Don't record any metrics if UI was displayed before this point e.g.
-  // warning dialogs.
-  if (startup_metric_utils::WasNonBrowserUIDisplayed())
-    return;
-
-// CurrentProcessInfo::CreationTime() is currently only implemented on Mac and
-// Windows.
-#if defined(OS_MACOSX) || defined(OS_WIN)
-  const base::Time* process_creation_time =
-      base::CurrentProcessInfo::CreationTime();
-
-  if (!is_first_run && process_creation_time) {
-    RecordPreReadExperimentTime("Startup.BrowserMessageLoopStartTime",
-        base::Time::Now() - *process_creation_time);
-  }
-#endif  // defined(OS_MACOSX) || defined(OS_WIN)
-
-  // Record collected startup metrics.
-  startup_metric_utils::OnBrowserStartupComplete(is_first_run);
-
-  // Deletes self.
-  new LoadCompleteListener();
-}
-
-// This code is specific to the Windows-only PreReadExperiment field-trial.
-void RecordPreReadExperimentTime(const char* name, base::TimeDelta time) {
-  DCHECK(name != NULL);
-
-  // This gets called with different histogram names, so we don't want to use
-  // the UMA_HISTOGRAM_CUSTOM_TIMES macro--it uses a static variable, and the
-  // first call wins.
-  AddPreReadHistogramTime(name, time);
-
-#if defined(OS_WIN)
-#if defined(GOOGLE_CHROME_BUILD)
-  // The pre-read experiment is Windows and Google Chrome specific.
-  scoped_ptr<base::Environment> env(base::Environment::Create());
-
-  // Only record the sub-histogram result if the experiment is running
-  // (environment variable is set, and valid).
-  std::string pre_read_percentage;
-  if (env->GetVar(chrome::kPreReadEnvironmentVariable, &pre_read_percentage)) {
-    std::string uma_name(name);
-
-    // We want XP to record a separate histogram, as the loader on XP
-    // is very different from the Vista and Win7 loaders.
-    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
-}