[signin][dice] Log Signin_SigninPage_Shown user action.

Bug: 792394
Change-Id: Ie5d044a4c7847c34223ee8c2f9471d78316f9511
Reviewed-on: https://chromium-review.googlesource.com/814435
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523074}

chrome/browser/signin/dice_tab_helper.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/signin/dice_tab_helper.h"
 
 #include "base/logging.h"
+#include "base/metrics/user_metrics.h"
 #include "chrome/browser/signin/dice_tab_helper.h"
 #include "chrome/browser/signin/signin_util.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -15,13 +16,9 @@
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(DiceTabHelper);
 
 DiceTabHelper::DiceTabHelper(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents),
-      signin_access_point_(signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN),
-      signin_reason_(signin_metrics::Reason::REASON_UNKNOWN_REASON),
-      should_start_sync_after_web_signin_(true) {
-}
+    : content::WebContentsObserver(web_contents) {}
 
-DiceTabHelper::~DiceTabHelper() {}
+DiceTabHelper::~DiceTabHelper() = default;
 
 void DiceTabHelper::InitializeSigninFlow(
     signin_metrics::AccessPoint access_point,
@@ -29,6 +26,13 @@ void DiceTabHelper::InitializeSigninFlow(
   signin_access_point_ = access_point;
   signin_reason_ = reason;
   should_start_sync_after_web_signin_ = true;
+  did_finish_loading_signin_page_ = false;
+
+  if (signin_reason_ == signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT) {
+    signin_metrics::LogSigninAccessPointStarted(access_point);
+    signin_metrics::RecordSigninUserActionForAccessPoint(access_point);
+    base::RecordAction(base::UserMetricsAction("Signin_SigninPage_Loading"));
+  }
 }
 
 void DiceTabHelper::DidStartNavigation(
@@ -56,11 +60,16 @@ void DiceTabHelper::DidStartNavigation(
     should_start_sync_after_web_signin_ = false;
     return;
   }
+}
 
-  // TODO(msarda): Figure out if this condition can be restricted even more
-  // (e.g. avoid starting sync after a browser initiated navigation).
-  // if (!navigation_handle->IsRendererInitiated()) {
-  //  // Avoid starting sync if the navigations comes from the browser.
-  //  should_start_sync_after_web_signin_ = false;
-  //}
+void DiceTabHelper::DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                                  const GURL& validated_url) {
+  if (!should_start_sync_after_web_signin_ || did_finish_loading_signin_page_)
+    return;
+
+  if (validated_url.GetOrigin() == GaiaUrls::GetInstance()->gaia_url()) {
+    VLOG(1) << "Finished loading sign-in page: " << validated_url.spec();
+    did_finish_loading_signin_page_ = true;
+    base::RecordAction(base::UserMetricsAction("Signin_SigninPage_Shown"));
+  }
 }

chrome/browser/signin/dice_tab_helper.h

@@ -10,6 +10,9 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace content {
+class RenderFrameHost;
+}
 // Tab helper used for DICE to mark that sync should start after a web sign-in
 // with a Google account.
 class DiceTabHelper : public content::WebContentsUserData<DiceTabHelper>,
@@ -36,14 +39,19 @@ class DiceTabHelper : public content::WebContentsUserData<DiceTabHelper>,
   // content::WebContentsObserver:
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override;
 
  private:
   friend class content::WebContentsUserData<DiceTabHelper>;
   explicit DiceTabHelper(content::WebContents* web_contents);
 
-  signin_metrics::AccessPoint signin_access_point_;
-  signin_metrics::Reason signin_reason_;
-  bool should_start_sync_after_web_signin_;
+  signin_metrics::AccessPoint signin_access_point_ =
+      signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN;
+  signin_metrics::Reason signin_reason_ =
+      signin_metrics::Reason::REASON_UNKNOWN_REASON;
+  bool should_start_sync_after_web_signin_ = true;
+  bool did_finish_loading_signin_page_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(DiceTabHelper);
 };

