PerfProvider: Restructure collection parameters into a struct

chrome/browser/metrics/perf/perf_provider_chromeos.h

Index: chrome/browser/metrics/perf/perf_provider_chromeos.h
diff --git a/chrome/browser/metrics/perf/perf_provider_chromeos.h b/chrome/browser/metrics/perf/perf_provider_chromeos.h
index 64b28d2330bbfb3f354df5942c47c6652d3f50bb..721693af4bcd85f0f1aa2ac6bc538d87ddae4b6a 100644
--- a/chrome/browser/metrics/perf/perf_provider_chromeos.h
+++ b/chrome/browser/metrics/perf/perf_provider_chromeos.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_METRICS_PERF_PERF_PROVIDER_CHROMEOS_H_
 
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -35,6 +36,78 @@ class PerfProvider : public base::NonThreadSafe,
   bool GetSampledProfiles(std::vector<SampledProfile>* sampled_profiles);
 
  protected:
+  typedef int64 TimeDeltaInternalType;
+
+  class CollectionParams {
+   public:
+    class TriggerParams {
+     public:
+      TriggerParams(int64 sampling_factor,
+                    base::TimeDelta max_collection_delay);
+
+      // Limit the number of profiles collected.
+      int64 sampling_factor() const { return sampling_factor_; }
+      void set_sampling_factor(int64 factor) { sampling_factor_ = factor; }
+      // Add a random delay before collecting after the trigger.
+      // The delay should be randomly selected between 0 and this value.
+      base::TimeDelta max_collection_delay() const {
+        return base::TimeDelta::FromInternalValue(max_collection_delay_);
+      }
+      void set_max_collection_delay(base::TimeDelta delay) {

[Alexei Svitkine (slow), 2015/10/09 17:27:21]

Nit: Is this setter used?

[dhsharp, 2015/10/09 18:29:48]

It's used in an upcoming change to set the value from Finch.

+        max_collection_delay_ = delay.ToInternalValue();
+      }
+     private:
+      TriggerParams() = default;  // POD
+      int64 sampling_factor_;
+      TimeDeltaInternalType max_collection_delay_;
+    };

[Alexei Svitkine (slow), 2015/10/09 17:27:21]

How about just making these structs with const fields that are only initialized
by the ctor?

Then, you don't need all the extra getters and only need to add member functions
for things tht do computations, such as the TimeDelta functions.

[dhsharp, 2015/10/09 18:29:48]

I'm not sure I know what you're suggesting.

These fields will need to be altered by Finch later, in an upcoming change.
Finch doesn't have to overwrite all the fields if they aren't specified in the
params, though, so making a new struct and copying into
PerfProvider::collection_params_ would be inefficient.

I also would like to point out that one of the features of this class is to be
POD so that kDefaultParameters can be statically initialized. Making the fields
const and initializing them in the constructor might make this class non-POD,
unless it also has a defaulted constructor... but with const fields that doesn't
make sense, as you would never be able to fill them in (without const_cast).

If you're suggesting the fields be TimeDeltas, this is where I started. But that
doesn't work, because TimeDelta is not a POD, making this class non-POD. (I also
went through a lengthy process to try to make TimeDelta a POD, or an alternative
TimeDeltaPOD class, but this didn't work out.)

If you're suggesting I make the TimeDeltaInternalType members public, I don't
like this, since the only correct way to use them is converting them to and from
TimeDelta. I much prefer the encapsulation of the getters and setters. It seems
to me it's the use case that setters and getters were actually designed for.

Or did you mean something else?

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

I was hoping the header file could be made much more concise by avoiding the
need for getters. However, you raise good points that they can't be const given
you want to set them (in an upcoming CL), and that you don't want to expose the
underlying TimeDeltaInternalType members. Fair enough. I take back this
suggestion.

+
+    CollectionParams(base::TimeDelta collection_duration,
+                     base::TimeDelta periodic_interval,
+                     TriggerParams resume_from_suspend,
+                     TriggerParams restore_session);
+
+    // Time perf is run for.
+    base::TimeDelta collection_duration() const {
+      return base::TimeDelta::FromInternalValue(collection_duration_);
+    }
+    void set_collection_duration(base::TimeDelta duration) {
+      collection_duration_ = duration.ToInternalValue();
+    }
+
+    // For PERIODIC_COLLECTION, partition time since login into successive
+    // intervals of this duration. In each interval, a random time is picked to
+    // collect a profile.
+    base::TimeDelta periodic_interval() const {
+      return base::TimeDelta::FromInternalValue(periodic_interval_);
+    }
+    void set_periodic_interval(base::TimeDelta interval) {
+      periodic_interval_ = interval.ToInternalValue();
+    }
+
+    // Parameters for RESUME_FROM_SUSPEND and RESTORE_SESSION collections:
+    const TriggerParams& resume_from_suspend() const {
+      return resume_from_suspend_;
+    }
+    TriggerParams& mutable_resume_from_suspend() {
+      return resume_from_suspend_;
+    }
+    const TriggerParams& restore_session() const {
+      return restore_session_;
+    }
+    TriggerParams& mutable_restore_session() {
+      return restore_session_;
+    }
+
+   private:
+    CollectionParams() = default;  // POD
+
+    TimeDeltaInternalType collection_duration_;
+    TimeDeltaInternalType periodic_interval_;
+    TriggerParams resume_from_suspend_;
+    TriggerParams restore_session_;
+  };
+
   // Parses a PerfDataProto from serialized data |perf_data|, if it exists.
   // Parses a PerfStatProto from serialized data |perf_stat|, if it exists.
   // Only one of these may contain data. If both |perf_data| and |perf_stat|
@@ -53,6 +126,8 @@ class PerfProvider : public base::NonThreadSafe,
       const std::vector<uint8>& perf_stat);
 
  private:
+  static const CollectionParams kDefaultParameters;
+
   // Class that listens for changes to the login state. When a normal user logs
   // in, it updates PerfProvider to start collecting data.
   class LoginObserver : public chromeos::LoginState::Observer {
@@ -111,6 +186,9 @@ class PerfProvider : public base::NonThreadSafe,
       const base::TimeDelta& time_after_restore,
       int num_tabs_restored);
 
+  // Parameters controlling how profiles are collected.
+  CollectionParams collection_params_;
+
   // Vector of SampledProfile protobufs containing perf profiles.
   std::vector<SampledProfile> cached_perf_data_;