Add a switch to emit browser histograms.

chrome/browser/ui/startup/startup_browser_creator.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/ui/startup/startup_browser_creator.h"
 
 #include <algorithm>   // For max().
 #include <set>
 
 #include "apps/app_load_service.h"
 #include "apps/switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
+#include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
 #include "chrome/browser/auto_launch_trial.h"
 #include "chrome/browser/automation/automation_provider.h"
 #include "chrome/browser/automation/automation_provider_list.h"
@@ skipping 188 matching lines @@
   Profile* profile_to_activate_;
   // Set once we attempted to activate a profile. We only get one shot at this.
   bool activated_profile_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileLaunchObserver);
 };
 
 base::LazyInstance<ProfileLaunchObserver> profile_launch_observer =
     LAZY_INSTANCE_INITIALIZER;
 
+void DumpBrowserHistograms(const base::FilePath& output_file) {
+  // To be called only on the blocking pool.

[Peter Kasting, 2013/11/13 03:49:21]

Nit: I'd put a comment like this above the function as "This function should
only be called on the blocking pool."

[grt (UTC plus 2), 2013/11/13 15:13:00]

Done.

+  base::ThreadRestrictions::AssertIOAllowed();
+
+  if (base::PathExists(output_file))

[Peter Kasting, 2013/11/13 03:49:21]

Maybe we should overwrite or append to the file instead of doing nothing?

[grt (UTC plus 2), 2013/11/13 15:13:00]

I considered but discarded both. Overwrite introduces a possibility for
accidental data loss, while a pure append would result in an invalid JSON file.
I think this is sufficiently simple to leave as-is unless you feel strongly
otherwise.

+    return;
+
+  std::string output_string;
+  base::StatisticsRecorder::WriteJSON(std::string(), &output_string);

[Peter Kasting, 2013/11/13 03:49:21]

Could you perhaps also change the WriteJSON() method to return the string by
value instead of by outparam?  This should be just as fast due to RVO.  (See my
email to chromium-dev tonight about this issue.)  It should also allow making
HistogramMessageFilter's code a little simpler as an added bonus.

[grt (UTC plus 2), 2013/11/13 15:13:00]

Thanks for the suggestion. I'm going to pass on this both to keep the change
small and because RVO isn't as reliable as taking the out param.

[Peter Kasting, 2013/11/13 19:18:26]

Then can we do this as a followon change?  It's reasonable to not do it here,
but especially for strings, our style guide explicitly suggests returning by
value in preference to using outparams.

[grt (UTC plus 2), 2013/11/14 05:08:51]

I've gone ahead and made this change for the new StatisticsRecorder method
introduced here. I suspect the original author (in
https://codereview.chromium.org/55363003/) had it taking the string as an out
param because of its similarity to StatisticsRecorder::WriteHTMLGraph and
StatisticsRecorder::WriteGraph, both of which append to an existing string. I
have not changed those, but I have renamed the new method to ToJSON (with a doc
comment), and have improved the doc comments for the other two Write* methods.

+  if (output_string.size() <= INT_MAX) {

[Peter Kasting, 2013/11/13 03:49:21]

Urgh... I wonder if we can assume histograms are significantly smaller than
INT_MAX total bytes, such that this check isn't really necessary.

(If it is necessary, we may want to consider changing the file_util interfaces
to use 64-bit length types.)

[grt (UTC plus 2), 2013/11/13 15:13:00]

I feel pretty confident that they'll be smaller than MAX_INT, but I don't like
writing incorrect code. I'm checking with jschuh@ (owner of crbug.com/167187) to
see if he has a cleaner suggestion.

[Peter Kasting, 2013/11/13 19:18:26]

Well, if your definition of correct code is that we always size-check all values
before putting them into a type, I've got about 100 million other places in
Chrome for you to fix.

INT_MAX on the targets we build on ought to allow for 2 gigabytes of histogram
data.  Could we really even come within two orders of magnitude of this value? 
Furthermore, would we really be able to allocate a single 2-gigabyte string in
memory to return this value, in addition to all the memory used elsewhere in
Chrome, not least the original memory used for the histogram data itself?

It seems like this is beyond "let's be technically correct" and into the
way-paranoid state.  Yet writing it here implies that this actually can happen,
which misleads readers; and the added verbosity doesn't help anything.  I really
don't think we should do this.

[grt (UTC plus 2), 2013/11/14 05:08:51]

Likely not.
If not now on an x64 system with an x64 build of Chrome, then soon, yes.
I've removed it for now since I agree it's not likely today, certainly not with
32-bit builds. I'm still waiting with curiosity for jschuh to get back to me,
though. I feel less comfortable with incorrect casts than you do. These things
tend to get casually copy-n-pasted around the codebase. Having a consistent rule
for proper range checking scales better than ad-hoc decisions.

+    file_util::WriteFile(output_file, output_string.data(),
+                         static_cast<int>(output_string.size()));
+  }
+}
+
 }  // namespace
 
 StartupBrowserCreator::StartupBrowserCreator()
     : is_default_browser_dialog_suppressed_(false),
       show_main_browser_window_(true) {
 }
 
 StartupBrowserCreator::~StartupBrowserCreator() {}
 
 // static
@@ skipping 355 matching lines @@
 
     // Skip browser launch since app mode launches its app window.
     silent_launch = true;
   }
 #endif
 
 #if defined(TOOLKIT_VIEWS) && defined(USE_X11)
   ui::TouchFactory::SetTouchDeviceListFromCommandLine();
 #endif
 
+  if (!process_startup) {

[Peter Kasting, 2013/11/13 03:49:21]

I lack sufficient familiarity with this file to truly understand
|process_statup| and |silent_launch|.  Could you find a reviewer who's touched
these before and can verify you're using them correctly?

[grt (UTC plus 2), 2013/11/13 15:13:00]

Done.

+    base::FilePath output_file(
+        command_line.GetSwitchValuePath(switches::kDumpBrowserHistograms));
+    if (!output_file.empty()) {
+      BrowserThread::PostBlockingPoolTask(
+          FROM_HERE,
+          base::Bind(&DumpBrowserHistograms, output_file));
+    }
+    silent_launch = true;
+  }
+
   // If we don't want to launch a new browser window or tab (in the case
   // of an automation request), we are done here.
   if (silent_launch)
     return true;
 
   // Check for --load-and-launch-app.
   if (command_line.HasSwitch(apps::kLoadAndLaunchApp) &&
       !IncognitoModePrefs::ShouldLaunchIncognito(
           command_line, last_used_profile->GetPrefs())) {
     CommandLine::StringType path = command_line.GetSwitchValueNative(
@@ skipping 139 matching lines @@
 
 // static
 bool StartupBrowserCreator::ActivatedProfile() {
   return profile_launch_observer.Get().activated_profile();
 }
 
 bool HasPendingUncleanExit(Profile* profile) {
   return profile->GetLastSessionExitType() == Profile::EXIT_CRASHED &&
       !profile_launch_observer.Get().HasBeenLaunched(profile);
 }