[Translate] Integrate TranslateEventProto UMA logging into TranslateManager.

chrome/browser/translate/chrome_translate_client.cc

 // Copyright 2014 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 "chrome/browser/translate/chrome_translate_client.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/strings/string_split.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/translate/language_model_factory.h"
 #include "chrome/browser/translate/translate_accept_languages_factory.h"
 #include "chrome/browser/translate/translate_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/translate/translate_bubble_factory.h"
 #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"
 #include "components/translate/core/browser/page_translated_details.h"
 #include "components/translate/core/browser/translate_accept_languages.h"
 #include "components/translate/core/browser/translate_download_manager.h"
 #include "components/translate/core/browser/translate_infobar_delegate.h"
 #include "components/translate/core/browser/translate_manager.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "components/translate/core/common/language_detection_details.h"
@@ skipping 152 matching lines @@
 #endif
 
   // Bubble UI.
   if (step == translate::TRANSLATE_STEP_BEFORE_TRANSLATE) {
     // TODO(droger): Move this logic out of UI code.
     GetLanguageState().SetTranslateEnabled(true);
     // In the new UI, continue offering translation after the user navigates to
     // 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);
 }
 
 translate::TranslateDriver* ChromeTranslateClient::GetTranslateDriver() {
   return &translate_driver_;
 }
 
 PrefService* ChromeTranslateClient::GetPrefs() {
   DCHECK(web_contents());
@@ skipping 95 matching lines @@
 #if !defined(OS_ANDROID)
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
 
   // |browser| might be NULL when testing. In this case, Show(...) should be
   // called because the implementation for testing is used.
   if (!browser) {
     TranslateBubbleFactory::Show(NULL, web_contents(), step, error_type);
     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 ||
       step == translate::TRANSLATE_STEP_AFTER_TRANSLATE) {
     if (GetLanguageState().InTranslateNavigation())
       return;
   }
 
   TranslateBubbleFactory::Show(browser->window(), web_contents(), step,
                                error_type);
 #else
   NOTREACHED();
 #endif
 }