[dice] Record Signin.SigninCompletedAccessPoint.* histograms

This CL records the following histograms:
* Signin.SigninCompletedAccessPoint.WithDefault
* Signin.SigninCompletedAccessPoint.NotDefault
* Signin.SigninCompletedAccessPoint.NewAccount

Bug: 832066
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I42ecf6be254dcf51133d565e90df56ae1f69ef93
Reviewed-on: https://chromium-review.googlesource.com/1010362
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550615}

chrome/browser/signin/chrome_signin_helper.cc

@@ -263,6 +263,7 @@ void ProcessMirrorHeaderUIThread(
 // Creates a DiceTurnOnSyncHelper.
 void CreateDiceTurnOnSyncHelper(Profile* profile,
                                 signin_metrics::AccessPoint access_point,
+                                signin_metrics::PromoAction promo_action,
                                 signin_metrics::Reason reason,
                                 content::WebContents* web_contents,
                                 const std::string& account_id) {
@@ -273,7 +274,7 @@ void CreateDiceTurnOnSyncHelper(Profile* profile,
   // DiceTurnSyncOnHelper is suicidal (it will kill itself once it finishes
   // enabling sync).
   new DiceTurnSyncOnHelper(
-      profile, browser, access_point, reason, account_id,
+      profile, browser, access_point, promo_action, reason, account_id,
       DiceTurnSyncOnHelper::SigninAbortedMode::REMOVE_ACCOUNT);
 }
 
@@ -307,12 +308,16 @@ void ProcessDiceHeaderUIThread(
 
   signin_metrics::AccessPoint access_point =
       signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN;
+  signin_metrics::PromoAction promo_action =
+      signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO;
   signin_metrics::Reason reason = signin_metrics::Reason::REASON_UNKNOWN_REASON;
+
   bool is_sync_signin_tab = false;
   DiceTabHelper* tab_helper = DiceTabHelper::FromWebContents(web_contents);
   if (signin::IsDicePrepareMigrationEnabled() && tab_helper) {
     is_sync_signin_tab = true;
     access_point = tab_helper->signin_access_point();
+    promo_action = tab_helper->signin_promo_action();
     reason = tab_helper->signin_reason();
   }
 
@@ -324,7 +329,7 @@ void ProcessDiceHeaderUIThread(
           web_contents, profile->GetPrefs(),
           SigninManagerFactory::GetForProfile(profile), is_sync_signin_tab,
           base::BindOnce(&CreateDiceTurnOnSyncHelper, base::Unretained(profile),
-                         access_point, reason),
+                         access_point, promo_action, reason),
           base::BindOnce(&ShowDiceSigninError, base::Unretained(profile))));
 }
 #endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)

chrome/browser/signin/dice_tab_helper.cc

@@ -26,6 +26,7 @@ void DiceTabHelper::InitializeSigninFlow(
     signin_metrics::PromoAction promo_action) {
   signin_access_point_ = access_point;
   signin_reason_ = reason;
+  signin_promo_action_ = promo_action;
   did_finish_loading_signin_page_ = false;
 
   if (signin_reason_ == signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT) {

chrome/browser/signin/dice_tab_helper.h

@@ -24,6 +24,10 @@ class DiceTabHelper : public content::WebContentsUserData<DiceTabHelper>,
     return signin_access_point_;
   }
 
+  signin_metrics::PromoAction signin_promo_action() {
+    return signin_promo_action_;
+  }
+
   signin_metrics::Reason signin_reason() { return signin_reason_; }
 
   // Initializes the DiceTabHelper for a new signin flow. Must be called once
@@ -42,6 +46,8 @@ class DiceTabHelper : public content::WebContentsUserData<DiceTabHelper>,
 
   signin_metrics::AccessPoint signin_access_point_ =
       signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN;
+  signin_metrics::PromoAction signin_promo_action_ =
+      signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO;
   signin_metrics::Reason signin_reason_ =
       signin_metrics::Reason::REASON_UNKNOWN_REASON;
   bool did_finish_loading_signin_page_ = false;

chrome/browser/signin/signin_ui_util.cc

@@ -48,13 +48,15 @@ void CreateDiceTurnSyncOnHelper(
     Profile* profile,
     Browser* browser,
     signin_metrics::AccessPoint signin_access_point,
+    signin_metrics::PromoAction signin_promo_action,
     signin_metrics::Reason signin_reason,
     const std::string& account_id,
     DiceTurnSyncOnHelper::SigninAbortedMode signin_aborted_mode) {
   // DiceTurnSyncOnHelper is suicidal (it will delete itself once it finishes
   // enabling sync).
-  new DiceTurnSyncOnHelper(profile, browser, signin_access_point, signin_reason,
-                           account_id, signin_aborted_mode);
+  new DiceTurnSyncOnHelper(profile, browser, signin_access_point,
+                           signin_promo_action, signin_reason, account_id,
+                           signin_aborted_mode);
 }
 }  // namespace
 #endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
