[EoC] Moving metrics reporter to service proxy

* Creates a CSMetricsReporterProvider, which creates CSMetricsReporter
* Gives the CCSService repsponsibility for proxy creation and injection
  of a freshly made metrics reporter.
* Moves metrics reporting to CCSServiceProxy

Bug: 824183
Change-Id: I68df53707160a08038f7cf9666a13c0cd5d56dee
Reviewed-on: https://chromium-review.googlesource.com/1006481
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550243}

chrome/browser/android/contextual_suggestions/contextual_suggestions_bridge.cc

@@ -43,8 +43,7 @@ static jlong JNI_ContextualSuggestionsBridge_Init(
           ContextualContentSuggestionsServiceFactory::GetForProfile(profile);
 
   std::unique_ptr<ContextualContentSuggestionsServiceProxy> service_proxy =
-      std::make_unique<ContextualContentSuggestionsServiceProxy>(
-          contextual_suggestions_service);
+      contextual_suggestions_service->CreateProxy();
 
   ContextualSuggestionsBridge* contextual_suggestions_bridge =
       new ContextualSuggestionsBridge(env, std::move(service_proxy));
@@ -77,6 +76,7 @@ ContextualSuggestionsBridge::~ContextualSuggestionsBridge() {}
 
 void ContextualSuggestionsBridge::Destroy(JNIEnv* env,
                                           const JavaParamRef<jobject>& obj) {
+  service_proxy_->FlushMetrics();
   delete this;
 }
 

chrome/browser/ntp_snippets/contextual_content_suggestions_service_factory.cc

@@ -108,12 +108,13 @@ ContextualContentSuggestionsServiceFactory::BuildServiceInstanceFor(
               std::make_unique<suggestions::ImageDecoderImpl>(),
               request_context.get()),
           pref_service, contextual_suggestions_database.get());
-  auto metrics_reporter = std::make_unique<
-      contextual_suggestions::ContextualSuggestionsMetricsReporter>();
+  auto metrics_reporter_provider = std::make_unique<
+      contextual_suggestions::ContextualSuggestionsMetricsReporterProvider>();
   auto* service = new ContextualContentSuggestionsService(
       std::move(contextual_suggestions_fetcher),
       std::move(cached_image_fetcher),
-      std::move(contextual_suggestions_database), std::move(metrics_reporter));
+      std::move(contextual_suggestions_database),
+      std::move(metrics_reporter_provider));
 
   return service;
 }

components/ntp_snippets/contextual/contextual_content_suggestions_service.cc

@@ -15,27 +15,28 @@
 #include "components/ntp_snippets/remote/cached_image_fetcher.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider_impl.h"
