[ios] Changes Bookmark cache to use Table row instead of offset

Bookmarks caches the tableView scroll offset, so whenever
bookmarks is closed and re-opened, we maintain the scroll offset
and the user is in the same position as before dismissal.

As part of supporting dynamic type on Bookmarks this approach is
no longer valid since:
-The row height might change due to the size of the font.
-The row height is not calculated unless the cell is dequeued,
this means that when the content offset is set to the cached value
(when re-oppening Bookmarks) the position offset is correct, but
the content being displayed are not. This happens because the cells
that weren't displayed when setting the offset manually used the
estimatedRowHeight instead of their real height. This messes up the
cache value.

In order to support dynamic height/type this CL changes the cache
so it stores the tableView row instead of the tableView scroll
offset. It also updates the pref values so they store an int instead
of a double.

It also deletes BookmarkHomeSharedState cellHeightPt and updates
the estimatedRowHeight value to 65.

This CL doesn't introduce dynamic height yet, that will be done in
a follow up CL.

Bug: 869671
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I6cd5f8baa8b385d3045ab5e9d0ff54a9d8202b63
Reviewed-on: https://chromium-review.googlesource.com/1157684
Commit-Queue: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587821}

ios/chrome/browser/pref_names.cc

@@ -64,8 +64,7 @@ const char kHttpServerProperties[] = "net.http_server_properties";
 const char kIosBookmarkCachedFolderId[] = "ios.bookmark.cached_folder_id";
 
 // Caches the scroll position of Bookmarks.
-const char kIosBookmarkCachedScrollPosition[] =
-    "ios.bookmark.cached_scroll_position";
+const char kIosBookmarkCachedTopMostRow[] = "ios.bookmark.cached_top_most_row";
 
 // Preference that keep information about where to create a new bookmark.
 const char kIosBookmarkFolderDefault[] = "ios.bookmark.default_folder";

ios/chrome/browser/pref_names.h

@@ -22,7 +22,7 @@ extern const char kDefaultCharset[];
 extern const char kEnableDoNotTrack[];
 extern const char kHttpServerProperties[];
 extern const char kIosBookmarkCachedFolderId[];
-extern const char kIosBookmarkCachedScrollPosition[];
+extern const char kIosBookmarkCachedTopMostRow[];
 extern const char kIosBookmarkFolderDefault[];
 extern const char kIosBookmarkPromoAlreadySeen[];
 extern const char kIosBookmarkSigninPromoDisplayedCount[];

ios/chrome/browser/ui/bookmarks/bookmark_home_shared_state.h

@@ -97,9 +97,6 @@ typedef NS_ENUM(NSInteger, BookmarkHomeItemType) {
 // Desired favicon size, in points.
 + (CGFloat)desiredFaviconSizePt;
 
-// Cell height, in points.
-+ (CGFloat)cellHeightPt;
-
 // Minimium spacing between keyboard and the titleText when creating new folder,
 // in points.
 + (CGFloat)keyboardSpacingPt;

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

@@ -19,9 +19,6 @@ const CGFloat kMinFaviconSizePt = 16.0;
 // Desired favicon size, in points.
 const CGFloat kDesiredFaviconSizePt = 32.0;
 
-// Cell height, in points.
-const CGFloat kCellHeightPt = 56.0;
-
 // Minimium spacing between keyboard and the titleText when creating new folder,
 // in points.
 const CGFloat kKeyboardSpacingPt = 16.0;
@@ -85,10 +82,6 @@ const NSUInteger kMaxDownloadFaviconCount = 50;
   return kDesiredFaviconSizePt;
 }
 
-+ (CGFloat)cellHeightPt {
-  return kCellHeightPt;
-}
-
 + (CGFloat)keyboardSpacingPt {
   return kKeyboardSpacingPt;
 }

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

@@ -92,6 +92,18 @@ const CGFloat kFallbackIconDefaultBackgroundColor = 0xf1f3f4;
 // Grayscale fallback favicon light gray text color.
 const CGFloat kFallbackIconDefaultTextColorWhitePercentage = 0.66;
 
