[Translate] Integrate TranslateEventProto UMA logging into TranslateManager.

chrome/browser/translate/chrome_translate_client.cc

Index: chrome/browser/translate/chrome_translate_client.cc
diff --git a/chrome/browser/translate/chrome_translate_client.cc b/chrome/browser/translate/chrome_translate_client.cc
index 06ce2bbbcd236307ca099d12faf0e7064c05534a..85751ce025f517f9e85bab67fc0b4564e7e09a9f 100644
--- a/chrome/browser/translate/chrome_translate_client.cc
+++ b/chrome/browser/translate/chrome_translate_client.cc
@@ -26,6 +26,7 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/theme_resources.h"
+#include "components/metrics/proto/translate_event.pb.h"
 #include "components/prefs/pref_service.h"
 #include "components/translate/core/browser/language_model.h"
 #include "components/translate/core/browser/language_state.h"
@@ -198,12 +199,21 @@ void ChromeTranslateClient::ShowTranslateUI(
     // another page.
     if (!base::FeatureList::IsEnabled(translate::kTranslateUI2016Q2) &&
         !GetLanguageState().HasLanguageChanged()) {
+      if (translate_manager_) {

[groby-ooo-7-16, 2016/10/25 18:53:10]

Should never be NULL - just DCHECK if you want to verify, but you can also
safely ignore. (See below)

[Roger McFarlane (Chromium), 2016/10/25 20:07:56]

translate_manager_ is reset to NULL on WebContentsDestroyed().

https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translat...

I observed a couple of other places that were checking for NULL before
dereferencing translate_manager_, presumably for the same reason, and followed
suit.

Happy to change these to a DCHECK at the top of the method is you're sure this
will never be called after, or in contention with, WebContentsDestroyed.

[groby-ooo-7-16, 2016/10/25 23:17:21]

We really shouldn't have to - ChromeTranslateClient is WebContentsUserData,
which means its lifetime is scoped to the lifetime of the WebContents.

WebContentsDestroyed is only there to clean up resources allocated.
If it would be called after WebContentsDestroyed, we'd have a lifetime issue -
somebody is holding on to this instead of looking it up from the WebContents.

If it's called in contention with WCD, I'd be very sad, because this should all
happen on the same thread.

So yes, please DCHECK only. 

[Roger McFarlane (Chromium), 2016/11/07 20:51:58]

Done.

+        translate_manager_->RecordTranslateEvent(
+            metrics::TranslateEventProto::MATCHES_PREVIOUS_LANGUAGE);
+      }
       return;
     }
 
     if (!triggered_from_menu &&
-        GetTranslatePrefs()->IsTooOftenDenied(source_language))
+        GetTranslatePrefs()->IsTooOftenDenied(source_language)) {
+      if (translate_manager_) {
+        translate_manager_->RecordTranslateEvent(
+            metrics::TranslateEventProto::LANGUAGE_DISABLED_BY_AUTO_BLACKLIST);
+      }
       return;
+    }
   }
   ShowBubble(step, error_type);
 }
@@ -319,16 +329,28 @@ void ChromeTranslateClient::ShowBubble(
     return;
   }
 
-  if (web_contents() != browser->tab_strip_model()->GetActiveWebContents())
+  if (web_contents() != browser->tab_strip_model()->GetActiveWebContents()) {
+    if (translate_manager_ &&

[groby-ooo-7-16, 2016/10/25 18:53:10]

Is there a specific reason you're checking translate_manager_? It should be a
class invariant that this is not NULL. So if this happens when you test, we
might have a bug.

If it's just to ensure the invariant, I'd suggest just a DCHECK at the top of
the function.

[Roger McFarlane (Chromium), 2016/10/25 20:07:56]

Acknowledged. See previous commentary re: WebContentsDestroyed().

+        step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) {

[groby-ooo-7-16, 2016/10/25 18:53:10]

Why is this just recorded for BEFORE_TRANSLATE?

[Roger McFarlane (Chromium), 2016/10/25 20:07:56]

We're capturing the outcome of showing the translate prompt to the user. All of
the other steps occur after the user has interacted with the prompt in an
affirmative manner (which will have already been captured).

I.e., BEFORE_TRANSLATE is the state in which the user interacts. The others are
informative states to the user (translate in progress, translate complete).

[groby-ooo-7-16, 2016/10/25 23:17:21]

If you don't mind, could you add a comment here? Because I know I'll forget, and
ask the same question again three months from here :)

[Roger McFarlane (Chromium), 2016/11/07 20:51:58]

Done.

+      translate_manager_->RecordTranslateEvent(
+          metrics::TranslateEventProto::WEBCONTENTS_NOT_ACTIVE);
+    }
     return;
+  }
 
-  // This ShowBubble function is also used for upating the existing bubble.
+  // This ShowBubble function is also used for updating the existing bubble.
   // However, with the bubble shown, any browser windows are NOT activated
   // because the bubble takes the focus from the other widgets including the
   // browser windows. So it is checked that |browser| is the last activated
   // browser, not is now activated.
-  if (browser != chrome::FindLastActive())
+  if (browser != chrome::FindLastActive()) {
+    if (translate_manager_ &&
+        step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) {
+      translate_manager_->RecordTranslateEvent(
+          metrics::TranslateEventProto::BROSWER_WINDOW_NOT_ACTIVE);

[groby-ooo-7-16, 2016/10/25 18:53:10]

See above :)

[Roger McFarlane (Chromium), 2016/10/25 20:07:56]

Acknowledged. See previous commentary re: WebContentsDestroyed().

+    }
     return;

[groby-ooo-7-16, 2016/10/25 18:53:10]

On a wider scale, I'm wondering it this should return a reason for every
early-out point, and we then centralize the recording at the call site (or in a
wrapper)

[Roger McFarlane (Chromium), 2016/10/25 20:07:56]

In the most recent patchset, I've done something similar with the downstream
"Show" functionality. I could continue to percolate a result up the chain.

[groby-ooo-7-16, 2016/10/25 23:17:21]

Is it worth doing in this patchset, or should we postpone this to stage two? 

[Roger McFarlane (Chromium), 2016/11/07 20:51:58]

Done.

+  }
 
   // During auto-translating, the bubble should not be shown.
   if (step == translate::TRANSLATE_STEP_TRANSLATING ||