Refactor MetricsStateManager class out of MetricsService.

chrome/browser/metrics/metrics_service.h

 // 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.
 
 // This file defines a service that collects information about the user
 // experience in order to help improve future versions of the app.
 
 #ifndef CHROME_BROWSER_METRICS_METRICS_SERVICE_H_
 #define CHROME_BROWSER_METRICS_METRICS_SERVICE_H_
 
@@ skipping 51 matching lines @@
 struct WebPluginInfo;
 }
 
 namespace extensions {
 class ExtensionDownloader;
 class ManifestFetchData;
 class MetricsPrivateGetIsCrashReportingEnabledFunction;
 }
 
 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 {
 class URLFetcher;
 }
 
 namespace prerender {
 bool IsOmniboxEnabled(Profile* profile);
 }
 
@@ skipping 40 matching lines @@
     STARTUP_TIMEBOMB_ARM = 300,
     THREAD_WATCHER_START = 400,
     MAIN_MESSAGE_LOOP_RUN = 500,
     SHUTDOWN_TIMEBOMB_ARM = 600,
     SHUTDOWN_COMPLETE = 700,
   };
 
   enum ReportingState {
     REPORTING_ENABLED,
     REPORTING_DISABLED,
   };

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

nit: Still needed?

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

Nope. Removed.

 
   MetricsService();
   virtual ~MetricsService();
 
   // Initializes metrics recording state. Updates various bookkeeping values in
   // prefs and sets up the scheduler. This is a separate function rather than
   // 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.
   void StartRecordingForTests();
 
   // Shuts down the metrics system. Should be called at shutdown, or if metrics
   // are turned off.
   void Stop();
 
   // Enable/disable transmission of accumulated logs and crash reports (dumps).
   // Calling Start() automatically enables reporting, but sending is
   // asyncronous so this can be called immediately after Start() to prevent
   // any uploading.
   void EnableReporting();
   void DisableReporting();
 
   // Returns the client ID for this client, or the empty string if metrics
   // recording is not currently running.
   std::string GetClientId();
 
   // 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);
-
-  // Force the client ID to be generated. This is useful in case it's needed
-  // before recording.
-  void ForceClientIdCreation();
+  // 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();
 
   // 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);
 #if defined(OS_ANDROID)
   static void RegisterPrefsAndroid(PrefRegistrySimple* registry);
 #endif  // defined(OS_ANDROID)
 
   // Set up notifications which indicate that a user is performing work. This is
   // useful to allow some features to sleep, until the machine becomes active,
@@ skipping 113 matching lines @@
     NEED_TO_SHUTDOWN = ~CLEANLY_SHUTDOWN
   };
 
   // Designates which entropy source was returned from this MetricsService.
   // This is used for testing to validate that we return the correct source
   // depending on the state of the service.
   enum EntropySourceReturned {
     LAST_ENTROPY_NONE,
     LAST_ENTROPY_LOW,
     LAST_ENTROPY_HIGH,
   };

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

nit: Still needed?

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

Nope! Removed.

 
   struct ChildProcessStats;
 
   typedef std::vector<SyntheticTrialGroup> SyntheticTrialGroups;
 
   // First part of the init task. Called on the FILE thread to load hardware
   // class information.
   static void InitTaskGetHardwareClass(base::WeakPtr<MetricsService> self,
                                        base::MessageLoopProxy* target_loop);
 
@@ skipping 28 matching lines @@
   // Get the amount of uptime since this process started and since the last
   // call to this function.  Also updates the cumulative uptime metric (stored
   // as a pref) for uninstall.  Uptimes are measured using TimeTicks, which
   // guarantees that it is monotonic and does not jump if the user changes
   // his/her clock.  The TimeTicks implementation also makes the clock not
   // count time the computer is suspended.
   void GetUptimes(PrefService* pref,
                   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).
   void EnableRecording();
   void DisableRecording();
 
   // If in_idle is true, sets idle_since_last_transmission to true.
   // If in_idle is false and idle_since_last_transmission_ is true, sets
   // idle_since_last_transmission to false and starts the timer (provided
   // starting the timer is permitted).
   void HandleIdleSinceLastTransmission(bool in_idle);
 
   // Set up client ID, session ID, etc.
