Skip to content

Commit

Permalink
[sync] Add virtual functions to BookmarkModelView
Browse files Browse the repository at this point in the history 
No behavioral differences, pure refactoring with semi-trivial
plumbing.

Adding virtual functions to BookmarkModelView allows, in later patches,
the customization of how the sync-relevant bookmark tree maps to the
local BookmarkModel.

For now, a single subclass is introduced, which implements the very
same behavior as before the patch.

Design doc (sorry, Googlers only):
https://docs.google.com/document/d/1i9XuP0lXOvEbvlwXre-EXMum4dxs_UToCsCcImIPEQU/edit?usp=sharing&resourcekey=0-1t7DIB0kTwaaGMmxtiZLqw

Change-Id: I187d03790d3f4b91a0826509f381bcfeeff0e234
Bug: 1494120
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4967493
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Ankush Singh <ankushkush@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215583}
  • Loading branch information
Mikel Astiz authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent caf4895 commit 8c96a26
Show file tree
Hide file tree
Showing 17 changed files with 291 additions and 96 deletions.
12 changes: 10 additions & 2 deletions chrome/browser/bookmarks/chrome_bookmark_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "components/history/core/browser/url_database.h"
#include "components/offline_pages/buildflags/buildflags.h"
#include "components/power_bookmarks/core/suggested_save_location_provider.h"
#include "components/sync_bookmarks/bookmark_model_view.h"
#include "components/sync_bookmarks/bookmark_sync_service.h"
#include "components/undo/bookmark_undo_service.h"

Expand Down Expand Up @@ -212,12 +213,19 @@ void ChromeBookmarkClient::DecodeBookmarkSyncMetadata(
const std::string& metadata_str,
const base::RepeatingClosure& schedule_save_closure) {
local_or_syncable_bookmark_sync_service_->DecodeBookmarkSyncMetadata(
metadata_str, schedule_save_closure, model_);
metadata_str, schedule_save_closure,
std::make_unique<
sync_bookmarks::BookmarkModelViewUsingLocalOrSyncableNodes>(model_));
// TODO(crbug.com/1494120): Pass along sync metadata once BookmarkClient API
// is capable of reading it from BookmarkModel.
if (account_bookmark_sync_service_) {
// TODO(crbug.com/1494120): `BookmarkModelViewUsingLocalOrSyncableNodes` is
// not the right thing to use here, because the underlying model is shared.
account_bookmark_sync_service_->DecodeBookmarkSyncMetadata(
std::string(), schedule_save_closure, model_);
std::string(), schedule_save_closure,
std::make_unique<
sync_bookmarks::BookmarkModelViewUsingLocalOrSyncableNodes>(
model_));
}
}

