Revert of Add multi-dimensional test infrastructure to RAPPOR + tests (patchset #5 id:80001 of https://codereview.chromium.org/1386493002/ )

Reason for revert:
builds failing for
https://codereview.chromium.org/1374363002/.

Original issue's description:
> Add multi-dimensional test infrastructure to RAPPOR + tests
>
> This is a first draft for adding support for multi-d tests within TestRapporService. The approach I chose was to create a TestSample class that shadow-copied all fields, which gets added to a map when RecordSampleObj is called. This was the simplest way I could think to do this without refactoring the original implementation too much.
>
> Calling GetRecordedSampleForMetric(string) yields the shadow object, which contains the type, flag fields, and string fields.
>
> It also might be possible to reduce the string-sample case to this more general one.
>
> BUG=538217
>
> Committed: https://crrev.com/1707804853121a976910512b28d31ada3088f276
> Cr-Commit-Position: refs/heads/master@{#352756}

TBR=holte@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org,csharrison@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=538217

Review URL: https://codereview.chromium.org/1387353002

Cr-Commit-Position: refs/heads/master@{#352764}

7 files changed, 51 insertions, 225 deletions

diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc b/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
index c09f97f..fb4e5f3 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer_unittest.cc
@@ -9,13 +9,11 @@
 #include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
-#include "components/rappor/rappor_utils.h"
 #include "components/rappor/test_rappor_service.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/web_contents_tester.h"
-#include "testing/gtest/include/gtest/gtest.h"
 
 namespace page_load_metrics {
 
@@ -25,9 +23,6 @@ const char kDefaultTestUrl[] = "https://google.com";
 const char kDefaultTestUrlAnchor[] = "https://google.com#samepage";
 const char kDefaultTestUrl2[] = "https://whatever.com";
 
-const char kRapporMetricsNameCoarseTiming[] =
-    "PageLoad.CoarseTiming.NavigationToFirstLayout";
-
 const char kHistogramNameFirstLayout[] =
     "PageLoad.Timing2.NavigationToFirstLayout";
 const char kHistogramNameDomContent[] =
@@ -465,69 +460,4 @@ TEST_F(MetricsWebContentsObserverTest,
   histogram_tester_.ExpectBucketCount(
       kHistogramNameEvents, PAGE_LOAD_SUCCESSFUL_FIRST_LAYOUT_BACKGROUND, 1);
 }
-
-TEST_F(MetricsWebContentsObserverTest, NoRappor) {
-  rappor::TestSample::Shadow* sample_obj =
-      rappor_tester_.GetRecordedSampleForMetric(kRapporMetricsNameCoarseTiming);
-  EXPECT_EQ(sample_obj, nullptr);
-}
-
-TEST_F(MetricsWebContentsObserverTest, RapporLongPageLoad) {
-  PageLoadTiming timing;
-  timing.navigation_start = base::Time::FromDoubleT(1);
-  timing.first_layout = base::TimeDelta::FromSeconds(40);
-
-  content::WebContentsTester* web_contents_tester =
-      content::WebContentsTester::For(web_contents());
-  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
-
-  observer_->OnMessageReceived(
-      PageLoadMetricsMsg_TimingUpdated(observer_->routing_id(), timing),
-      main_rfh());
-
-  // Navigate again to force logging RAPPOR.
-  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
-  // TODO(csharrison) Check coarse histogram has no samples once we have support
-  // for multi-dimensional rappor testing.
-  rappor::TestSample::Shadow* sample_obj =
-      rappor_tester_.GetRecordedSampleForMetric(kRapporMetricsNameCoarseTiming);
-  const auto& string_it = sample_obj->string_fields.find("Domain");
-  EXPECT_NE(string_it, sample_obj->string_fields.end());
-  EXPECT_EQ(string_it->second,
-            rappor::GetDomainAndRegistrySampleFromGURL(GURL(kDefaultTestUrl)));
-
-  const auto& flag_it = sample_obj->flag_fields.find("IsSlow");
-  EXPECT_NE(flag_it, sample_obj->flag_fields.end());
-  EXPECT_EQ(flag_it->second, static_cast<uint64_t>(true));
-}
-
-TEST_F(MetricsWebContentsObserverTest, RapporQuickPageLoad) {
-  PageLoadTiming timing;
-  timing.navigation_start = base::Time::FromDoubleT(1);
-  timing.first_layout = base::TimeDelta::FromSeconds(1);
-
-  content::WebContentsTester* web_contents_tester =
-      content::WebContentsTester::For(web_contents());
-  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
-
-  observer_->OnMessageReceived(
-      PageLoadMetricsMsg_TimingUpdated(observer_->routing_id(), timing),
-      main_rfh());
-
-  // Navigate again to force logging RAPPOR.
-  web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl2));
-  // TODO(csharrison) Check coarse histogram has no samples once we have support
-  // for multi-dimensional rappor testing.
-  rappor::TestSample::Shadow* sample_obj =
-      rappor_tester_.GetRecordedSampleForMetric(kRapporMetricsNameCoarseTiming);
-  const auto& string_it = sample_obj->string_fields.find("Domain");
-  EXPECT_NE(string_it, sample_obj->string_fields.end());
-  EXPECT_EQ(string_it->second,
-            rappor::GetDomainAndRegistrySampleFromGURL(GURL(kDefaultTestUrl)));
-
-  const auto& flag_it = sample_obj->flag_fields.find("IsSlow");
-  EXPECT_NE(flag_it, sample_obj->flag_fields.end());
-  EXPECT_EQ(flag_it->second, static_cast<uint64_t>(false));
-}
-
 }  // namespace page_load_metrics
diff --git a/components/rappor/rappor_parameters.h b/components/rappor/rappor_parameters.h
index 44cfbab..807b246 100644
--- a/components/rappor/rappor_parameters.h
+++ b/components/rappor/rappor_parameters.h
@@ -9,18 +9,6 @@
 
 namespace rappor {
 
-// The type of data stored in a metric.
-enum RapporType {
-  // Generic metrics from UMA opt-in users.
-  UMA_RAPPOR_TYPE = 0,
-  // Generic metrics for SafeBrowsing users.
-  SAFEBROWSING_RAPPOR_TYPE,
-  // Deprecated: Use UMA_RAPPOR_TYPE for new metrics
-  ETLD_PLUS_ONE_RAPPOR_TYPE,
-  NUM_RAPPOR_TYPES,
-  COARSE_RAPPOR_TYPE = SAFEBROWSING_RAPPOR_TYPE,
-};
-
 enum Probability {
   PROBABILITY_75,    // 75%
   PROBABILITY_50,    // 50%
@@ -72,40 +60,6 @@ struct RapporParameters {
   RecordingGroup recording_group;
 };
 
-namespace internal {
-
-const RapporParameters kRapporParametersForType[NUM_RAPPOR_TYPES] = {
-    // UMA_RAPPOR_TYPE
-    {128 /* Num cohorts */,
-     4 /* Bloom filter size bytes */,
-     2 /* Bloom filter hash count */,
-     rappor::PROBABILITY_50 /* Fake data probability */,
-     rappor::PROBABILITY_50 /* Fake one probability */,
-     rappor::PROBABILITY_75 /* One coin probability */,
-     rappor::PROBABILITY_25 /* Zero coin probability */,
-     UMA_RAPPOR_GROUP /* Recording group */},
-    // SAFEBROWSING_RAPPOR_TYPE
-    {128 /* Num cohorts */,
-     1 /* Bloom filter size bytes */,
-     2 /* Bloom filter hash count */,
-     rappor::PROBABILITY_50 /* Fake data probability */,
-     rappor::PROBABILITY_50 /* Fake one probability */,
-     rappor::PROBABILITY_75 /* One coin probability */,
-     rappor::PROBABILITY_25 /* Zero coin probability */,
-     SAFEBROWSING_RAPPOR_GROUP /* Recording group */},
-    // ETLD_PLUS_ONE_RAPPOR_TYPE
-    {128 /* Num cohorts */,
-     16 /* Bloom filter size bytes */,
-     2 /* Bloom filter hash count */,
-     rappor::PROBABILITY_50 /* Fake data probability */,
-     rappor::PROBABILITY_50 /* Fake one probability */,
-     rappor::PROBABILITY_75 /* One coin probability */,
-     rappor::PROBABILITY_25 /* Zero coin probability */,
-     UMA_RAPPOR_GROUP /* Recording group */},
-};
-
-}
-
 }  // namespace rappor
 
 #endif  // COMPONENTS_RAPPOR_RAPPOR_PARAMETERS_H_
diff --git a/components/rappor/rappor_service.cc b/components/rappor/rappor_service.cc
index fe8aed9..e266232 100644
--- a/components/rappor/rappor_service.cc
+++ b/components/rappor/rappor_service.cc
@@ -47,6 +47,36 @@ GURL GetServerUrl() {
     return GURL(kDefaultServerUrl);
 }
 
+const RapporParameters kRapporParametersForType[NUM_RAPPOR_TYPES] = {
+    // UMA_RAPPOR_TYPE
+    {128 /* Num cohorts */,
+     4 /* Bloom filter size bytes */,
+     2 /* Bloom filter hash count */,
+     rappor::PROBABILITY_50 /* Fake data probability */,
+     rappor::PROBABILITY_50 /* Fake one probability */,
+     rappor::PROBABILITY_75 /* One coin probability */,
+     rappor::PROBABILITY_25 /* Zero coin probability */,
+     UMA_RAPPOR_GROUP /* Recording group */},
+    // SAFEBROWSING_RAPPOR_TYPE
+    {128 /* Num cohorts */,
+     1 /* Bloom filter size bytes */,
+     2 /* Bloom filter hash count */,
+     rappor::PROBABILITY_50 /* Fake data probability */,
+     rappor::PROBABILITY_50 /* Fake one probability */,
+     rappor::PROBABILITY_75 /* One coin probability */,
+     rappor::PROBABILITY_25 /* Zero coin probability */,
+     SAFEBROWSING_RAPPOR_GROUP /* Recording group */},
+    // ETLD_PLUS_ONE_RAPPOR_TYPE
+    {128 /* Num cohorts */,
+     16 /* Bloom filter size bytes */,
+     2 /* Bloom filter hash count */,
+     rappor::PROBABILITY_50 /* Fake data probability */,
+     rappor::PROBABILITY_50 /* Fake one probability */,
+     rappor::PROBABILITY_75 /* One coin probability */,
+     rappor::PROBABILITY_25 /* Zero coin probability */,
+     UMA_RAPPOR_GROUP /* Recording group */},
+};
+
 }  // namespace
 
 RapporService::RapporService(
@@ -212,8 +242,7 @@ void RapporService::RecordSample(const std::string& metric_name,
   if (!IsInitialized())
     return;
   DCHECK_LT(type, NUM_RAPPOR_TYPES);
-  const RapporParameters& parameters =
-      internal::kRapporParametersForType[type];
+  const RapporParameters& parameters = kRapporParametersForType[type];
   DVLOG(2) << "Recording sample \"" << sample
            << "\" for metric \"" << metric_name
            << "\" of type: " << type;
@@ -252,7 +281,7 @@ scoped_ptr<Sample> RapporService::CreateSample(RapporType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsInitialized());
   return scoped_ptr<Sample>(
-      new Sample(cohort_, internal::kRapporParametersForType[type]));
+      new Sample(cohort_, kRapporParametersForType[type]));
 }
 
 void RapporService::RecordSampleObj(const std::string& metric_name,
diff --git a/components/rappor/rappor_service.h b/components/rappor/rappor_service.h
index 677070d..d34d502 100644
--- a/components/rappor/rappor_service.h
+++ b/components/rappor/rappor_service.h
@@ -32,6 +32,18 @@ class LogUploaderInterface;
 class RapporMetric;
 class RapporReports;
 
+// The type of data stored in a metric.
+enum RapporType {
+  // Generic metrics from UMA opt-in users.
+  UMA_RAPPOR_TYPE = 0,
+  // Generic metrics for SafeBrowsing users.
+  SAFEBROWSING_RAPPOR_TYPE,
+  // Deprecated: Use UMA_RAPPOR_TYPE for new metrics
+  ETLD_PLUS_ONE_RAPPOR_TYPE,
+  NUM_RAPPOR_TYPES,
+  COARSE_RAPPOR_TYPE = SAFEBROWSING_RAPPOR_TYPE,
+};
+
 // This class provides an interface for recording samples for rappor metrics,
 // and periodically generates and uploads reports based on the collected data.
 class RapporService {
@@ -61,7 +73,7 @@ class RapporService {
   void Update(int recording_groups, bool may_upload);
 
   // Constructs a Sample object for the caller to record fields in.
-  virtual scoped_ptr<Sample> CreateSample(RapporType);
+  scoped_ptr<Sample> CreateSample(RapporType);
 
   // Records a Sample of rappor metric specified by |metric_name|.
   //
@@ -77,8 +89,8 @@ class RapporService {
   // This will result in a report setting two metrics "MyMetric.Field1" and
   // "MyMetric.Field2", and they will both be generated from the same sample,
   // to allow for correllations to be computed.
-  virtual void RecordSampleObj(const std::string& metric_name,
-                               scoped_ptr<Sample> sample);
+  void RecordSampleObj(const std::string& metric_name,
+                       scoped_ptr<Sample> sample);
 
   // Records a sample of the rappor metric specified by |metric_name|.
   // Creates and initializes the metric, if it doesn't yet exist.
diff --git a/components/rappor/sample.h b/components/rappor/sample.h
index accda42..5eaf1de 100644
--- a/components/rappor/sample.h
+++ b/components/rappor/sample.h
@@ -24,21 +24,20 @@ class TestSamplerFactory;
 // same Rappor report, enabling analysis of correlations between those fields.
 class Sample {
  public:
-  virtual ~Sample();
+  ~Sample();
 
   // Sets a string value field in this sample.
-  virtual void SetStringField(
-      const std::string& field_name, const std::string& value);
+  void SetStringField(const std::string& field_name, const std::string& value);
 
   // Sets a group of boolean flags as a field in this sample.
   // |flags| should be a set of boolean flags stored in the lowest |num_flags|
   // bits of |flags|.
-  virtual void SetFlagsField(const std::string& field_name,
+  void SetFlagsField(const std::string& field_name,
                      uint64_t flags,
                      size_t num_flags);
 
   // Generate randomized reports and store them in |reports|.
-  virtual void ExportMetrics(const std::string& secret,
+  void ExportMetrics(const std::string& secret,
                      const std::string& metric_name,
                      RapporReports* reports) const;
 
@@ -47,7 +46,6 @@ class Sample {
  private:
   friend class TestSamplerFactory;
   friend class RapporService;
-  friend class TestSample;
 
   // Constructs a sample.  Instead of calling this directly, call
   // RapporService::MakeSampleObj to create a sample.
diff --git a/components/rappor/test_rappor_service.cc b/components/rappor/test_rappor_service.cc
index 23f0c41..8d347c5 100644
--- a/components/rappor/test_rappor_service.cc
+++ b/components/rappor/test_rappor_service.cc
@@ -4,7 +4,6 @@
 
 #include "components/rappor/test_rappor_service.h"
 
-#include "base/logging.h"
 #include "components/rappor/byte_vector_utils.h"
 #include "components/rappor/proto/rappor_metric.pb.h"
 #include "components/rappor/rappor_parameters.h"
@@ -21,34 +20,6 @@ bool MockIsIncognito(bool* is_incognito) {
 
 }  // namespace
 
-TestSample::TestSample(RapporType type)
-    : Sample(0, internal::kRapporParametersForType[type]), shadow_(type) {}
-
-TestSample::~TestSample() {}
-
-void TestSample::SetStringField(const std::string& field_name,
-                                const std::string& value) {
-  shadow_.string_fields[field_name] = value;
-  Sample::SetStringField(field_name, value);
-}
-
-void TestSample::SetFlagsField(const std::string& field_name,
-                               uint64_t flags,
-                               size_t num_flags) {
-  shadow_.flag_fields[field_name] = flags;
-  Sample::SetFlagsField(field_name, flags, num_flags);
-}
-
-TestSample::Shadow::Shadow(RapporType type) : type(type) {}
-
-TestSample::Shadow::Shadow(const TestSample::Shadow& other) {
-  type = other.type;
-  flag_fields = other.flag_fields;
-  string_fields = other.string_fields;
-}
-
-TestSample::Shadow::~Shadow() {}
-
 TestRapporService::TestRapporService()
     : RapporService(&test_prefs_, base::Bind(&MockIsIncognito, &is_incognito_)),
       next_rotation_(base::TimeDelta()),
@@ -63,22 +34,6 @@ TestRapporService::TestRapporService()
 
 TestRapporService::~TestRapporService() {}
 
-scoped_ptr<Sample> TestRapporService::CreateSample(RapporType type) {
-  scoped_ptr<TestSample> test_sample(new TestSample(type));
-  return test_sample.Pass();
-}
-
-void TestRapporService::RecordSampleObj(const std::string& metric_name,
-                                        scoped_ptr<Sample> sample) {
-  TestSample* test_sample = static_cast<TestSample*>(sample.get());
-  // Erase the previous sample if we logged one.
-  shadows_.erase(metric_name);
-  shadows_.insert(std::pair<std::string, TestSample::Shadow>(
-      metric_name, test_sample->GetShadow()));
-  // Original version is still called.
-  RapporService::RecordSampleObj(metric_name, sample.Pass());
-}
-
 void TestRapporService::RecordSample(const std::string& metric_name,
                                      RapporType type,
                                      const std::string& sample) {
@@ -101,14 +56,6 @@ void TestRapporService::GetReports(RapporReports* reports) {
   ExportMetrics(reports);
 }
 
-TestSample::Shadow* TestRapporService::GetRecordedSampleForMetric(
-    const std::string& metric_name) {
-  ShadowMap::iterator it = shadows_.find(metric_name);
-  if (it == shadows_.end())
-    return nullptr;
-  return &(it->second);
-}
-
 bool TestRapporService::GetRecordedSampleForMetric(
     const std::string& metric_name,
     std::string* sample,
diff --git a/components/rappor/test_rappor_service.h b/components/rappor/test_rappor_service.h
index a1d2cbd..ead80a8 100644
--- a/components/rappor/test_rappor_service.h
+++ b/components/rappor/test_rappor_service.h
@@ -14,35 +14,6 @@
 
 namespace rappor {
 
-// This class duplicates the functionality of Sample, with a cohort initialized
-// always to 0. It keeps a shadow object around which copies every flag field
-// and string field set on the Sample.
-class TestSample : public Sample {
- public:
-  TestSample(RapporType type);
-  ~TestSample() override;
-
-  void SetStringField(const std::string& field_name,
-                      const std::string& value) override;
-
-  void SetFlagsField(const std::string& field_name,
-                     uint64_t flags,
-                     size_t num_flags) override;
-  struct Shadow {
-    Shadow(RapporType type);
-    Shadow(const TestSample::Shadow& other);
-    ~Shadow();
-    RapporType type;
-    std::map<std::string, uint64_t> flag_fields;
-    std::map<std::string, std::string> string_fields;
-  };
-
-  Shadow GetShadow() { return shadow_; }
-
- private:
-  Shadow shadow_;
-};
-
 // This class provides a simple instance that can be instantiated by tests
 // and examined to check that metrics were recorded.  It assumes the most
 // permissive settings so that any metric can be recorded.
@@ -52,11 +23,7 @@ class TestRapporService : public RapporService {
 
   ~TestRapporService() override;
 
-  scoped_ptr<Sample> CreateSample(RapporType type) override;
-
   // Intercepts the sample being recorded and saves it in a test structure.
-  void RecordSampleObj(const std::string& metric_name,
-                       scoped_ptr<Sample> sample) override;
   void RecordSample(const std::string& metric_name,
                     RapporType type,
                     const std::string& sample) override;
@@ -71,13 +38,6 @@ class TestRapporService : public RapporService {
   // This clears the internal map of metrics.
   void GetReports(RapporReports* reports);
 
-  // Gets the recorded sample for |metric_name|. This returns the shadow object
-  // for the sample, which contains the string fields, flag fields, and type.
-  // Limitation: if the metric was logged more than once, this will return the
-  // latest sample that was logged.
-  TestSample::Shadow* GetRecordedSampleForMetric(
-      const std::string& metric_name);
-
   // Gets the recorded sample/type for a |metric_name|, and returns whether the
   // recorded metric was found. Limitation: if the metric was logged more than
   // once, this will return the latest sample that was logged.
@@ -108,10 +68,6 @@ class TestRapporService : public RapporService {
   };
   typedef std::map<std::string, RapporSample> SamplesMap;
   SamplesMap samples_;
-  // Recording a TestSample inserts its shadow into this map, which has all of
-  // its fields copied.
-  typedef std::map<std::string, TestSample::Shadow> ShadowMap;
-  ShadowMap shadows_;
 
   TestingPrefServiceSimple test_prefs_;