Refactor MetricsStateManager class out of MetricsService.

chrome/browser/metrics/metrics_state_manager.h

+// 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.
+
+#ifndef CHROME_BROWSER_METRICS_METRICS_STATE_MANAGER_H_
+#define CHROME_BROWSER_METRICS_METRICS_STATE_MANAGER_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/metrics/field_trial.h"
+
+class PrefService;
+class PrefRegistrySimple;
+
+namespace metrics {
+
+class ClonedInstallDetector;
+
+// Responsible for managing MetricsService state prefs, specifically the UMA
+// client id and low entropy source.
+class MetricsStateManager {
+ public:
+  explicit MetricsStateManager(PrefService* local_state);
+  virtual ~MetricsStateManager();
+
+  // Returns true if the user opted in to sending metric reports.
+  bool IsMetricsReportingEnabled();

[Ilya Sherman, 2014/05/02 05:12:38]

I'm not comfortable with exposing this as a public method, in any class
interface.  It makes it too easy for someone to try to branch production logic
based on whether or not UMA is enabled, perhaps thereby worsening the user
experience for UMA users (and this discouraging UMA usage), rather than simply
making metrics reporting efficient.  Note that the existing MetricsServiceHelper
class used friend declarations to enforce an OWNERS review on any new clients of
this functionality.  I think that we should do something similar here.

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

It's not a static method, so someone needs to have a pointer to an instance of
this class to call it. The intention is that there's only one instance of this
class owned by the metrics sub-system and that other code can't access it.
Therefore, it's safe to keep it public.

To clarify the above intention, I've expanded the class comment to mention that
code outside the metrics directory shouldn't be using this class directly.

[Ilya Sherman, 2014/05/02 19:35:06]

It's pretty easy to construct an instance of this class, though.  I'd very much
prefer to implement the code in some way that requires a metrics OWNERS code
review in order for new clients to depend on this functionality.

[Alexei Svitkine (slow), 2014/05/02 20:21:31]

In the previous patchset, I added this comment to the class def:

"Code outside the metrics directory should not be instantiating or using this
class directly."

I think that's a strong enough wording to discourage use of this class. In
general, if someone is instantiating multiple instances of this class, it would
be quite bad.

One possibility would be to make the constructor a factory method instead and
have a check that it's not called more than once. But this will require changing
tests to not hit it. Personally, I don't think it's worth it.

(I'm not a big fan of friend declarations for this purpose, even though I
realise we have precedent for it in some MetricsService code.)

[Ilya Sherman, 2014/05/02 20:30:06]

I saw the comment -- thanks for adding it! -- but I don't think that a comment
is sufficient.  It's easy to miss a comment; and it's generally not obvious to
someone who hasn't worked on UMA why it's important not to branch on the UMA
enabled state in feature code.  This isn't just a theoretical concern: Somebody
recently IMed me asking how to query whether UMA is enabled, for just such a
reason.  If they had instead chosen to try code search, there's a good chance
they would have just implemented a CL that did the branching.

So, I think it's quite important to ensure that nobody can add a dependency on
this method without triggering a metrics OWNERS review that blocks the CL.  I'm
not wed to using friend declarations to achieve this result, but it's the best
mechanism that I've found so far.

[Steven Holte, 2014/05/02 20:39:01]

Hm, it looks like there is a fairly common xyz::internal namespace convention
used in chrome for "package" privacy.  Would that be appropriate here?

[Alexei Svitkine (slow), 2014/05/02 20:45:51]

I'm okay with doing this as well, but perhaps this should wait till we move
things to the component? I'd imagine we'd want to do this on a lot of the helper
classes... Ilya, what do you think? (I'm okay with doing that in this CL as
well, if you prefer.)

[Ilya Sherman, 2014/05/02 20:49:48]

Yeah, once we move to a component, I bet we can enforce hiding things via DEPS. 
(Though, it's tricky: There is code like the crash reporter that needs to know
whether UMA is enabled.)  Even before then, though, I think it's important to
enforce an OWNERS review to ensure that the API remains private.

[Alexei Svitkine (slow), 2014/05/02 21:49:52]

Per offline discussion, went with a Create() factory method that enforces that a
single instance of the class exists at a time. We can re-evaluated once this
moves to a component.

+
+  // Returns the client ID for this client, or the empty string if the user is
+  // not opted in to metrics reporting.
+  const std::string& client_id() const { return client_id_; }
+
+  // Force the client ID to be generated. This is useful in case it's needed
+  // before recording.
+  void ForceClientIdCreation();
+
+  // Check if this install was cloned or imaged from another machine. If a

[Ilya Sherman, 2014/05/02 05:12:38]

nit: "Check" -> "Checks"

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

Done.

+  // clone is detected, reset the client id and low entropy source. This

[Ilya Sherman, 2014/05/02 05:12:38]

nit: "reset" -> "resets"

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

Done.

+  // should not be called more than once.
+  void CheckForClonedInstall();
+
+  // Returns the preferred entropy provider used to seed persistent activities
+  // based on whether or not metrics reporting will be permitted on this client.

[Ilya Sherman, 2014/05/02 05:12:38]

nit: "will be permitted" -> "is permitted"?

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

Done.

+  //
+  // If metrics reporting is enabled, this method returns an entropy provider
+  // that has a high source of entropy, partially based on the client ID.
+  // Otherwise, an entropy provider that is based on a low entropy source is
+  // returned.
+  scoped_ptr<const base::FieldTrial::EntropyProvider> CreateEntropyProvider();
+
+  // Registers local state prefs used by this class.
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, EntropySourceUsed_Low);
+  FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, EntropySourceUsed_High);
+  FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, LowEntropySource0NotReset);
+  FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest,
+                           PermutedEntropyCacheClearedWhenLowEntropyReset);
+  FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, ResetMetricsIDs);
+
+  // Designates which entropy source was returned from this MetricsService.