+// Estimated TableView row height.
+const CGFloat kEstimatedRowHeight = 65.0;
+
+// TableView rows that are hidden by the NavigationBar, causing them to be
+// "visible" for the tableView but not for the user. This is used to calculate
+// the top most visibile table view indexPath row.
+// TODO(crbug.com/879001): This value is aproximate based on the standard (no
+// dynamic type) height. If the dynamic font is too large or too small it will
+// result in a small offset on the cache, in order to prevent this we need to
+// calculate this value dynamically.
+const int kRowsHiddenByNavigationBar = 2;
+
 // NetworkTrafficAnnotationTag for fetching favicon from a Google server.
 const net::NetworkTrafficAnnotationTag kTrafficAnnotation =
     net::DefineNetworkTrafficAnnotation("bookmarks_get_large_icon", R"(
@@ -128,6 +140,7 @@ std::vector<GURL> GetUrlsToOpen(const std::vector<const BookmarkNode*>& nodes) {
 const CGFloat kShadowOpacity = 0.12f;
 // Shadow radius for the NavigationController Toolbar.
 const CGFloat kShadowRadius = 12.0f;
+
 }  // namespace
 
 // An AlertCoordinator with the "Action Sheet" style that does not provide an
@@ -199,9 +212,9 @@ const CGFloat kShadowRadius = 12.0f;
 @property(nonatomic, assign) BookmarksContextBarState contextBarState;
 
 // When the view is first shown on the screen, this property represents the
-// cached value of the y of the content offset of the table view. This
+// cached value of the top most visible indexPath row of the table view. This
 // property is set to nil after it is used.
-@property(nonatomic, strong) NSNumber* cachedContentPosition;
+@property(nonatomic, assign) int cachedIndexPathRow;
 
 // Set to YES, only when this view controller instance is being created
 // from cached path. Once the view controller is shown, this is set to NO.
@@ -242,7 +255,7 @@ const CGFloat kShadowRadius = 12.0f;
 @synthesize homeDelegate = _homeDelegate;
 @synthesize contextBarState = _contextBarState;
 @synthesize dispatcher = _dispatcher;
-@synthesize cachedContentPosition = _cachedContentPosition;
+@synthesize cachedIndexPathRow = _cachedIndexPathRow;
 @synthesize isReconstructingFromCache = _isReconstructingFromCache;
 @synthesize sharedState = _sharedState;
 @synthesize mediator = _mediator;
@@ -311,15 +324,15 @@ const CGFloat kShadowRadius = 12.0f;
   [stack addObject:self];
 
   int64_t cachedFolderID;
-  double cachedScrollPosition;
+  int cachedIndexPathRow;
   // If cache is present then reconstruct the last visited bookmark from
   // cache.
   if (![BookmarkPathCache
-          getBookmarkUIPositionCacheWithPrefService:self.browserState
+          getBookmarkTopMostRowCacheWithPrefService:self.browserState
                                                         ->GetPrefs()
                                               model:self.bookmarks
                                            folderId:&cachedFolderID
-                                     scrollPosition:&cachedScrollPosition] ||
+                                         topMostRow:&cachedIndexPathRow] ||
       cachedFolderID == self.bookmarks->root_node()->id()) {
     return stack;
   }
@@ -351,9 +364,7 @@ const CGFloat kShadowRadius = 12.0f;
       [self setupNavigationForBookmarkHomeViewController:controller
                                        usingBookmarkNode:node];
     if (nodeID == cachedFolderID) {
-      [controller
-          setCachedContentPosition:[NSNumber
-                                       numberWithDouble:cachedScrollPosition]];
+      controller.cachedIndexPathRow = cachedIndexPathRow;
     }
     [stack addObject:controller];
   }
@@ -409,11 +420,16 @@ const CGFloat kShadowRadius = 12.0f;
 
 - (void)viewDidLayoutSubviews {
   [super viewDidLayoutSubviews];
-  // Set the content position after views are laid out, to ensure the right
-  // window of rows is shown. Once used, reset self.cachedContentPosition.
-  if (self.cachedContentPosition) {
-    [self setContentPosition:self.cachedContentPosition.floatValue];
-    self.cachedContentPosition = nil;
+  // Check that the tableView still contains as many rows, and that
+  // |self.cachedIndexPathRow| is not 0.
+  if (self.cachedIndexPathRow &&
+      [self.tableView numberOfRowsInSection:0] > self.cachedIndexPathRow) {
+    NSIndexPath* indexPath =
+        [NSIndexPath indexPathForRow:self.cachedIndexPathRow inSection:0];
+    [self.tableView scrollToRowAtIndexPath:indexPath
+                          atScrollPosition:UITableViewScrollPositionTop
+                                  animated:NO];
+    self.cachedIndexPathRow = 0;
   }
 }
 
@@ -456,8 +472,7 @@ const CGFloat kShadowRadius = 12.0f;
 
   // Configure the table view.
   self.sharedState.tableView.accessibilityIdentifier = @"bookmarksTableView";
-  self.sharedState.tableView.estimatedRowHeight =
-      [BookmarkHomeSharedState cellHeightPt];
+  self.sharedState.tableView.estimatedRowHeight = kEstimatedRowHeight;
   self.tableView.sectionHeaderHeight = 0;
   // Setting a sectionFooterHeight of 0 will be the same as not having a
   // footerView, which shows a cell separator for the last cell. Removing this
@@ -496,13 +511,13 @@ const CGFloat kShadowRadius = 12.0f;
   DCHECK([self isViewLoaded]);
 }
 
