Remove UKM-related duplicate code.

(Another CL for non-UKM-related duplicate code forthcoming. I
wanted to keep the CL sizes reasonable.)

Bug: 1066297
Change-Id: I699d3e72ff69c3844b1f6838bd1b329d2804bcd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135826
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757884}

chrome/browser/metrics/chrome_metrics_services_manager_client.h

@@ -24,6 +24,7 @@ class MetricsStateManager;
 
 // Used only for testing.
 namespace internal {
+// TODO(crbug.com/1068796): Replace kMetricsReportingFeature with a better name.
 extern const base::Feature kMetricsReportingFeature;
 }
 }

chrome/test/BUILD.gn

@@ -247,6 +247,7 @@ static_library("test_support") {
       # target.
       "//chrome/app:test_support",
       "//components/ukm:test_support",
+      "//components/ukm:ukm_test_helper",
       "//components/zoom:test_support",
     ]
     sources += [
@@ -5804,6 +5805,7 @@ if (!is_android) {
       "//components/sync",
       "//components/sync:test_support_model",
       "//components/ukm:test_support",
+      "//components/ukm:ukm_test_helper",
       "//content/app/resources",
       "//content/test:test_support",
       "//crypto:platform",

components/ukm/BUILD.gn

@@ -120,3 +120,19 @@ test("ukm_unittests") {
     "//components/test:run_all_unittests",
   ]
 }
+
+static_library("ukm_test_helper") {
+  testonly = true
+  sources = [
+    "ukm_test_helper.cc",
+    "ukm_test_helper.h",
+  ]
+  deps = [
+    ":ukm",
+    "//base",
+    "//components/metrics",
+    "//services/metrics/public/cpp:metrics_cpp",
+    "//third_party/metrics_proto",
+    "//third_party/zlib/google:compression_utils",
+  ]
+}

components/ukm/ukm_recorder_impl.h

@@ -24,13 +24,13 @@
 
 namespace metrics {
 class UkmBrowserTestBase;
-class UkmEGTestHelper;
 }
 
 namespace ukm {
 class Report;
 class UkmRecorderImplTest;
 class UkmSource;
+class UkmTestHelper;
 class UkmUtilsForTest;
 
 namespace debug {
@@ -129,9 +129,9 @@ class UkmRecorderImpl : public UkmRecorder {
 
  private:
   friend ::metrics::UkmBrowserTestBase;
-  friend ::metrics::UkmEGTestHelper;
   friend ::ukm::debug::UkmDebugDataExtractor;
   friend ::ukm::UkmRecorderImplTest;
+  friend ::ukm::UkmTestHelper;
   friend ::ukm::UkmUtilsForTest;
   FRIEND_TEST_ALL_PREFIXES(UkmRecorderImplTest, IsSampledIn);
   FRIEND_TEST_ALL_PREFIXES(UkmRecorderImplTest, PurgeExtensionRecordings);

components/ukm/ukm_service.h

@@ -30,12 +30,12 @@ FORWARD_DECLARE_TEST(IOSChromeMetricsServiceClientTest,
 namespace metrics {
 class MetricsServiceClient;
 class UkmBrowserTestBase;
-class UkmEGTestHelper;
 class UkmDemographicMetricsProvider;
 }
 
 namespace ukm {
 class Report;
+class UkmTestHelper;
 
 namespace debug {
 class UkmDebugDataExtractor;
@@ -110,7 +110,7 @@ class UkmService : public UkmRecorderImpl {
 
  private:
   friend ::metrics::UkmBrowserTestBase;
-  friend ::metrics::UkmEGTestHelper;
+  friend ::ukm::UkmTestHelper;
   friend ::ukm::debug::UkmDebugDataExtractor;
   friend ::ukm::UkmUtilsForTest;
   FRIEND_TEST_ALL_PREFIXES(::ChromeMetricsServiceClientTest,

components/ukm/ukm_test_helper.cc

+// Copyright 2020 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.
+
+#include "components/ukm/ukm_test_helper.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/feature_list.h"
+#include "base/run_loop.h"
+#include "base/stl_util.h"
+#include "components/metrics/unsent_log_store.h"
+#include "third_party/zlib/google/compression_utils.h"
+
+namespace ukm {
+
+UkmTestHelper::UkmTestHelper(UkmService* ukm_service)
+    : ukm_service_(ukm_service) {}
+
+bool UkmTestHelper::IsExtensionRecordingEnabled() const {
+  return ukm_service_ ? ukm_service_->extensions_enabled_ : false;
+}
+
+bool UkmTestHelper::IsRecordingEnabled() const {
+  return ukm_service_ ? ukm_service_->recording_enabled_ : false;
+}
+
+bool UkmTestHelper::IsReportUserNoisedUserBirthYearAndGenderEnabled() {
+  return base::FeatureList::IsEnabled(
+      ukm::UkmService::kReportUserNoisedUserBirthYearAndGender);
+}
+
+uint64_t UkmTestHelper::GetClientId() {
+  return ukm_service_->client_id_;
+}
+
+std::unique_ptr<Report> UkmTestHelper::GetUkmReport() {
+  if (!HasUnsentLogs())
+    return nullptr;
+
+  metrics::UnsentLogStore* log_store =
+      ukm_service_->reporting_service_.ukm_log_store();
+  if (log_store->has_staged_log()) {
+    // For testing purposes, we examine the content of a staged log without
+    // ever sending the log, so discard any previously staged log.
+    log_store->DiscardStagedLog();
+  }
+
+  log_store->StageNextLog();
+  if (!log_store->has_staged_log())
+    return nullptr;
+
+  std::string uncompressed_log_data;
+  if (!compression::GzipUncompress(log_store->staged_log(),
+                                   &uncompressed_log_data))
+    return nullptr;
+
+  std::unique_ptr<ukm::Report> report = std::make_unique<ukm::Report>();
+  if (!report->ParseFromString(uncompressed_log_data))
+    return nullptr;
+
+  return report;
+}
+
+UkmSource* UkmTestHelper::GetSource(SourceId source_id) {
+  if (!ukm_service_)
+    return nullptr;
+
+  auto it = ukm_service_->sources().find(source_id);
+  return it == ukm_service_->sources().end() ? nullptr : it->second.get();
+}
+
+bool UkmTestHelper::HasSource(SourceId source_id) {
+  return ukm_service_ && base::Contains(ukm_service_->sources(), source_id);
+}
+
+bool UkmTestHelper::IsSourceObsolete(SourceId source_id) {
+  return ukm_service_ &&
+         base::Contains(ukm_service_->recordings_.obsolete_source_ids,
+                        source_id);
+}
+
+void UkmTestHelper::RecordSourceForTesting(SourceId source_id) {
+  if (ukm_service_)
+    ukm_service_->UpdateSourceURL(source_id, GURL("http://example.com"));
+}
+
+void UkmTestHelper::BuildAndStoreLog() {
+  // Wait for initialization to complete before flushing.
+  base::RunLoop run_loop;
+  ukm_service_->SetInitializationCompleteCallbackForTesting(
+      run_loop.QuitClosure());
+  run_loop.Run();
+
+  ukm_service_->Flush();
+}
+
+bool UkmTestHelper::HasUnsentLogs() {
+  return ukm_service_ &&
+         ukm_service_->reporting_service_.ukm_log_store()->has_unsent_logs();
+}
+
+}  // namespace ukm

components/ukm/ukm_test_helper.h

+// Copyright 2020 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 COMPONENTS_UKM_UKM_TEST_HELPER_H_
+#define COMPONENTS_UKM_UKM_TEST_HELPER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "components/ukm/ukm_service.h"
+#include "services/metrics/public/cpp/ukm_source.h"
+#include "services/metrics/public/cpp/ukm_source_id.h"
+#include "third_party/metrics_proto/ukm/report.pb.h"
+
+namespace ukm {
+
+// Helper class that provides access to UkmService internals. This class is a
+// friend of UkmService and UkmRecorderImpl.
+class UkmTestHelper {
+ public:
+  explicit UkmTestHelper(UkmService* ukm_service);
+  ~UkmTestHelper() = default;
+
+  // Returns true if |ukm_service_| records extensions.
+  bool IsExtensionRecordingEnabled() const;
+
+  // Returns true if |ukm_service_| has recording enabled.
+  bool IsRecordingEnabled() const;
+
+  // Returns true if |ukm_service_| if the following feature is enabled:
+  // kReportUserNoisedUserBirthYearAndGender.
+  static bool IsReportUserNoisedUserBirthYearAndGenderEnabled();
+
+  // Returns |ukm_service_|'s client ID.
+  uint64_t GetClientId();
+
+  // Creates and returns a UKM report if there are unsent logs from which a
+  // report can be generated. Returns nullptr otherwise.
+  std::unique_ptr<Report> GetUkmReport();
+
+  // Returns the UkmSource corresponding to |source_id|, if present; otherwise,
+  // returns nullptr.
+  UkmSource* GetSource(SourceId source_id);
+
+  // Returns true if |ukm_service_| has a source corresponding to |source_id|.
+  bool HasSource(SourceId source_id);
+
+  // Returns true if UkmSource denoted by |source_id| is marked as obsolete.
+  bool IsSourceObsolete(SourceId source_id);
+
+  // Adds a dummy source with |source_id| to |ukm_service_|.
+  void RecordSourceForTesting(SourceId source_id);
+
+  // Creates a log and stores it in |ukm_service_|'s UnsentLogStore.
+  void BuildAndStoreLog();
+
+  // Reeturns true if |ukm_service_| has logs to send.
+  bool HasUnsentLogs();
+
+ private:
+  UkmService* const ukm_service_;
+};
+
+}  // namespace ukm
+
+#endif  // COMPONENTS_UKM_UKM_TEST_HELPER_H_

ios/chrome/browser/metrics/BUILD.gn

@@ -329,6 +329,7 @@ source_set("test_support") {
     "//components/network_time",
     "//components/sync/base",
     "//components/ukm",
+    "//components/ukm:ukm_test_helper",
     "//ios/chrome/app:app_internal",
     "//ios/chrome/app/strings",
     "//ios/chrome/browser",
@@ -344,7 +345,6 @@ source_set("test_support") {
     "//ios/chrome/test/earl_grey:test_support",
     "//ios/testing/earl_grey:earl_grey_support",
     "//net:test_support",
-    "//services/metrics/public/cpp:metrics_cpp",
     "//third_party/metrics_proto",
 
     #TODO(crbug.com/1066297): Remove the below line.
@@ -386,14 +386,16 @@ source_set("eg_app_support+eg2") {
     "//components/network_time",
     "//components/sync/base",
     "//components/ukm",
+    "//components/ukm:ukm_test_helper",
     "//ios/chrome/browser",
     "//ios/chrome/browser/browser_state",
     "//ios/chrome/browser/metrics",
     "//ios/chrome/browser/metrics:metrics_internal",
     "//ios/chrome/test/app:test_support",
     "//ios/testing:nserror_support",
-    "//services/metrics/public/cpp:metrics_cpp",
     "//third_party/metrics_proto",
+
+    #TODO(crbug.com/1066297): Remove the below line.
     "//third_party/zlib/google:compression_utils",
   ]
 }

ios/chrome/browser/metrics/metrics_app_interface.h

@@ -32,24 +32,20 @@ const NSTimeInterval kSyncUKMOperationsTimeout = 10.0;
 + (void)stopOverridingMetricsAndCrashReportingForTesting;
 + (BOOL)setMetricsAndCrashReportingForTesting:(BOOL)enabled;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Returns whether UKM recording is |enabled|.
 + (BOOL)checkUKMRecordingEnabled:(BOOL)enabled;
 
 // Returns YES if the ReportUserNoisedUserBirthYearAndGender feature is enabled.
 + (BOOL)isReportUserNoisedUserBirthYearAndGenderEnabled WARN_UNUSED_RESULT;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Returns the current UKM client ID.
 + (uint64_t)UKMClientID;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Checks whether a sourceID is registered for UKM.
-+ (BOOL)UKMHasDummySource:(int64_t)sourceId;
++ (BOOL)UKMHasDummySource:(int64_t)sourceID;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Adds a new sourceID for UKM.
-+ (void)UKMRecordDummySource:(int64_t)sourceId;
++ (void)UKMRecordDummySource:(int64_t)sourceID;
 
 // TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Sets the network time to approximately now.
@@ -60,12 +56,10 @@ const NSTimeInterval kSyncUKMOperationsTimeout = 10.0;
 // year, pre-noise.
 + (int)noisedBirthYear:(int)rawBirthYear;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // If data are available, creates a UKM Report and stores it in the
 // UKM service's UnsentLogStore.
 + (void)buildAndStoreUKMLog;
 
-// TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Returns YES if the UKM service has logs to send.
 + (BOOL)hasUnsentUKMLogs;
 

ios/chrome/browser/metrics/metrics_app_interface.mm

@@ -6,7 +6,6 @@
 
 #include <string>
 
-#include "base/feature_list.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ -20,13 +19,13 @@
 #include "components/network_time/network_time_tracker.h"
 #include "components/sync/base/pref_names.h"
 #include "components/ukm/ukm_service.h"
+#include "components/ukm/ukm_test_helper.h"
 #include "ios/chrome/browser/application_context.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state_manager.h"
 #include "ios/chrome/browser/metrics/ios_chrome_metrics_service_accessor.h"
 #import "ios/chrome/test/app/histogram_test_util.h"
 #import "ios/testing/nserror_util.h"
-#include "services/metrics/public/cpp/ukm_source_id.h"
 #include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h"
 #include "third_party/metrics_proto/ukm/report.pb.h"
 #include "third_party/zlib/google/compression_utils.h"
@@ -41,6 +40,10 @@ bool g_metrics_enabled = false;
 
 chrome_test_util::HistogramTester* g_histogram_tester = nullptr;
 
+ukm::UkmService* GetUkmService() {
+  return GetApplicationContext()->GetMetricsServicesManager()->GetUkmService();
+}
+
 // TODO(crbug.com/1066297): Refactor to remove duplicate code.
 // Returns an UMA log if the metrics service has a staged log.
 std::unique_ptr<metrics::ChromeUserMetricsExtension> GetLastUmaLog() {
@@ -65,101 +68,6 @@ std::unique_ptr<metrics::ChromeUserMetricsExtension> GetLastUmaLog() {
 
 }  // namespace
 
-namespace metrics {
-
-// Helper class that provides access to UKM internals.
-// This class is a friend of UKMService and UkmRecorderImpl.
-class UkmEGTestHelper {
- public:
-  UkmEGTestHelper() {}
-  UkmEGTestHelper(const UkmEGTestHelper&) = delete;
-  UkmEGTestHelper& operator=(UkmEGTestHelper&) = delete;
-
-  static bool ukm_enabled() {
-    auto* service = ukm_service();
-    return service ? service->recording_enabled_ : false;
-  }
-
-  static bool IsReportUserNoisedUserBirthYearAndGenderEnabled() {
-    return base::FeatureList::IsEnabled(
-        ukm::UkmService::kReportUserNoisedUserBirthYearAndGender);
-  }
-
-  static uint64_t client_id() {
-    auto* service = ukm_service();
-    return service ? service->client_id_ : 0;
-  }
-
-  static bool HasDummySource(int64_t source_id_as_int64) {
-    auto* service = ukm_service();
-    ukm::SourceId source_id = source_id_as_int64;
-    return service && base::Contains(service->sources(), source_id);
-  }
-
-  static void RecordDummySource(ukm::SourceId source_id_as_int64) {
-    ukm::UkmService* service = ukm_service();
-    ukm::SourceId source_id = source_id_as_int64;
-    if (service)
-      service->UpdateSourceURL(source_id, GURL("http://example.com"));
-  }
-
-  static void BuildAndStoreUkmLog() {
-    ukm::UkmService* service = ukm_service();
-
-    // Wait for initialization to complete before flushing.
-    base::RunLoop run_loop;
-    service->SetInitializationCompleteCallbackForTesting(
-        run_loop.QuitClosure());
-    run_loop.Run();
-
-    service->Flush();
-  }
-
-  static bool HasUnsentUkmLogs() {
-    ukm::UkmService* service = ukm_service();
-    return service->reporting_service_.ukm_log_store()->has_unsent_logs();
-  }
-
-  static std::unique_ptr<ukm::Report> GetUKMReport() {
-    if (!HasUnsentUkmLogs()) {
-      return nullptr;
-    }
-
-    metrics::UnsentLogStore* log_store =
-        ukm_service()->reporting_service_.ukm_log_store();
-    if (log_store->has_staged_log()) {
-      // For testing purposes, we are examining the content of a staged log
-      // without ever sending the log, so discard any previously staged log.
-      log_store->DiscardStagedLog();
-    }
-    log_store->StageNextLog();
-    if (!log_store->has_staged_log()) {
-      return nullptr;
-    }
-
-    std::string uncompressed_log_data;
-    if (!compression::GzipUncompress(log_store->staged_log(),
-                                     &uncompressed_log_data)) {
-      return nullptr;
-    }
-
-    std::unique_ptr<ukm::Report> report = std::make_unique<ukm::Report>();
-    if (!report->ParseFromString(uncompressed_log_data)) {
-      return nullptr;
-    }
-    return report;
-  }
-
- private:
-  static ukm::UkmService* ukm_service() {
-    return GetApplicationContext()
-        ->GetMetricsServicesManager()
-        ->GetUkmService();
-  }
-};
-
-}  // namespace metrics
-
 @implementation MetricsAppInterface : NSObject
 
 + (void)overrideMetricsAndCrashReportingForTesting {
@@ -181,28 +89,32 @@ class UkmEGTestHelper {
 }
 
 + (BOOL)checkUKMRecordingEnabled:(BOOL)enabled {
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+
   ConditionBlock condition = ^{
-    return metrics::UkmEGTestHelper::ukm_enabled() == enabled;
+    return ukm_test_helper.IsRecordingEnabled() == enabled;
   };
   return base::test::ios::WaitUntilConditionOrTimeout(
       syncher::kSyncUKMOperationsTimeout, condition);
 }
 
 + (BOOL)isReportUserNoisedUserBirthYearAndGenderEnabled {
-  return metrics::UkmEGTestHelper::
-      IsReportUserNoisedUserBirthYearAndGenderEnabled();
+  return ukm::UkmTestHelper::IsReportUserNoisedUserBirthYearAndGenderEnabled();
 }
 
 + (uint64_t)UKMClientID {
-  return metrics::UkmEGTestHelper::client_id();
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  return ukm_test_helper.GetClientId();
 }
 
-+ (BOOL)UKMHasDummySource:(int64_t)sourceId {
-  return metrics::UkmEGTestHelper::HasDummySource(sourceId);
++ (BOOL)UKMHasDummySource:(int64_t)sourceID {
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  return ukm_test_helper.HasSource(sourceID);
 }
 
 + (void)UKMRecordDummySource:(int64_t)sourceID {
-  metrics::UkmEGTestHelper::RecordDummySource(sourceID);
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  ukm_test_helper.RecordSourceForTesting(sourceID);
 }
 
 + (void)setNetworkTimeForTesting {
@@ -230,16 +142,18 @@ class UkmEGTestHelper {
 }
 
 + (void)buildAndStoreUKMLog {
-  metrics::UkmEGTestHelper::BuildAndStoreUkmLog();
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  ukm_test_helper.BuildAndStoreLog();
 }
 
 + (BOOL)hasUnsentUKMLogs {
-  return metrics::UkmEGTestHelper::HasUnsentUkmLogs();
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  return ukm_test_helper.HasUnsentLogs();
 }
 
 + (BOOL)UKMReportHasBirthYear:(int)year gender:(int)gender {
-  std::unique_ptr<ukm::Report> report =
-      metrics::UkmEGTestHelper::GetUKMReport();
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  std::unique_ptr<ukm::Report> report = ukm_test_helper.GetUkmReport();
   int noisedBirthYear = [self noisedBirthYear:year];
 
   return report && gender == report->user_demographics().gender() &&
@@ -247,8 +161,9 @@ class UkmEGTestHelper {
 }
 
 + (BOOL)UKMReportHasUserDemographics {
-  std::unique_ptr<ukm::Report> report =
-      metrics::UkmEGTestHelper::GetUKMReport();
+  ukm::UkmTestHelper ukm_test_helper(GetUkmService());
+  // TODO(crbug.com/1066910): Maybe ukm::Report*?
+  std::unique_ptr<ukm::Report> report = ukm_test_helper.GetUkmReport();
   return report && report->has_user_demographics();
 }