Added way to handle translations that were cancelled without an error in

the new Translate metrics.

The initial implementation of the new Translate metrics assumed that
when a translation failed or was cancelled there would be some error.
There are a few cases where this is false, including when a translation
is interrupted for a new translation with a different source or target
language. This CL adds a second input parameter to
TranslateMetricsLogger.LogTranslationFinished so we can specify if a
translation finished successfully and if there was any translation error
separately.

Bug: 1155291
Change-Id: I762fe069ee9b69969a7ade84f5d5933cf43d8552
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583107
Commit-Queue: Max Curran <curranmax@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841171}

chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer_unittest.cc

@@ -59,8 +59,10 @@ class MockTranslateMetricsLoggerContainer
   }
 
   void LogTranslationFinished(
+      bool was_successful,
       translate::TranslateErrors::Type error_type) override {
-    mock_translate_metrics_logger_->LogTranslationFinished(error_type);
+    mock_translate_metrics_logger_->LogTranslationFinished(was_successful,
+                                                           error_type);
   }
 
   void LogReversion() override {

components/translate/content/browser/content_translate_driver.cc

@@ -328,9 +328,8 @@ void ContentTranslateDriver::OnPageTranslated(
     TranslateErrors::Type error_type) {
   if (cancelled) {
     // Informs the |TranslateMetricsLogger| that the translation was cancelled.
-    DCHECK(error_type != TranslateErrors::NONE);
     translate_manager_->GetActiveTranslateMetricsLogger()
-        ->LogTranslationFinished(error_type);
+        ->LogTranslationFinished(false, error_type);
     return;
   }
 

components/translate/core/browser/mock_translate_metrics_logger.h

@@ -35,7 +35,7 @@ class MockTranslateMetricsLogger : public TranslateMetricsLogger {
   MOCK_METHOD0(LogAutofillAssistantDeferredTriggerDecision, void());
   MOCK_METHOD0(LogInitialState, void());
   MOCK_METHOD0(LogTranslationStarted, void());
-  MOCK_METHOD1(LogTranslationFinished, void(TranslateErrors::Type));
+  MOCK_METHOD2(LogTranslationFinished, void(bool, TranslateErrors::Type));
   MOCK_METHOD0(LogReversion, void());
   MOCK_METHOD1(LogUIChange, void(bool));
   MOCK_METHOD1(LogOmniboxIconChange, void(bool));

components/translate/core/browser/translate_manager.cc

@@ -529,7 +529,8 @@ void TranslateManager::PageTranslated(const std::string& source_lang,
                                      false);
   NotifyTranslateError(error_type);
 
-  GetActiveTranslateMetricsLogger()->LogTranslationFinished(error_type);
+  GetActiveTranslateMetricsLogger()->LogTranslationFinished(
+      error_type == TranslateErrors::NONE, error_type);
 }
 
 void TranslateManager::OnTranslateScriptFetchComplete(
@@ -552,7 +553,7 @@ void TranslateManager::OnTranslateScriptFetchComplete(
         TranslateErrors::NETWORK, false);
     NotifyTranslateError(TranslateErrors::NETWORK);
     GetActiveTranslateMetricsLogger()->LogTranslationFinished(
-        TranslateErrors::NETWORK);
+        false, TranslateErrors::NETWORK);
   }
 }
 

components/translate/core/browser/translate_metrics_logger.h

