Add UMA metrics to recovery component.

chrome/browser/component_updater/recovery_component_installer.cc

Index: chrome/browser/component_updater/recovery_component_installer.cc
diff --git a/chrome/browser/component_updater/recovery_component_installer.cc b/chrome/browser/component_updater/recovery_component_installer.cc
index 240bce1008c05415aca55ed2f1919e664eaab7a7..983397fd34841b538ea604e98af5f050cb7067b4 100644
--- a/chrome/browser/component_updater/recovery_component_installer.cc
+++ b/chrome/browser/component_updater/recovery_component_installer.cc
@@ -16,6 +16,7 @@
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
@@ -59,6 +60,14 @@ enum ChromeRecoveryExitCode {
   EXIT_CODE_ELEVATION_NEEDED = 2,
 };
 
+enum RecoveryComponentEvent {

[gab, 2015/01/09 20:08:44]

If you make this an enum class (woot C++11!!) then you can explicitly use
RecoveryComponentEvent::RUN_NON_ELEVATED, etc. everywhere rather than polluting
this namespace and have to use RCE_ prefixes.

I prefer this, more explicit and less namespace pollution.

[robertshield, 2015/01/09 21:37:21]

Sadly without the implicit conversion to int, that would require explicit casts
to play nicely with the UMA_HISTOGRAM macros afaict. Will leave as is unless you
know of a nice fix for this.

[gab, 2015/01/09 22:41:22]

Hmmm, really? I was sure I'd done this in the past, but maybe not...

[robertshield, 2015/01/09 22:45:32]

I tried it and it complained about requiring a cast to convert between
RecoveryComponentEvent and the type required. Could do this, but that is uglier
than the current solution. 
Also,
http://stackoverflow.com/questions/8357240/how-to-automatically-convert-stron...

[gab, 2015/01/12 19:12:20]

Ah ok, right, for some reason I thought I'd done this, but now that I think
further what I have is a switch based on a enum class and then the histogram
themselves don't report the enumeration value itself but a specific histogram
(w/, e.g., a count) for each value.

[robertshield, 2015/01/12 23:04:19]

Acknowledged.

+  RCE_RUN_NON_ELEVATED = 0,
+  RCE_ELEVATION_NEEDED = 1,
+  RCE_RUN_ELEVATED = 2,
+  RCE_COMPONENT_DOWNLOAD_ERROR = 3,
+  RCE_MAX = 4

[gab, 2015/01/09 20:08:44]

Don't explicitly label the max.

(I also prefer to call it _COUNT or _END than _MAX -- as _MAX to me sounds like
it's the value of the last item, not a past-the-end value).

[robertshield, 2015/01/09 21:37:21]

Done.

+};
+
 #if !defined(OS_CHROMEOS)
 // Checks if elevated recovery simulation switch was present on the command
 // line. This is for testing purpose.
@@ -66,7 +75,7 @@ bool SimulatingElevatedRecovery() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kSimulateElevatedRecovery);
 }
-#endif
+#endif  // !defined(OS_CHROMEOS)
 
 #if defined(OS_WIN)
 scoped_ptr<base::DictionaryValue> ReadManifest(const base::FilePath& manifest) {
@@ -108,6 +117,9 @@ void DoElevatedInstallRecoveryComponent(const base::FilePath& path) {
     cmdline.AppendSwitchASCII("version", version.GetString());
   }
 
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_RUN_ELEVATED,
+                            RCE_MAX);

[gab, 2015/01/09 20:08:44]

This is slightly misleading, this event doesn't actually mean that it was *ran*
elevated, only that the UAC was brought up, right?

Perhaps we should log an attempt here and then another event when succeeding
(i.e. we get a handle from base::LaunchElevatedProcess()?), this would also help
quantify how bad the UAC really is?

[robertshield, 2015/01/09 21:37:21]

Right, the intent of this patch initially was to record what Chrome thought it
was trying to do, not whether the recovery component actually worked. 

On further reflection, why not do both. Expanded CL accordingly.

+
   base::LaunchOptions options;
   options.start_hidden = true;
   base::LaunchElevatedProcess(cmdline, options);
@@ -119,7 +131,7 @@ void ElevatedInstallRecoveryComponent(const base::FilePath& installer_path) {
       base::Bind(&DoElevatedInstallRecoveryComponent, installer_path),
       true);
 }
-#endif
+#endif  // defined(OS_WIN)
 
 }  // namespace
 
@@ -194,6 +206,8 @@ RecoveryComponentInstaller::RecoveryComponentInstaller(const Version& version,
 }
 
 void RecoveryComponentInstaller::OnUpdateError(int error) {
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event",
+                            RCE_COMPONENT_DOWNLOAD_ERROR, RCE_MAX);
   NOTREACHED() << "Recovery component update error: " << error;
 }
 
@@ -205,6 +219,9 @@ void WaitForInstallToComplete(base::Process process,
   const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600);
   if (process.WaitForExitWithTimeout(kMaxWaitTime, &installer_exit_code) &&
       installer_exit_code == EXIT_CODE_ELEVATION_NEEDED) {
+    UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_ELEVATION_NEEDED,
+                              RCE_MAX);
+
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
@@ -217,6 +234,9 @@ void WaitForInstallToComplete(base::Process process,
 bool RecoveryComponentInstaller::RunInstallCommand(
     const base::CommandLine& cmdline,
     const base::FilePath& installer_folder) const {
+  UMA_HISTOGRAM_ENUMERATION("RecoveryComponent.Event", RCE_RUN_NON_ELEVATED,
+                            RCE_MAX);
+
   base::LaunchOptions options;
   options.start_hidden = true;
   base::Process process = base::LaunchProcess(cmdline, options);
@@ -241,7 +261,7 @@ bool RecoveryComponentInstaller::RunInstallCommand(
     const base::FilePath&) const {
   return base::LaunchProcess(cmdline, base::LaunchOptions()).IsValid();
 }
-#endif
+#endif  // defined(OS_WIN)
 
 bool RecoveryComponentInstaller::Install(const base::DictionaryValue& manifest,
                                          const base::FilePath& unpack_path) {
@@ -322,7 +342,7 @@ void RegisterRecoveryComponent(ComponentUpdateService* cus,
       FROM_HERE,
       base::Bind(&RecoveryRegisterHelper, cus, prefs),
       base::TimeDelta::FromSeconds(6));
-#endif
+#endif  // !defined(OS_CHROMEOS)
 }
 
 void RegisterPrefsForRecoveryComponent(PrefRegistrySimple* registry) {
@@ -338,7 +358,7 @@ void AcceptedElevatedRecoveryInstall(PrefService* prefs) {
 #if defined(OS_WIN)
   ElevatedInstallRecoveryComponent(
       prefs->GetFilePath(prefs::kRecoveryComponentUnpackPath));
-#endif
+#endif  // OS_WIN
   prefs->SetBoolean(prefs::kRecoveryComponentNeedsElevation, false);
 }