Expand Down
1 change: 1 addition & 0 deletions components/browser_sync/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ source_set("unit_tests") {
"//components/sync_bookmarks",
"//components/sync_device_info",
"//components/sync_device_info:test_support",
"//components/undo",
"//components/version_info",
"//components/version_info:generate_version_info",
"//components/webdata/common",
Expand Down
1 change: 1 addition & 0 deletions components/browser_sync/DEPS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ include_rules = [
"+components/sync_preferences",
"+components/sync_sessions",
"+components/sync_user_events",
"+components/undo",
"+components/url_formatter",
"+components/version_info",
"+components/webdata/common",
Expand Down
64 changes: 34 additions & 30 deletions components/browser_sync/sync_client_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
#include "base/metrics/histogram_functions.h"
#include "base/ranges/algorithm.h"
#include "base/strings/utf_string_conversions.h"
#include "components/bookmarks/browser/bookmark_model.h"
#include "components/password_manager/core/browser/password_form.h"
#include "components/password_manager/core/browser/password_store_consumer.h"
#include "components/password_manager/core/browser/password_store_interface.h"
#include "components/reading_list/core/dual_reading_list_model.h"
#include "components/sync/base/data_type_histogram.h"
#include "components/sync/service/local_data_description.h"
#include "components/sync_bookmarks/bookmark_model_view.h"
#include "components/sync_bookmarks/bookmark_sync_service.h"
#include "components/sync_bookmarks/local_bookmark_model_merger.h"
#include "components/url_formatter/elide_url.h"
#include "ui/base/models/tree_node_iterator.h"
Expand Down Expand Up @@ -67,14 +67,14 @@ syncer::LocalDataDescription CreateLocalDataDescription(syncer::ModelType type,
// Returns urls of all the bookmarks which can be moved to the account store,
// i.e. it does not include folders nor managed bookmarks.
std::vector<GURL> GetAllUserBookmarksExcludingFolders(
bookmarks::BookmarkModel* model) {
sync_bookmarks::BookmarkModelView* model) {
std::vector<GURL> bookmarked_urls;
ui::TreeNodeIterator<const bookmarks::BookmarkNode> iterator(
model->root_node());
while (iterator.has_next()) {
const bookmarks::BookmarkNode* const node = iterator.Next();
// Skip folders and managed bookmarks.
if (node->is_url() && !model->client()->IsNodeManaged(node)) {
// Skip folders and non-syncable nodes (e.g. managed bookmarks).
if (node->is_url() && model->IsNodeSyncable(node)) {
bookmarked_urls.push_back(node->url());
}
}
Expand All @@ -98,16 +98,17 @@ syncer::ModelTypeSet FilterUsableTypes(
syncer::ModelTypeSet types,
password_manager::PasswordStoreInterface* profile_password_store,
password_manager::PasswordStoreInterface* account_password_store,
bookmarks::BookmarkModel* local_bookmark_model,
bookmarks::BookmarkModel* account_bookmark_model,
sync_bookmarks::BookmarkSyncService* local_bookmark_sync_service,
sync_bookmarks::BookmarkSyncService* account_bookmark_sync_service,
reading_list::DualReadingListModel* reading_list_model) {
if (!profile_password_store || !account_password_store ||
!account_password_store->IsAbleToSavePasswords()) {
types.Remove(syncer::PASSWORDS);
}

if (!local_bookmark_model || !account_bookmark_model ||
!local_bookmark_model->loaded() || !account_bookmark_model->loaded()) {
if (!local_bookmark_sync_service || !account_bookmark_sync_service ||
!local_bookmark_sync_service->bookmark_model_view() ||
!account_bookmark_sync_service->bookmark_model_view()) {
types.Remove(syncer::BOOKMARKS);
}

Expand Down Expand Up @@ -161,7 +162,8 @@ class LocalDataQueryHelper::LocalDataQueryRequest
weak_ptr_factory_.GetWeakPtr());
}
if (types_.Has(syncer::BOOKMARKS)) {
CHECK(helper_->local_bookmark_model_);
CHECK(helper_->local_bookmark_sync_service_);
CHECK(helper_->local_bookmark_sync_service_->bookmark_model_view());
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce(
Expand Down Expand Up @@ -194,8 +196,8 @@ class LocalDataQueryHelper::LocalDataQueryRequest
}

void FetchLocalBookmarks() {
std::vector<GURL> bookmarked_urls =
GetAllUserBookmarksExcludingFolders(helper_->local_bookmark_model_);
std::vector<GURL> bookmarked_urls = GetAllUserBookmarksExcludingFolders(
helper_->local_bookmark_sync_service_->bookmark_model_view());
result_.emplace(syncer::BOOKMARKS,
CreateLocalDataDescription(
syncer::BOOKMARKS, std::move(bookmarked_urls),
Expand Down Expand Up @@ -237,13 +239,13 @@ class LocalDataQueryHelper::LocalDataQueryRequest
LocalDataQueryHelper::LocalDataQueryHelper(
password_manager::PasswordStoreInterface* profile_password_store,
password_manager::PasswordStoreInterface* account_password_store,
bookmarks::BookmarkModel* local_bookmark_model,
bookmarks::BookmarkModel* account_bookmark_model,
sync_bookmarks::BookmarkSyncService* local_bookmark_sync_service,
sync_bookmarks::BookmarkSyncService* account_bookmark_sync_service,
reading_list::DualReadingListModel* dual_reading_list_model)
: profile_password_store_(profile_password_store),
account_password_store_(account_password_store),
local_bookmark_model_(local_bookmark_model),
account_bookmark_model_(account_bookmark_model),
local_bookmark_sync_service_(local_bookmark_sync_service),
account_bookmark_sync_service_(account_bookmark_sync_service),
dual_reading_list_model_(dual_reading_list_model) {}

LocalDataQueryHelper::~LocalDataQueryHelper() = default;
Expand All @@ -254,7 +256,8 @@ void LocalDataQueryHelper::Run(
std::map<syncer::ModelType, syncer::LocalDataDescription>)> callback) {
syncer::ModelTypeSet usable_types = FilterUsableTypes(
types, profile_password_store_, account_password_store_,
local_bookmark_model_, account_bookmark_model_, dual_reading_list_model_);
local_bookmark_sync_service_, account_bookmark_sync_service_,
dual_reading_list_model_);
// Create a request to query info about local data of all `usable_types`.
std::unique_ptr<LocalDataQueryRequest> request_ptr =
std::make_unique<LocalDataQueryRequest>(this, usable_types,
Expand Down Expand Up @@ -308,18 +311,18 @@ class LocalDataMigrationHelper::LocalDataMigrationRequest
weak_ptr_factory_.GetWeakPtr());
}
if (types_.Has(syncer::BOOKMARKS)) {
CHECK(helper_->local_bookmark_model_);
CHECK(helper_->account_bookmark_model_);
CHECK(helper_->local_bookmark_sync_service_);
CHECK(helper_->account_bookmark_sync_service_);
CHECK(helper_->local_bookmark_sync_service_->bookmark_model_view());
CHECK(helper_->account_bookmark_sync_service_->bookmark_model_view());
// Merge all local bookmarks into the account bookmark model.
sync_bookmarks::BookmarkModelView local_model_view(
helper_->local_bookmark_model_);
sync_bookmarks::BookmarkModelView account_model_view(
helper_->account_bookmark_model_);
sync_bookmarks::LocalBookmarkModelMerger(&local_model_view,
&account_model_view)
sync_bookmarks::LocalBookmarkModelMerger(
helper_->local_bookmark_sync_service_->bookmark_model_view(),
helper_->account_bookmark_sync_service_->bookmark_model_view())
.Merge();
// Remove all bookmarks from the local model.
helper_->local_bookmark_model_->RemoveAllUserBookmarks();
helper_->local_bookmark_sync_service_->bookmark_model_view()
->RemoveAllUserBookmarks();
}
if (types_.Has(syncer::READING_LIST)) {
CHECK(helper_->dual_reading_list_model_);
Expand Down Expand Up @@ -418,21 +421,22 @@ class LocalDataMigrationHelper::LocalDataMigrationRequest
LocalDataMigrationHelper::LocalDataMigrationHelper(
password_manager::PasswordStoreInterface* profile_password_store,
password_manager::PasswordStoreInterface* account_password_store,
bookmarks::BookmarkModel* local_bookmark_model,
bookmarks::BookmarkModel* account_bookmark_model,
sync_bookmarks::BookmarkSyncService* local_bookmark_sync_service,
sync_bookmarks::BookmarkSyncService* account_bookmark_sync_service,
reading_list::DualReadingListModel* dual_reading_list_model)
: profile_password_store_(profile_password_store),
account_password_store_(account_password_store),
local_bookmark_model_(local_bookmark_model),
account_bookmark_model_(account_bookmark_model),
local_bookmark_sync_service_(local_bookmark_sync_service),
account_bookmark_sync_service_(account_bookmark_sync_service),
dual_reading_list_model_(dual_reading_list_model) {}

LocalDataMigrationHelper::~LocalDataMigrationHelper() = default;

void LocalDataMigrationHelper::Run(syncer::ModelTypeSet types) {
syncer::ModelTypeSet usable_types = FilterUsableTypes(
types, profile_password_store_, account_password_store_,
local_bookmark_model_, account_bookmark_model_, dual_reading_list_model_);
local_bookmark_sync_service_, account_bookmark_sync_service_,
dual_reading_list_model_);
// Create a request to move all local data of all `usable_types` to the
// account store.
std::unique_ptr<LocalDataMigrationRequest> request_ptr =
Expand Down
24 changes: 12 additions & 12 deletions components/browser_sync/sync_client_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@
#include "base/functional/callback.h"
#include "components/sync/base/model_type.h"

namespace bookmarks {
class BookmarkModel;
} // namespace bookmarks

namespace password_manager {
class PasswordStoreInterface;
} // namespace password_manager
Expand All @@ -24,6 +20,10 @@ namespace reading_list {
class DualReadingListModel;
} // namespace reading_list

namespace sync_bookmarks {
class BookmarkSyncService;
} // namespace sync_bookmarks

namespace syncer {
struct LocalDataDescription;
} // namespace syncer
Expand All @@ -39,8 +39,8 @@ class LocalDataQueryHelper {
LocalDataQueryHelper(
password_manager::PasswordStoreInterface* profile_password_store,
password_manager::PasswordStoreInterface* account_password_store,
bookmarks::BookmarkModel* local_bookmark_model,
bookmarks::BookmarkModel* account_bookmark_model,
sync_bookmarks::BookmarkSyncService* local_bookmark_sync_service,
sync_bookmarks::BookmarkSyncService* account_bookmark_sync_service,
reading_list::DualReadingListModel* dual_reading_list_model);
~LocalDataQueryHelper();

Expand Down Expand Up @@ -68,8 +68,8 @@ class LocalDataQueryHelper {
raw_ptr<password_manager::PasswordStoreInterface> profile_password_store_;
raw_ptr<password_manager::PasswordStoreInterface> account_password_store_;
// For BOOKMARKS.
raw_ptr<bookmarks::BookmarkModel> local_bookmark_model_;
raw_ptr<bookmarks::BookmarkModel> account_bookmark_model_;
raw_ptr<sync_bookmarks::BookmarkSyncService> local_bookmark_sync_service_;
raw_ptr<sync_bookmarks::BookmarkSyncService> account_bookmark_sync_service_;
// For READING_LIST.
raw_ptr<reading_list::DualReadingListModel> dual_reading_list_model_;
};
Expand All @@ -80,8 +80,8 @@ class LocalDataMigrationHelper {
LocalDataMigrationHelper(
password_manager::PasswordStoreInterface* profile_password_store,
password_manager::PasswordStoreInterface* account_password_store,
bookmarks::BookmarkModel* local_bookmark_model,
bookmarks::BookmarkModel* account_bookmark_model,
sync_bookmarks::BookmarkSyncService* local_bookmark_sync_service,
sync_bookmarks::BookmarkSyncService* account_bookmark_sync_service,
reading_list::DualReadingListModel* dual_reading_list_model);
~LocalDataMigrationHelper();

Expand All @@ -103,8 +103,8 @@ class LocalDataMigrationHelper {
raw_ptr<password_manager::PasswordStoreInterface> profile_password_store_;
raw_ptr<password_manager::PasswordStoreInterface> account_password_store_;
// For BOOKMARKS.
raw_ptr<bookmarks::BookmarkModel> local_bookmark_model_;
raw_ptr<bookmarks::BookmarkModel> account_bookmark_model_;
raw_ptr<sync_bookmarks::BookmarkSyncService> local_bookmark_sync_service_;
raw_ptr<sync_bookmarks::BookmarkSyncService> account_bookmark_sync_service_;
// For READING_LIST.
raw_ptr<reading_list::DualReadingListModel> dual_reading_list_model_;
};
Expand Down

0 comments on commit 8c96a26

Please sign in to comment.