Extract the handling of the clean exit beacon from MetricsService. The extracted implementation sup…

components/metrics/clean_exit_beacon.cc

+// Copyright 2014 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 "components/metrics/clean_exit_beacon.h"
+
+#include "base/logging.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+
+#if defined(OS_WIN)
+#include "base/metrics/histogram.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/registry.h"
+#endif
+
+namespace {
+
+// True if the previous run of the program exited cleanly.
+const char kStabilityExitedCleanly[] =
+    "user_experience_metrics.stability.exited_cleanly";

[Alexei Svitkine (slow), 2014/09/11 15:23:28]

What's the reason for moving the pref? I think we want to keep a central place
where all the metrics prefs are listed, so I prefer this is moved back.

[erikwright (departed), 2014/09/11 15:39:14]

Exposing the name of a preference permits violation of encapsulation. This code
encapsulates the access to this value, and it would be inappropriate for any
other code to directly read/write the value. Hiding the name of the pref
enforces that. This is a pattern that I've used elsewhere and I think should be
generally adopted any time it is not reasonable for arbitrary code to manipulate
a preference value.

[Alexei Svitkine (slow), 2014/09/11 15:57:28]

I agree in principle, but at the same time we lose the benefit of seeing all the
prefs in one place.

In practice, I think for this component, I'd rather keep all the names in one
place but have them private to the component (i.e. not used by anything outside
of the component). I think this is already mostly the case, though
chrome/common/pref_names.h does import the metrics pref names file, which we
should probably change.

I think if we wanted to do it the way you suggest, we should do it all at once
for all these prefs, so that there's consistency, rather than one-offs which
results in an inconsistent intermediate state. If we want to do it all at once,
it should be in a separate CL.

So for now, let's keep the name where it is and if you want, add a comment
saying that it should only be accessed through CleanExitBeacon.

[erikwright (departed), 2014/09/11 19:00:29]

Done. I didn't add a comment, as it's not really any different from the other
constants in that file.

+
+}  // namespace
+
+#if defined(OS_WIN)

[Alexei Svitkine (slow), 2014/09/11 15:23:28]

There's too many ifdefs for my preference.

How about we make the client method available on all platforms, but return an
empty string on others.

We can also provide a default implementation that returns an empty string, so
that clients that don't care about Windows (e.g. Chromecast), don't need to
implement it.

Then, we can eliminate most of the ifdefs. We'd only need to keep them in the
implementation of this file, and even here, we can just put it around the body
of the code (rather than around the method signature itself).

[erikwright (departed), 2014/09/11 15:39:14]

It seems a bit strange to declare and implement (even just by inheritance) a
method whose sole purpose is to return a Windows registry key on non-Windows
platforms. That is partially why I felt making the storage method (in addition
to the location) abstract was better.
I'm not quite sure. Because you will end up with an unused member, which Clang
may warn about. Or else (if you ifdef the member declaration and assignment) you
will have an unused parameter (which I believe will not lead to a warning).

[Alexei Svitkine (slow), 2014/09/11 15:57:28]

My original thought would have been for the method to be an accessor for
PRODUCT_STRING_PATH. Then, it wouldn't need to be tied to Windows registry and
could also be re-used for other things later, if desired. Maybe let's do that
instead?
I don't think we have warnings about unused members.

[erikwright (departed), 2014/09/11 16:03:14]

I think the location used for storage in the registry should be fully controlled
by the embedder, as it is desired that it be the same as all other registry
usage of the product. Remember you would need both the vendor and the product
name, and to know how to compose them (Google\Chrome or just Chromium) in order
to get the same result as elsewhere.

[Alexei Svitkine (slow), 2014/09/11 17:07:19]

Okay, then let's go with the with my suggestion of just having a default impl of
that method that returns an empty string, which should minimize the amount of
ifdefs.

[erikwright (departed), 2014/09/11 19:00:28]

Done.

+CleanExitBeacon::CleanExitBeacon(const base::string16& backup_registry_key,
+                                 PrefService* local_state)
+    : backup_registry_key_(backup_registry_key),
+      local_state_(local_state),
+      initial_value_(local_state->GetBoolean(kStabilityExitedCleanly)) {
+  DCHECK_NE(PrefService::INITIALIZATION_STATUS_WAITING,
+            local_state_->GetInitializationStatus());
+  enum {
+    DIRTY_DIRTY,
+    DIRTY_CLEAN,
+    CLEAN_DIRTY,
+    CLEAN_CLEAN,
+    MISSING_DIRTY,
+    MISSING_CLEAN,
+    NUM_CONSISTENCY_ENUMS
+  } consistency = DIRTY_DIRTY;
+
+  base::win::RegKey regkey;
+  DWORD value = 0u;
+  if (regkey.Open(HKEY_CURRENT_USER,
+                  backup_registry_key_.c_str(),
+                  KEY_ALL_ACCESS) == ERROR_SUCCESS &&
+      regkey.ReadValueDW(base::ASCIIToUTF16(kStabilityExitedCleanly).c_str(),
+                         &value) == ERROR_SUCCESS) {
+    if (value)
+      consistency = initial_value_ ? CLEAN_CLEAN : CLEAN_DIRTY;
+    else
+      consistency = initial_value_ ? DIRTY_CLEAN : DIRTY_DIRTY;
+  } else {
+    consistency = initial_value_ ? MISSING_CLEAN : MISSING_DIRTY;
+  }
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.CleanExitBeaconConsistency", consistency, NUM_CONSISTENCY_ENUMS);
+}
+#else
+CleanExitBeacon::CleanExitBeacon(PrefService* local_state)
+    : local_state_(local_state),
+      initial_value_(local_state->GetBoolean(kStabilityExitedCleanly)) {
+  DCHECK_NE(PrefService::INITIALIZATION_STATUS_WAITING,
+            local_state_->GetInitializationStatus());
+}
+#endif
+
+CleanExitBeacon::~CleanExitBeacon() {
+}
+
+void CleanExitBeacon::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(kStabilityExitedCleanly, true);
+}
+
+void CleanExitBeacon::WriteBeaconValue(bool value) {
+  local_state_->SetBoolean(kStabilityExitedCleanly, value);
+  local_state_->CommitPendingWrite();
+
+#if defined(OS_WIN)
+  base::win::RegKey regkey;
+  if (regkey.Open(HKEY_CURRENT_USER,
+                  backup_registry_key_.c_str(),
+                  KEY_ALL_ACCESS) == ERROR_SUCCESS) {
+    regkey.WriteValue(base::ASCIIToUTF16(kStabilityExitedCleanly).c_str(),
+                      value ? 1u : 0u);
+  }
+#endif
+}