-- (void)cachePosition {
-  // Cache position for BookmarkTableView.
+- (void)cacheIndexPathRow {
+  // Cache IndexPathRow for BookmarkTableView.
+  int topMostVisibleIndexPathRow = [self topMostVisibleIndexPathRow];
   [BookmarkPathCache
-      cacheBookmarkUIPositionWithPrefService:self.browserState->GetPrefs()
+      cacheBookmarkTopMostRowWithPrefService:self.browserState->GetPrefs()
                                     folderId:_rootNode->id()
-                              scrollPosition:static_cast<double>(
-                                                 self.contentPosition)];
+                                  topMostRow:topMostVisibleIndexPathRow];
 }
 
 #pragma mark - BookmarkHomeConsumer
@@ -679,7 +694,7 @@ const CGFloat kShadowRadius = 12.0f;
 - (void)openAllNodes:(const std::vector<const bookmarks::BookmarkNode*>&)nodes
          inIncognito:(BOOL)inIncognito
               newTab:(BOOL)newTab {
-  [self cachePosition];
+  [self cacheIndexPathRow];
   std::vector<GURL> urls = GetUrlsToOpen(nodes);
   [self.homeDelegate bookmarkHomeViewControllerWantsDismissal:self
                                              navigationToUrls:urls
@@ -853,16 +868,16 @@ const CGFloat kShadowRadius = 12.0f;
     return;
 
   int64_t unusedFolderId;
-  double unusedScrollPosition;
+  int unusedIndexPathRow;
   // Bookmark Model is loaded after presenting Bookmarks,  we need to check
   // again here if restoring of cache position is needed.  It is to prevent
   // crbug.com/765503.
   if ([BookmarkPathCache
-          getBookmarkUIPositionCacheWithPrefService:self.browserState
+          getBookmarkTopMostRowCacheWithPrefService:self.browserState
                                                         ->GetPrefs()
                                               model:self.bookmarks
                                            folderId:&unusedFolderId
-                                     scrollPosition:&unusedScrollPosition]) {
+                                         topMostRow:&unusedIndexPathRow]) {
     self.isReconstructingFromCache = YES;
   }
 
@@ -995,7 +1010,7 @@ const CGFloat kShadowRadius = 12.0f;
 // Saves the current position and asks the delegate to open the url, if delegate
 // is set, otherwise opens the URL using loader.
 - (void)dismissWithURL:(const GURL&)url {
-  [self cachePosition];
+  [self cacheIndexPathRow];
   if (self.homeDelegate) {
     std::vector<GURL> urls;
     if (url.is_valid())
@@ -1123,23 +1138,27 @@ const CGFloat kShadowRadius = 12.0f;
   return self.sharedState.tableViewDisplayedRootNode != NULL;
 }
 
-- (CGFloat)contentPosition {
+- (int)topMostVisibleIndexPathRow {
+  // If on root node screen, return 0.
   if (self.sharedState.tableViewDisplayedRootNode ==
       self.sharedState.bookmarkModel->root_node()) {
     return 0;
   }
-  // Divided the scroll position by cell height so that it will stay correct in
-  // case the cell height is changed in future.
-  return self.sharedState.tableView.contentOffset.y /
-         [BookmarkHomeSharedState cellHeightPt];
-}
 
-- (void)setContentPosition:(CGFloat)position {
-  // The scroll position was divided by the cell height when stored.
-  [self.sharedState.tableView
-      setContentOffset:CGPointMake(
-                           0,
-                           position * [BookmarkHomeSharedState cellHeightPt])];
+  // If no rows in table, return 0.
+  NSArray* visibleIndexPaths = [self.tableView indexPathsForVisibleRows];
+  if (!visibleIndexPaths.count)
+    return 0;
+
+  // If the first row is still visible, return 0.
+  NSIndexPath* topMostIndexPath = [visibleIndexPaths objectAtIndex:0];
+  if (topMostIndexPath.row == 0)
+    return 0;
+
+  // Return the first visible row not covered by the NavigationBar.
+  topMostIndexPath =
+      [visibleIndexPaths objectAtIndex:kRowsHiddenByNavigationBar];
+  return topMostIndexPath.row;
 }
 
 - (void)navigateAway {
@@ -1871,7 +1890,7 @@ const CGFloat kShadowRadius = 12.0f;
   NSInteger sectionIdentifier = [self.sharedState.tableViewModel
       sectionIdentifierForSection:indexPath.section];
   if (sectionIdentifier == BookmarkHomeSectionIdentifierBookmarks) {
-    return [BookmarkHomeSharedState cellHeightPt];
+    return kEstimatedRowHeight;
   }
   return UITableViewAutomaticDimension;
 }

ios/chrome/browser/ui/bookmarks/bookmark_path_cache.h

@@ -17,28 +17,29 @@ class PrefRegistrySyncable;
 
 class PrefService;
 
-// Stores and retrieves the bookmark UI position that the user was last viewing.
+// Stores and retrieves the bookmark top most row that the user was last
+// viewing.
 @interface BookmarkPathCache : NSObject
 
 // Registers the feature preferences.
 + (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry;
 
-// Caches the bookmark UI position that the user was last viewing.
-+ (void)cacheBookmarkUIPositionWithPrefService:(PrefService*)prefService
+// Caches the bookmark top most row that the user was last viewing.
++ (void)cacheBookmarkTopMostRowWithPrefService:(PrefService*)prefService
                                       folderId:(int64_t)folderId
-                                scrollPosition:(double)scrollPosition;
+                                    topMostRow:(int)topMostRow;
 
-// Gets the bookmark UI position that the user was last viewing. Returns YES if
-// a valid cache exists. |folderId| and |scrollPosition| are out variables, only
+// Gets the bookmark top most row that the user was last viewing. Returns YES if
+// a valid cache exists. |folderId| and |topMostRow| are out variables, only
 // populated if the return is YES.
-+ (BOOL)getBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService
++ (BOOL)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
                                             model:
                                                 (bookmarks::BookmarkModel*)model
                                          folderId:(int64_t*)folderId
-                                   scrollPosition:(double*)scrollPosition;
+                                       topMostRow:(int*)topMostRow;
 
-// Clears the bookmark UI position cache.
-+ (void)clearBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService;
+// Clears the bookmark top most row cache.
++ (void)clearBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService;
 
 @end
 

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

@@ -23,29 +23,24 @@ const int64_t kFolderNone = -1;
 }  // namespace
 
 @implementation BookmarkPathCache
-// Registers the feature preferences.
+
 + (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry {
   registry->RegisterInt64Pref(prefs::kIosBookmarkCachedFolderId, kFolderNone);
-  registry->RegisterDoublePref(prefs::kIosBookmarkCachedScrollPosition, 0);
+  registry->RegisterIntegerPref(prefs::kIosBookmarkCachedTopMostRow, 0);
 }
 
-// Caches the bookmark UI position that the user was last viewing.
-+ (void)cacheBookmarkUIPositionWithPrefService:(PrefService*)prefService
++ (void)cacheBookmarkTopMostRowWithPrefService:(PrefService*)prefService
                                       folderId:(int64_t)folderId
-                                scrollPosition:(double)scrollPosition {
+                                    topMostRow:(int)topMostRow {
   prefService->SetInt64(prefs::kIosBookmarkCachedFolderId, folderId);
-  prefService->SetDouble(prefs::kIosBookmarkCachedScrollPosition,
-                         scrollPosition);
+  prefService->SetInteger(prefs::kIosBookmarkCachedTopMostRow, topMostRow);
 }
 
-// Gets the bookmark UI position that the user was last viewing. Returns YES if
-// a valid cache exists. |folderId| and |scrollPosition| are out variables, only
-// populated if the return is YES.
-+ (BOOL)getBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService
++ (BOOL)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
                                             model:
                                                 (bookmarks::BookmarkModel*)model
                                          folderId:(int64_t*)folderId
-                                   scrollPosition:(double*)scrollPosition {
+                                       topMostRow:(int*)topMostRow {
   *folderId = prefService->GetInt64(prefs::kIosBookmarkCachedFolderId);
 
   // If the cache was at root node, consider it as nothing was cached.
@@ -59,13 +54,11 @@ const int64_t kFolderNone = -1;
   if (!bookmark)
     return NO;
 
-  *scrollPosition =
-      prefService->GetDouble(prefs::kIosBookmarkCachedScrollPosition);
+  *topMostRow = prefService->GetInteger(prefs::kIosBookmarkCachedTopMostRow);
   return YES;
 }
 
-// Clears the bookmark UI position cache.
-+ (void)clearBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService {
++ (void)clearBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService {
   prefService->SetInt64(prefs::kIosBookmarkCachedFolderId, kFolderNone);
 }
 

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

@@ -34,19 +34,20 @@ TEST_F(BookmarkPathCacheTest, TestPathCache) {
   const BookmarkNode* mobileNode = _bookmarkModel->mobile_node();
   const BookmarkNode* f1 = AddFolder(mobileNode, @"f1");
   int64_t folderId = f1->id();
-  double position = 23;
-  [BookmarkPathCache cacheBookmarkUIPositionWithPrefService:&prefs_
+  int topMostRow = 23;
+  [BookmarkPathCache cacheBookmarkTopMostRowWithPrefService:&prefs_
                                                    folderId:folderId
-                                             scrollPosition:position];
+                                                 topMostRow:topMostRow];
 
   int64_t resultFolderId;
-  double resultPosition;
-  [BookmarkPathCache getBookmarkUIPositionCacheWithPrefService:&prefs_
-                                                         model:_bookmarkModel
-                                                      folderId:&resultFolderId
-                                                scrollPosition:&resultPosition];
+  int resultTopMostRow;
+  [BookmarkPathCache
+      getBookmarkTopMostRowCacheWithPrefService:&prefs_
+                                          model:_bookmarkModel
+                                       folderId:&resultFolderId
+                                     topMostRow:&resultTopMostRow];
   EXPECT_EQ(folderId, resultFolderId);
-  EXPECT_EQ(position, resultPosition);
+  EXPECT_EQ(topMostRow, resultTopMostRow);
 }
 
 TEST_F(BookmarkPathCacheTest, TestPathCacheWhenFolderDeleted) {
@@ -54,21 +55,21 @@ TEST_F(BookmarkPathCacheTest, TestPathCacheWhenFolderDeleted) {
   const BookmarkNode* mobileNode = _bookmarkModel->mobile_node();
   const BookmarkNode* f1 = AddFolder(mobileNode, @"f1");
   int64_t folderId = f1->id();
-  double position = 23;
-  [BookmarkPathCache cacheBookmarkUIPositionWithPrefService:&prefs_
+  int topMostRow = 23;
+  [BookmarkPathCache cacheBookmarkTopMostRowWithPrefService:&prefs_
                                                    folderId:folderId
-                                             scrollPosition:position];
+                                                 topMostRow:topMostRow];
 
   // Delete the folder.
   _bookmarkModel->Remove(f1);
 
   int64_t unusedFolderId;
-  double unusedPosition;
+  int unusedTopMostRow;
   BOOL result = [BookmarkPathCache
-      getBookmarkUIPositionCacheWithPrefService:&prefs_
+      getBookmarkTopMostRowCacheWithPrefService:&prefs_
                                           model:_bookmarkModel
                                        folderId:&unusedFolderId
-                                 scrollPosition:&unusedPosition];
+                                     topMostRow:&unusedTopMostRow];
   ASSERT_FALSE(result);
 }
 

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

@@ -206,7 +206,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
   ios::ChromeBrowserState* browser_state =
       chrome_test_util::GetOriginalBrowserState();
   [BookmarkPathCache
-      clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
+      clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
 }
 
 #pragma mark - BookmarksTestCase Tests
@@ -1785,7 +1785,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
   ios::ChromeBrowserState* browser_state =
       chrome_test_util::GetOriginalBrowserState();
   [BookmarkPathCache
-      clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
+      clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
 }
 
 #pragma mark - BookmarksTestCaseEntries Tests
@@ -2795,7 +2795,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
   ios::ChromeBrowserState* browser_state =
       chrome_test_util::GetOriginalBrowserState();
   [BookmarkPathCache
-      clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
+      clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
 }
 
 #pragma mark - BookmarksTestCasePromo Tests
@@ -3009,7 +3009,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
   ios::ChromeBrowserState* browser_state =
       chrome_test_util::GetOriginalBrowserState();
   [BookmarkPathCache
-      clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
+      clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
 }
 
 #pragma mark - BookmarksTestCaseAccessibility Tests
@@ -3185,7 +3185,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
   ios::ChromeBrowserState* browser_state =
       chrome_test_util::GetOriginalBrowserState();
   [BookmarkPathCache
-      clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
+      clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
 }
 
 #pragma mark - BookmarksTestFolders Tests