0

Add support for BookmarkClient to provide suggested save locations

Historically, save nodes have by default been the last modified folder
in bookmarks. This patch adds the ability for clients to specify a
folder to save to without necessarily making platform-specific
changes. The new method is provided a URL to make a decision on.

Bug: b:291326480
Change-Id: I7c0049afb957a0bebca2356ca47e06a8d05cc528
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4808795
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187901}
This commit is contained in:
Matt Jones
2023-08-24 17:08:36 +00:00
committed by Chromium LUCI CQ
parent 31958ea226
commit 4a0b828348
5 changed files with 66 additions and 4 deletions

@ -10,6 +10,10 @@ namespace bookmarks {
void BookmarkClient::Init(BookmarkModel* model) {}
const BookmarkNode* BookmarkClient::GetSuggestedSaveLocation(const GURL& url) {
return nullptr;
}
base::CancelableTaskTracker::TaskId BookmarkClient::GetFaviconImageForPageURL(
const GURL& page_url,
favicon_base::FaviconImageCallback callback,

@ -43,6 +43,12 @@ class BookmarkClient {
// Called during initialization of BookmarkModel.
virtual void Init(BookmarkModel* model);
// Gets a bookmark folder that the provided URL can be saved to. If nullptr is
// returned, the bookmark is saved to the default location (usually this is
// the last modified folder). This affords features the option to override the
// default folder if relevant for the URL.
virtual const BookmarkNode* GetSuggestedSaveLocation(const GURL& url);
// Requests a favicon from the history cache for the web page at |page_url|
// for icon type favicon_base::IconType::kFavicon. |callback| is run when the
// favicon has been fetched, which returns gfx::Image is a multi-resolution

@ -545,7 +545,8 @@ const BookmarkNode* AddIfNotBookmarked(BookmarkModel* model,
base::RecordAction(base::UserMetricsAction("BookmarkAdded"));
const auto* parent_to_use = parent ? parent : GetParentForNewNodes(model);
const auto* parent_to_use =
parent ? parent : GetParentForNewNodes(model, url);
return model->AddNewURL(parent_to_use, parent_to_use->children().size(),
title, url);
}
@ -623,11 +624,17 @@ bool HasDescendantsOf(const std::vector<const BookmarkNode*>& list,
return false;
}
const BookmarkNode* GetParentForNewNodes(BookmarkModel* model) {
const BookmarkNode* GetParentForNewNodes(BookmarkModel* model,
const GURL& url) {
#if BUILDFLAG(IS_ANDROID)
if (!HasUserCreatedBookmarks(model))
return model->mobile_node();
#endif
const BookmarkNode* parent = model->client()->GetSuggestedSaveLocation(url);
if (parent) {
return parent;
}
std::vector<const BookmarkNode*> nodes =
GetMostRecentlyModifiedUserFolders(model, 1);
DCHECK(!nodes.empty()); // This list is always padded with default folders.

@ -203,8 +203,10 @@ bool HasDescendantsOf(const std::vector<const BookmarkNode*>& list,
const BookmarkNode* root);
// Returns the parent to add new nodes to, never returns null (as long as
// the model is loaded).
const BookmarkNode* GetParentForNewNodes(BookmarkModel* model);
// the model is loaded). If |url| is non-empty, features will have the
// opportunity to suggest contextually relevant folders.
const BookmarkNode* GetParentForNewNodes(BookmarkModel* model,
const GURL& url = GURL());
} // namespace bookmarks

@ -17,6 +17,7 @@
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "components/bookmarks/browser/base_bookmark_model_observer.h"
#include "components/bookmarks/browser/bookmark_client.h"
#include "components/bookmarks/browser/bookmark_model.h"
#include "components/bookmarks/browser/bookmark_model_observer.h"
#include "components/bookmarks/browser/bookmark_node_data.h"
@ -87,6 +88,26 @@ class BookmarkUtilsTest : public testing::Test,
base::HistogramTester histogram_;
};
// A bookmark client that suggests a save location for new nodes.
class SuggestFolderClient : public TestBookmarkClient {
public:
SuggestFolderClient() = default;
SuggestFolderClient(const SuggestFolderClient&) = delete;
SuggestFolderClient& operator=(const SuggestFolderClient&) = delete;
~SuggestFolderClient() override = default;
const BookmarkNode* GetSuggestedSaveLocation(const GURL& url) override {
return suggested_save_location_.get();
}
void SetSuggestedSaveLocation(const BookmarkNode* node) {
suggested_save_location_ = node;
}
private:
raw_ptr<const BookmarkNode> suggested_save_location_;
};
TEST_F(BookmarkUtilsTest, GetBookmarksMatchingPropertiesWordPhraseQuery) {
std::unique_ptr<BookmarkModel> model(TestBookmarkClient::CreateModel());
const BookmarkNode* node1 = model->AddURL(model->other_node(), 0, u"foo bar",
@ -485,6 +506,28 @@ TEST_F(BookmarkUtilsTest, GetParentForNewNodes) {
EXPECT_EQ(2u, index);
}
// Ensures the BookmarkClient has the power to suggest the parent for new nodes.
TEST_F(BookmarkUtilsTest, GetParentForNewNodes_ClientOverride) {
std::unique_ptr<SuggestFolderClient> client =
std::make_unique<SuggestFolderClient>();
SuggestFolderClient* client_ptr = client.get();
std::unique_ptr<BookmarkModel> model(
TestBookmarkClient::CreateModelWithClient(std::move(client)));
const BookmarkNode* folder_to_suggest =
model->AddFolder(model->bookmark_bar_node(), 0, u"Suggested");
const BookmarkNode* folder1 =
model->AddFolder(model->bookmark_bar_node(), 1, u"Folder 1");
ASSERT_EQ(folder1, GetParentForNewNodes(model.get(), GURL()));
client_ptr->SetSuggestedSaveLocation(folder_to_suggest);
ASSERT_EQ(folder_to_suggest, GetParentForNewNodes(model.get(), GURL()));
client_ptr = nullptr;
}
// Verifies that meta info is copied when nodes are cloned.
TEST_F(BookmarkUtilsTest, CloneMetaInfo) {
std::unique_ptr<BookmarkModel> model(TestBookmarkClient::CreateModel());