Connect TranslateMetricsLogger to TranslateManager.

This connection will eventually allow Chrome Translate to log metrics with the relevant TranslateMetricsLogger. The TranslateManager will keep a WeakPtr to the newest relevant TranslateMetricsLogger. Any part of Chrome Translate can request this “active” TranslateMetricsLogger in order to log metrics and/or events. In the event this WeakPtr is null, the TranslateManager will return a NullTranslateMetricsLogger (a null implementation of TranslateMetricsLogger), so that the callee doesn’t have to check if the returned value is null or not.

Bug: 1114868

Change-Id: Ifa775ccd5918d4111e8b4ae44ea7f1c1ea034043
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419260Reviewed-by: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Max Curran <curranmax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815751}

chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.cc

@@ -4,15 +4,25 @@
 
 #include "chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h"
 
+#include "chrome/browser/translate/chrome_translate_client.h"
+#include "components/translate/core/browser/translate_manager.h"
 #include "components/translate/core/browser/translate_metrics_logger_impl.h"
 
 std::unique_ptr<TranslatePageLoadMetricsObserver>
-TranslatePageLoadMetricsObserver::CreateIfNeeded() {
-  // TODO(curranamx): Connect the new TranslateMetricsLogger to a
-  // TranslateManager. https://crbug.com/1114868.
+TranslatePageLoadMetricsObserver::CreateIfNeeded(
+    content::WebContents* web_contents) {
+  // Gets the |TranslateManager| associated with the same |WebContents| as this
+  // |TranslatePageLoadMetricsObserver|.
+  translate::TranslateManager* translate_manager =
+      ChromeTranslateClient::GetManagerFromWebContents(web_contents);
+  if (translate_manager == nullptr)
+    return nullptr;
+
+  // Creates a new |TranslateMetricsLoggerImpl| and associates with the above
+  // |TranslateManager|.
   std::unique_ptr<translate::TranslateMetricsLogger> translate_metrics_logger =
-      std::make_unique<translate::TranslateMetricsLoggerImpl>();
-
+      std::make_unique<translate::TranslateMetricsLoggerImpl>(
+          translate_manager->GetWeakPtr());
   return std::make_unique<TranslatePageLoadMetricsObserver>(
       std::move(translate_metrics_logger));
 }
@@ -28,8 +38,6 @@ TranslatePageLoadMetricsObserver::OnStart(
     content::NavigationHandle* navigation_handle,
     const GURL& currently_committed_url,
     bool started_in_foreground) {
-  DCHECK(translate_metrics_logger_ != nullptr);
-
   translate_metrics_logger_->OnPageLoadStart(started_in_foreground);
   return CONTINUE_OBSERVING;
 }
@@ -37,16 +45,12 @@ TranslatePageLoadMetricsObserver::OnStart(
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 TranslatePageLoadMetricsObserver::OnHidden(
     const page_load_metrics::mojom::PageLoadTiming& timing) {
-  DCHECK(translate_metrics_logger_ != nullptr);
-
   translate_metrics_logger_->OnForegroundChange(false);
   return CONTINUE_OBSERVING;
 }
 
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 TranslatePageLoadMetricsObserver::OnShown() {
-  DCHECK(translate_metrics_logger_ != nullptr);
-
   translate_metrics_logger_->OnForegroundChange(true);
   return CONTINUE_OBSERVING;
 }
@@ -54,15 +58,11 @@ TranslatePageLoadMetricsObserver::OnShown() {
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 TranslatePageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
     const page_load_metrics::mojom::PageLoadTiming& timing) {
-  DCHECK(translate_metrics_logger_ != nullptr);
-
   translate_metrics_logger_->RecordMetrics(false);
   return CONTINUE_OBSERVING;
 }
 
 void TranslatePageLoadMetricsObserver::OnComplete(
     const page_load_metrics::mojom::PageLoadTiming& timing) {
-  DCHECK(translate_metrics_logger_ != nullptr);
-
   translate_metrics_logger_->RecordMetrics(true);
 }

chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h

@@ -24,7 +24,8 @@ class NavigationHandle;
 class TranslatePageLoadMetricsObserver
     : public page_load_metrics::PageLoadMetricsObserver {
  public:
-  static std::unique_ptr<TranslatePageLoadMetricsObserver> CreateIfNeeded();
+  static std::unique_ptr<TranslatePageLoadMetricsObserver> CreateIfNeeded(
+      content::WebContents* web_contents);
 
   explicit TranslatePageLoadMetricsObserver(
       std::unique_ptr<translate::TranslateMetricsLogger>

chrome/browser/page_load_metrics/page_load_metrics_initialize.cc

@@ -167,8 +167,9 @@ void PageLoadMetricsEmbedder::RegisterEmbedderObservers(
           web_contents()->GetBrowserContext()));
   tracker->AddObserver(std::make_unique<DataUseMetricsObserver>());
   std::unique_ptr<TranslatePageLoadMetricsObserver> translate_observer =
-      TranslatePageLoadMetricsObserver::CreateIfNeeded();
-  if (translate_observer != nullptr)
+      TranslatePageLoadMetricsObserver::CreateIfNeeded(
+          tracker->GetWebContents());
+  if (translate_observer)
     tracker->AddObserver(std::move(translate_observer));
 }
 

components/translate/core/browser/translate_manager.cc

@@ -36,6 +36,8 @@
 #include "components/translate/core/browser/translate_error_details.h"
 #include "components/translate/core/browser/translate_init_details.h"
 #include "components/translate/core/browser/translate_language_list.h"
+#include "components/translate/core/browser/translate_metrics_logger.h"
+#include "components/translate/core/browser/translate_metrics_logger_impl.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "components/translate/core/browser/translate_ranker.h"
 #include "components/translate/core/browser/translate_script.h"
@@ -137,6 +139,8 @@ TranslateManager::TranslateManager(TranslateClient* translate_client,
       translate_driver_(translate_client_->GetTranslateDriver()),
       translate_ranker_(translate_ranker),
       language_model_(language_model),
+      null_translate_metrics_logger_(
+          std::make_unique<NullTranslateMetricsLogger>()),
       language_state_(translate_driver_),
       translate_event_(std::make_unique<metrics::TranslateEventProto>()) {}
 
@@ -1146,4 +1150,18 @@ void TranslateManager::SetPredefinedTargetLanguage(
   language_state_.SetPredefinedTargetLanguage(language_code);
 }
 
+TranslateMetricsLogger* TranslateManager::GetActiveTranslateMetricsLogger() {
+  // If |active_translate_metrics_logger_| is not null, return that. Otherwise
+  // return |null_translate_metrics_logger_|. This way the callee doesn't have
+  // to check if the returned value is null.
+  return active_translate_metrics_logger_
+             ? active_translate_metrics_logger_.get()
+             : null_translate_metrics_logger_.get();
+}
+
+void TranslateManager::RegisterTranslateMetricsLogger(
+    base::WeakPtr<TranslateMetricsLogger> translate_metrics_logger) {
+  active_translate_metrics_logger_ = translate_metrics_logger;
+}
+
 }  // namespace translate

components/translate/core/browser/translate_manager.h

@@ -32,10 +32,13 @@ namespace translate {
 
 class TranslateClient;
 class TranslateDriver;
+class TranslateMetricsLogger;
 class TranslatePrefs;
 class TranslateRanker;
 struct TranslateTriggerDecision;
 
+class NullTranslateMetricsLogger;
+
 namespace testing {
 class TranslateManagerTest;
 }  // namespace testing
@@ -197,6 +200,17 @@ class TranslateManager {
   // Sets target language.
   void SetPredefinedTargetLanguage(const std::string& language_code);
 
+  // Returns a reference to |active_translate_metrics_logger_|. In the event
+  // that this value is null, a |NullTranslateMetricsLogger| (a null
+  // implementation) will be returned. This guarantees that the returned value
+  // is always non-null.
+  TranslateMetricsLogger* GetActiveTranslateMetricsLogger();
+
+  // Sets |active_translate_metrics_logger_| to the given
+  // |translate_metrics_logger|.
+  void RegisterTranslateMetricsLogger(
+      base::WeakPtr<TranslateMetricsLogger> translate_metrics_logger);
+
  private:
   friend class translate::testing::TranslateManagerTest;
 
@@ -306,6 +320,9 @@ class TranslateManager {
   TranslateRanker* translate_ranker_;        // Weak.
   language::LanguageModel* language_model_;  // Weak.
 
+  base::WeakPtr<TranslateMetricsLogger> active_translate_metrics_logger_;
+  std::unique_ptr<NullTranslateMetricsLogger> null_translate_metrics_logger_;
+
   LanguageState language_state_;
 
   std::unique_ptr<metrics::TranslateEventProto> translate_event_;

components/translate/core/browser/translate_manager_unittest.cc

@@ -26,6 +26,7 @@
 #include "components/translate/core/browser/translate_browser_metrics.h"
 #include "components/translate/core/browser/translate_client.h"
 #include "components/translate/core/browser/translate_download_manager.h"
+#include "components/translate/core/browser/translate_metrics_logger_impl.h"
 #include "components/translate/core/browser/translate_pref_names.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "components/translate/core/common/translate_constants.h"
@@ -180,6 +181,10 @@ class TranslateManagerTest : public ::testing::Test {
                                            translate_prefs_);
   }
 
+  NullTranslateMetricsLogger* null_translate_metrics_logger() {
+    return translate_manager_->null_translate_metrics_logger_.get();
+  }
+
   // Required to instantiate a net::test::MockNetworkChangeNotifier, because it
   // uses ObserverListThreadSafe.
   base::test::TaskEnvironment task_environment_;
@@ -1081,6 +1086,42 @@ TEST_F(TranslateManagerTest, InitiateManualTranslation) {
   histogram_tester.ExpectTotalCount(kInitiationStatusName, 0);
 }
 
+TEST_F(TranslateManagerTest, GetActiveTranslateMetricsLogger) {
+  PrepareTranslateManager();
+  std::unique_ptr<TranslateMetricsLogger> translate_metrics_logger_a =
+      std::make_unique<TranslateMetricsLoggerImpl>(
+          translate_manager_->GetWeakPtr());
+  std::unique_ptr<TranslateMetricsLogger> translate_metrics_logger_b =
+      std::make_unique<TranslateMetricsLoggerImpl>(
+          translate_manager_->GetWeakPtr());
+
+  // Before either |TranslateMetricsLogger| begins, we expect
+  // |GetActiveTranslateMetricsLogger| to return the null implementation.
+  EXPECT_EQ(translate_manager_->GetActiveTranslateMetricsLogger(),
+            null_translate_metrics_logger());
+
+  // Now that the page load has begun for |translate_metrics_logger_a|, we
+  // expect |GetActiveTranslateMetricsLogger| to return
+  // |translate_metrics_logger_a|.
+  translate_metrics_logger_a->OnPageLoadStart(true);
+  EXPECT_EQ(translate_manager_->GetActiveTranslateMetricsLogger(),
+            translate_metrics_logger_a.get());
+
+  // Once the page load starts for |translate_metrics_logger_b|, we
+  // expect |GetActiveTranslateMetricsLogger| to return
+  // |translate_metrics_logger_b|, even if |translate_metrics_logger_a| hasn't
+  // been destroyed yet.
+  translate_metrics_logger_b->OnPageLoadStart(true);
+  EXPECT_EQ(translate_manager_->GetActiveTranslateMetricsLogger(),
+            translate_metrics_logger_b.get());
+
+  // Once |translate_metrics_logger_b| is destroyed, we expect that
+  // |GetActiveTranslateMetricsLogger| to return the null implementation.
+  translate_metrics_logger_b.reset();
+  EXPECT_EQ(translate_manager_->GetActiveTranslateMetricsLogger(),
+            null_translate_metrics_logger());
+}
+
 }  // namespace testing
 
 }  // namespace translate

components/translate/core/browser/translate_metrics_logger.h

@@ -7,6 +7,8 @@
 
 namespace translate {
 
+class TranslateManager;
+
 // TranslateMetricsLogger tracks and logs various UKM and UMA metrics for Chrome
 // Translate over the course of a page load.
 class TranslateMetricsLogger {
@@ -24,6 +26,11 @@ class TranslateMetricsLogger {
   // Logs all stored page load metrics. If is_final is |true| then RecordMetrics
   // won't be called again.
   virtual void RecordMetrics(bool is_final) = 0;
+
+  // TODO(curranmax): Split this into two interfaces. One for the interaction
+  // with the |TranslatePageLoadMetricsObserver|, and the other for the
+  // interaction with |TranslateManager| and the rest of the Translate code.
+  // https://crbug.com/1114868.
 };
 
 }  // namespace translate

components/translate/core/browser/translate_metrics_logger_impl.cc

@@ -4,9 +4,21 @@
 
 #include "components/translate/core/browser/translate_metrics_logger_impl.h"
 
+#include "components/translate/core/browser/translate_manager.h"
+
 namespace translate {
 
+TranslateMetricsLoggerImpl::TranslateMetricsLoggerImpl(
+    base::WeakPtr<TranslateManager> translate_manager)
+    : translate_manager_(translate_manager) {}
+
+TranslateMetricsLoggerImpl::~TranslateMetricsLoggerImpl() = default;
+
 void TranslateMetricsLoggerImpl::OnPageLoadStart(bool is_foreground) {
+  if (translate_manager_)
+    translate_manager_->RegisterTranslateMetricsLogger(
+        weak_method_factory_.GetWeakPtr());
+
   is_foreground_ = is_foreground;
 }
 

components/translate/core/browser/translate_metrics_logger_impl.h

@@ -7,16 +7,30 @@
 
 #include <memory>
 
+#include "base/memory/weak_ptr.h"
 #include "components/translate/core/browser/translate_metrics_logger.h"
 
 namespace translate {
 
+class NullTranslateMetricsLogger : public TranslateMetricsLogger {
+ public:
+  NullTranslateMetricsLogger() = default;
+
+  // TranslateMetricsLogger
+  void OnPageLoadStart(bool is_foreground) override {}
+  void OnForegroundChange(bool is_foreground) override {}
+  void RecordMetrics(bool is_final) override {}
+};
+
+class TranslateManager;
+
 // TranslateMetricsLogger tracks and logs various UKM and UMA metrics for Chrome
 // Translate over the course of a page load.
 class TranslateMetricsLoggerImpl : public TranslateMetricsLogger {
  public:
-  TranslateMetricsLoggerImpl() = default;
-  ~TranslateMetricsLoggerImpl() override = default;
+  explicit TranslateMetricsLoggerImpl(
+      base::WeakPtr<TranslateManager> translate_manager);
+  ~TranslateMetricsLoggerImpl() override;
 
   TranslateMetricsLoggerImpl(const TranslateMetricsLoggerImpl&) = delete;
   TranslateMetricsLoggerImpl& operator=(const TranslateMetricsLoggerImpl&) =
@@ -27,11 +41,11 @@ class TranslateMetricsLoggerImpl : public TranslateMetricsLogger {
   void OnForegroundChange(bool is_foreground) override;
   void RecordMetrics(bool is_final) override;
 
-  // TODO(curranmax): Connect to TranslateManager so metrics can be collected
-  // from the rest of the Translate code. https://crbug.com/1114868.
   // TODO(curranmax): Add appropriate functions for the Translate code to log
   // relevant events. https://crbug.com/1114868.
  private:
+  base::WeakPtr<TranslateManager> translate_manager_;
+
   // Since |RecordMetrics()| can be called multiple times, such as when Chrome
   // is backgrounded and reopened, we use |sequence_no_| to differentiate the
   // recorded UKM protos.
@@ -40,6 +54,8 @@ class TranslateMetricsLoggerImpl : public TranslateMetricsLogger {
   // Tracks if the associated page is in the foreground (|true|) or the
   // background (|false|)
   bool is_foreground_{false};
+
+  base::WeakPtrFactory<TranslateMetricsLoggerImpl> weak_method_factory_{this};
 };
 
 }  // namespace translate