Implementing sign-in promo histograms for bookmark

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/chrome/browser/ui/bookmarks/bookmark_collection_view.h"
 
 #import <UIKit/UIGestureRecognizerSubclass.h>
 #include <algorithm>
 #include <map>
 #include <memory>
 
 #include "base/logging.h"
 #include "base/mac/bind_objc_block.h"
 #include "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/favicon/core/fallback_url_util.h"
 #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"
 #import "ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h"
 #import "ios/chrome/browser/ui/bookmarks/bookmark_collection_cells.h"
 #import "ios/chrome/browser/ui/bookmarks/bookmark_collection_view_background.h"
 #import "ios/chrome/browser/ui/bookmarks/bookmark_promo_cell.h"
 #import "ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h"
 #import "ios/chrome/browser/ui/bookmarks/bookmark_utils_ios.h"
 #include "ios/chrome/browser/ui/ui_util.h"
@@ skipping 34 matching lines @@
 CGFloat rowMarginTablet = 24.0;
 CGFloat rowHeight = 48.0;
 // Minimal acceptable favicon size, in points.
 CGFloat minFaviconSizePt = 16;
 
 // Delay in seconds to which the empty background view will be shown when the
 // collection view is empty.
 // 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,
                                      SigninPromoViewConsumer,
                                      UICollectionViewDataSource,
                                      UICollectionViewDelegateFlowLayout,
                                      UIGestureRecognizerDelegate> {
   // A vector of folders to display in the collection view.
   std::vector<const BookmarkNode*> _subFolders;
   // A vector of bookmark urls to display in the collection view.
@@ skipping 45 matching lines @@
 @synthesize folder = _folder;
 @synthesize bookmarkModel = _bookmarkModel;
 @synthesize collectionView = _collectionView;
 @synthesize editing = _editing;
 @synthesize emptyCollectionBackgroundView = _emptyCollectionBackgroundView;
 @synthesize loader = _loader;
 @synthesize longPressRecognizer = _longPressRecognizer;
 @synthesize browserState = _browserState;
 @synthesize shadow = _shadow;
 
++ (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry {
+  registry->RegisterIntegerPref(prefs::kIosBookmarkSigninPromoDisplayedCount,
+                                0);
+}
+
 #pragma mark - Initialization
 
 - (void)setupViews {
   self.backgroundColor = bookmark_utils_ios::mainBackgroundColor();
   UICollectionViewFlowLayout* layout =
       [[UICollectionViewFlowLayout alloc] init];
 
   UICollectionView* collectionView =
       [[UICollectionView alloc] initWithFrame:self.bounds
                          collectionViewLayout:layout];
@@ skipping 173 matching lines @@
   if (newPromoState != _promoVisible) {
     // This is awful, but until the old code to do the refresh when switching
     // in and out of edit mode is fixed, this is probably the cleanest thing to
     // do.
     _promoVisible = newPromoState;
     if (experimental_flags::IsSigninPromoEnabled()) {
       if (!_promoVisible) {
         _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 {
   return [self shouldShowPromoCell] ? 0 : -1;
 }
 
 - (NSInteger)folderSection {
   return [self shouldShowPromoCell] ? 1 : 0;
 }
 
@@ skipping 408 matching lines @@
     if (experimental_flags::IsSigninPromoEnabled()) {
       BookmarkSigninPromoCell* signinPromoCell = [self.collectionView
           dequeueReusableCellWithReuseIdentifier:[BookmarkSigninPromoCell
                                                      reuseIdentifier]
                                     forIndexPath:indexPath];
       signinPromoCell.signinPromoView.delegate = _signinPromoViewMediator;
       [[_signinPromoViewMediator createConfigurator]
           configureSigninPromoView:signinPromoCell.signinPromoView];
       __weak BookmarkCollectionView* weakSelf = self;
       signinPromoCell.closeButtonAction = ^() {
-        [weakSelf.delegate bookmarkCollectionViewDismissPromo:self];
+        [weakSelf signinPromoCloseButtonAction];
       };
       return signinPromoCell;
     } else {
       BookmarkPromoCell* promoCell = [self.collectionView
           dequeueReusableCellWithReuseIdentifier:[BookmarkPromoCell
                                                      reuseIdentifier]
                                     forIndexPath:indexPath];
       promoCell.delegate = self;
       return promoCell;
     }
   }
   const BookmarkNode* node = [self nodeAtIndexPath:indexPath];
 
   if (indexPath.section == self.folderSection)
     return [self cellForFolder:node indexPath:indexPath];
 
   BookmarkItemCell* cell = [self cellForBookmark:node indexPath:indexPath];
   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])
     return nil;
 
   BookmarkHeaderSeparatorView* view = [self.collectionView
       dequeueReusableSupplementaryViewOfKind:
           UICollectionElementKindSectionHeader
                          withReuseIdentifier:[BookmarkHeaderSeparatorView
                                                  reuseIdentifier]
@@ skipping 440 matching lines @@
 
 - (BOOL)shouldShowPromoCell {
   return _promoVisible;
 }
 
 - (BOOL)isPromoActive {
   return NO;
 }
 
 @end