Simplyfing SigninPromoViewMediator API

Removing 3 properties for metrics, all related to the access point
value.

Bug: 661794
Change-Id: I4f97831e1bf775d9c660d6658e6dfa3bf857260d
Reviewed-on: https://chromium-review.googlesource.com/575137Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487452}

ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h

@@ -17,14 +17,6 @@
 namespace ios {
 class ChromeBrowserState;
 
-// Enums to choose which histograms is used to record the user actions.
-enum class SigninPromoViewHistograms {
-  // No histograms.
-  None,
-  // Histograms: MobileSignInPromo.BookmarkManager.*.
-  Bookmarks,
-};
-
 // Enums for the sign-in promo view state.
 enum class SigninPromoViewState {
   // None of the buttons has been used yet.
@@ -53,17 +45,6 @@ enum class SigninPromoViewState {
 // contains nil.
 @property(nonatomic, readonly, strong) ChromeIdentity* defaultIdentity;
 
-// Access point used to send user action metrics.
-@property(nonatomic) signin_metrics::AccessPoint accessPoint;
-
-// Preference key to count how many time the sign-in promo view is seen. The
-// value should point to static storage.
-@property(nonatomic) const char* displayedCountPreferenceKey;
-// Preference key, set to true when the sign-in promo view is seen too much. The
-// value should point to static storage.
-@property(nonatomic) const char* alreadySeenSigninViewPreferenceKey;
-// Histograms to use for the user actions.
-@property(nonatomic) ios::SigninPromoViewHistograms histograms;
 // Sign-in promo view state.
 @property(nonatomic) ios::SigninPromoViewState signinPromoViewState;
 
@@ -71,7 +52,11 @@ enum class SigninPromoViewState {
 - (instancetype)init NS_UNAVAILABLE;
 
 // Initialises with browser state.
+// * |browserState| is the current browser state.
+// * |accessPoint| only ACCESS_POINT_SETTINGS, ACCESS_POINT_BOOKMARK_MANAGER,
+// ACCESS_POINT_RECENT_TABS, ACCESS_POINT_TAB_SWITCHER are supported.
 - (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
+                         accessPoint:(signin_metrics::AccessPoint)accessPoint
     NS_DESIGNATED_INITIALIZER;
 
 - (SigninPromoViewConfigurator*)createConfigurator;

ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm

@@ -34,6 +34,20 @@
 namespace {
 const int kAutomaticSigninPromoViewDismissCount = 20;
 
+#if DCHECK_IS_ON()
+bool IsSupportedAccessPoint(signin_metrics::AccessPoint access_point) {
+  switch (access_point) {
+    case signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS:
+    case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER:
+    case signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS:
+    case signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER:
+      return true;
+    default:
+      return false;
+  }
+}
+#endif  // DCHECK_IS_ON()
+
 void RecordSigninUserActionForAccessPoint(
     signin_metrics::AccessPoint access_point) {
   switch (access_point) {
@@ -121,6 +135,56 @@ void RecordSigninNewAccountUserActionForAccessPoint(
       break;
   }
 }
+
+void RecordImpressionsTilSigninButtonsHistogramForAccessPoint(
+    signin_metrics::AccessPoint access_point,
+    int displayed_count) {
+  DCHECK_EQ(signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER,
+            access_point);
+  UMA_HISTOGRAM_COUNTS_100(
+      "MobileSignInPromo.BookmarkManager.ImpressionsTilSigninButtons",
+      displayed_count);
+}
+
+void RecordImpressionsTilDismissHistogramForAccessPoint(
+    signin_metrics::AccessPoint access_point,
+    int displayed_count) {
+  DCHECK_EQ(signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER,
+            access_point);
+  UMA_HISTOGRAM_COUNTS_100(
+      "MobileSignInPromo.BookmarkManager.ImpressionsTilDismiss",
+      displayed_count);
+}
+
+void RecordImpressionsTilXButtonHistogramForAccessPoint(
+    signin_metrics::AccessPoint access_point,
+    int displayed_count) {
+  DCHECK_EQ(signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER,
+            access_point);
+  UMA_HISTOGRAM_COUNTS_100(
+      "MobileSignInPromo.BookmarkManager.ImpressionsTilXButton",
+      displayed_count);
+}
+
+const char* DisplayedCountPreferenceKey(
+    signin_metrics::AccessPoint access_point) {
+  switch (access_point) {
+    case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER:
+      return prefs::kIosBookmarkSigninPromoDisplayedCount;
+    default:
+      return nullptr;
+  }
+}
+
+const char* AlreadySeenSigninViewPreferenceKey(
+    signin_metrics::AccessPoint access_point) {
+  switch (access_point) {
+    case signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER:
+      return prefs::kIosBookmarkPromoAlreadySeen;
+    default:
+      return nullptr;
+  }
+}
 }  // namespace
 
 @interface SigninPromoViewMediator ()<ChromeIdentityServiceObserver,
@@ -129,6 +193,7 @@ void RecordSigninNewAccountUserActionForAccessPoint(
 
 @implementation SigninPromoViewMediator {
   ios::ChromeBrowserState* _browserState;
+  signin_metrics::AccessPoint _accessPoint;
   std::unique_ptr<ChromeIdentityServiceObserverBridge> _identityServiceObserver;
   std::unique_ptr<ChromeBrowserProviderObserverBridge> _browserProviderObserver;
   UIImage* _identityAvatar;
@@ -137,16 +202,14 @@ void RecordSigninNewAccountUserActionForAccessPoint(
 
 @synthesize consumer = _consumer;
 @synthesize defaultIdentity = _defaultIdentity;
-@synthesize accessPoint = _accessPoint;
-@synthesize displayedCountPreferenceKey = _displayedCountPreferenceKey;
-@synthesize alreadySeenSigninViewPreferenceKey =
-    _alreadySeenSigninViewPreferenceKey;
-@synthesize histograms = _histograms;
 @synthesize signinPromoViewState = _signinPromoViewState;
 
-- (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState {
+- (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
+                         accessPoint:(signin_metrics::AccessPoint)accessPoint {
   self = [super init];
   if (self) {
+    DCHECK(IsSupportedAccessPoint(accessPoint));
+    _accessPoint = accessPoint;
     _browserState = browserState;
     NSArray* identities = ios::GetChromeBrowserProvider()
                               ->GetChromeIdentityService()
@@ -215,19 +278,14 @@ void RecordSigninNewAccountUserActionForAccessPoint(
   DCHECK(![self isInvalidOrClosed] ||
          _signinPromoViewState != ios::SigninPromoViewState::Unused);
   _signinPromoViewState = ios::SigninPromoViewState::SigninStarted;
-  if (!_displayedCountPreferenceKey)
+  const char* displayedCountPreferenceKey =
+      DisplayedCountPreferenceKey(_accessPoint);
+  if (!displayedCountPreferenceKey)
     return;
   PrefService* prefs = _browserState->GetPrefs();
-  int displayedCount = prefs->GetInteger(_displayedCountPreferenceKey);
-  switch (_histograms) {
-    case ios::SigninPromoViewHistograms::Bookmarks:
-      UMA_HISTOGRAM_COUNTS_100(
-          "MobileSignInPromo.BookmarkManager.ImpressionsTilSigninButtons",
-          displayedCount);
-      break;
-    case ios::SigninPromoViewHistograms::None:
-      break;
-  }
+  int displayedCount = prefs->GetInteger(displayedCountPreferenceKey);
+  RecordImpressionsTilSigninButtonsHistogramForAccessPoint(_accessPoint,
+                                                           displayedCount);
 }
 
 - (void)signinPromoViewVisible {
@@ -235,15 +293,19 @@ void RecordSigninNewAccountUserActionForAccessPoint(
   if (_isSigninPromoViewVisible)
     return;
   _isSigninPromoViewVisible = YES;
-  if (!_displayedCountPreferenceKey)
+  const char* displayedCountPreferenceKey =
+      DisplayedCountPreferenceKey(_accessPoint);
+  if (!displayedCountPreferenceKey)
     return;
   PrefService* prefs = _browserState->GetPrefs();
-  int displayedCount = prefs->GetInteger(_displayedCountPreferenceKey);
+  int displayedCount = prefs->GetInteger(displayedCountPreferenceKey);
   ++displayedCount;
-  prefs->SetInteger(_displayedCountPreferenceKey, displayedCount);
+  prefs->SetInteger(displayedCountPreferenceKey, displayedCount);
+  const char* alreadySeenSigninViewPreferenceKey =
+      AlreadySeenSigninViewPreferenceKey(_accessPoint);
   if (displayedCount >= kAutomaticSigninPromoViewDismissCount &&
-      _alreadySeenSigninViewPreferenceKey) {
-    prefs->SetBoolean(prefs::kIosBookmarkPromoAlreadySeen, true);
+      alreadySeenSigninViewPreferenceKey) {
+    prefs->SetBoolean(alreadySeenSigninViewPreferenceKey, true);
   }
 }
 
@@ -255,19 +317,14 @@ void RecordSigninNewAccountUserActionForAccessPoint(
 - (void)signinPromoViewClosed {
   DCHECK(_isSigninPromoViewVisible && ![self isInvalidOrClosed]);
   _signinPromoViewState = ios::SigninPromoViewState::Closed;
-  if (!_displayedCountPreferenceKey)
+  const char* displayedCountPreferenceKey =
+      DisplayedCountPreferenceKey(_accessPoint);
+  if (!displayedCountPreferenceKey)
     return;
   PrefService* prefs = _browserState->GetPrefs();
-  int displayedCount = prefs->GetInteger(_displayedCountPreferenceKey);
-  switch (_histograms) {
-    case ios::SigninPromoViewHistograms::Bookmarks:
-      UMA_HISTOGRAM_COUNTS_100(
-          "MobileSignInPromo.BookmarkManager.ImpressionsTilXButton",
-          displayedCount);
-      break;
-    case ios::SigninPromoViewHistograms::None:
-      break;
-  }
+  int displayedCount = prefs->GetInteger(displayedCountPreferenceKey);
+  RecordImpressionsTilXButtonHistogramForAccessPoint(_accessPoint,
+                                                     displayedCount);
 }
 
 - (void)signinPromoViewRemoved {
@@ -276,7 +333,9 @@ void RecordSigninNewAccountUserActionForAccessPoint(
   _signinPromoViewState = ios::SigninPromoViewState::Invalid;
   // If the sign-in promo view has been used at least once, it should not be
   // counted as dismissed (even if the sign-in has been canceled).
-  if (!_displayedCountPreferenceKey || !wasUnused)
+  const char* displayedCountPreferenceKey =
+      DisplayedCountPreferenceKey(_accessPoint);
+  if (!displayedCountPreferenceKey || !wasUnused)
     return;
   // If the sign-in view is removed when the user is authenticated, then the
   // sign-in has been done by another view, and this mediator cannot be counted
@@ -286,16 +345,9 @@ void RecordSigninNewAccountUserActionForAccessPoint(
   if (authService->IsAuthenticated())
     return;
   PrefService* prefs = _browserState->GetPrefs();
-  int displayedCount = prefs->GetInteger(_displayedCountPreferenceKey);
-  switch (_histograms) {
-    case ios::SigninPromoViewHistograms::Bookmarks:
-      UMA_HISTOGRAM_COUNTS_100(
-          "MobileSignInPromo.BookmarkManager.ImpressionsTilDismiss",
-          displayedCount);
-      break;
-    case ios::SigninPromoViewHistograms::None:
-      break;
-  }
+  int displayedCount = prefs->GetInteger(displayedCountPreferenceKey);
+  RecordImpressionsTilDismissHistogramForAccessPoint(_accessPoint,
+                                                     displayedCount);
 }
 
 - (void)signinCallback {

ios/chrome/browser/ui/authentication/signin_promo_view_mediator_unittest.mm

@@ -26,7 +26,10 @@ class SigninPromoViewMediatorTest : public PlatformTest {
  protected:
   void SetUp() override {
     consumer_ = OCMStrictProtocolMock(@protocol(SigninPromoViewConsumer));
-    mediator_ = [[SigninPromoViewMediator alloc] initWithBrowserState:nil];
+    mediator_ = [[SigninPromoViewMediator alloc]
+        initWithBrowserState:nil
+                 accessPoint:signin_metrics::AccessPoint::
+                                 ACCESS_POINT_SETTINGS];
     mediator_.consumer = consumer_;
 
     signin_promo_view_ = OCMStrictClassMock([SigninPromoView class]);

ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm

@@ -354,17 +354,11 @@ CGFloat minFaviconSizePt = 16;
       [_signinPromoViewMediator signinPromoViewRemoved];
       _signinPromoViewMediator = nil;
     } else {
-      _signinPromoViewMediator =
-          [[SigninPromoViewMediator alloc] initWithBrowserState:_browserState];
+      _signinPromoViewMediator = [[SigninPromoViewMediator alloc]
+          initWithBrowserState:_browserState
+                   accessPoint:signin_metrics::AccessPoint::
+                                   ACCESS_POINT_BOOKMARK_MANAGER];
       _signinPromoViewMediator.consumer = self;
-      _signinPromoViewMediator.displayedCountPreferenceKey =
-          prefs::kIosBookmarkSigninPromoDisplayedCount;
-      _signinPromoViewMediator.alreadySeenSigninViewPreferenceKey =
-          prefs::kIosBookmarkPromoAlreadySeen;
-      _signinPromoViewMediator.histograms =
-          ios::SigninPromoViewHistograms::Bookmarks;
-      _signinPromoViewMediator.accessPoint =
-          signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER;
       [_signinPromoViewMediator signinPromoViewVisible];
     }
   }

ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_view_controller.mm

@@ -826,10 +826,10 @@ enum CellType {
     case CELL_OTHER_DEVICES_SIGNIN_PROMO: {
       if (!_signinPromoViewMediator) {
         _signinPromoViewMediator = [[SigninPromoViewMediator alloc]
-            initWithBrowserState:_browserState];
+            initWithBrowserState:_browserState
+                     accessPoint:signin_metrics::AccessPoint::
+                                     ACCESS_POINT_RECENT_TABS];
         _signinPromoViewMediator.consumer = self;
-        _signinPromoViewMediator.accessPoint =
-            signin_metrics::AccessPoint::ACCESS_POINT_RECENT_TABS;
       }
       contentViewTopMargin = kSigninPromoViewTopMargin;
       SigninPromoView* signinPromoView =

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

@@ -341,7 +341,9 @@ void SigninObserverBridge::GoogleSignedOut(const std::string& account_id,
         displayedCount < kAutomaticSigninPromoViewDismissCount) {
       if (!_signinPromoViewMediator) {
         _signinPromoViewMediator = [[SigninPromoViewMediator alloc]
-            initWithBrowserState:_browserState];
+            initWithBrowserState:_browserState
+                     accessPoint:signin_metrics::AccessPoint::
+                                     ACCESS_POINT_SETTINGS];
         _signinPromoViewMediator.consumer = self;
         prefs->SetInteger(prefs::kIosSettingsSigninPromoDisplayedCount,
                           displayedCount + 1);

ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm

@@ -242,10 +242,10 @@ const CGFloat kSubtitleMinimunLineHeight = 24.0;
       constraintEqualToAnchor:self.centerYAnchor
                      constant:kContainerOriginYOffset]
       .active = YES;
-  _signinPromoViewMediator =
-      [[SigninPromoViewMediator alloc] initWithBrowserState:_browserState];
-  _signinPromoViewMediator.accessPoint =
-      signin_metrics::AccessPoint::ACCESS_POINT_TAB_SWITCHER;
+  _signinPromoViewMediator = [[SigninPromoViewMediator alloc]
+      initWithBrowserState:_browserState
+               accessPoint:signin_metrics::AccessPoint::
+                               ACCESS_POINT_TAB_SWITCHER];
   _signinPromoView.delegate = _signinPromoViewMediator;
   _signinPromoViewMediator.consumer = self;
   [[_signinPromoViewMediator createConfigurator]