@@ -101,7 +101,8 @@ class TranslateMetricsLogger {
   // Tracks the state of Translate over the course of the page load.
   virtual void LogInitialState() = 0;
   virtual void LogTranslationStarted() = 0;
-  virtual void LogTranslationFinished(TranslateErrors::Type error_type) = 0;
+  virtual void LogTranslationFinished(bool was_successful,
+                                      TranslateErrors::Type error_type) = 0;
   virtual void LogReversion() = 0;
   virtual void LogUIChange(bool is_ui_shown) = 0;
   virtual void LogOmniboxIconChange(bool is_omnibox_icon_show) = 0;

components/translate/core/browser/translate_metrics_logger_impl.cc

@@ -169,9 +169,12 @@ void TranslateMetricsLoggerImpl::LogTranslationStarted() {
 }
 
 void TranslateMetricsLoggerImpl::LogTranslationFinished(
+    bool was_successful,
     TranslateErrors::Type error_type) {
-  // The translation succeeded if and only if there were no translation errors.
-  if (error_type == TranslateErrors::NONE) {
+  // Note that a translation can fail (i.e. was_successful is false) and have an
+  // error type of NONE in some cases. One case where this happens is when a
+  // translation is interrupted midway through.
+  if (was_successful) {
     UpdateTimeTranslated(previous_state_is_translated_, is_foreground_);
     num_translations_++;
 
@@ -188,9 +191,11 @@ void TranslateMetricsLoggerImpl::LogTranslationFinished(
     // Update the initial state if it was dependent on this translation..
     if (is_initial_state_dependent_on_in_progress_translation_)
       initial_state_is_translated_ = previous_state_is_translated_;
+  }
 
-    // Check if this was the first error, and then increment the number of
-    // errors for this page load.
+  // If there was some error, checks if this was the first error, and increments
+  // the error count.
+  if (error_type != TranslateErrors::NONE) {
     if (first_translate_error_type_ == TranslateErrors::NONE)
       first_translate_error_type_ = error_type;
     num_translate_errors_++;

components/translate/core/browser/translate_metrics_logger_impl.h

@@ -47,7 +47,8 @@ class NullTranslateMetricsLogger : public TranslateMetricsLogger {
   void LogAutofillAssistantDeferredTriggerDecision() override {}
   void LogInitialState() override {}
   void LogTranslationStarted() override {}
-  void LogTranslationFinished(TranslateErrors::Type error_type) override {}
+  void LogTranslationFinished(bool was_successful,
+                              TranslateErrors::Type error_type) override {}
   void LogReversion() override {}
   void LogUIChange(bool is_ui_shown) override {}
   void LogOmniboxIconChange(bool is_omnibox_icon_shown) override {}
@@ -90,7 +91,8 @@ class TranslateMetricsLoggerImpl : public TranslateMetricsLogger {
   void LogAutofillAssistantDeferredTriggerDecision() override;
   void LogInitialState() override;
   void LogTranslationStarted() override;
-  void LogTranslationFinished(TranslateErrors::Type error_type) override;
+  void LogTranslationFinished(bool was_successful,
+                              TranslateErrors::Type error_type) override;
   void LogReversion() override;
   void LogUIChange(bool is_ui_shown) override;
   void LogOmniboxIconChange(bool is_omnibox_icon_shown) override;

components/translate/core/browser/translate_metrics_logger_impl_unittest.cc

@@ -97,7 +97,8 @@ TEST_F(TranslateMetricsLoggerImplTest, MultipleRecordMetrics) {
   translate_metrics_logger()->LogInitialState();
   translate_metrics_logger()->LogUIChange(true);
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
   translate_metrics_logger()->LogReversion();
 
   // Simulate |RecordMetrics| being called multiple times.
@@ -173,7 +174,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
   translate_metrics_logger()->LogInitialState();
 
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -186,7 +188,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
   translate_metrics_logger()->LogInitialState();
 
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NETWORK);
+  translate_metrics_logger()->LogTranslationFinished(false,
+                                                     TranslateErrors::NETWORK);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -212,9 +215,11 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
   translate_metrics_logger()->LogInitialState();
 
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NETWORK);
+  translate_metrics_logger()->LogTranslationFinished(false,
+                                                     TranslateErrors::NETWORK);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -229,7 +234,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
   ResetTest();
   translate_metrics_logger()->LogTranslationStarted();
   translate_metrics_logger()->LogInitialState();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -241,7 +247,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
   ResetTest();
   translate_metrics_logger()->LogTranslationStarted();
   translate_metrics_logger()->LogInitialState();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NETWORK);
+  translate_metrics_logger()->LogTranslationFinished(false,
+                                                     TranslateErrors::NETWORK);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -269,7 +276,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslationAndReversion) {
 
   for (int i = 0; i < num_translations_and_reversions; i++) {
     translate_metrics_logger()->LogTranslationStarted();
-    translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+    translate_metrics_logger()->LogTranslationFinished(true,
+                                                       TranslateErrors::NONE);
     translate_metrics_logger()->LogReversion();
   }
 
@@ -298,7 +306,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslateErrors) {
   // Simulates the translations with the predefined errors.
   for (auto translate_error_type : kTranslateErrorTypes) {
     translate_metrics_logger()->LogTranslationStarted();
-    translate_metrics_logger()->LogTranslationFinished(translate_error_type);
+    translate_metrics_logger()->LogTranslationFinished(
+        translate_error_type == TranslateErrors::NONE, translate_error_type);
   }
 
   translate_metrics_logger()->RecordMetrics(true);
@@ -322,7 +331,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslateState) {
   translate_metrics_logger()->LogInitialState();
 
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
   translate_metrics_logger()->LogUIChange(true);
   translate_metrics_logger()->LogOmniboxIconChange(true);
 
@@ -339,7 +349,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogTranslateState) {
   translate_metrics_logger()->LogUIChange(true);
   translate_metrics_logger()->LogOmniboxIconChange(true);
   translate_metrics_logger()->LogInitialState();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   translate_metrics_logger()->LogReversion();
   translate_metrics_logger()->LogUIChange(false);
@@ -374,7 +385,8 @@ TEST_F(TranslateMetricsLoggerImplTest, TrackTimeTranslatedAndNotTranslated) {
 
   // Translate the page (while still in the background).
   translate_metrics_logger()->LogTranslationStarted();
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   test_clock.Advance(delay3);
 
@@ -413,7 +425,8 @@ TEST_F(TranslateMetricsLoggerImplTest,
   test_clock.Advance(delay2);
 
   // Translation finally finishes.
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   test_clock.Advance(delay3);
 
@@ -492,7 +505,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogMaxTimeToTranslate) {
 
   translate_metrics_logger()->LogTranslationStarted();
   test_clock.Advance(default_delay);
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NONE);
+  translate_metrics_logger()->LogTranslationFinished(true,
+                                                     TranslateErrors::NONE);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -505,7 +519,8 @@ TEST_F(TranslateMetricsLoggerImplTest, LogMaxTimeToTranslate) {
 
   translate_metrics_logger()->LogTranslationStarted();
   test_clock.Advance(default_delay);
-  translate_metrics_logger()->LogTranslationFinished(TranslateErrors::NETWORK);
+  translate_metrics_logger()->LogTranslationFinished(false,
+                                                     TranslateErrors::NETWORK);
 
   translate_metrics_logger()->RecordMetrics(true);
 
@@ -540,6 +555,7 @@ TEST_F(TranslateMetricsLoggerImplTest, LogMaxTimeToTranslate) {
     translate_metrics_logger()->LogTranslationStarted();
     test_clock.Advance(test.time_to_translate);
     translate_metrics_logger()->LogTranslationFinished(
+        test.translate_error_type == TranslateErrors::NONE,
         test.translate_error_type);
   }