[ios] Migrate BookmarkPathCache away from LegacyBookmarkModel
Usage of LegacyBookmarkModel is getting replaced with
bookmarks::BookmarkModel. The specific case of BookmarkPathCache,
tackled in this patch, implies that the bookmark's node ID is sufficient
to uniquely identify a bookmark node, as there is exactly one
BookmarkModel instance (as opposed to having two, which was the case
until recent cleanups).
This also means the second preference, namely
"ios.bookmark.cached_folder_model", becomes unused. It is still kept
around and written to as a safety net and rollback path in case recent code changes run into issues.
On the calling site, in BookmarksHomeViewController,
LegacyBookmarkModel continues to be used as the migration is
complex, to be tackled in future patches. Temporarily, it exercises
LegacyBookmarkModel::underlying_model() to access the underlying
bookmarks::BookmarkModel instance, requiring that the two injected
LegacyBookmarkModel instances share the very same underlying
BookmarkModel (which is guaranteed in practice).
Bug: 346918509
Change-Id: Ife26ea58b35260fae1f38afa4d1cba54d868aa01
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5678973
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1324125}
diff --git a/ios/chrome/browser/bookmarks/model/bookmarks_utils.cc b/ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
index 7f9535f8..49c7913d 100644
--- a/ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
+++ b/ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
@@ -122,12 +122,6 @@
return nodes;
}
-bool IsPrimaryPermanentNode(const BookmarkNode* node,
- LegacyBookmarkModel* model) {
- std::vector<const BookmarkNode*> primary_nodes(PrimaryPermanentNodes(model));
- return base::Contains(primary_nodes, node);
-}
-
bool IsLastUsedBookmarkFolderSet(PrefService* prefs) {
return prefs->GetInt64(prefs::kIosBookmarkLastUsedFolderReceivingBookmarks) ==
kLastUsedBookmarkFolderNone;
diff --git a/ios/chrome/browser/bookmarks/model/bookmarks_utils.h b/ios/chrome/browser/bookmarks/model/bookmarks_utils.h
index 5d76d04..640af35d 100644
--- a/ios/chrome/browser/bookmarks/model/bookmarks_utils.h
+++ b/ios/chrome/browser/bookmarks/model/bookmarks_utils.h
@@ -65,11 +65,6 @@
std::vector<const bookmarks::BookmarkNode*> PrimaryPermanentNodes(
LegacyBookmarkModel* model);
-// Returns whether `node` is a primary permanent node in the sense of
-// `PrimaryPermanentNodes`.
-bool IsPrimaryPermanentNode(const bookmarks::BookmarkNode* node,
- LegacyBookmarkModel* model);
-
// Whether a bookmark was manually moved by the user to a different folder since
// last signin/signout.
bool IsLastUsedBookmarkFolderSet(PrefService* prefs);
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h
index 762b948..7705419 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h
@@ -8,9 +8,12 @@
#import <UIKit/UIKit.h>
enum class BookmarkModelType;
-class LegacyBookmarkModel;
class PrefService;
+namespace bookmarks {
+class BookmarkModel;
+} // namespace bookmarks
+
namespace user_prefs {
class PrefRegistrySyncable;
} // namespace user_prefs
@@ -31,14 +34,11 @@
// 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)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
- localOrSyncableModel:
- (LegacyBookmarkModel*)localOrSyncableModel
- accountModel:
- (LegacyBookmarkModel*)accountModel
- folderId:(int64_t*)folderId
- modelType:(BookmarkModelType*)modelType
- topMostRow:(int*)topMostRow;
++ (BOOL)bookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
+ bookmarkModel:
+ (const bookmarks::BookmarkModel*)model
+ folderId:(int64_t*)folderId
+ topMostRow:(int*)topMostRow;
// Clears the bookmark top most row cache.
+ (void)clearBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService;
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.mm b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.mm
index 1201256..6621a6d 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.mm
@@ -4,13 +4,13 @@
#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h"
+#import "components/bookmarks/browser/bookmark_model.h"
#import "components/bookmarks/browser/bookmark_node.h"
#import "components/pref_registry/pref_registry_syncable.h"
#import "components/prefs/pref_service.h"
#import "ios/chrome/browser/bookmarks/model/bookmark_model_type.h"
-#import "ios/chrome/browser/bookmarks/model/legacy_bookmark_model.h"
-#import "ios/chrome/browser/shared/model/prefs/pref_names.h"
#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h"
+#import "ios/chrome/browser/shared/model/prefs/pref_names.h"
using bookmarks::BookmarkNode;
@@ -38,33 +38,22 @@
prefService->SetInteger(prefs::kIosBookmarkCachedTopMostRow, topMostRow);
}
-+ (BOOL)getBookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
- localOrSyncableModel:
- (LegacyBookmarkModel*)localOrSyncableModel
- accountModel:
- (LegacyBookmarkModel*)accountModel
- folderId:(int64_t*)folderId
- modelType:(BookmarkModelType*)modelType
- topMostRow:(int*)topMostRow {
++ (BOOL)bookmarkTopMostRowCacheWithPrefService:(PrefService*)prefService
+ bookmarkModel:(const bookmarks::BookmarkModel*)
+ bookmarkModel
+ folderId:(int64_t*)folderId
+ topMostRow:(int*)topMostRow {
*folderId = prefService->GetInt64(prefs::kIosBookmarkCachedFolderId);
- *modelType = static_cast<BookmarkModelType>(
- prefService->GetInt64(prefs::kIosBookmarkCachedFolderModel));
- LegacyBookmarkModel* model;
- if (*modelType == BookmarkModelType::kLocalOrSyncable) {
- model = localOrSyncableModel;
- } else {
- model = accountModel;
- }
// If the cache was at root node, consider it as nothing was cached.
if (*folderId == kFolderNone ||
- *folderId == model->subtle_root_node_with_unspecified_children()->id()) {
+ *folderId == bookmarkModel->root_node()->id()) {
return NO;
}
// Create bookmark Path.
const BookmarkNode* bookmark =
- bookmark_utils_ios::FindFolderById(model, *folderId);
+ bookmark_utils_ios::FindFolderById(bookmarkModel, *folderId);
// The bookmark node is gone from model, maybe deleted remotely.
if (!bookmark) {
return NO;
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache_unittest.mm b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache_unittest.mm
index 48b1f6c..e607b6de 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache_unittest.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache_unittest.mm
@@ -4,12 +4,12 @@
#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h"
+#import "components/bookmarks/browser/bookmark_model.h"
#import "components/bookmarks/browser/bookmark_node.h"
#import "components/bookmarks/common/bookmark_metrics.h"
#import "components/sync_preferences/testing_pref_service_syncable.h"
#import "ios/chrome/browser/bookmarks/model/bookmark_ios_unit_test_support.h"
#import "ios/chrome/browser/bookmarks/model/bookmark_model_type.h"
-#import "ios/chrome/browser/bookmarks/model/legacy_bookmark_model.h"
#import "testing/gtest/include/gtest/gtest.h"
#import "testing/gtest_mac.h"
#import "testing/platform_test.h"
@@ -30,8 +30,7 @@
TEST_F(BookmarkPathCacheTest, TestPathCache) {
// Try to store and retrieve a cache.
- const BookmarkNode* mobile_node =
- local_or_syncable_bookmark_model_->subtle_mobile_node();
+ const BookmarkNode* mobile_node = bookmark_model_->mobile_node();
const BookmarkNode* f1 = AddFolder(mobile_node, u"f1");
int64_t folder_id = f1->id();
int topmost_row = 23;
@@ -42,25 +41,19 @@
topMostRow:topmost_row];
int64_t result_folder_id;
- BookmarkModelType model_type;
int result_topmost_row;
[BookmarkPathCache
- getBookmarkTopMostRowCacheWithPrefService:&prefs_
- localOrSyncableModel:
- local_or_syncable_bookmark_model_
- accountModel:account_bookmark_model_
- folderId:&result_folder_id
- modelType:&model_type
- topMostRow:&result_topmost_row];
+ bookmarkTopMostRowCacheWithPrefService:&prefs_
+ bookmarkModel:bookmark_model_
+ folderId:&result_folder_id
+ topMostRow:&result_topmost_row];
EXPECT_EQ(folder_id, result_folder_id);
EXPECT_EQ(topmost_row, result_topmost_row);
- EXPECT_EQ(model_type, BookmarkModelType::kLocalOrSyncable);
}
TEST_F(BookmarkPathCacheTest, TestPathCacheWhenFolderDeleted) {
// Try to store and retrieve a cache after the cached path is deleted.
- const BookmarkNode* mobile_node =
- local_or_syncable_bookmark_model_->subtle_mobile_node();
+ const BookmarkNode* mobile_node = bookmark_model_->mobile_node();
const BookmarkNode* f1 = AddFolder(mobile_node, u"f1");
int64_t folder_id = f1->id();
int topmost_row = 23;
@@ -71,20 +64,16 @@
topMostRow:topmost_row];
// Delete the folder.
- local_or_syncable_bookmark_model_->Remove(
- f1, bookmarks::metrics::BookmarkEditSource::kOther, FROM_HERE);
+ bookmark_model_->Remove(f1, bookmarks::metrics::BookmarkEditSource::kOther,
+ FROM_HERE);
int64_t unused_folder_id;
- BookmarkModelType model_type;
int unused_topmost_row;
BOOL result = [BookmarkPathCache
- getBookmarkTopMostRowCacheWithPrefService:&prefs_
- localOrSyncableModel:
- local_or_syncable_bookmark_model_
- accountModel:account_bookmark_model_
- folderId:&unused_folder_id
- modelType:&model_type
- topMostRow:&unused_topmost_row];
+ bookmarkTopMostRowCacheWithPrefService:&prefs_
+ bookmarkModel:bookmark_model_
+ folderId:&unused_folder_id
+ topMostRow:&unused_topmost_row];
ASSERT_FALSE(result);
}
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h
index 6bf48a9..7c6c4d3 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h
@@ -70,14 +70,11 @@
NodeSet FindNodesByNodeReferences(const bookmarks::BookmarkModel* model,
const NodeReferenceSet& references);
-// Finds bookmark node passed in `id`, in the `model`.
-const bookmarks::BookmarkNode* FindNodeById(LegacyBookmarkModel* model,
- int64_t id);
-
// Finds bookmark node passed in `id`, in the `model`. Returns null if the
// node is found but not a folder.
-const bookmarks::BookmarkNode* FindFolderById(LegacyBookmarkModel* model,
- int64_t id);
+const bookmarks::BookmarkNode* FindFolderById(
+ const bookmarks::BookmarkModel* model,
+ int64_t id);
// The iOS code is doing some munging of the bookmark folder names in order
// to display a slighly different wording for the default folders.
@@ -312,7 +309,7 @@
// MobileBookmarks (3) --> Test1(76) will be returned as [3, 76], where the
// first element always represents a permanent folder. Returns nullptr if the
// folder is not found or is the root node.
-NSArray<NSNumber*>* CreateBookmarkPath(LegacyBookmarkModel* model,
+NSArray<NSNumber*>* CreateBookmarkPath(const bookmarks::BookmarkModel* model,
int64_t folder_id);
// Converts NSString entered by the user to a GURL.
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm
index f383759..986acbd 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm
@@ -134,13 +134,11 @@
return nodes;
}
-const BookmarkNode* FindNodeById(LegacyBookmarkModel* model, int64_t id) {
- DCHECK(model);
- return model->GetNodeById(id);
-}
-
-const BookmarkNode* FindFolderById(LegacyBookmarkModel* model, int64_t id) {
- const BookmarkNode* node = FindNodeById(model, id);
+const BookmarkNode* FindFolderById(const bookmarks::BookmarkModel* model,
+ int64_t id) {
+ CHECK(model);
+ const bookmarks::BookmarkNode* node =
+ bookmarks::GetBookmarkNodeByID(model, id);
return (node && node->is_folder()) ? node : nullptr;
}
@@ -844,7 +842,7 @@
#pragma mark - Cache position in table view.
-NSArray<NSNumber*>* CreateBookmarkPath(LegacyBookmarkModel* model,
+NSArray<NSNumber*>* CreateBookmarkPath(const bookmarks::BookmarkModel* model,
int64_t folder_id) {
const BookmarkNode* bookmark = FindFolderById(model, folder_id);
if (!bookmark || bookmark->is_root()) {
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios_unittest.mm b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios_unittest.mm
index e4044df..eb5a71a 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios_unittest.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios_unittest.mm
@@ -217,11 +217,9 @@
}
TEST_F(BookmarkIOSUtilsUnitTest, TestCreateBookmarkPath) {
- const BookmarkNode* mobileNode =
- local_or_syncable_bookmark_model_->subtle_mobile_node();
+ const BookmarkNode* mobileNode = bookmark_model_->mobile_node();
const BookmarkNode* f1 = AddFolder(mobileNode, u"f1");
- NSArray<NSNumber*>* path =
- CreateBookmarkPath(local_or_syncable_bookmark_model_, f1->id());
+ NSArray<NSNumber*>* path = CreateBookmarkPath(bookmark_model_, f1->id());
NSMutableArray<NSNumber*>* expectedPath = [NSMutableArray array];
[expectedPath addObject:[NSNumber numberWithLongLong:mobileNode->id()]];
[expectedPath addObject:[NSNumber numberWithLongLong:f1->id()]];
@@ -229,8 +227,7 @@
}
TEST_F(BookmarkIOSUtilsUnitTest, TestCreateNilBookmarkPath) {
- NSArray<NSNumber*>* path =
- CreateBookmarkPath(local_or_syncable_bookmark_model_, 999);
+ NSArray<NSNumber*>* path = CreateBookmarkPath(bookmark_model_, 999);
EXPECT_TRUE(path == nil);
}
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.mm b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.mm
index c166e6f..ad579c5 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.mm
@@ -29,6 +29,13 @@
#import "ios/chrome/browser/bookmarks/model/bookmarks_utils.h"
#import "ios/chrome/browser/bookmarks/model/legacy_bookmark_model.h"
#import "ios/chrome/browser/bookmarks/model/managed_bookmark_service_factory.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_ui_constants.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_home_node_item.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_table_cell_title_editing.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmark_promo_controller.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_consumer.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/synced_bookmarks_bridge.h"
#import "ios/chrome/browser/shared/model/browser/browser.h"
#import "ios/chrome/browser/shared/model/browser_state/chrome_browser_state.h"
#import "ios/chrome/browser/shared/model/prefs/pref_names.h"
@@ -45,13 +52,6 @@
#import "ios/chrome/browser/ui/authentication/cells/table_view_signin_promo_item.h"
#import "ios/chrome/browser/ui/authentication/signin_presenter.h"
#import "ios/chrome/browser/ui/authentication/signin_promo_view_mediator.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_ui_constants.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_home_node_item.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_table_cell_title_editing.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmark_promo_controller.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_consumer.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/synced_bookmarks_bridge.h"
#import "ios/chrome/common/ui/colors/semantic_color_names.h"
#import "ios/chrome/grit/ios_strings.h"
#import "ui/base/l10n/l10n_util.h"
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller.mm b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller.mm
index 83a36508..3a0e524c 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller.mm
@@ -16,6 +16,7 @@
#import "base/numerics/safe_conversions.h"
#import "base/ranges/algorithm.h"
#import "base/strings/sys_string_conversions.h"
+#import "components/bookmarks/browser/bookmark_node.h"
#import "components/bookmarks/common/bookmark_features.h"
#import "components/bookmarks/common/bookmark_metrics.h"
#import "components/bookmarks/common/bookmark_pref_names.h"
@@ -30,6 +31,19 @@
#import "ios/chrome/browser/bookmarks/model/legacy_bookmark_model.h"
#import "ios/chrome/browser/bookmarks/model/local_or_syncable_bookmark_model_factory.h"
#import "ios/chrome/browser/bookmarks/model/managed_bookmark_service_factory.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_navigation_controller.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_ui_constants.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_home_node_item.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_table_cell_title_edit_delegate.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/cells/table_view_bookmarks_folder_item.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator_delegate.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_coordinator.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_coordinator_delegate.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_consumer.h"
+#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.h"
#import "ios/chrome/browser/drag_and_drop/model/drag_item_util.h"
#import "ios/chrome/browser/drag_and_drop/model/table_view_url_drag_drop_handler.h"
#import "ios/chrome/browser/favicon/model/favicon_loader.h"
@@ -62,19 +76,6 @@
#import "ios/chrome/browser/sync/model/sync_service_factory.h"
#import "ios/chrome/browser/ui/authentication/cells/signin_promo_view_configurator.h"
#import "ios/chrome/browser/ui/authentication/cells/table_view_signin_promo_item.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_navigation_controller.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_path_cache.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_ui_constants.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_home_node_item.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/cells/bookmark_table_cell_title_edit_delegate.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/cells/table_view_bookmarks_folder_item.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator_delegate.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_coordinator.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_coordinator_delegate.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_consumer.h"
-#import "ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_mediator.h"
#import "ios/chrome/browser/ui/incognito_reauth/incognito_reauth_scene_agent.h"
#import "ios/chrome/browser/ui/keyboard/UIKeyCommand+Chrome.h"
#import "ios/chrome/browser/ui/menu/browser_action_factory.h"
@@ -97,6 +98,8 @@
#import "ui/base/l10n/l10n_util_mac.h"
#import "ui/strings/grit/ui_strings.h"
+namespace {
+
using bookmark_utils_ios::BookmarkNodeReference;
using bookmark_utils_ios::FindNodeReferenceByNodes;
using bookmarks::BookmarkNode;
@@ -106,8 +109,6 @@
// collections.
using IntegerPair = std::pair<NSInteger, NSInteger>;
-namespace {
-
typedef NS_ENUM(NSInteger, BookmarksContextBarState) {
BookmarksContextBarNone, // No state.
BookmarksContextBarDefault, // No selection is possible in this state.
@@ -228,7 +229,7 @@
std::unique_ptr<BookmarkModelBridge> _accountBookmarkModelBridge;
// The bookmark node that was choosen by an entity outside of the Bookmarks UI
// and is selected when the view is loaded.
- raw_ptr<const bookmarks::BookmarkNode> _externalBookmark;
+ raw_ptr<const BookmarkNode> _externalBookmark;
// Whether the view controller was requested to shutdown.
BOOL _isShutDown;
// Whether the navigation controller is being dismissed.
@@ -262,6 +263,8 @@
->AsWeakPtr();
_accountBookmarkModelBridge = std::make_unique<BookmarkModelBridge>(
self, _accountBookmarkModel.get());
+ CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
+ _accountBookmarkModel->underlying_model());
}
return self;
}
@@ -290,7 +293,7 @@
self.sharingCoordinator = nil;
}
-- (void)setExternalBookmark:(const bookmarks::BookmarkNode*)node {
+- (void)setExternalBookmark:(const BookmarkNode*)node {
_externalBookmark = node;
}
@@ -305,6 +308,12 @@
return YES;
}
+- (bookmarks::BookmarkModel*)underlyingBookmarkModel {
+ CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
+ _accountBookmarkModel->underlying_model());
+ return _localOrSyncableBookmarkModel->underlying_model();
+}
+
- (NSArray<BookmarksHomeViewController*>*)cachedViewControllerStack {
// This method is only designed to be called for the view controller
// associated with the root node.
@@ -320,45 +329,33 @@
[stack addObject:self];
int64_t cachedFolderID;
- BookmarkModelType modelType;
int cachedIndexPathRow;
// If cache is present then reconstruct the last visited bookmark from
// cache.
if (![BookmarkPathCache
- getBookmarkTopMostRowCacheWithPrefService:self.browserState
- ->GetPrefs()
- localOrSyncableModel:
- _localOrSyncableBookmarkModel.get()
- accountModel:_accountBookmarkModel.get()
- folderId:&cachedFolderID
- modelType:&modelType
- topMostRow:&cachedIndexPathRow] ||
+ bookmarkTopMostRowCacheWithPrefService:self.browserState->GetPrefs()
+ bookmarkModel:[self underlyingBookmarkModel]
+ folderId:&cachedFolderID
+ topMostRow:&cachedIndexPathRow] ||
cachedFolderID == _localOrSyncableBookmarkModel
->subtle_root_node_with_unspecified_children()
->id()) {
return stack;
}
- base::WeakPtr<LegacyBookmarkModel> folderModel;
- if (modelType == BookmarkModelType::kAccount) {
- folderModel = _accountBookmarkModel;
- } else {
- folderModel = _localOrSyncableBookmarkModel;
- }
- NSArray<NSNumber*>* path =
- bookmark_utils_ios::CreateBookmarkPath(folderModel.get(), cachedFolderID);
+ NSArray<NSNumber*>* path = bookmark_utils_ios::CreateBookmarkPath(
+ [self underlyingBookmarkModel], cachedFolderID);
if (!path) {
return stack;
}
for (NSUInteger ii = 0; ii < [path count]; ii++) {
int64_t nodeID = [[path objectAtIndex:ii] longLongValue];
- const BookmarkNode* node =
- bookmark_utils_ios::FindFolderById(folderModel.get(), nodeID);
+ const BookmarkNode* node = bookmark_utils_ios::FindFolderById(
+ [self underlyingBookmarkModel], nodeID);
DCHECK(node);
// if node is an empty permanent node, stop.
- if (node->children().empty() &&
- IsPrimaryPermanentNode(node, folderModel.get())) {
+ if (node->children().empty() && node->is_permanent_node()) {
break;
}
@@ -653,7 +650,7 @@
- (void)loadFaviconAtIndexPath:(NSIndexPath*)indexPath
forCell:(UITableViewCell*)cell
fallbackToGoogleServer:(BOOL)fallbackToGoogleServer {
- const bookmarks::BookmarkNode* node = [self nodeAtIndexPath:indexPath];
+ const BookmarkNode* node = [self nodeAtIndexPath:indexPath];
if (node->is_folder()) {
return;
}
@@ -733,8 +730,7 @@
canEditNode:(BOOL)canEditNode {
const BookmarkNode* bookmarkNode = [self nodeAtIndexPath:indexPath];
DCHECK_EQ(bookmarkNode, FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(),
- nodeReference));
+ [self underlyingBookmarkModel], nodeReference));
DCHECK_EQ(bookmarkNode->type(), BookmarkNode::URL);
GURL nodeURL = bookmarkNode->url();
// Record that this context menu was shown to the user.
@@ -812,9 +808,8 @@
indexPath:(NSIndexPath*)indexPath
canEditNode:(BOOL)canEditNode {
const BookmarkNode* folderNode = [self nodeAtIndexPath:indexPath];
- DCHECK_EQ(folderNode, FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(),
- nodeReference));
+ DCHECK_EQ(folderNode, FindNodeByNodeReference([self underlyingBookmarkModel],
+ nodeReference));
DCHECK_EQ(folderNode->type(), BookmarkNode::FOLDER);
// Record that this context menu was shown to the user.
RecordMenuShown(kMenuScenarioHistogramBookmarkFolder);
@@ -851,11 +846,9 @@
userAction:(const char*)userAction {
DCHECK(!_folderChooserCoordinator);
DCHECK(nodeReferences.size() > 0);
- CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
- _accountBookmarkModel->underlying_model());
bookmark_utils_ios::NodeSet nodes =
bookmark_utils_ios::FindNodesByNodeReferences(
- _localOrSyncableBookmarkModel->underlying_model(), nodeReferences);
+ [self underlyingBookmarkModel], nodeReferences);
if (nodes.size() == 0) {
// While the contextual menu was opened, the nodes might have been removed.
// If the nodes don't exist anymore, there nothing to do.
@@ -876,10 +869,8 @@
// Deletes the `nodeIDs` if they still exist and records `userAction`.
- (void)deleteBookmarkNodeWithReference:(BookmarkNodeReference)nodeReference
userAction:(const char*)userAction {
- CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
- _accountBookmarkModel->underlying_model());
- const bookmarks::BookmarkNode* node = FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(), nodeReference);
+ const BookmarkNode* node =
+ FindNodeByNodeReference([self underlyingBookmarkModel], nodeReference);
if (!node) {
// While the contextual menu was opened, the nodes might have been removed.
// If the nodes don't exist anymore, there nothing to do.
@@ -918,11 +909,9 @@
// Opens the editor for `nodeID` node, if it still exists. The node has to be
// a bookmark node.
- (void)editBookmarkNodeWithReference:(BookmarkNodeReference)nodeReference {
- CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
- _accountBookmarkModel->underlying_model());
- const bookmarks::BookmarkNode* bookmarkNode =
+ const BookmarkNode* bookmarkNode =
bookmark_utils_ios::FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(), nodeReference);
+ [self underlyingBookmarkModel], nodeReference);
if (!bookmarkNode) {
// While the contextual menu was opened, the node might has been removed.
// If the node doesn't exist anymore, there nothing to do.
@@ -938,11 +927,9 @@
// Opens the editor for `nodeID` node, if it still exists. The node has to be
// a folder node.
- (void)editFolderNodeWithReference:(BookmarkNodeReference)nodeReference {
- CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
- _accountBookmarkModel->underlying_model());
- const bookmarks::BookmarkNode* bookmarkNode =
+ const BookmarkNode* bookmarkNode =
bookmark_utils_ios::FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(), nodeReference);
+ [self underlyingBookmarkModel], nodeReference);
if (!bookmarkNode) {
// While the contextual menu was opened, the node might has been removed.
// If the node doesn't exist anymore, there nothing to do.
@@ -1074,7 +1061,7 @@
[self dismissWithURL:url];
}
-- (void)handleSelectFolderForNavigation:(const bookmarks::BookmarkNode*)folder {
+- (void)handleSelectFolderForNavigation:(const BookmarkNode*)folder {
if (!self.mediator.currentlyShowingSearchResults) {
BookmarksHomeViewController* controller =
[self createControllerWithDisplayedFolderNode:folder];
@@ -1084,7 +1071,7 @@
[self jumpToFolder:folder];
}
-- (void)jumpToFolder:(const bookmarks::BookmarkNode*)folder {
+- (void)jumpToFolder:(const BookmarkNode*)folder {
// Clear bookmark path cache.
if (_isBeingDismissed) {
// The navigation controller is being dismissed.
@@ -1092,24 +1079,20 @@
return;
}
int64_t unusedFolderId;
- BookmarkModelType modelType;
int unusedIndexPathRow;
PrefService* prefService = self.browserState->GetPrefs();
while ([BookmarkPathCache
- getBookmarkTopMostRowCacheWithPrefService:prefService
- localOrSyncableModel:_localOrSyncableBookmarkModel
- .get()
- accountModel:_accountBookmarkModel.get()
- folderId:&unusedFolderId
- modelType:&modelType
- topMostRow:&unusedIndexPathRow]) {
+ bookmarkTopMostRowCacheWithPrefService:prefService
+ bookmarkModel:[self underlyingBookmarkModel]
+ folderId:&unusedFolderId
+ topMostRow:&unusedIndexPathRow]) {
[BookmarkPathCache clearBookmarkTopMostRowCacheWithPrefService:prefService];
}
// Rebuild folder controller list, going back up the tree.
NSMutableArray<BookmarksHomeViewController*>* stack = [NSMutableArray array];
- std::vector<const bookmarks::BookmarkNode*> nodes;
- const bookmarks::BookmarkNode* cursor = folder;
+ std::vector<const BookmarkNode*> nodes;
+ const BookmarkNode* cursor = folder;
while (cursor) {
// Build reversed list of nodes to restore bookmark path below.
nodes.insert(nodes.begin(), cursor);
@@ -1128,7 +1111,7 @@
}
// Reconstruct bookmark path cache.
- for (const bookmarks::BookmarkNode* node : nodes) {
+ for (const BookmarkNode* node : nodes) {
[BookmarkPathCache
cacheBookmarkTopMostRowWithPrefService:prefService
folderId:node->id()
@@ -1169,8 +1152,7 @@
completion:completion];
}
-- (void)handleSelectEditNodes:
- (const std::set<const bookmarks::BookmarkNode*>&)nodes {
+- (void)handleSelectEditNodes:(const std::set<const BookmarkNode*>&)nodes {
// Early return if bookmarks table is not in edit mode.
if (!self.mediator.currentlyInEditMode) {
return;
@@ -1186,7 +1168,7 @@
return;
}
if (nodes.size() == 1) {
- const bookmarks::BookmarkNode* node = *nodes.begin();
+ const BookmarkNode* node = *nodes.begin();
if (node->is_url()) {
[self setContextBarState:BookmarksContextBarSingleURLSelection];
} else {
@@ -1229,8 +1211,7 @@
NOTREACHED_IN_MIGRATION();
}
-- (void)handleMoveNode:(const bookmarks::BookmarkNode*)node
- toPosition:(size_t)position {
+- (void)handleMoveNode:(const BookmarkNode*)node toPosition:(size_t)position {
[self.snackbarCommandsHandler
showSnackbarMessage:
bookmark_utils_ios::UpdateBookmarkPositionWithUndoToast(
@@ -1271,18 +1252,18 @@
- (void)bookmarksFolderChooserCoordinatorDidConfirm:
(BookmarksFolderChooserCoordinator*)coordinator
withSelectedFolder:
- (const bookmarks::BookmarkNode*)folder {
+ (const BookmarkNode*)folder {
DCHECK(_folderChooserCoordinator);
DCHECK(folder);
// Copy the list of edited nodes from BookmarksFolderChooserCoordinator
// as the reference may become invalid when `_folderChooserCoordinator`
// is set to nil (if `self` holds the last reference to the object).
- std::set<const bookmarks::BookmarkNode*> editedNodesSet =
+ std::set<const BookmarkNode*> editedNodesSet =
_folderChooserCoordinator.editedNodes;
// TODO(crbug.com/40268466): Change the type of `editedNodes` to std::vector.
- std::vector<const bookmarks::BookmarkNode*> editedNodesVector(
- editedNodesSet.begin(), editedNodesSet.end());
+ std::vector<const BookmarkNode*> editedNodesVector(editedNodesSet.begin(),
+ editedNodesSet.end());
[self stopFolderChooserCoordinator];
DCHECK(!folder->is_url());
@@ -1334,20 +1315,15 @@
}
int64_t unusedFolderId;
- BookmarkModelType modelType;
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
- getBookmarkTopMostRowCacheWithPrefService:self.browserState
- ->GetPrefs()
- localOrSyncableModel:
- _localOrSyncableBookmarkModel.get()
- accountModel:_accountBookmarkModel.get()
- folderId:&unusedFolderId
- modelType:&modelType
- topMostRow:&unusedIndexPathRow]) {
+ bookmarkTopMostRowCacheWithPrefService:self.browserState->GetPrefs()
+ bookmarkModel:[self underlyingBookmarkModel]
+ folderId:&unusedFolderId
+ topMostRow:&unusedIndexPathRow]) {
self.isReconstructingFromCache = YES;
}
@@ -1383,22 +1359,22 @@
}];
}
-- (void)didChangeNode:(const bookmarks::BookmarkNode*)bookmarkNode {
+- (void)didChangeNode:(const BookmarkNode*)bookmarkNode {
// No-op here. Bookmarks might be refreshed in BookmarksHomeMediator.
}
-- (void)didChangeChildrenForNode:(const bookmarks::BookmarkNode*)bookmarkNode {
+- (void)didChangeChildrenForNode:(const BookmarkNode*)bookmarkNode {
// No-op here. Bookmarks might be refreshed in BookmarksHomeMediator.
}
-- (void)didMoveNode:(const bookmarks::BookmarkNode*)bookmarkNode
- fromParent:(const bookmarks::BookmarkNode*)oldParent
- toParent:(const bookmarks::BookmarkNode*)newParent {
+- (void)didMoveNode:(const BookmarkNode*)bookmarkNode
+ fromParent:(const BookmarkNode*)oldParent
+ toParent:(const BookmarkNode*)newParent {
// No-op here. Bookmarks might be refreshed in BookmarksHomeMediator.
}
-- (void)didDeleteNode:(const bookmarks::BookmarkNode*)node
- fromFolder:(const bookmarks::BookmarkNode*)folder {
+- (void)didDeleteNode:(const BookmarkNode*)node
+ fromFolder:(const BookmarkNode*)folder {
if (self.displayedFolderNode == node) {
[self setTableViewEditing:NO];
}
@@ -1443,7 +1419,7 @@
// Returns a bookmark node reference for `bookmarkNode`.
- (BookmarkNodeReference)bookmarkNodeReferenceWithNode:
- (const bookmarks::BookmarkNode*)bookmarkNode {
+ (const BookmarkNode*)bookmarkNode {
return BookmarkNodeReference(bookmarkNode->id());
}
@@ -1491,7 +1467,7 @@
- (void)setupNavigationForBookmarksHomeViewController:
(BookmarksHomeViewController*)viewController
usingBookmarkNode:
- (const bookmarks::BookmarkNode*)node {
+ (const BookmarkNode*)node {
viewController.navigationItem.leftBarButtonItem.action = @selector(back);
// Disable large titles on every VC but the root controller.
if (node != _localOrSyncableBookmarkModel
@@ -1605,7 +1581,7 @@
}
- (BookmarksHomeViewController*)createControllerWithDisplayedFolderNode:
- (const bookmarks::BookmarkNode*)displayedFolderNode {
+ (const BookmarkNode*)displayedFolderNode {
BookmarksHomeViewController* controller =
[[BookmarksHomeViewController alloc] initWithBrowser:_browser.get()];
controller.displayedFolderNode = displayedFolderNode;
@@ -1622,7 +1598,7 @@
- (void)restoreRowSelection {
// Create a new selectedNodesForEditMode set to check if some selected nodes
// are removed.
- std::set<const bookmarks::BookmarkNode*> newEditNodes;
+ std::set<const BookmarkNode*> newEditNodes;
// Add selected nodes to selectedNodesForEditMode only if they are not removed
// (still exist in the table).
@@ -1754,8 +1730,8 @@
return [self.tableViewModel numberOfItemsInSection:section] > 0;
}
-- (std::vector<const bookmarks::BookmarkNode*>)selectedNodesForEditMode {
- std::vector<const bookmarks::BookmarkNode*> nodes;
+- (std::vector<const BookmarkNode*>)selectedNodesForEditMode {
+ std::vector<const BookmarkNode*> nodes;
if (self.mediator.currentlyShowingSearchResults) {
// Create a vector of edit nodes in the same order as the selected nodes.
base::ranges::copy(self.mediator.selectedNodesForEditMode,
@@ -1826,10 +1802,8 @@
// Triggers the URL sharing flow for `bookmarkNodeID` node, if it still exists.
- (void)shareBookmarkNodeWithReference:(BookmarkNodeReference)nodeReference
indexPath:(NSIndexPath*)indexPath {
- CHECK_EQ(_localOrSyncableBookmarkModel->underlying_model(),
- _accountBookmarkModel->underlying_model());
- const bookmarks::BookmarkNode* bookmarkNode = FindNodeByNodeReference(
- _localOrSyncableBookmarkModel->underlying_model(), nodeReference);
+ const BookmarkNode* bookmarkNode =
+ FindNodeByNodeReference([self underlyingBookmarkModel], nodeReference);
if (!bookmarkNode) {
// While the contextual menu was opened, the node might has been removed.
// If the node doesn't exist anymore, there nothing to do.
@@ -1954,7 +1928,7 @@
if ([self isAnyControllerPresenting]) {
return;
}
- const std::set<const bookmarks::BookmarkNode*> nodes =
+ const std::set<const BookmarkNode*> nodes =
self.mediator.selectedNodesForEditMode;
switch (self.contextBarState) {
case BookmarksContextBarDefault:
@@ -1987,7 +1961,7 @@
if ([self isAnyControllerPresenting]) {
return;
}
- const std::set<const bookmarks::BookmarkNode*> nodes =
+ const std::set<const BookmarkNode*> nodes =
self.mediator.selectedNodesForEditMode;
// Center button is shown and is clickable only when at least
// one node is selected.
@@ -2197,9 +2171,8 @@
if ([strongSelf isIncognitoForced]) {
return;
}
- std::vector<const bookmarks::BookmarkNode*>
- selectedNodesForEditMode =
- [strongSelf selectedNodesForEditMode];
+ std::vector<const BookmarkNode*> selectedNodesForEditMode =
+ [strongSelf selectedNodesForEditMode];
[strongSelf
openAllURLs:GetUrlsToOpen(selectedNodesForEditMode)
inIncognito:NO
@@ -2220,9 +2193,8 @@
if (![strongSelf isIncognitoAvailable]) {
return;
}
- std::vector<const bookmarks::BookmarkNode*>
- selectedNodesForEditMode =
- [strongSelf selectedNodesForEditMode];
+ std::vector<const BookmarkNode*> selectedNodesForEditMode =
+ [strongSelf selectedNodesForEditMode];
[strongSelf
openAllURLs:GetUrlsToOpen(selectedNodesForEditMode)
inIncognito:YES
@@ -2376,8 +2348,7 @@
}
- (void)configureCoordinator:(AlertCoordinator*)coordinator
- forMixedAndMultiFolderSelection:
- (const std::set<const bookmarks::BookmarkNode*>)nodes {
+ forMixedAndMultiFolderSelection:(const std::set<const BookmarkNode*>)nodes {
__weak BookmarksHomeViewController* weakSelf = self;
coordinator.alertController.view.accessibilityIdentifier =
kBookmarksHomeContextMenuIdentifier;
@@ -2811,7 +2782,7 @@
return nil;
}
- const bookmarks::BookmarkNode* node = [self nodeAtIndexPath:indexPath];
+ const BookmarkNode* node = [self nodeAtIndexPath:indexPath];
if (!node || node->is_folder()) {
return nil;
}
diff --git a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller_unittest.mm b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller_unittest.mm
index 685b8db..a0e79df0 100644
--- a/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller_unittest.mm
+++ b/ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_home_view_controller_unittest.mm
@@ -198,4 +198,58 @@
}
}
+TEST_F(BookmarksHomeViewControllerTest, CachedViewControllerStack) {
+ @autoreleasepool {
+ id mockSnackbarCommandHandler =
+ OCMProtocolMock(@protocol(SnackbarCommands));
+
+ // Set up ApplicationCommands mock. Because ApplicationCommands conforms
+ // to SettingsCommands, that needs to be mocked and dispatched
+ // as well.
+ id mockApplicationCommandHandler =
+ OCMProtocolMock(@protocol(ApplicationCommands));
+ id mockSettingsCommandHandler =
+ OCMProtocolMock(@protocol(SettingsCommands));
+
+ CommandDispatcher* dispatcher = browser_->GetCommandDispatcher();
+ [dispatcher startDispatchingToTarget:mockSnackbarCommandHandler
+ forProtocol:@protocol(SnackbarCommands)];
+ [dispatcher startDispatchingToTarget:mockApplicationCommandHandler
+ forProtocol:@protocol(ApplicationCommands)];
+ [dispatcher startDispatchingToTarget:mockSettingsCommandHandler
+ forProtocol:@protocol(SettingsCommands)];
+
+ const bookmarks::BookmarkNode* mobileNode =
+ local_or_syncable_bookmark_model_->subtle_mobile_node();
+ const bookmarks::BookmarkNode* folder = AddFolder(mobileNode, u"foo");
+ AddBookmark(folder, u"bar");
+
+ BookmarksHomeViewController* controller =
+ [[BookmarksHomeViewController alloc] initWithBrowser:browser_.get()];
+ controller.applicationCommandsHandler = mockApplicationCommandHandler;
+ controller.snackbarCommandsHandler = mockSnackbarCommandHandler;
+ controller.displayedFolderNode = folder;
+
+ // Closing should populate the cache.
+ [controller keyCommand_close];
+
+ controller.displayedFolderNode =
+ local_or_syncable_bookmark_model_
+ ->subtle_root_node_with_unspecified_children();
+
+ NSArray<BookmarksHomeViewController*>* stack =
+ [controller cachedViewControllerStack];
+ ASSERT_EQ(3u, stack.count);
+ EXPECT_EQ(folder, stack[2].displayedFolderNode);
+ EXPECT_EQ(mobileNode, stack[1].displayedFolderNode);
+ EXPECT_EQ(local_or_syncable_bookmark_model_
+ ->subtle_root_node_with_unspecified_children(),
+ stack[0].displayedFolderNode);
+
+ [stack[0] shutdown];
+ [stack[1] shutdown];
+ [stack[2] shutdown];
+ }
+}
+
} // namespace
