Implementing sign-in promo histograms for bookmark

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

Index: ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm b/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm
index 28cc755dbb44cc7bc27c3d588966e6491437fb92..03043e7ddf684349e76f15b5ad30c7740416abec 100644
--- a/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm
+++ b/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm
@@ -19,10 +19,12 @@
 #include "components/favicon/core/large_icon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "ios/chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "ios/chrome/browser/bookmarks/bookmarks_utils.h"
 #include "ios/chrome/browser/experimental_flags.h"
 #include "ios/chrome/browser/favicon/ios_chrome_large_icon_service_factory.h"
+#include "ios/chrome/browser/pref_names.h"
 #import "ios/chrome/browser/ui/authentication/signin_promo_view.h"
 #import "ios/chrome/browser/ui/authentication/signin_promo_view_configurator.h"
 #import "ios/chrome/browser/ui/authentication/signin_promo_view_consumer.h"
@@ -77,6 +79,18 @@ CGFloat minFaviconSizePt = 16;
 // This delay should not be too small to let enough time to load bookmarks
 // from network.
 const NSTimeInterval kShowEmptyBookmarksBackgroundRefreshDelay = 1.0;
+
+// The histogram used to record the number of time was seen before the bookmark

[sdefresne, 2017/06/19 09:07:50]

Why are those constants here and not in signin_promo_view_mediator.mm? Passing
them via std::string property causes lots of allocations and copying for no
benefits as far as I can see. If you really need the values to be properties,
maybe have them of type "const char*" with a comment that they have to point to
static storage.

[jlebel, 2017/06/19 14:38:12]

The sign-in promo mediator is used also for in "other devices". Those histograms
should not be updated anywhere else. I'm using those properties to activate the
histograms only for bookmark. The same histograms will be implemented soon for
settings view. Settings view doesn't use yet the mediator (for historical
reasons).
But it should, and it will. So with those properties, both views will have their
own histograms.

+// view has been closed.
+const char kBookmarksDismissalCount[] =
+    "MobileSignInPromo.BookmarkManager.DismissalCount";
+// The histogram used to record the number of time was seen before the user uses
+// the sign-in promo.
+const char kBookmarksCountTilSignin[] =

[sdefresne, 2017/06/19 09:07:50]

Shouldn't this be "kBookmarksCountTillSignin"?

[jlebel, 2017/06/19 14:38:12]

See signin_promo_view_mediator.h

+    "MobileSignInPromo.BookmarkManager.CountTilSignin";
+// The histogram used to record the number of time was seen before the user
+// dissmisses the sign-in promo.
+const char kBookmarkCountTilX[] = "MobileSignInPromo.BookmarkManager.CountTilX";

[sdefresne, 2017/06/19 09:07:50]

Shouldn't this be "kBookmarkCountTillX"?

[jlebel, 2017/06/19 14:38:12]

See signin_promo_view_mediator.h

 }
 
 @interface BookmarkCollectionView ()<BookmarkPromoCellDelegate,
@@ -142,6 +156,11 @@ const NSTimeInterval kShowEmptyBookmarksBackgroundRefreshDelay = 1.0;
 @synthesize browserState = _browserState;
 @synthesize shadow = _shadow;
 
++ (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry {
+  registry->RegisterIntegerPref(prefs::kIosBookmarkSigninPromoDisplayedCount,
+                                0);
+}
+
 #pragma mark - Initialization
 
 - (void)setupViews {
@@ -335,16 +354,35 @@ const NSTimeInterval kShowEmptyBookmarksBackgroundRefreshDelay = 1.0;
         _signinPromoViewMediator.consumer = nil;
         _signinPromoViewMediator = nil;
       } else {
-        _signinPromoViewMediator = [[SigninPromoViewMediator alloc] init];
+        _signinPromoViewMediator = [[SigninPromoViewMediator alloc]
+            initWithBrowserState:_browserState];
         _signinPromoViewMediator.consumer = self;
+        _signinPromoViewMediator.displayedCountPreferenceKey =
+            prefs::kIosBookmarkSigninPromoDisplayedCount;
+        _signinPromoViewMediator.alreadySeenSigninViewPreferenceKey =
+            prefs::kIosBookmarkPromoAlreadySeen;
+        _signinPromoViewMediator.dismissalCountHistogramName =
+            kBookmarksDismissalCount;
+        _signinPromoViewMediator.countTilXHistogramName = kBookmarkCountTilX;
+        _signinPromoViewMediator.countTilSigninHistogramName =
+            kBookmarksCountTilSignin;
         _signinPromoViewMediator.accessPoint =
             signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_MANAGER;
+        [_signinPromoViewMediator signinPromoViewVisible];
       }
     }
     [self.collectionView reloadData];
   }
 }
 
+- (void)wasShown {
+  [_signinPromoViewMediator signinPromoViewVisible];
+}
+
+- (void)wasHidden {
+  [_signinPromoViewMediator signinPromoViewHidden];
+}
+
 #pragma mark - Sections
 
 - (NSInteger)promoSection {
@@ -773,7 +811,7 @@ const NSTimeInterval kShowEmptyBookmarksBackgroundRefreshDelay = 1.0;
           configureSigninPromoView:signinPromoCell.signinPromoView];
       __weak BookmarkCollectionView* weakSelf = self;
       signinPromoCell.closeButtonAction = ^() {
-        [weakSelf.delegate bookmarkCollectionViewDismissPromo:self];
+        [weakSelf signinPromoCloseButtonAction];
       };
       return signinPromoCell;
     } else {
@@ -794,6 +832,12 @@ const NSTimeInterval kShowEmptyBookmarksBackgroundRefreshDelay = 1.0;
   return cell;
 }
 
+// Removes the sign-in promo view.
+- (void)signinPromoCloseButtonAction {
+  [_signinPromoViewMediator signinPromoViewDismissed];
+  [_delegate bookmarkCollectionViewDismissPromo:self];
+}
+
 // Create a header view for the element at |indexPath|.
 - (UICollectionReusableView*)headerAtIndexPath:(NSIndexPath*)indexPath {
   if (![self needsSectionHeaderForSection:indexPath.section])