Implementing sign-in promo histograms for settings

ios/chrome/browser/ui/settings/settings_collection_view_controller.mm

Index: ios/chrome/browser/ui/settings/settings_collection_view_controller.mm
diff --git a/ios/chrome/browser/ui/settings/settings_collection_view_controller.mm b/ios/chrome/browser/ui/settings/settings_collection_view_controller.mm
index 03077d2bb7b4bdafcb7c61388f2c1ff03881aef4..16b271b44caae822365f9aab8a34065a56b29228 100644
--- a/ios/chrome/browser/ui/settings/settings_collection_view_controller.mm
+++ b/ios/chrome/browser/ui/settings/settings_collection_view_controller.mm
@@ -7,6 +7,7 @@
 #include <memory>
 
 #import "base/mac/foundation_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/scoped_observer.h"
 #include "base/strings/sys_string_conversions.h"
@@ -15,6 +16,7 @@
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/util.h"
@@ -95,6 +97,8 @@ namespace {
 
 const CGFloat kAccountProfilePhotoDimension = 40.0f;
 
+const int kAutomaticSigninPromoViewDismissCount = 20;
+
 typedef NS_ENUM(NSInteger, SectionIdentifier) {
   SectionIdentifierSignIn = kSectionIdentifierEnumZero,
   SectionIdentifierBasics,
@@ -222,6 +226,9 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
   CollectionViewDetailItem* _defaultSearchEngineItem;
   CollectionViewDetailItem* _savePasswordsDetailItem;
   CollectionViewDetailItem* _autoFillDetailItem;
+
+  // YES if the user used at least once the sign-in promo view buttons.
+  BOOL _signinStarted;
 }
 
 // Stops observing browser state services. This is required during the shutdown
@@ -305,6 +312,17 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
   [self updateSearchCell];
 }
 
+- (void)viewDidDisappear:(BOOL)animated {
+  [super viewDidDisappear:animated];
+  if (!_signinStarted && _signinPromoViewMediator) {
+    PrefService* prefs = _browserState->GetPrefs();
+    int displayedCount =
+        prefs->GetInteger(prefs::kIosSettingsSigninPromoDisplayedCount);
+    UMA_HISTOGRAM_COUNTS_100("MobileSignInPromo.SettingsManager.DismissalCount",
+                             displayedCount);
+  }
+}
+
 #pragma mark SettingsRootCollectionViewController
 
 - (void)loadModel {
@@ -324,10 +342,17 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
           base::UserMetricsAction("Signin_Impression_FromSettings"));
       _hasRecordedSigninImpression = YES;
     }
-    if (experimental_flags::IsSigninPromoEnabled()) {
+    PrefService* prefs = _browserState->GetPrefs();
+    int displayedCount =
+        prefs->GetInteger(prefs::kIosSettingsSigninPromoDisplayedCount);
+    if (experimental_flags::IsSigninPromoEnabled() &&
+        displayedCount < kAutomaticSigninPromoViewDismissCount) {
       _signinPromoViewMediator =
           [[SigninPromoViewMediator alloc] initWithBrowserState:_browserState];
       _signinPromoViewMediator.consumer = self;
+      ++displayedCount;

[sdefresne, 2017/06/26 06:34:35]

nit: it is clearer in my opinion to just pass "displayedCount + 1" and to mark
the variable "displayedCount" as const

  prefs->SetInteger(prefs::kIosSettingsSeigninPromoDisplayedCount,
                    displayedCount + 1);

[jlebel, 2017/06/26 19:19:21]

Done.

+      prefs->SetInteger(prefs::kIosSettingsSigninPromoDisplayedCount,
+                        displayedCount);
     }
     [model addItem:[self signInTextItem]
         toSectionWithIdentifier:SectionIdentifierSignIn];
@@ -407,8 +432,7 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
 #pragma mark - Model Items
 
 - (CollectionViewItem*)signInTextItem {
-  if (experimental_flags::IsSigninPromoEnabled()) {
-    DCHECK(_signinPromoViewMediator);
+  if (_signinPromoViewMediator) {
     SigninPromoItem* signinPromoItem =
         [[SigninPromoItem alloc] initWithType:ItemTypeSigninPromo];
     signinPromoItem.configurator =
@@ -1148,6 +1172,7 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
 
 - (void)signinPromoViewDidTapSigninWithNewAccount:
     (SigninPromoView*)signinPromoView {
+  [self sendCountTillSigninHistogram];
   DCHECK(!_signinPromoViewMediator.defaultIdentity);
   base::RecordAction(
       base::UserMetricsAction("Signin_SigninNewAccount_FromSettings"));
@@ -1158,6 +1183,7 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
 
 - (void)signinPromoViewDidTapSigninWithDefaultAccount:
     (SigninPromoView*)signinPromoView {
+  [self sendCountTillSigninHistogram];
   ChromeIdentity* identity = _signinPromoViewMediator.defaultIdentity;
   DCHECK(identity);
   base::RecordAction(
@@ -1169,6 +1195,7 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
 
 - (void)signinPromoViewDidTapSigninWithOtherAccount:
     (SigninPromoView*)signinPromoView {
+  [self sendCountTillSigninHistogram];
   DCHECK(_signinPromoViewMediator.defaultIdentity);
   base::RecordAction(
       base::UserMetricsAction("Signin_SigninNotDefault_FromSettings"));
@@ -1177,4 +1204,15 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
                                    PROMO_ACTION_NOT_DEFAULT];
 }
 
+#pragma mark - Metrics
+
+- (void)sendCountTillSigninHistogram {

[sdefresne, 2017/06/26 06:34:35]

nit: in your other CL and the histogram.xml file you used "Til" instead of
"Till", maybe use the same naming convention here too

[jlebel, 2017/06/26 19:19:21]

Yes, I definitely made a mistake here. Thanks.

+  _signinStarted = YES;
+  PrefService* prefs = _browserState->GetPrefs();
+  int displayedCount =
+      prefs->GetInteger(prefs::kIosSettingsSigninPromoDisplayedCount);
+  UMA_HISTOGRAM_COUNTS_100("MobileSignInPromo.SettingsManager.CountTilSignin",
+                           displayedCount);
+}
+
 @end