Commit 334e04d7 authored by calamity's avatar calamity Committed by Commit Bot

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

This reverts commit 7f98222a.

Reason for revert: Suspected of causing BookmarksTestCase/testCachePositionIsRecreated to fail.

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-uirefresh-simulator/2187


Original change's description:
> [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}

TBR=marq@chromium.org,kkhorimoto@chromium.org,sczs@chromium.org

Change-Id: I14635c3f977857714a76feefae6535d8df0cf35b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 869671
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/1198647Reviewed-by: default avatarcalamity <calamity@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587948}
parent 9b94b578
......@@ -64,7 +64,8 @@ const char kHttpServerProperties[] = "net.http_server_properties";
const char kIosBookmarkCachedFolderId[] = "ios.bookmark.cached_folder_id";
// Caches the scroll position of Bookmarks.
const char kIosBookmarkCachedTopMostRow[] = "ios.bookmark.cached_top_most_row";
const char kIosBookmarkCachedScrollPosition[] =
"ios.bookmark.cached_scroll_position";
// Preference that keep information about where to create a new bookmark.
const char kIosBookmarkFolderDefault[] = "ios.bookmark.default_folder";
......
......@@ -22,7 +22,7 @@ extern const char kDefaultCharset[];
extern const char kEnableDoNotTrack[];
extern const char kHttpServerProperties[];
extern const char kIosBookmarkCachedFolderId[];
extern const char kIosBookmarkCachedTopMostRow[];
extern const char kIosBookmarkCachedScrollPosition[];
extern const char kIosBookmarkFolderDefault[];
extern const char kIosBookmarkPromoAlreadySeen[];
extern const char kIosBookmarkSigninPromoDisplayedCount[];
......
......@@ -97,6 +97,9 @@ 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;
......
......@@ -19,6 +19,9 @@ 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;
......@@ -82,6 +85,10 @@ const NSUInteger kMaxDownloadFaviconCount = 50;
return kDesiredFaviconSizePt;
}
+ (CGFloat)cellHeightPt {
return kCellHeightPt;
}
+ (CGFloat)keyboardSpacingPt {
return kKeyboardSpacingPt;
}
......
......@@ -92,18 +92,6 @@ 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"(
......@@ -140,7 +128,6 @@ 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
......@@ -212,9 +199,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 top most visible indexPath row of the table view. This
// cached value of the y of the content offset of the table view. This
// property is set to nil after it is used.
@property(nonatomic, assign) int cachedIndexPathRow;
@property(nonatomic, strong) NSNumber* cachedContentPosition;
// 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.
......@@ -255,7 +242,7 @@ const CGFloat kShadowRadius = 12.0f;
@synthesize homeDelegate = _homeDelegate;
@synthesize contextBarState = _contextBarState;
@synthesize dispatcher = _dispatcher;
@synthesize cachedIndexPathRow = _cachedIndexPathRow;
@synthesize cachedContentPosition = _cachedContentPosition;
@synthesize isReconstructingFromCache = _isReconstructingFromCache;
@synthesize sharedState = _sharedState;
@synthesize mediator = _mediator;
......@@ -324,15 +311,15 @@ const CGFloat kShadowRadius = 12.0f;
[stack addObject:self];
int64_t cachedFolderID;
int cachedIndexPathRow;
double cachedScrollPosition;
// If cache is present then reconstruct the last visited bookmark from
// cache.
if (![BookmarkPathCache
getBookmarkTopMostRowCacheWithPrefService:self.browserState
getBookmarkUIPositionCacheWithPrefService:self.browserState
->GetPrefs()
model:self.bookmarks
folderId:&cachedFolderID
topMostRow:&cachedIndexPathRow] ||
scrollPosition:&cachedScrollPosition] ||
cachedFolderID == self.bookmarks->root_node()->id()) {
return stack;
}
......@@ -364,7 +351,9 @@ const CGFloat kShadowRadius = 12.0f;
[self setupNavigationForBookmarkHomeViewController:controller
usingBookmarkNode:node];
if (nodeID == cachedFolderID) {
controller.cachedIndexPathRow = cachedIndexPathRow;
[controller
setCachedContentPosition:[NSNumber
numberWithDouble:cachedScrollPosition]];
}
[stack addObject:controller];
}
......@@ -420,16 +409,11 @@ const CGFloat kShadowRadius = 12.0f;
- (void)viewDidLayoutSubviews {
[super viewDidLayoutSubviews];
// 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;
// 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;
}
}
......@@ -472,7 +456,8 @@ const CGFloat kShadowRadius = 12.0f;
// Configure the table view.
self.sharedState.tableView.accessibilityIdentifier = @"bookmarksTableView";
self.sharedState.tableView.estimatedRowHeight = kEstimatedRowHeight;
self.sharedState.tableView.estimatedRowHeight =
[BookmarkHomeSharedState cellHeightPt];
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
......@@ -511,13 +496,13 @@ const CGFloat kShadowRadius = 12.0f;
DCHECK([self isViewLoaded]);
}
- (void)cacheIndexPathRow {
// Cache IndexPathRow for BookmarkTableView.
int topMostVisibleIndexPathRow = [self topMostVisibleIndexPathRow];
- (void)cachePosition {
// Cache position for BookmarkTableView.
[BookmarkPathCache
cacheBookmarkTopMostRowWithPrefService:self.browserState->GetPrefs()
cacheBookmarkUIPositionWithPrefService:self.browserState->GetPrefs()
folderId:_rootNode->id()
topMostRow:topMostVisibleIndexPathRow];
scrollPosition:static_cast<double>(
self.contentPosition)];
}
#pragma mark - BookmarkHomeConsumer
......@@ -694,7 +679,7 @@ const CGFloat kShadowRadius = 12.0f;
- (void)openAllNodes:(const std::vector<const bookmarks::BookmarkNode*>&)nodes
inIncognito:(BOOL)inIncognito
newTab:(BOOL)newTab {
[self cacheIndexPathRow];
[self cachePosition];
std::vector<GURL> urls = GetUrlsToOpen(nodes);
[self.homeDelegate bookmarkHomeViewControllerWantsDismissal:self
navigationToUrls:urls
......@@ -868,16 +853,16 @@ const CGFloat kShadowRadius = 12.0f;
return;
int64_t unusedFolderId;
int unusedIndexPathRow;
double unusedScrollPosition;
// 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
getBookmarkTopMostRowCacheWithPrefService:self.browserState
getBookmarkUIPositionCacheWithPrefService:self.browserState
->GetPrefs()
model:self.bookmarks
folderId:&unusedFolderId
topMostRow:&unusedIndexPathRow]) {
scrollPosition:&unusedScrollPosition]) {
self.isReconstructingFromCache = YES;
}
......@@ -1010,7 +995,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 cacheIndexPathRow];
[self cachePosition];
if (self.homeDelegate) {
std::vector<GURL> urls;
if (url.is_valid())
......@@ -1138,27 +1123,23 @@ const CGFloat kShadowRadius = 12.0f;
return self.sharedState.tableViewDisplayedRootNode != NULL;
}
- (int)topMostVisibleIndexPathRow {
// If on root node screen, return 0.
- (CGFloat)contentPosition {
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];
}
// 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)setContentPosition:(CGFloat)position {
// The scroll position was divided by the cell height when stored.
[self.sharedState.tableView
setContentOffset:CGPointMake(
0,
position * [BookmarkHomeSharedState cellHeightPt])];
}
- (void)navigateAway {
......@@ -1890,7 +1871,7 @@ const CGFloat kShadowRadius = 12.0f;
NSInteger sectionIdentifier = [self.sharedState.tableViewModel
sectionIdentifierForSection:indexPath.section];
if (sectionIdentifier == BookmarkHomeSectionIdentifierBookmarks) {
return kEstimatedRowHeight;
return [BookmarkHomeSharedState cellHeightPt];
}
return UITableViewAutomaticDimension;
}
......
......@@ -17,29 +17,28 @@ class PrefRegistrySyncable;
class PrefService;
// Stores and retrieves the bookmark top most row that the user was last
// viewing.
// Stores and retrieves the bookmark UI position that the user was last viewing.
@interface BookmarkPathCache : NSObject
// Registers the feature preferences.
+ (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry;
// Caches the bookmark top most row that the user was last viewing.
+ (void)cacheBookmarkTopMostRowWithPrefService:(PrefService*)prefService
// Caches the bookmark UI position that the user was last viewing.
+ (void)cacheBookmarkUIPositionWithPrefService:(PrefService*)prefService
folderId:(int64_t)folderId
topMostRow:(int)topMostRow;
scrollPosition:(double)scrollPosition;
// 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
// 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)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
+ (BOOL)getBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService
model:
(bookmarks::BookmarkModel*)model
folderId:(int64_t*)folderId
topMostRow:(int*)topMostRow;
scrollPosition:(double*)scrollPosition;
// Clears the bookmark top most row cache.
+ (void)clearBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService;
// Clears the bookmark UI position cache.
+ (void)clearBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService;
@end
......
......@@ -23,24 +23,29 @@ const int64_t kFolderNone = -1;
} // namespace
@implementation BookmarkPathCache
// Registers the feature preferences.
+ (void)registerBrowserStatePrefs:(user_prefs::PrefRegistrySyncable*)registry {
registry->RegisterInt64Pref(prefs::kIosBookmarkCachedFolderId, kFolderNone);
registry->RegisterIntegerPref(prefs::kIosBookmarkCachedTopMostRow, 0);
registry->RegisterDoublePref(prefs::kIosBookmarkCachedScrollPosition, 0);
}
+ (void)cacheBookmarkTopMostRowWithPrefService:(PrefService*)prefService
// Caches the bookmark UI position that the user was last viewing.
+ (void)cacheBookmarkUIPositionWithPrefService:(PrefService*)prefService
folderId:(int64_t)folderId
topMostRow:(int)topMostRow {
scrollPosition:(double)scrollPosition {
prefService->SetInt64(prefs::kIosBookmarkCachedFolderId, folderId);
prefService->SetInteger(prefs::kIosBookmarkCachedTopMostRow, topMostRow);
prefService->SetDouble(prefs::kIosBookmarkCachedScrollPosition,
scrollPosition);
}
+ (BOOL)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
// 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
model:
(bookmarks::BookmarkModel*)model
folderId:(int64_t*)folderId
topMostRow:(int*)topMostRow {
scrollPosition:(double*)scrollPosition {
*folderId = prefService->GetInt64(prefs::kIosBookmarkCachedFolderId);
// If the cache was at root node, consider it as nothing was cached.
......@@ -54,11 +59,13 @@ const int64_t kFolderNone = -1;
if (!bookmark)
return NO;
*topMostRow = prefService->GetInteger(prefs::kIosBookmarkCachedTopMostRow);
*scrollPosition =
prefService->GetDouble(prefs::kIosBookmarkCachedScrollPosition);
return YES;
}
+ (void)clearBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService {
// Clears the bookmark UI position cache.
+ (void)clearBookmarkUIPositionCacheWithPrefService:(PrefService*)prefService {
prefService->SetInt64(prefs::kIosBookmarkCachedFolderId, kFolderNone);
}
......
......@@ -34,20 +34,19 @@ TEST_F(BookmarkPathCacheTest, TestPathCache) {
const BookmarkNode* mobileNode = _bookmarkModel->mobile_node();
const BookmarkNode* f1 = AddFolder(mobileNode, @"f1");
int64_t folderId = f1->id();
int topMostRow = 23;
[BookmarkPathCache cacheBookmarkTopMostRowWithPrefService:&prefs_
double position = 23;
[BookmarkPathCache cacheBookmarkUIPositionWithPrefService:&prefs_
folderId:folderId
topMostRow:topMostRow];
scrollPosition:position];
int64_t resultFolderId;
int resultTopMostRow;
[BookmarkPathCache
getBookmarkTopMostRowCacheWithPrefService:&prefs_
model:_bookmarkModel
folderId:&resultFolderId
topMostRow:&resultTopMostRow];
double resultPosition;
[BookmarkPathCache getBookmarkUIPositionCacheWithPrefService:&prefs_
model:_bookmarkModel
folderId:&resultFolderId
scrollPosition:&resultPosition];
EXPECT_EQ(folderId, resultFolderId);
EXPECT_EQ(topMostRow, resultTopMostRow);
EXPECT_EQ(position, resultPosition);
}
TEST_F(BookmarkPathCacheTest, TestPathCacheWhenFolderDeleted) {
......@@ -55,21 +54,21 @@ TEST_F(BookmarkPathCacheTest, TestPathCacheWhenFolderDeleted) {
const BookmarkNode* mobileNode = _bookmarkModel->mobile_node();
const BookmarkNode* f1 = AddFolder(mobileNode, @"f1");
int64_t folderId = f1->id();
int topMostRow = 23;
[BookmarkPathCache cacheBookmarkTopMostRowWithPrefService:&prefs_
double position = 23;
[BookmarkPathCache cacheBookmarkUIPositionWithPrefService:&prefs_
folderId:folderId
topMostRow:topMostRow];
scrollPosition:position];
// Delete the folder.
_bookmarkModel->Remove(f1);
int64_t unusedFolderId;
int unusedTopMostRow;
double unusedPosition;
BOOL result = [BookmarkPathCache
getBookmarkTopMostRowCacheWithPrefService:&prefs_
getBookmarkUIPositionCacheWithPrefService:&prefs_
model:_bookmarkModel
folderId:&unusedFolderId
topMostRow:&unusedTopMostRow];
scrollPosition:&unusedPosition];
ASSERT_FALSE(result);
}
......
......@@ -206,7 +206,7 @@ id<GREYMatcher> TappableBookmarkNodeWithLabel(NSString* label) {
ios::ChromeBrowserState* browser_state =
chrome_test_util::GetOriginalBrowserState();
[BookmarkPathCache
clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
clearBookmarkUIPositionCacheWithPrefService: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
clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
clearBookmarkUIPositionCacheWithPrefService: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
clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
clearBookmarkUIPositionCacheWithPrefService: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
clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
clearBookmarkUIPositionCacheWithPrefService: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
clearBookmarkTopMostRowCacheWithPrefService:browser_state->GetPrefs()];
clearBookmarkUIPositionCacheWithPrefService:browser_state->GetPrefs()];
}
#pragma mark - BookmarksTestFolders Tests
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment