Refactor MetricsStateManager class out of MetricsService.

chrome/browser/metrics/metrics_service.h

Index: chrome/browser/metrics/metrics_service.h
===================================================================
--- chrome/browser/metrics/metrics_service.h	(revision 267523)
+++ chrome/browser/metrics/metrics_service.h	(working copy)
@@ -70,6 +70,7 @@
 
 namespace metrics {
 class ClonedInstallDetector;

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

nit: Still needed?

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

Nope. Removed.

+class MetricsStateManager;
 }
 
 namespace net {
@@ -140,13 +141,17 @@
   // being done by the constructor so that field trials could be created before
   // this is run. Takes |reporting_state| parameter which specifies whether UMA
   // is enabled.

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

nit: Please update the docs.

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

Done.

-  void InitializeMetricsRecordingState(ReportingState reporting_state);
+  void InitializeMetricsRecordingState();
 
   // Starts the metrics system, turning on recording and uploading of metrics.
   // Should be called when starting up with metrics enabled, or when metrics
   // are turned on.
   void Start();

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

When is it appropriate to call Start(), without regard for the reporting enabled
state?

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

It's still used by private Android code that uses a different pref. Ideally, we
should eventually refactor things so that this one is not exposed.

But it would have to be a more involved cross-repo change, so out of scope for
this CL.

More details about how Android does things here: http://crbug.com/362192

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

Ok.  If this is used only by Android:
(1) Can we rename the method to something less seductive for non-Android
clients?
(2) If not, let's at least add documentation to indicate that this method should
not be used by any new clients.

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

We can't rename it in this CL since the Android code that uses it is in another
repo. So it would take several cross-repo iterations to get to that. At which
point, we might as well just fix it properly with those same iterations.

I'm okay to add documentation for it, if you feel strongly about it, but to be
honest I see this going away in the medium term future and don't really see new
things appearing that will start to use this, so it doesn't seem worth it to
me... Let me know what you prefer.

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

I'd prefer to include the documentation as part of this CL.  It's common for
cleanup work to get preempted, so I'd prefer to assume that it's *possible* that
the code implemented in this CL will remain in place for long enough for someone
to come along and wonder whether they should use it.

 
+  // If metrics reporting is enabled, starts the metrics service. Returns
+  // whether the metrics service was started.
+  bool StartIfMetricsReportingEnabled();

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

nit: Perhaps drop "Metrics" from this method name?

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

I prefer not to change the name, as otherwise it would make it confusing w.r.t.
EnableReporting()/DisableReporting() - whereas those simply control the state of
MetricsService and not the UMA opt in pref.

Even the current name is not great, but at least I've kept it consistent with
the terminology we use elsewhere (i.e. the previous method name from
chrome_browser_main.cc and the IsMetricsReportingEnabled() name from the helper
below).

Though if you have another suggestion for a name, I'm all ears.

+
   // Starts the metrics system in a special test-only mode. Metrics won't ever
   // be uploaded or persisted in this mode, but metrics will be recorded in
   // memory.
@@ -169,24 +174,13 @@
 
   // Returns the preferred entropy provider used to seed persistent activities
   // based on whether or not metrics reporting will be permitted on this client.
-  // The caller must determine if metrics reporting will be enabled for this
-  // client and pass that state in as |reporting_will_be_enabled|.
   //
-  // If |reporting_will_be_enabled| is true, 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.
-  //
-  // Note that this reporting state can not be checked by reporting_active()
-  // because this method may need to be called before the MetricsService needs
-  // to be started.
-  scoped_ptr<const base::FieldTrial::EntropyProvider> CreateEntropyProvider(
-      ReportingState reporting_state);
+  // 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();
 
-  // Force the client ID to be generated. This is useful in case it's needed
-  // before recording.
-  void ForceClientIdCreation();
-
   // At startup, prefs needs to be called with a list of all the pref names and
   // types we'll be using.
   static void RegisterPrefs(PrefRegistrySimple* registry);
@@ -368,22 +362,6 @@
                   base::TimeDelta* incremental_uptime,
                   base::TimeDelta* uptime);
 
-  // Reset the client id and low entropy source if the kMetricsResetMetricIDs
-  // pref is true.
-  void ResetMetricsIDsIfNecessary();
-
-  // 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_;
-  }
-
   // Turns recording on or off.
   // DisableRecording() also forces a persistent save of logging state (if
   // anything has been recorded, or transmitted).
@@ -397,11 +375,8 @@
   void HandleIdleSinceLastTransmission(bool in_idle);
 
   // Set up client ID, session ID, etc.
-  void InitializeMetricsState(ReportingState reporting_state);
+  void InitializeMetricsState();
 
-  // Generates a new client ID to use to identify self to metrics server.
-  static std::string GenerateClientID();
-
   // Schedule the next save of LocalState information.  This is called
   // automatically by the task that performs each save to schedule the next one.
   void ScheduleNextStateSave();
@@ -512,6 +487,8 @@
   void GetCurrentSyntheticFieldTrials(
       std::vector<chrome_variations::ActiveGroupId>* synthetic_trials);
 
+  scoped_ptr<metrics::MetricsStateManager> state_manager_;

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

nit: Docs, please.

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

Done.

+
   base::ActionCallback action_callback_;
 
   content::NotificationRegistrar registrar_;
@@ -563,12 +540,6 @@
   // The HTTP pipelining test server.
   std::string http_pipelining_test_server_;
 
-  // 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_;
-
   // Whether the MetricsService object has received any notifications since
   // the last time a transmission was sent.
   bool idle_since_last_transmission_;
@@ -609,9 +580,6 @@
   scoped_refptr<chromeos::ExternalMetrics> external_metrics_;
 #endif
 
-  // The last entropy source returned by this service, used for testing.
-  EntropySourceReturned entropy_source_returned_;
-
   // Stores the time of the first call to |GetUptimes()|.
   base::TimeTicks first_updated_time_;
 
@@ -628,19 +596,10 @@
   // Field trial groups that map to Chrome configuration states.
   SyntheticTrialGroups synthetic_trial_groups_;
 
-  scoped_ptr<metrics::ClonedInstallDetector> cloned_install_detector_;
-
-  FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ClientIdCorrectlyFormatted);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess);
-  FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, LowEntropySource0NotReset);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest,
                            PermutedEntropyCacheClearedWhenLowEntropyReset);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, RegisterSyntheticTrial);
-  FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ResetMetricsIDs);
-  FRIEND_TEST_ALL_PREFIXES(MetricsServiceBrowserTest,
-                           CheckLowEntropySourceUsed);
-  FRIEND_TEST_ALL_PREFIXES(MetricsServiceReportingTest,
-                           CheckHighEntropySourceUsed);
 
   DISALLOW_COPY_AND_ASSIGN(MetricsService);
 };