[Ilya Sherman, 2014/05/02 05:12:38]

nit: Please update the comment.

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

Done.

+  // This is used for testing to validate that we return the correct source
+  // depending on the state of the service.
+  enum EntropySourceReturned {

[Ilya Sherman, 2014/05/02 05:12:38]

nit: Perhaps name this "LastEntropySource", to match the constant names?

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

I actually went with EntropySourceType and renamed the constants to match, which
I think are a bit better as names.

+    LAST_ENTROPY_NONE,
+    LAST_ENTROPY_LOW,
+    LAST_ENTROPY_HIGH,
+  };
+
+  // Returns the low entropy source for this client. This is a random value
+  // that is non-identifying amongst browser clients. This method will
+  // generate the entropy source value if it has not been called before.
+  int GetLowEntropySource();
+
+  // Returns the first entropy source that was returned by this service since
+  // start up, or NONE if neither was returned yet. This is exposed for testing
+  // only.
+  EntropySourceReturned entropy_source_returned() const {
+    return entropy_source_returned_;
+  }
+
+  // Reset the client id and low entropy source if the kMetricsResetMetricIDs
+  // pref is true.
+  void ResetMetricsIDsIfNecessary();
+
+  PrefService* local_state_;
+
+  // TODO(asvitkine): Update this when the user toggles this state.

[Ilya Sherman, 2014/05/02 05:12:38]

Are you intending to address this TODO as part of this CL?  If not, why is it
safe not to?

[Alexei Svitkine (slow), 2014/05/02 15:21:46]

It was safe because it was only checked during startup, so the cached value
would be correct.

However, thinking about it more, I don't find it's worth the trouble to cache it
- so I've removed this now and made the function do the full check each time now
(it shouldn't be a very big overhead).

+  bool metrics_reporting_enabled_;
+
+  // The identifier that's sent to the server with the log reports.
+  std::string client_id_;
+
+  // The non-identifying low entropy source value.
+  int low_entropy_source_;
+
+  // The last entropy source returned by this service, used for testing.
+  EntropySourceReturned entropy_source_returned_;
+
+  scoped_ptr<ClonedInstallDetector> cloned_install_detector_;
+
+  DISALLOW_COPY_AND_ASSIGN(MetricsStateManager);
+};
+
+}  // namespace metrics
+
+#endif  // CHROME_BROWSER_METRICS_METRICS_STATE_MANAGER_H_