Small cleanup for ChromeBrowserMain:

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
===================================================================
--- chrome/browser/chrome_browser_main.cc	(revision 210624)
+++ chrome/browser/chrome_browser_main.cc	(working copy)
@@ -14,6 +14,7 @@
 #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"
@@ -89,10 +90,6 @@
 #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"
@@ -202,10 +199,6 @@
 #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 {
@@ -560,7 +553,6 @@
       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),
@@ -696,6 +688,65 @@
   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.
 
@@ -719,9 +770,6 @@
 // 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();
@@ -749,9 +797,6 @@
   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() {
@@ -982,10 +1027,6 @@
   }
 #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(
@@ -1430,7 +1471,9 @@
 #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));
@@ -1462,12 +1505,6 @@
   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.
@@ -1645,13 +1682,6 @@
   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));
@@ -1662,25 +1692,6 @@
   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.
@@ -1732,63 +1743,3 @@
 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
-}