[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}diff --git a/chrome/browser/signin/dice_tab_helper.cc b/chrome/browser/signin/dice_tab_helper.cc
index b9e98a5..5897268 100644
--- a/chrome/browser/signin/dice_tab_helper.cc
+++ b/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 @@
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 @@
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"));
+ }
}
diff --git a/chrome/browser/signin/dice_tab_helper.h b/chrome/browser/signin/dice_tab_helper.h
index 8e9aecf..c336b0cf 100644
--- a/chrome/browser/signin/dice_tab_helper.h
+++ b/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 @@
// 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);
};
diff --git a/chrome/browser/signin/dice_tab_helper_unittest.cc b/chrome/browser/signin/dice_tab_helper_unittest.cc
index 0c66541..a6b01e9 100644
--- a/chrome/browser/signin/dice_tab_helper_unittest.cc
+++ b/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 @@
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)));
+}
diff --git a/chrome/browser/ui/signin_view_controller.cc b/chrome/browser/ui/signin_view_controller.cc
index 83a9450..201e8ce 100644
--- a/chrome/browser/ui/signin_view_controller.cc
+++ b/chrome/browser/ui/signin_view_controller.cc
@@ -155,11 +155,6 @@
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*