@@ -133,6 +135,7 @@ void EnableSyncFromPromo(
         void(Profile* profile,
              Browser* browser,
              signin_metrics::AccessPoint signin_access_point,
+             signin_metrics::PromoAction signin_promo_action,
              signin_metrics::Reason signin_reason,
              const std::string& account_id,
              DiceTurnSyncOnHelper::SigninAbortedMode signin_aborted_mode)>
@@ -191,8 +194,9 @@ void EnableSyncFromPromo(
   signin_metrics::LogSigninAccessPointStarted(access_point, promo_action);
   signin_metrics::RecordSigninUserActionForAccessPoint(access_point);
   std::move(create_dice_turn_sync_on_helper_callback)
-      .Run(profile, browser, access_point,
-           signin_metrics::Reason::REASON_UNKNOWN_REASON, account.account_id,
+      .Run(profile, browser, access_point, promo_action,
+           signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT,
+           account.account_id,
            DiceTurnSyncOnHelper::SigninAbortedMode::KEEP_ACCOUNT);
 #else
   NOTREACHED();

chrome/browser/signin/signin_ui_util.h

@@ -82,6 +82,7 @@ void EnableSyncFromPromo(
         void(Profile* profile,
              Browser* browser,
              signin_metrics::AccessPoint signin_access_point,
+             signin_metrics::PromoAction signin_promo_action,
              signin_metrics::Reason signin_reason,
              const std::string& account_id,
              DiceTurnSyncOnHelper::SigninAbortedMode signin_aborted_mode)>

chrome/browser/signin/signin_ui_util_unittest.cc

@@ -91,6 +91,8 @@ class DiceSigninUiUtilTest : public BrowserWithTestWindowTest {
     Browser* browser = nullptr;
     signin_metrics::AccessPoint signin_access_point =
         signin_metrics::AccessPoint::ACCESS_POINT_MAX;
+    signin_metrics::PromoAction signin_promo_action =
+        signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO;
     signin_metrics::Reason signin_reason = signin_metrics::Reason::REASON_MAX;
     std::string account_id;
     DiceTurnSyncOnHelper::SigninAbortedMode signin_aborted_mode =
@@ -101,6 +103,7 @@ class DiceSigninUiUtilTest : public BrowserWithTestWindowTest {
       Profile* profile,
       Browser* browser,
       signin_metrics::AccessPoint signin_access_point,
+      signin_metrics::PromoAction signin_promo_action,
       signin_metrics::Reason signin_reason,
       const std::string& account_id,
       DiceTurnSyncOnHelper::SigninAbortedMode signin_aborted_mode) {
@@ -109,6 +112,8 @@ class DiceSigninUiUtilTest : public BrowserWithTestWindowTest {
     create_dice_turn_sync_on_helper_params_.browser = browser;
     create_dice_turn_sync_on_helper_params_.signin_access_point =
         signin_access_point;
+    create_dice_turn_sync_on_helper_params_.signin_promo_action =
+        signin_promo_action;
     create_dice_turn_sync_on_helper_params_.signin_reason = signin_reason;
     create_dice_turn_sync_on_helper_params_.account_id = account_id;
     create_dice_turn_sync_on_helper_params_.signin_aborted_mode =
@@ -231,12 +236,12 @@ TEST_F(DiceSigninUiUtilTest, EnableSyncWithExistingAccount) {
 
     EnableSync(GetAccountTrackerService()->GetAccountInfo(account_id),
                is_default_promo_account);
-    ASSERT_TRUE(create_dice_turn_sync_on_helper_called_);
-    ExpectOneSigninStartedHistograms(
-        histogram_tester,
+    signin_metrics::PromoAction expected_promo_action =
         is_default_promo_account
             ? signin_metrics::PromoAction::PROMO_ACTION_WITH_DEFAULT
-            : signin_metrics::PromoAction::PROMO_ACTION_NOT_DEFAULT);
+            : signin_metrics::PromoAction::PROMO_ACTION_NOT_DEFAULT;
+    ASSERT_TRUE(create_dice_turn_sync_on_helper_called_);
+    ExpectOneSigninStartedHistograms(histogram_tester, expected_promo_action);
 
     EXPECT_EQ(1, user_action_tester.GetActionCount(
                      "Signin_Signin_FromBookmarkBubble"));
@@ -248,7 +253,9 @@ TEST_F(DiceSigninUiUtilTest, EnableSyncWithExistingAccount) {
     EXPECT_EQ(account_id, create_dice_turn_sync_on_helper_params_.account_id);
     EXPECT_EQ(signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE,
               create_dice_turn_sync_on_helper_params_.signin_access_point);
-    EXPECT_EQ(signin_metrics::Reason::REASON_UNKNOWN_REASON,
+    EXPECT_EQ(expected_promo_action,
+              create_dice_turn_sync_on_helper_params_.signin_promo_action);
+    EXPECT_EQ(signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT,
               create_dice_turn_sync_on_helper_params_.signin_reason);
     EXPECT_EQ(DiceTurnSyncOnHelper::SigninAbortedMode::KEEP_ACCOUNT,
               create_dice_turn_sync_on_helper_params_.signin_aborted_mode);

chrome/browser/ui/sync/one_click_signin_sync_starter.cc

@@ -485,7 +485,9 @@ void OneClickSigninSyncStarter::SigninFailed(
 }
 
 void OneClickSigninSyncStarter::SigninSuccess() {
-  signin_metrics::LogSigninAccessPointCompleted(signin_access_point_);
+  signin_metrics::LogSigninAccessPointCompleted(
+      signin_access_point_,
+      signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
   signin_metrics::LogSigninReason(signin_reason_);
   base::RecordAction(base::UserMetricsAction("Signin_Signin_Succeed"));
 }

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

@@ -49,6 +49,7 @@ AccountInfo GetAccountInfo(Profile* profile, const std::string& account_id) {
 DiceTurnSyncOnHelper::DiceTurnSyncOnHelper(
     Profile* profile,
     signin_metrics::AccessPoint signin_access_point,
+    signin_metrics::PromoAction signin_promo_action,
     signin_metrics::Reason signin_reason,
     const std::string& account_id,
     SigninAbortedMode signin_aborted_mode,
@@ -58,6 +59,7 @@ DiceTurnSyncOnHelper::DiceTurnSyncOnHelper(
       signin_manager_(SigninManagerFactory::GetForProfile(profile)),
       token_service_(ProfileOAuth2TokenServiceFactory::GetForProfile(profile)),
       signin_access_point_(signin_access_point),
+      signin_promo_action_(signin_promo_action),
       signin_reason_(signin_reason),
       signin_aborted_mode_(signin_aborted_mode),
       account_info_(GetAccountInfo(profile, account_id)),
@@ -109,12 +111,14 @@ DiceTurnSyncOnHelper::DiceTurnSyncOnHelper(
     Profile* profile,
     Browser* browser,
     signin_metrics::AccessPoint signin_access_point,
+    signin_metrics::PromoAction signin_promo_action,
     signin_metrics::Reason signin_reason,
     const std::string& account_id,
     SigninAbortedMode signin_aborted_mode)
     : DiceTurnSyncOnHelper(
           profile,
           signin_access_point,
+          signin_promo_action,
           signin_reason,
           account_id,
           signin_aborted_mode,
@@ -315,7 +319,8 @@ DiceTurnSyncOnHelper::GetProfileSyncService() {
 void DiceTurnSyncOnHelper::SigninAndShowSyncConfirmationUI() {
   // Signin.
   signin_manager_->OnExternalSigninCompleted(account_info_.email);
-  signin_metrics::LogSigninAccessPointCompleted(signin_access_point_);
+  signin_metrics::LogSigninAccessPointCompleted(signin_access_point_,
+                                                signin_promo_action_);
   signin_metrics::LogSigninReason(signin_reason_);
   base::RecordAction(base::UserMetricsAction("Signin_Signin_Succeed"));
 

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h

@@ -95,6 +95,7 @@ class DiceTurnSyncOnHelper : public SyncStartupTracker::Observer {
   // in the token service.
   DiceTurnSyncOnHelper(Profile* profile,
                        signin_metrics::AccessPoint signin_access_point,
+                       signin_metrics::PromoAction signin_promo_action,
                        signin_metrics::Reason signin_reason,
                        const std::string& account_id,
                        SigninAbortedMode signin_aborted_mode,
@@ -104,6 +105,7 @@ class DiceTurnSyncOnHelper : public SyncStartupTracker::Observer {
   DiceTurnSyncOnHelper(Profile* profile,
                        Browser* browser,
                        signin_metrics::AccessPoint signin_access_point,
+                       signin_metrics::PromoAction signin_promo_action,
                        signin_metrics::Reason signin_reason,
                        const std::string& account_id,
                        SigninAbortedMode signin_aborted_mode);
@@ -189,6 +191,7 @@ class DiceTurnSyncOnHelper : public SyncStartupTracker::Observer {
   SigninManager* signin_manager_;
   ProfileOAuth2TokenService* token_service_;
   const signin_metrics::AccessPoint signin_access_point_;
+  const signin_metrics::PromoAction signin_promo_action_;
   const signin_metrics::Reason signin_reason_;
 
   // Whether the refresh token should be deleted if the Sync flow is aborted.

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc

@@ -53,6 +53,8 @@ const char kEnterpriseGaiaID[] = "enterprise_gaia_id";
 
 const signin_metrics::AccessPoint kAccessPoint =
     signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER;
+const signin_metrics::PromoAction kSigninPromoAction =
+    signin_metrics::PromoAction::PROMO_ACTION_WITH_DEFAULT;
 const signin_metrics::Reason kSigninReason =
     signin_metrics::Reason::REASON_REAUTHENTICATION;
 
@@ -224,8 +226,8 @@ class DiceTurnSyncOnHelperTest : public testing::Test {
   DiceTurnSyncOnHelper* CreateDiceTurnOnSyncHelper(
       DiceTurnSyncOnHelper::SigninAbortedMode mode) {
     return new DiceTurnSyncOnHelper(
-        profile(), kAccessPoint, kSigninReason, account_id_, mode,
-        std::make_unique<TestDiceTurnSyncOnHelperDelegate>(this));
+        profile(), kAccessPoint, kSigninPromoAction, kSigninReason, account_id_,
+        mode, std::make_unique<TestDiceTurnSyncOnHelperDelegate>(this));
   }
 
   void UseEnterpriseAccount() {

chrome/browser/ui/webui/signin/inline_login_handler.cc

@@ -108,7 +108,9 @@ void InlineLoginHandler::ContinueHandleInitializeMessage() {
   if (reason != signin_metrics::Reason::REASON_REAUTHENTICATION &&
       reason != signin_metrics::Reason::REASON_UNLOCK &&
       reason != signin_metrics::Reason::REASON_ADD_SECONDARY_ACCOUNT) {
-    signin_metrics::LogSigninAccessPointStarted(access_point);
+    signin_metrics::LogSigninAccessPointStarted(
+        access_point,
+        signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
     signin_metrics::RecordSigninUserActionForAccessPoint(access_point);
     base::RecordAction(base::UserMetricsAction("Signin_SigninPage_Loading"));
     params.SetBoolean("isLoginPrimaryAccount", true);

components/signin/core/browser/signin_metrics.cc

@@ -60,16 +60,6 @@ DifferentPrimaryAccounts ComparePrimaryAccounts(bool primary_accounts_same,
   return COOKIE_AND_TOKEN_PRIMARIES_DIFFERENT;
 }
 
-void LogSigninAccessPointStarted(AccessPoint access_point) {
-  LogSigninAccessPointStarted(access_point,
-                              PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
-}
-
-void LogSigninAccessPointCompleted(AccessPoint access_point) {
-  LogSigninAccessPointCompleted(access_point,
-                                PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
-}
-
 void LogSigninAccessPointStarted(AccessPoint access_point,
                                  PromoAction promo_action) {
   UMA_HISTOGRAM_ENUMERATION("Signin.SigninStartedAccessPoint",

components/signin/core/browser/signin_metrics.h

@@ -301,11 +301,7 @@ enum class ReportingType { PERIODIC, ON_CHANGE };
 // Histograms
 // -----------------------------------------------------------------------------
 
-// Tracks the access point of sign in on desktop.
-void LogSigninAccessPointStarted(AccessPoint access_point);
-void LogSigninAccessPointCompleted(AccessPoint access_point);
-
-// Tracks the access point of sign in on iOS.
+// Tracks the access point of sign in.
 void LogSigninAccessPointStarted(AccessPoint access_point,
                                  PromoAction promo_action);
 void LogSigninAccessPointCompleted(AccessPoint access_point,

ios/chrome/browser/ui/first_run/first_run_chrome_signin_view_controller.mm

@@ -91,7 +91,8 @@ NSString* const kSignInSkipButtonAccessibilityIdentifier =
     _hasRecordedSigninStarted = YES;
     base::RecordAction(base::UserMetricsAction("Signin_Signin_FromStartPage"));
     signin_metrics::LogSigninAccessPointStarted(
-        signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE);
+        signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE,
+        signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
   }
 
   // Save the version number to prevent showing the SSO Recall promo on the next

ios/chrome/browser/ui/promos/signin_promo_view_controller.mm

@@ -99,7 +99,8 @@ NSSet* GaiaIdSetWithIdentities(NSArray* identities) {
     base::RecordAction(
         base::UserMetricsAction("Signin_Signin_FromSigninPromo"));
     signin_metrics::LogSigninAccessPointStarted(
-        signin_metrics::AccessPoint::ACCESS_POINT_SIGNIN_PROMO);
+        signin_metrics::AccessPoint::ACCESS_POINT_SIGNIN_PROMO,
+        signin_metrics::PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO);
   }
 
   [self recordPromoDisplayed];