+#include "contextual_content_suggestions_service_proxy.h"
 #include "ui/gfx/image/image.h"
 
 namespace ntp_snippets {
 
-using contextual_suggestions::ContextualSuggestionsMetricsReporter;
 using contextual_suggestions::Cluster;
+using contextual_suggestions::ContextualSuggestionsMetricsReporterProvider;
 using contextual_suggestions::FetchClustersCallback;
 
 ContextualContentSuggestionsService::ContextualContentSuggestionsService(
     std::unique_ptr<ContextualSuggestionsFetcher>
         contextual_suggestions_fetcher,
     std::unique_ptr<CachedImageFetcher> image_fetcher,
     std::unique_ptr<RemoteSuggestionsDatabase> contextual_suggestions_database,
-    std::unique_ptr<ContextualSuggestionsMetricsReporter> metrics_reporter)
+    std::unique_ptr<ContextualSuggestionsMetricsReporterProvider>
+        metrics_reporter_provider)
     : contextual_suggestions_database_(
           std::move(contextual_suggestions_database)),
       contextual_suggestions_fetcher_(
           std::move(contextual_suggestions_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
-      metrics_reporter_(std::move(metrics_reporter)),
-      last_ukm_source_id_(ukm::kInvalidSourceId) {}
+      metrics_reporter_provider_(std::move(metrics_reporter_provider)) {}
 
 ContextualContentSuggestionsService::~ContextualContentSuggestionsService() =
     default;
@@ -77,26 +78,13 @@ void ContextualContentSuggestionsService::FetchContextualSuggestionImageLegacy(
   FetchContextualSuggestionImage(suggestion_id, image_url, std::move(callback));
 }
 
-void ContextualContentSuggestionsService::ReportEvent(
-    ukm::SourceId ukm_source_id,
-    contextual_suggestions::ContextualSuggestionsEvent event) {
-  DCHECK(ukm_source_id != ukm::kInvalidSourceId);
-
-  // Flush the previous page (if any) and setup the new page.
-  if (ukm_source_id != last_ukm_source_id_) {
-    if (last_ukm_source_id_ != ukm::kInvalidSourceId)
-      metrics_reporter_->Flush();
-    last_ukm_source_id_ = ukm_source_id;
-    metrics_reporter_->SetupForPage(ukm_source_id);
-  }
-
-  metrics_reporter_->RecordEvent(event);
+std::unique_ptr<
+    contextual_suggestions::ContextualContentSuggestionsServiceProxy>
+ContextualContentSuggestionsService::CreateProxy() {
+  return std::make_unique<
+      contextual_suggestions::ContextualContentSuggestionsServiceProxy>(
+      this, metrics_reporter_provider_->CreateMetricsReporter());
 }
 
-void ContextualContentSuggestionsService::Shutdown() {
-  if (last_ukm_source_id_ != ukm::kInvalidSourceId)
-    metrics_reporter_->Flush();
-  last_ukm_source_id_ = ukm::kInvalidSourceId;
-}
 
 }  // namespace ntp_snippets

components/ntp_snippets/contextual/contextual_content_suggestions_service.h

@@ -20,6 +20,10 @@
 #include "components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 
+namespace contextual_suggestions {
+class ContextualContentSuggestionsServiceProxy;
+}
+
 namespace ntp_snippets {
 
 using contextual_suggestions::Cluster;
@@ -32,16 +36,15 @@ class RemoteSuggestionsDatabase;
 // for contextual suggestion, using caching.
 class ContextualContentSuggestionsService : public KeyedService {
  public:
-
   ContextualContentSuggestionsService(
       std::unique_ptr<ContextualSuggestionsFetcher>
           contextual_suggestions_fetcher,
       std::unique_ptr<CachedImageFetcher> image_fetcher,
       std::unique_ptr<RemoteSuggestionsDatabase>
           contextual_suggestions_database,
       std::unique_ptr<
-          contextual_suggestions::ContextualSuggestionsMetricsReporter>
-          metrics_reporter);
+          contextual_suggestions::ContextualSuggestionsMetricsReporterProvider>
+          metrics_reporter_provider);
   ~ContextualContentSuggestionsService() override;
 
   using FetchContextualSuggestionsCallback =
@@ -72,13 +75,9 @@ class ContextualContentSuggestionsService : public KeyedService {
       const ContentSuggestion::ID& suggestion_id,
       ImageFetchedCallback callback);
 
-  // Used to report events using various metrics (e.g. UMA, UKM).
-  virtual void ReportEvent(
-      ukm::SourceId sourceId,
-      contextual_suggestions::ContextualSuggestionsEvent event);
-
-  // KeyedService overrides.
-  void Shutdown() override;
+  std::unique_ptr<
+      contextual_suggestions::ContextualContentSuggestionsServiceProxy>
+  CreateProxy();
 
  private:
   // Cache for images of contextual suggestions, needed by CachedImageFetcher.
@@ -89,12 +88,9 @@ class ContextualContentSuggestionsService : public KeyedService {
 
   std::unique_ptr<CachedImageFetcher> image_fetcher_;
 
-  std::unique_ptr<contextual_suggestions::ContextualSuggestionsMetricsReporter>
-      metrics_reporter_;
-
-  // The most recent SourceId in use by metrics_reporter_, or
-  // ukm::kInvalidSourceId.
-  ukm::SourceId last_ukm_source_id_;
+  std::unique_ptr<
+      contextual_suggestions::ContextualSuggestionsMetricsReporterProvider>
+      metrics_reporter_provider_;
 
   // Look up by ContentSuggestion::ID::id_within_category() aka std::string to
   // get image URL.

components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy.cc

@@ -26,8 +26,12 @@ GURL ImageUrlFromId(const std::string& image_id) {
 
 ContextualContentSuggestionsServiceProxy::
     ContextualContentSuggestionsServiceProxy(
-        ntp_snippets::ContextualContentSuggestionsService* service)
-    : service_(service), weak_ptr_factory_(this) {}
+        ntp_snippets::ContextualContentSuggestionsService* service,
+        std::unique_ptr<ContextualSuggestionsMetricsReporter> metrics_reporter)
+    : service_(service),
+      metrics_reporter_(std::move(metrics_reporter)),
+      last_ukm_source_id_(ukm::kInvalidSourceId),
+      weak_ptr_factory_(this) {}
 
 ContextualContentSuggestionsServiceProxy::
     ~ContextualContentSuggestionsServiceProxy() {}
@@ -81,7 +85,23 @@ void ContextualContentSuggestionsServiceProxy::ClearState() {
 void ContextualContentSuggestionsServiceProxy::ReportEvent(
     ukm::SourceId ukm_source_id,
     ContextualSuggestionsEvent event) {
-  service_->ReportEvent(ukm_source_id, event);
+  DCHECK(ukm_source_id != ukm::kInvalidSourceId);
+
+  // Flush the previous page (if any) and setup the new page.
+  if (ukm_source_id != last_ukm_source_id_) {
+    if (last_ukm_source_id_ != ukm::kInvalidSourceId)
+      metrics_reporter_->Flush();
+    last_ukm_source_id_ = ukm_source_id;
+    metrics_reporter_->SetupForPage(ukm_source_id);
+  }
+
+  metrics_reporter_->RecordEvent(event);
+}
+
+void ContextualContentSuggestionsServiceProxy::FlushMetrics() {
+  if (last_ukm_source_id_ != ukm::kInvalidSourceId)
+    metrics_reporter_->Flush();
+  last_ukm_source_id_ = ukm::kInvalidSourceId;
 }
 
 void ContextualContentSuggestionsServiceProxy::CacheSuggestions(

components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy.h

@@ -28,8 +28,9 @@ class ContextualContentSuggestionsServiceProxy {
   using ClustersCallback = ntp_snippets::FetchClustersCallback;
   using Cluster = ntp_snippets::Cluster;
 
-  explicit ContextualContentSuggestionsServiceProxy(
-      ntp_snippets::ContextualContentSuggestionsService* service);
+  ContextualContentSuggestionsServiceProxy(
+      ntp_snippets::ContextualContentSuggestionsService* service,
+      std::unique_ptr<ContextualSuggestionsMetricsReporter> metrics_reporter);
   ~ContextualContentSuggestionsServiceProxy();
 
   // Fetches contextual suggestions for a given |url|.
@@ -53,6 +54,9 @@ class ContextualContentSuggestionsServiceProxy {
   // Reports user interface event to the service.
   void ReportEvent(ukm::SourceId, ContextualSuggestionsEvent event);
 
+  // Ensures that all metrics are properly flushed.
+  void FlushMetrics();
+
  private:
   void CacheSuggestions(ClustersCallback callback,
                         std::string peek_text,
@@ -62,6 +66,14 @@ class ContextualContentSuggestionsServiceProxy {
   // Cache of contextual suggestions.
   std::map<std::string, ntp_snippets::ContextualSuggestion> suggestions_;
 
+  // Sink for reporting metrics for this proxy.
+  std::unique_ptr<contextual_suggestions::ContextualSuggestionsMetricsReporter>
+      metrics_reporter_;
+
+  // The most recent SourceId in use by metrics_reporter_, or
+  // ukm::kInvalidSourceId.
+  ukm::SourceId last_ukm_source_id_;
+
   // Weak pointer factory to cancel pending callbacks.
   base::WeakPtrFactory<ContextualContentSuggestionsServiceProxy>
       weak_ptr_factory_;

components/ntp_snippets/contextual/contextual_content_suggestions_service_proxy_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "components/ntp_snippets/contextual/contextual_content_suggestions_service.h"
+#include "components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h"
 #include "components/ntp_snippets/remote/cached_image_fetcher.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -91,8 +92,10 @@ class ContextualContentSuggestionsServiceProxyTest : public testing::Test {
 
 void ContextualContentSuggestionsServiceProxyTest::SetUp() {
   service_ = std::make_unique<FakeContextualContentSuggestionsService>();
+  auto metrics_reporter =
+      std::make_unique<ContextualSuggestionsMetricsReporter>();
   proxy_ = std::make_unique<ContextualContentSuggestionsServiceProxy>(
-      service_.get());
+      service_.get(), std::move(metrics_reporter));
 }
 
 TEST_F(ContextualContentSuggestionsServiceProxyTest,

components/ntp_snippets/contextual/contextual_content_suggestions_service_unittest.cc

@@ -78,13 +78,13 @@ class ContextualContentSuggestionsServiceTest : public testing::Test {
     std::unique_ptr<FakeContextualSuggestionsFetcher> fetcher =
         std::make_unique<FakeContextualSuggestionsFetcher>();
     fetcher_ = fetcher.get();
-    auto metrics_reporter = std::make_unique<
-        contextual_suggestions::ContextualSuggestionsMetricsReporter>();
+    auto metrics_reporter_provider = std::make_unique<
+        contextual_suggestions::ContextualSuggestionsMetricsReporterProvider>();
     source_ = std::make_unique<ContextualContentSuggestionsService>(
         std::move(fetcher),
         std::make_unique<FakeCachedImageFetcher>(&pref_service_),
         std::unique_ptr<RemoteSuggestionsDatabase>(),
-        std::move(metrics_reporter));
+        std::move(metrics_reporter_provider));
   }
 
   FakeContextualSuggestionsFetcher* fetcher() { return fetcher_; }

components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.cc

@@ -10,6 +10,17 @@
 
 namespace contextual_suggestions {
 
+ContextualSuggestionsMetricsReporterProvider::
+    ContextualSuggestionsMetricsReporterProvider() = default;
+
+ContextualSuggestionsMetricsReporterProvider::
+    ~ContextualSuggestionsMetricsReporterProvider() = default;
+
+std::unique_ptr<ContextualSuggestionsMetricsReporter>
+ContextualSuggestionsMetricsReporterProvider::CreateMetricsReporter() {
+  return std::make_unique<ContextualSuggestionsMetricsReporter>();
+}
+
 ContextualSuggestionsMetricsReporter::ContextualSuggestionsMetricsReporter()
     : sheet_peeked_(false),
       sheet_opened_(false),

components/ntp_snippets/contextual/contextual_suggestions_metrics_reporter.h

@@ -62,6 +62,24 @@ enum ContextualSuggestionsEvent {
   kMaxValue = SUGGESTION_CLICKED,
 };
 
+class ContextualSuggestionsMetricsReporter;
+
+// Class producing |ContextualSuggestionsMetricsReporter| instances. It enables
+// classes like |ContextualContentSuggestionService| to produce metrics
+// reporters when needed, e.g. creation of service proxy, without knowing how to
+// initialize them.
+class ContextualSuggestionsMetricsReporterProvider {
+ public:
+  ContextualSuggestionsMetricsReporterProvider();
+  virtual ~ContextualSuggestionsMetricsReporterProvider();
+
+  virtual std::unique_ptr<ContextualSuggestionsMetricsReporter>
+  CreateMetricsReporter();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ContextualSuggestionsMetricsReporterProvider);
+};
+
 // Tracks various UKM and UMA metrics based on reports of events that take place
 // within the Contextual Suggestions component.
 //