chrome/browser/signin/dice_tab_helper_unittest.cc

@@ -4,10 +4,13 @@
 
 #include "chrome/browser/signin/dice_tab_helper.h"
 
+#include "base/test/histogram_tester.h"
+#include "base/test/user_action_tester.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/signin/core/browser/signin_metrics.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_web_contents_factory.h"
+#include "google_apis/gaia/gaia_urls.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // Tests DiceTabHelper intialization.
@@ -35,3 +38,55 @@ TEST(DiceTabHelperTest, Initialization) {
   EXPECT_EQ(access_point, dice_tab_helper->signin_access_point());
   EXPECT_EQ(reason, dice_tab_helper->signin_reason());
 }
+
+// Tests DiceTabHelper metrics.
+TEST(DiceTabHelperTest, Metrics) {
+  base::UserActionTester ua_tester;
+  base::HistogramTester h_tester;
+  content::TestBrowserThreadBundle thread_bundle;
+  TestingProfile profile;
+  content::TestWebContentsFactory factory;
+  content::WebContents* web_contents = factory.CreateWebContents(&profile);
+  DiceTabHelper::CreateForWebContents(web_contents);
+  DiceTabHelper* dice_tab_helper = DiceTabHelper::FromWebContents(web_contents);
+
+  // No metrics are logged when the Dice tab helper is created.
+  EXPECT_EQ(0, ua_tester.GetActionCount("Signin_Signin_FromStartPage"));
+  EXPECT_EQ(0, ua_tester.GetActionCount("Signin_SigninPage_Loading"));
+  EXPECT_EQ(0, ua_tester.GetActionCount("Signin_SigninPage_Shown"));
+
+  // Check metrics logged when the Dice tab helper is initialized.
+  dice_tab_helper->InitializeSigninFlow(
+      signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE,
+      signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT);
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_Signin_FromStartPage"));
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_SigninPage_Loading"));
+  EXPECT_EQ(0, ua_tester.GetActionCount("Signin_SigninPage_Shown"));
+  EXPECT_EQ(1, h_tester.GetBucketCount(
+                   "Signin.SigninStartedAccessPoint",
+                   static_cast<int>(
+                       signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE)));
+
+  // First call to did finish load does logs any Signin_SigninPage_Shown user
+  // action.
+  GURL signin_page_url = GaiaUrls::GetInstance()->gaia_url();
+  dice_tab_helper->DidFinishLoad(nullptr, signin_page_url);
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_SigninPage_Loading"));
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_SigninPage_Shown"));
+
+  // Second call to did finish load does not log any metrics.
+  dice_tab_helper->DidFinishLoad(nullptr, signin_page_url);
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_SigninPage_Loading"));
+  EXPECT_EQ(1, ua_tester.GetActionCount("Signin_SigninPage_Shown"));
+
+  // Check metrics are logged again when the Dice tab helper is re-initialized.
+  dice_tab_helper->InitializeSigninFlow(
+      signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE,
+      signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT);
+  EXPECT_EQ(2, ua_tester.GetActionCount("Signin_Signin_FromStartPage"));
+  EXPECT_EQ(2, ua_tester.GetActionCount("Signin_SigninPage_Loading"));
+  EXPECT_EQ(2, h_tester.GetBucketCount(
+                   "Signin.SigninStartedAccessPoint",
+                   static_cast<int>(
+                       signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE)));
+}

chrome/browser/ui/signin_view_controller.cc

@@ -155,11 +155,6 @@ void SigninViewController::ShowDiceSigninTab(
   DiceTabHelper::CreateForWebContents(active_contents);
   DiceTabHelper* tab_helper = DiceTabHelper::FromWebContents(active_contents);
   tab_helper->InitializeSigninFlow(access_point, signin_reason);
-
-  if (signin_reason == signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT) {
-    signin_metrics::LogSigninAccessPointStarted(access_point);
-    signin_metrics::RecordSigninUserActionForAccessPoint(access_point);
-  }
 }
 
 content::WebContents*