-  void InitializeMetricsState(ReportingState reporting_state);
-
-  // Generates a new client ID to use to identify self to metrics server.
-  static std::string GenerateClientID();
+  void InitializeMetricsState();
 
   // 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();
 
   // Save the LocalState information immediately. This should not be called by
   // anybody other than the scheduler to avoid doing too many writes. When you
   // make a change, call ScheduleNextStateSave() instead.
   void SaveLocalState();
 
@@ skipping 91 matching lines @@
 
   // Returns true if process of type |type| should be counted as a plugin
   // process, and false otherwise.
   static bool IsPluginProcess(int process_type);
 
   // Returns a list of synthetic field trials that were active for the entire
   // duration of the current log.
   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_;
 
   // Set to true when |ResetMetricsIDsIfNecessary| is called for the first time.
   // This prevents multiple resets within the same Chrome session.
   bool metrics_ids_reset_check_performed_;
 
   // Indicate whether recording and reporting are currently happening.
   // These should not be set directly, but by calling SetRecording and
@@ skipping 31 matching lines @@
 
   // The outstanding transmission appears as a URL Fetch operation.
   scoped_ptr<net::URLFetcher> current_fetch_;
 
   // The TCP/UDP echo server to collect network connectivity stats.
   std::string network_stats_server_;
 
   // 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_;
 
   // A number that identifies the how many times the app has been launched.
   int session_id_;
 
   // Maps WebContentses (corresponding to tabs) or Browsers (corresponding to
   // Windows) to a unique integer that we will use to identify them.
   // |next_window_id_| is used to track which IDs we have used so far.
@@ skipping 20 matching lines @@
   bool waiting_for_asynchronous_reporting_step_;
 
   // Number of async histogram fetch requests in progress.
   int num_async_histogram_fetches_in_progress_;
 
 #if defined(OS_CHROMEOS)
   // The external metric service is used to log ChromeOS UMA events.
   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_;
 
   // Stores the time of the last call to |GetUptimes()|.
   base::TimeTicks last_updated_time_;
 
   // Execution phase the browser is in.
   static ExecutionPhase execution_phase_;
 
   // Reduntant marker to check that we completed our shutdown, and set the
   // exited-cleanly bit in the prefs.
   static ShutdownCleanliness clean_shutdown_status_;
 
   // 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);
 };
 
 // This class limits and documents access to the IsMetricsReportingEnabled() and
 // IsCrashReportingEnabled() methods. Since these methods are private, each user
 // has to be explicitly declared as a 'friend' below.
 class MetricsServiceHelper {
  private:
   friend bool prerender::IsOmniboxEnabled(Profile* profile);
   friend class ChromeRenderMessageFilter;
   friend class ::CrashesDOMHandler;
   friend class extensions::ExtensionDownloader;
   friend class extensions::ManifestFetchData;
   friend class extensions::MetricsPrivateGetIsCrashReportingEnabledFunction;
   friend class ::FlashDOMHandler;
   friend class system_logs::ChromeInternalLogSource;
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, MetricsReportingEnabled);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, CrashReportingEnabled);
 
   // Returns true if prefs::kMetricsReportingEnabled is set.
   static bool IsMetricsReportingEnabled();

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

How does this compare to MetricsStateManager::IsMetricsReportingEnabled()?

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

It's different and sometimes incorrect (e.g. see http://crbug.com/362192). We
should fix it. But I opted not to do it in this CL to avoid making it even
bigger.

The MetricsStateManager::IsMetricsReportingEnabled() is a direct move of the
version in chrome_browser_main.cc and is meant to not change any behavior in
this CL.

I've added a TODO.

 
   // Returns true if crash reporting is enabled.  This is set at the platform
   // level for Android and ChromeOS, and otherwise is the same as
   // IsMetricsReportingEnabled for desktop Chrome.
   static bool IsCrashReportingEnabled();
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(MetricsServiceHelper);
 };
 
 #endif  // CHROME_BROWSER_METRICS_METRICS_SERVICE_H_