diff --git a/browser/BUILD.gn b/browser/BUILD.gn index be53c69e26ab..5a47432dd57e 100644 --- a/browser/BUILD.gn +++ b/browser/BUILD.gn @@ -28,8 +28,6 @@ source_set("browser_process") { "autocomplete/brave_autocomplete_provider_client.h", "autocomplete/brave_autocomplete_scheme_classifier.cc", "autocomplete/brave_autocomplete_scheme_classifier.h", - "bookmarks/brave_bookmark_client.cc", - "bookmarks/brave_bookmark_client.h", "brave_rewards/rewards_tab_helper.cc", "brave_rewards/rewards_tab_helper.h", "brave_shields/ad_block_pref_service_factory.cc", diff --git a/browser/bookmarks/brave_bookmark_client.cc b/browser/bookmarks/brave_bookmark_client.cc deleted file mode 100644 index 357840524003..000000000000 --- a/browser/bookmarks/brave_bookmark_client.cc +++ /dev/null @@ -1,23 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "brave/browser/bookmarks/brave_bookmark_client.h" - -BraveBookmarkClient::BraveBookmarkClient( - Profile* profile, - bookmarks::ManagedBookmarkService* managed_bookmark_service, - sync_bookmarks::BookmarkSyncService* bookmark_sync_service) : - ChromeBookmarkClient(profile, managed_bookmark_service, - bookmark_sync_service) { -} - -BraveBookmarkClient::~BraveBookmarkClient() {} - -bool BraveBookmarkClient::IsPermanentNodeVisible( - const bookmarks::BookmarkPermanentNode* node) { - if (node->type() == bookmarks::BookmarkNode::OTHER_NODE) - return false; - return ChromeBookmarkClient::IsPermanentNodeVisible(node); -} diff --git a/browser/bookmarks/brave_bookmark_client.h b/browser/bookmarks/brave_bookmark_client.h deleted file mode 100644 index 0a3c571bf385..000000000000 --- a/browser/bookmarks/brave_bookmark_client.h +++ /dev/null @@ -1,26 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef BRAVE_BROWSER_BOOKMARKS_BRAVE_BOOKMARK_CLIENT_H_ -#define BRAVE_BROWSER_BOOKMARKS_BRAVE_BOOKMARK_CLIENT_H_ - -#include "chrome/browser/bookmarks/chrome_bookmark_client.h" - -class BraveBookmarkClient : public ChromeBookmarkClient { - public: - BraveBookmarkClient( - Profile* profile, - bookmarks::ManagedBookmarkService* managed_bookmark_service, - sync_bookmarks::BookmarkSyncService* bookmark_sync_service); - ~BraveBookmarkClient() override; - - // bookmarks::BookmarkClient: - bool IsPermanentNodeVisible( - const bookmarks::BookmarkPermanentNode* node) override; - private: - DISALLOW_COPY_AND_ASSIGN(BraveBookmarkClient); -}; - -#endif // BRAVE_BROWSER_BOOKMARKS_BRAVE_BOOKMARK_CLIENT_H_ diff --git a/browser/bookmarks/brave_bookmark_client_browsertest.cc b/browser/bookmarks/brave_bookmark_client_browsertest.cc deleted file mode 100644 index 262998d1128a..000000000000 --- a/browser/bookmarks/brave_bookmark_client_browsertest.cc +++ /dev/null @@ -1,28 +0,0 @@ -/* Copyright (c) 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "base/strings/utf_string_conversions.h" -#include "chrome/browser/bookmarks/bookmark_model_factory.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ui/browser.h" -#include "chrome/test/base/in_process_browser_test.h" -#include "components/bookmarks/browser/bookmark_model.h" - -using BraveBookmarkClientTest = InProcessBrowserTest; - -IN_PROC_BROWSER_TEST_F(BraveBookmarkClientTest, IsPermanentNodeVisible) { - bookmarks::BookmarkModel* bookmark_model = - BookmarkModelFactory::GetForBrowserContext(browser()->profile()); - EXPECT_TRUE(bookmark_model->bookmark_bar_node()->IsVisible()); - // Other node invisible by default - EXPECT_FALSE(bookmark_model->other_node()->IsVisible()); - EXPECT_FALSE(bookmark_model->mobile_node()->IsVisible()); - - bookmark_model->AddURL(bookmark_model->other_node(), 0, - base::ASCIIToUTF16("A"), GURL("https://A.com")); - EXPECT_TRUE(bookmark_model->other_node()->IsVisible()); - BraveMigrateOtherNode(bookmark_model); - EXPECT_FALSE(bookmark_model->other_node()->IsVisible()); -} diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 1e4e57f4d129..692cfc0ef100 100644 --- a/browser/brave_profile_prefs.cc +++ b/browser/brave_profile_prefs.cc @@ -72,6 +72,9 @@ void RegisterProfilePrefsForMigration( #if BUILDFLAG(BRAVE_WALLET_ENABLED) brave_wallet::RegisterBraveWalletProfilePrefsForMigration(registry); #endif + + // Restore "Other Bookmarks" migration + registry->RegisterBooleanPref(kOtherBookmarksMigrated, false); } void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { diff --git a/browser/profiles/BUILD.gn b/browser/profiles/BUILD.gn index f59bf6b728a3..8a596b0ef0d7 100644 --- a/browser/profiles/BUILD.gn +++ b/browser/profiles/BUILD.gn @@ -24,6 +24,7 @@ source_set("profiles") { "//brave/browser/gcm_driver", "//brave/browser/tor", "//brave/browser/translate/buildflags", + "//brave/common:pref_names", "//brave/common/tor", "//brave/components/brave_ads/browser", "//brave/components/brave_rewards/browser", diff --git a/browser/profiles/brave_bookmark_model_loaded_observer.cc b/browser/profiles/brave_bookmark_model_loaded_observer.cc index 59e4f191809a..553141df0cb4 100644 --- a/browser/profiles/brave_bookmark_model_loaded_observer.cc +++ b/browser/profiles/brave_bookmark_model_loaded_observer.cc @@ -5,8 +5,11 @@ #include "brave/browser/profiles/brave_bookmark_model_loaded_observer.h" +#include "brave/common/pref_names.h" +#include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/profile_sync_service_factory.h" #include "components/bookmarks/browser/bookmark_model.h" +#include "components/prefs/pref_service.h" #include "brave/components/brave_sync/buildflags/buildflags.h" #if BUILDFLAG(ENABLE_BRAVE_SYNC) @@ -23,16 +26,20 @@ BraveBookmarkModelLoadedObserver::BraveBookmarkModelLoadedObserver( void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded( BookmarkModel* model, bool ids_reassigned) { + if (!profile_->GetPrefs()->GetBoolean(kOtherBookmarksMigrated)) { #if BUILDFLAG(ENABLE_BRAVE_SYNC) - BraveProfileSyncServiceImpl* brave_profile_service = + BraveProfileSyncServiceImpl* brave_profile_service = static_cast( ProfileSyncServiceFactory::GetForProfile(profile_)); - // When sync is enabled, we need to send migration records to other devices so - // it is handled in BraveProfileSyncServiceImpl::OnSyncReady - if (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled()) - BraveMigrateOtherNode(model); + // When sync is enabled, we need to send migration records to other devices + // so it is handled in BraveProfileSyncServiceImpl::OnSyncReady + if (!brave_profile_service || + (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled())) + BraveMigrateOtherNodeFolder(model); #else - BraveMigrateOtherNode(model); + BraveMigrateOtherNodeFolder(model); #endif + profile_->GetPrefs()->SetBoolean(kOtherBookmarksMigrated, true); + } BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned); } diff --git a/browser/profiles/brave_bookmark_model_loaded_observer_browsertest.cc b/browser/profiles/brave_bookmark_model_loaded_observer_browsertest.cc new file mode 100644 index 000000000000..81122acef3e4 --- /dev/null +++ b/browser/profiles/brave_bookmark_model_loaded_observer_browsertest.cc @@ -0,0 +1,75 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/common/pref_names.h" +#include "chrome/browser/bookmarks/bookmark_model_factory.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "components/bookmarks/browser/bookmark_model.h" +#include "components/prefs/pref_service.h" + +using BraveBookmarkModelLoadedObserverBrowserTest = InProcessBrowserTest; + +// This test to mainly testing kOtherBookmarksMigrated, granular testing for +// migration is in BookmarkModelTest::BraveMigrateOtherNodeFolder + +namespace { + +void CreateOtherBookmarksFolder(bookmarks::BookmarkModel* model) { + const bookmarks::BookmarkNode* other_node_folder = model->AddFolder( + model->bookmark_bar_node(), model->bookmark_bar_node()->children().size(), + model->other_node()->GetTitledUrlNodeTitle()); + model->AddFolder(other_node_folder, 0, base::ASCIIToUTF16("A")); +} + +} // namespace + +IN_PROC_BROWSER_TEST_F(BraveBookmarkModelLoadedObserverBrowserTest, + PRE_OtherBookmarksMigration) { + Profile* profile = browser()->profile(); + + profile->GetPrefs()->SetBoolean(kOtherBookmarksMigrated, false); + + bookmarks::BookmarkModel* bookmark_model = + BookmarkModelFactory::GetForBrowserContext(profile); + CreateOtherBookmarksFolder(bookmark_model); +} + +IN_PROC_BROWSER_TEST_F(BraveBookmarkModelLoadedObserverBrowserTest, + OtherBookmarksMigration) { + Profile* profile = browser()->profile(); + EXPECT_TRUE(profile->GetPrefs()->GetBoolean(kOtherBookmarksMigrated)); + + bookmarks::BookmarkModel* bookmark_model = + BookmarkModelFactory::GetForBrowserContext(profile); + + ASSERT_EQ(bookmark_model->other_node()->children().size(), 1u); + ASSERT_EQ(bookmark_model->bookmark_bar_node()->children().size(), 0u); +} + +IN_PROC_BROWSER_TEST_F(BraveBookmarkModelLoadedObserverBrowserTest, + PRE_NoOtherBookmarksMigration) { + Profile* profile = browser()->profile(); + + profile->GetPrefs()->SetBoolean(kOtherBookmarksMigrated, true); + + bookmarks::BookmarkModel* bookmark_model = + BookmarkModelFactory::GetForBrowserContext(profile); + + CreateOtherBookmarksFolder(bookmark_model); +} + +IN_PROC_BROWSER_TEST_F(BraveBookmarkModelLoadedObserverBrowserTest, + NoOtherBookmarksMigration) { + Profile* profile = browser()->profile(); + EXPECT_TRUE(profile->GetPrefs()->GetBoolean(kOtherBookmarksMigrated)); + + bookmarks::BookmarkModel* bookmark_model = + BookmarkModelFactory::GetForBrowserContext(profile); + + ASSERT_EQ(bookmark_model->other_node()->children().size(), 0u); + ASSERT_EQ(bookmark_model->bookmark_bar_node()->children().size(), 1u); +} diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 4ae7769914d5..4e866e1ff3d6 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -57,8 +57,6 @@ source_set("ui") { "bookmark/bookmark_prefs_service.h", "bookmark/bookmark_prefs_service_factory.cc", "bookmark/bookmark_prefs_service_factory.h", - "bookmark/recently_used_folders_combo_model.cc", - "bookmark/recently_used_folders_combo_model.h", "brave_browser_command_controller.cc", "brave_browser_command_controller.h", "brave_browser_content_setting_bubble_model_delegate.cc", diff --git a/browser/ui/bookmark/recently_used_folders_combo_model.cc b/browser/ui/bookmark/recently_used_folders_combo_model.cc deleted file mode 100644 index 0c36e0b5d99e..000000000000 --- a/browser/ui/bookmark/recently_used_folders_combo_model.cc +++ /dev/null @@ -1,17 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "brave/browser/ui/bookmark/recently_used_folders_combo_model.h" - -#include "components/bookmarks/browser/bookmark_model.h" - -BraveRecentlyUsedFoldersComboModel::BraveRecentlyUsedFoldersComboModel( - bookmarks::BookmarkModel* model, - const bookmarks::BookmarkNode* node) - : RecentlyUsedFoldersComboModel(model, node) { - RecentlyUsedFoldersComboModel::RemoveNode(model->other_node()); -} - -BraveRecentlyUsedFoldersComboModel::~BraveRecentlyUsedFoldersComboModel() {} diff --git a/browser/ui/bookmark/recently_used_folders_combo_model.h b/browser/ui/bookmark/recently_used_folders_combo_model.h deleted file mode 100644 index 0854eefb0128..000000000000 --- a/browser/ui/bookmark/recently_used_folders_combo_model.h +++ /dev/null @@ -1,22 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef BRAVE_BROWSER_UI_BOOKMARK_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ -#define BRAVE_BROWSER_UI_BOOKMARK_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ - -#include "chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h" - -class BraveRecentlyUsedFoldersComboModel - : public RecentlyUsedFoldersComboModel { - public: - BraveRecentlyUsedFoldersComboModel(bookmarks::BookmarkModel* model, - const bookmarks::BookmarkNode* node); - ~BraveRecentlyUsedFoldersComboModel() override; - - private: - DISALLOW_COPY_AND_ASSIGN(BraveRecentlyUsedFoldersComboModel); -}; - -#endif // BRAVE_BROWSER_UI_BOOKMARK_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ diff --git a/chromium_src/chrome/browser/bookmarks/bookmark_model_factory.cc b/chromium_src/chrome/browser/bookmarks/bookmark_model_factory.cc index 19b26fba7d6a..fe8a987253dc 100644 --- a/chromium_src/chrome/browser/bookmarks/bookmark_model_factory.cc +++ b/chromium_src/chrome/browser/bookmarks/bookmark_model_factory.cc @@ -7,6 +7,4 @@ #define GetBrowserContextRedirectedInIncognito \ GetBrowserContextRedirectedInIncognitoOverride -#define ChromeBookmarkClient BraveBookmarkClient #include "../../../../../chrome/browser/bookmarks/bookmark_model_factory.cc" -#undef ChromeBookmarkClient diff --git a/chromium_src/chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc b/chromium_src/chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc deleted file mode 100644 index 6dc959d030c7..000000000000 --- a/chromium_src/chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc +++ /dev/null @@ -1,64 +0,0 @@ -/* Copyright (c) 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "../../../../../../../chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc" // NOLINT - -namespace extensions { - -TEST_F(BookmarksApiUnittest, Create) { - bookmarks::BookmarkModel* model = - BookmarkModelFactory::GetForBrowserContext(profile()); - { - auto create_function = base::MakeRefCounted(); - const std::string other_node_id = - base::NumberToString(model->other_node()->id()); - // Specify other_node() as parent - const GURL url("https://brave.com"); - api_test_utils::RunFunction( - create_function.get(), - base::StringPrintf( - R"([{"parentId": "%s", - "title": "brave", - "url": "%s"}])", other_node_id.c_str(), url.spec().c_str()), - profile()); - auto* node = model->GetMostRecentlyAddedUserNodeForURL(url); - EXPECT_EQ(node->url(), url); - EXPECT_EQ(node->parent(), model->bookmark_bar_node()); - } - { - auto create_function = base::MakeRefCounted(); - // default parent - const GURL url("https://brave2.com"); - api_test_utils::RunFunction( - create_function.get(), - base::StringPrintf( - R"([{"title": "brave2", - "url": "%s"}])", url.spec().c_str()), - profile()); - auto* node = model->GetMostRecentlyAddedUserNodeForURL(url); - EXPECT_EQ(node->url(), url); - EXPECT_EQ(node->parent(), model->bookmark_bar_node()); - } -} - -TEST_F(BookmarksApiUnittest, Move) { - bookmarks::BookmarkModel* model = - BookmarkModelFactory::GetForBrowserContext(profile()); - const std::string other_node_id = - base::NumberToString(model->other_node()->id()); - const bookmarks::BookmarkNode* node = model->AddURL( - model->bookmark_bar_node(), 0, base::ASCIIToUTF16("brave"), - GURL("https://brave.com")); - auto move_function = base::MakeRefCounted(); - api_test_utils::RunFunction( - move_function.get(), - base::StringPrintf( - R"(["%s", {"parentId": "%s"}])", - base::NumberToString(node->id()).c_str(), other_node_id.c_str()), - profile()); - EXPECT_EQ(node->parent(), model->bookmark_bar_node()); -} - -} // namespace extensions diff --git a/chromium_src/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h b/chromium_src/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h deleted file mode 100644 index 78ac7c28c1ce..000000000000 --- a/chromium_src/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h +++ /dev/null @@ -1,16 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_BOOKMARKS_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ -#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_BOOKMARKS_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ - -#define BRAVE_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ \ - private: \ - friend class BraveRecentlyUsedFoldersComboModel; \ - public: -#include "../../../../../../chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h" // NOLINT -#undef BRAVE_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ - -#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_BOOKMARKS_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ diff --git a/chromium_src/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc b/chromium_src/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc deleted file mode 100644 index e7e635279da9..000000000000 --- a/chromium_src/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc +++ /dev/null @@ -1,10 +0,0 @@ -/* Copyright 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "brave/browser/ui/bookmark/recently_used_folders_combo_model.h" - -#define RecentlyUsedFoldersComboModel BraveRecentlyUsedFoldersComboModel -#include "../../../../../../../chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc" -#undef RecentlyUsedFoldersComboModel diff --git a/chromium_src/components/bookmarks/browser/bookmark_model.cc b/chromium_src/components/bookmarks/browser/bookmark_model.cc index fb12a23218ae..6b946ed15c49 100644 --- a/chromium_src/components/bookmarks/browser/bookmark_model.cc +++ b/chromium_src/components/bookmarks/browser/bookmark_model.cc @@ -7,22 +7,25 @@ namespace bookmarks { -// Move bookmarks under "Other Bookmarks" permanent node to a same name folder -// at the end of "Bookmark Bar" permanent node -void BraveMigrateOtherNode(BookmarkModel* model) { +// Move bookmarks under "Other Bookmarks" folder from +// https://github.com/brave/brave-core/pull/3620 to original permanent node +void BraveMigrateOtherNodeFolder(BookmarkModel* model) { CHECK(model); CHECK(model->loaded()); // Model must be loaded at this point - if (!model->other_node()->children().empty()) { - const bookmarks::BookmarkNode* new_other_node = - model->AddFolder(model->bookmark_bar_node(), - model->bookmark_bar_node()->children().size(), - model->other_node()->GetTitledUrlNodeTitle()); - size_t children_size = model->other_node()->children().size(); + if (!model->bookmark_bar_node()->children().size()) + return; + const bookmarks::BookmarkNode* possible_other_node_folder = + model->bookmark_bar_node()->children().back().get(); + if (possible_other_node_folder->is_folder() && + (possible_other_node_folder->GetTitledUrlNodeTitle() == + model->other_node()->GetTitledUrlNodeTitle())) { + size_t children_size = possible_other_node_folder->children().size(); for (size_t i = 0; i < children_size; ++i) { - model->Move(model->other_node()->children().front().get(), new_other_node, - i); + model->Move(possible_other_node_folder->children().front().get(), + model->other_node(), i); } + model->Remove(possible_other_node_folder); } } diff --git a/chromium_src/components/bookmarks/browser/bookmark_model.h b/chromium_src/components/bookmarks/browser/bookmark_model.h index ee14081e4873..0e927ae602cb 100644 --- a/chromium_src/components/bookmarks/browser/bookmark_model.h +++ b/chromium_src/components/bookmarks/browser/bookmark_model.h @@ -15,7 +15,7 @@ class BraveSyncServiceTestDelayedLoadModel; #include "../../../../../components/bookmarks/browser/bookmark_model.h" namespace bookmarks { -void BraveMigrateOtherNode(BookmarkModel* model); +void BraveMigrateOtherNodeFolder(BookmarkModel* model); } // namespace bookmarks #endif // BRAVE_CHROMIUM_SRC_COMPONENTS_BOOKMARKS_BROWSER_BOOKMARK_MODEL_H_ diff --git a/chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc b/chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc index 4189783b0e7a..afce42c2569a 100644 --- a/chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc +++ b/chromium_src/components/bookmarks/browser/bookmark_model_unittest.cc @@ -6,40 +6,69 @@ #include "../../../../../components/bookmarks/browser/bookmark_model_unittest.cc" namespace bookmarks { -TEST_F(BookmarkModelTest, BraveMigrateOtherNode) { +TEST_F(BookmarkModelTest, BraveMigrateOtherNodeFolder) { // -- Bookmarks // |-- A - // -- Other Bookmarks - // |-- B - // | |--B1.com - // |-- C.com + // |-- Other Bookmarks + // |-- B + // | |--B1.com + // |-- C.com + const bookmarks::BookmarkNode* other_node_folder = + model_->AddFolder(model_->bookmark_bar_node(), + model_->bookmark_bar_node()->children().size(), + model_->other_node()->GetTitledUrlNodeTitle()); model_->AddFolder(model_->bookmark_bar_node(), 0, ASCIIToUTF16("A")); const BookmarkNode* folder = - model_->AddFolder(model_->other_node(), 0, ASCIIToUTF16("B")); + model_->AddFolder(other_node_folder, 0, ASCIIToUTF16("B")); model_->AddURL(folder, 0, ASCIIToUTF16("B1"), GURL("https://B1.com")); - model_->AddURL(model_->other_node(), 1, ASCIIToUTF16("C"), - GURL("https://B.com")); + model_->AddURL(other_node_folder, 1, ASCIIToUTF16("C.com"), + GURL("https://C.com")); // After migration, it should be // -- Bookmarks // |-- A - // |-- Other Bookmarks - // |-- B - // | |--B1.com - // |-- C.com - BraveMigrateOtherNode(model_.get()); - ASSERT_EQ(model_->other_node()->children().size(), 0u); - ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 2u); + // -- Other Bookmarks + // |-- B + // | |--B1.com + // |-- C.com + BraveMigrateOtherNodeFolder(model_.get()); + ASSERT_EQ(model_->other_node()->children().size(), 2u); + ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 1u); EXPECT_EQ(model_->bookmark_bar_node()->children()[0]->GetTitle(), ASCIIToUTF16("A")); - EXPECT_EQ(model_->bookmark_bar_node()->children()[1]->GetTitle(), - model_->other_node()->GetTitle()); - const BookmarkNode* new_other_node = - model_->bookmark_bar_node()->children()[1].get(); - EXPECT_EQ(new_other_node->children()[0]->GetTitle(), ASCIIToUTF16("B")); - EXPECT_EQ(new_other_node->children()[0]->children()[0]->GetTitle(), + EXPECT_EQ(model_->other_node()->children()[0]->GetTitle(), ASCIIToUTF16("B")); + EXPECT_EQ(model_->other_node()->children()[0]->children()[0]->GetTitle(), ASCIIToUTF16("B1")); - EXPECT_EQ(new_other_node->children()[1]->GetTitle(), ASCIIToUTF16("C")); + EXPECT_EQ(model_->other_node()->children()[1]->GetTitle(), + ASCIIToUTF16("C.com")); + + // Empty folder + model_->AddFolder(model_->bookmark_bar_node(), + model_->bookmark_bar_node()->children().size(), + model_->other_node()->GetTitledUrlNodeTitle()); + BraveMigrateOtherNodeFolder(model_.get()); + ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 1u); + ASSERT_EQ(model_->other_node()->children().size(), 2u); +} + +TEST_F(BookmarkModelTest, BraveMigrateOtherNodeFolderNotExist) { + ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 0u); + BraveMigrateOtherNodeFolder(model_.get()); + ASSERT_EQ(model_->other_node()->children().size(), 0u); + + const BookmarkNode* folder = + model_->AddFolder(model_->bookmark_bar_node(), 0, ASCIIToUTF16("Other B")); + model_->AddURL(folder, 0, ASCIIToUTF16("B1"), GURL("https://B1.com")); + BraveMigrateOtherNodeFolder(model_.get()); + ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 1u); + ASSERT_EQ(model_->other_node()->children().size(), 0u); + + model_->AddURL(model_->bookmark_bar_node(), 1, + model_->other_node()->GetTitledUrlNodeTitle(), + GURL("https://other.bookmarks")); + BraveMigrateOtherNodeFolder(model_.get()); + ASSERT_EQ(model_->bookmark_bar_node()->children().size(), 2u); + ASSERT_EQ(model_->other_node()->children().size(), 0u); } } // namespace bookmarks diff --git a/chromium_src/components/bookmarks/browser/bookmark_utils.cc b/chromium_src/components/bookmarks/browser/bookmark_utils.cc deleted file mode 100644 index 983d7aee4d50..000000000000 --- a/chromium_src/components/bookmarks/browser/bookmark_utils.cc +++ /dev/null @@ -1,26 +0,0 @@ -/* Copyright (c) 2019 The Brave Authors. All rights reserved. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "components/bookmarks/browser/bookmark_utils.h" -namespace bookmarks { -// DeleteBookmarkFolders won't get a chance to delete other_node(), even with -// malicious usage deleting bookmark_bar_node() and other_node() are both -// prohibited so no need to redirect other_node() to bookmark_bar_node() -const BookmarkNode* GetBookmarkNodeByID_ChromiumImpl(const BookmarkModel* model, - int64_t id); -} // namespace bookmarks -#define GetBookmarkNodeByID GetBookmarkNodeByID_ChromiumImpl -#include "../../../../../components/bookmarks/browser/bookmark_utils.cc" -#undef GetBookmarkNodeByID -namespace bookmarks { - -const BookmarkNode* GetBookmarkNodeByID(const BookmarkModel* model, - int64_t id) { - if (id == model->other_node()->id()) - id = model->bookmark_bar_node()->id(); - return GetBookmarkNodeByID_ChromiumImpl(model, id); -} - -} // namespace bookmarks diff --git a/chromium_src/components/sync/engine_impl/commit.cc b/chromium_src/components/sync/engine_impl/commit.cc index e085ed4e077a..4f1c16788a7f 100644 --- a/chromium_src/components/sync/engine_impl/commit.cc +++ b/chromium_src/components/sync/engine_impl/commit.cc @@ -30,6 +30,7 @@ SyncerError PostBraveCommit(sync_pb::ClientToServerMessage* message, #include "brave/components/brave_sync/jslib_const.h" #include "brave/components/brave_sync/jslib_messages.h" #include "brave/components/brave_sync/jslib_messages_fwd.h" +#include "brave/components/brave_sync/tools.h" #include "components/sync/base/time.h" #include "components/sync/base/unique_position.h" @@ -37,7 +38,8 @@ namespace syncer { namespace { using brave_sync::jslib::MetaInfo; using brave_sync::jslib::SyncRecord; -const char kBookmarkBarTag[] = "bookmark_bar"; +// from components/sync_bookmarks/bookmark_model_merger.cc +const char kOtherBookmarksTag[] = "other_bookmarks"; void CreateSuccessfulCommitResponse( const sync_pb::SyncEntity& entity, @@ -54,6 +56,28 @@ void CreateSuccessfulCommitResponse( response->set_id_string(new_object_id); } +std::unique_ptr CreateOtherBookmarksRecord(SyncRecord* child) { + auto record = std::make_unique(); + record->objectData = brave_sync::jslib_const::SyncObjectData_BOOKMARK; + auto bookmark = std::make_unique(); + bookmark->site.title = brave_sync::tools::kOtherNodeName; + bookmark->site.customTitle = brave_sync::tools::kOtherNodeName; + // Special order reserved for "Other Bookmarks" folder, it only has effect on + // mobile. On desktop, it is used to distinguish "Other Bookmarks" from normal + // same name folder + bookmark->order = brave_sync::tools::kOtherNodeOrder; + bookmark->site.creationTime = child->GetBookmark().site.creationTime; + bookmark->isFolder = true; + + DCHECK(child->has_bookmark()); + record->objectId = child->GetBookmark().parentFolderObjectId; + record->action = brave_sync::jslib::SyncRecord::Action::A_CREATE; + record->syncTimestamp = child->syncTimestamp; + record->SetBookmark(std::move(bookmark)); + + return record; +} + brave_sync::RecordsListPtr ConvertCommitsToBraveRecords( sync_pb::ClientToServerMessage* message, sync_pb::ClientToServerResponse* response) { @@ -61,6 +85,7 @@ brave_sync::RecordsListPtr ConvertCommitsToBraveRecords( std::make_unique(); const sync_pb::CommitMessage& commit_message = message->commit(); const std::string cache_guid = commit_message.cache_guid(); + bool other_bookmarks_record_created = false; for (int i = 0; i < commit_message.entries_size(); ++i) { sync_pb::SyncEntity entity = commit_message.entries(i); std::string new_object_id; @@ -79,8 +104,8 @@ brave_sync::RecordsListPtr ConvertCommitsToBraveRecords( ProtoTimeToTime(bm_specifics.creation_time_us()); bookmark->site.favicon = bm_specifics.icon_url(); bookmark->isFolder = entity.folder(); - // only mattters for direct children of permanent nodes - bookmark->hideInToolbar = entity.parent_id_string() != kBookmarkBarTag; + // only matters for direct children of permanent nodes + bookmark->hideInToolbar = entity.parent_id_string() == kOtherBookmarksTag; bool skip_record = false; for (int i = 0; i < bm_specifics.meta_info_size(); ++i) { @@ -126,6 +151,12 @@ brave_sync::RecordsListPtr ConvertCommitsToBraveRecords( bookmark->metaInfo.push_back(metaInfo); record->SetBookmark(std::move(bookmark)); + + if (!other_bookmarks_record_created && + entity.parent_id_string() == kOtherBookmarksTag) { + record_list->push_back(CreateOtherBookmarksRecord(record.get())); + other_bookmarks_record_created = true; + } if (!skip_record) record_list->push_back(std::move(record)); } diff --git a/chromium_src/components/sync/engine_impl/get_updates_processor.cc b/chromium_src/components/sync/engine_impl/get_updates_processor.cc index 8822f7b34eda..ff0113e7e8e5 100644 --- a/chromium_src/components/sync/engine_impl/get_updates_processor.cc +++ b/chromium_src/components/sync/engine_impl/get_updates_processor.cc @@ -155,10 +155,16 @@ void AddBookmarkNode(sync_pb::SyncEntity* entity, const SyncRecord* record) { sync_pb::EntitySpecifics specifics; AddDefaultFieldValue(BOOKMARKS, &specifics); entity->set_id_string(record->objectId); - if (!bookmark_record.parentFolderObjectId.empty()) - entity->set_parent_id_string(bookmark_record.parentFolderObjectId); - else + if (!bookmark_record.parentFolderObjectId.empty()) { + // parentFolderObjectId is used for mobile to treat "Other Bookmarks" as + // normal folder + if (bookmark_record.hideInToolbar) + entity->set_parent_id_string(std::string(kOtherBookmarksFolderServerTag)); + else + entity->set_parent_id_string(bookmark_record.parentFolderObjectId); + } else { entity->set_parent_id_string(std::string(kBookmarkBarFolderServerTag)); + } entity->set_non_unique_name(bookmark_record.site.TryGetNonEmptyTitle()); entity->set_folder(bookmark_record.isFolder); diff --git a/common/pref_names.cc b/common/pref_names.cc index 9d9875730e50..9f0a1a5a056a 100644 --- a/common/pref_names.cc +++ b/common/pref_names.cc @@ -77,6 +77,7 @@ const char kAlwaysShowBookmarkBarOnNTP[] = const char kRemoteDebuggingEnabled[] = "brave.remote_debugging_enabled"; const char kAutocompleteEnabled[] = "brave.autocomplete_enabled"; const char kBraveDarkMode[] = "brave.dark_mode"; +const char kOtherBookmarksMigrated[] = "brave.other_bookmarks_migrated"; #if defined(OS_ANDROID) const char kDesktopModeEnabled[] = "brave.desktop_mode_enabled"; diff --git a/common/pref_names.h b/common/pref_names.h index 700efe66871c..7188e2725c3d 100644 --- a/common/pref_names.h +++ b/common/pref_names.h @@ -67,6 +67,7 @@ extern const char kAlwaysShowBookmarkBarOnNTP[]; extern const char kRemoteDebuggingEnabled[]; extern const char kAutocompleteEnabled[]; extern const char kBraveDarkMode[]; +extern const char kOtherBookmarksMigrated[]; #if defined(OS_ANDROID) extern const char kDesktopModeEnabled[]; diff --git a/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index f17afead0445..8cb90e479992 100644 --- a/components/brave_sync/BUILD.gn +++ b/components/brave_sync/BUILD.gn @@ -37,6 +37,7 @@ if (enable_brave_sync) { ":public", ":static_resources", "//base", + "//brave/common:common", "//chrome/common", "//components/bookmarks/browser", "//components/bookmarks/common", diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 5b24a7df4cdb..68e40caa6401 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -13,6 +13,7 @@ #include "base/metrics/histogram_macros.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" +#include "brave/common/pref_names.h" #include "brave/components/brave_sync/brave_sync_prefs.h" #include "brave/components/brave_sync/brave_sync_service_observer.h" #include "brave/components/brave_sync/client/brave_sync_client_impl.h" @@ -28,6 +29,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/chrome_sync_client.h" #include "components/bookmarks/browser/bookmark_model.h" +#include "components/prefs/pref_service.h" #include "components/signin/public/identity_manager/account_info.h" #include "components/sync/engine_impl/syncer.h" #include "content/public/browser/browser_thread.h" @@ -219,6 +221,8 @@ void BraveProfileSyncServiceImpl::OnNudgeSyncCycle(RecordsListPtr records) { for (auto& record : *records) { record->deviceId = brave_sync_prefs_->GetThisDeviceId(); + CheckOtherBookmarkRecord(record.get()); + CheckOtherBookmarkChildRecord(record.get()); } if (!records->empty()) { SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, std::move(records)); @@ -513,7 +517,10 @@ void BraveProfileSyncServiceImpl::OnSyncReadyBookmarksModelLoaded() { ProfileSyncService::GetUserSettings()->SetSyncRequested(true); } - BraveMigrateOtherNode(model_); + if (!sync_client_->GetPrefService()->GetBoolean(kOtherBookmarksMigrated)) { + BraveMigrateOtherNodeFolder(model_); + sync_client_->GetPrefService()->SetBoolean(kOtherBookmarksMigrated, true); + } } syncer::ModelTypeSet BraveProfileSyncServiceImpl::GetPreferredDataTypes() @@ -579,6 +586,17 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords( OnResolvedPreferences(*records.get()); } else if (category_name == kBookmarks) { for (auto& record : *records) { + if (IsOtherBookmarksFolder(record.get())) { + bool pass_to_syncer = false; + ProcessOtherBookmarksFolder(record.get(), &pass_to_syncer); + if (!pass_to_syncer) { + // We don't process "Other Bookmarks" folder in syncer when + // "Other Bookmaks" doesn't need to be remapped. + std::move(record); + continue; + } + } + ProcessOtherBookmarksChildren(record.get()); LoadSyncEntityInfo(record.get()); // We have to cache records when this function is triggered during // non-PollCycle (ex. compaction update) and wait for next available poll @@ -587,6 +605,7 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords( pending_received_records_ = std::make_unique(); pending_received_records_->push_back(std::move(record)); } + // Send records to syncer if (get_record_cb_) { backend_task_runner_->PostTask( @@ -713,13 +732,14 @@ void BraveProfileSyncServiceImpl::SetPermanentNodesOrder( std::string order; model_->bookmark_bar_node()->GetMetaInfo("order", &order); if (order.empty()) { - model_->SetNodeMetaInfo(model_->bookmark_bar_node(), "order", - base_order + "1"); + tools::AsMutable(model_->bookmark_bar_node()) + ->SetMetaInfo("order", base_order + "1"); } order.clear(); model_->other_node()->GetMetaInfo("order", &order); if (order.empty()) { - model_->SetNodeMetaInfo(model_->other_node(), "order", base_order + "2"); + tools::AsMutable(model_->other_node())->SetMetaInfo("order", + tools::kOtherNodeOrder); } brave_sync_prefs_->SetMigratedBookmarksVersion(2); } @@ -741,10 +761,8 @@ BraveProfileSyncServiceImpl::BookmarkNodeToSyncBookmark( // bookmark->site.lastAccessedTime - ignored bookmark->site.creationTime = node->date_added(); bookmark->site.favicon = node->icon_url() ? node->icon_url()->spec() : ""; - // Url may have type OTHER_NODE if it is in Deleted Bookmarks - bookmark->isFolder = (node->type() != bookmarks::BookmarkNode::URL && - node->type() != bookmarks::BookmarkNode::OTHER_NODE); - bookmark->hideInToolbar = node->parent() != model_->bookmark_bar_node(); + bookmark->isFolder = node->type() != bookmarks::BookmarkNode::URL; + bookmark->hideInToolbar = node->parent() == model_->other_node(); std::string object_id; node->GetMetaInfo("object_id", &object_id); @@ -787,9 +805,11 @@ void BraveProfileSyncServiceImpl::SaveSyncEntityInfo( int64_t version; bool result = base::StringToInt64(meta_info.value, &version); DCHECK(result); - model_->SetNodeMetaInfo(node, meta_info.key, std::to_string(++version)); + tools::AsMutable(node) + ->SetMetaInfo(meta_info.key, std::to_string(++version)); } else { - model_->SetNodeMetaInfo(node, meta_info.key, meta_info.value); + tools::AsMutable(node) + ->SetMetaInfo(meta_info.key, meta_info.value); } } } @@ -812,6 +832,131 @@ void BraveProfileSyncServiceImpl::LoadSyncEntityInfo( } } +bool BraveProfileSyncServiceImpl::IsOtherBookmarksFolder( + const jslib::SyncRecord* record) const { + auto bookmark = record->GetBookmark(); + if (!bookmark.isFolder) + return false; + + std::string other_node_object_id; + if (model_->other_node()->GetMetaInfo("object_id", &other_node_object_id) && + record->objectId == other_node_object_id) + return true; + + if (bookmark.order == tools::kOtherNodeOrder && + bookmark.site.title == tools::kOtherNodeName && + bookmark.site.customTitle == tools::kOtherNodeName) { + return true; + } + + return false; +} + +void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( + const jslib::SyncRecord* record, + bool* pass_to_syncer) { + std::string other_node_object_id; + // Save object_id for late joined desktop to catch up with current id + // iteration + if (!model_->other_node()->GetMetaInfo("object_id", &other_node_object_id) && + record->action == jslib::SyncRecord::Action::A_CREATE) { + tools::AsMutable(model_->other_node())->SetMetaInfo("object_id", + record->objectId); + } else { + // Out-of-date desktop will poll remote records before commiting local + // changes so we won't get old iteration id. That is why we always take + // remote id when it is different than what we have to catch up with current + // iteration + if (other_node_object_id != record->objectId) { + tools::AsMutable(model_->other_node())->SetMetaInfo("object_id", + record->objectId); + } + // DELETE won't reach here, because [DELETE, null] => [] in + // resolve-sync-objects but children records will go through. And we don't + // need to regenerate new object id for it. + + // Handle MOVE, RENAME + // REORDER (move under same parent) will be ignored + // Update will be resolved as Create because [UPDATE, null] => [CREATE] + auto bookmark = record->GetBookmark(); + if ((bookmark.order != tools::kOtherNodeOrder && + !bookmark.parentFolderObjectId.empty()) || + bookmark.site.title != tools::kOtherNodeName || + bookmark.site.customTitle != tools::kOtherNodeName) { + // Generate next iteration object id from current object_id which will be + // used to mapped normal folder + tools::AsMutable( + model_->other_node()) + ->SetMetaInfo("object_id", + tools::GenerateObjectIdForOtherNode( + other_node_object_id)); + *pass_to_syncer = true; + + // Add records to move direct children of other_node to this new folder + // with existing object id of the old "Other Bookmarks" folder + auto records_to_send = std::make_unique(); + for (size_t i = 0; i < model_->other_node()->children().size(); ++i) { + auto sync_record = + BookmarkNodeToSyncBookmark(model_->other_node()->children()[i].get()); + sync_record->mutable_bookmark()->parentFolderObjectId = + record->objectId; + sync_record->mutable_bookmark()->hideInToolbar = false; + sync_record->mutable_bookmark()->order = + bookmark.order + "." + std::to_string(i + 1); + LoadSyncEntityInfo(sync_record.get()); + + auto record_to_send = SyncRecord::Clone(*sync_record); + + // Append changes to remote records + if (!pending_received_records_) + pending_received_records_ = std::make_unique(); + pending_received_records_->push_back(std::move(sync_record)); + + // Send changes to other desktops + records_to_send->push_back(std::move(record_to_send)); + } + SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, + std::move(records_to_send)); + } + } +} + +void BraveProfileSyncServiceImpl::ProcessOtherBookmarksChildren( + jslib::SyncRecord* record) { + std::string other_node_object_id; + if (model_->other_node()->GetMetaInfo("object_id", &other_node_object_id) && + record->GetBookmark().parentFolderObjectId == other_node_object_id) { + record->mutable_bookmark()->hideInToolbar = true; + } +} +void BraveProfileSyncServiceImpl::CheckOtherBookmarkRecord( + jslib::SyncRecord* record) { + if (!IsOtherBookmarksFolder(record)) + return; + // Check if record has latest object id before sending + std::string other_node_object_id; + if (!model_->other_node()->GetMetaInfo("object_id", &other_node_object_id)) { + // first iteration + other_node_object_id = tools::GenerateObjectIdForOtherNode(std::string()); + tools::AsMutable(model_->other_node())->SetMetaInfo("object_id", + other_node_object_id); + } + DCHECK(!other_node_object_id.empty()); + if (record->objectId != other_node_object_id) + record->objectId = other_node_object_id; +} + +void BraveProfileSyncServiceImpl::CheckOtherBookmarkChildRecord( + jslib::SyncRecord* record) { + if (record->GetBookmark().hideInToolbar && + record->GetBookmark().parentFolderObjectId.empty()) { + std::string other_node_object_id; + model_->other_node()->GetMetaInfo("object_id", &other_node_object_id); + DCHECK(!other_node_object_id.empty()); + record->mutable_bookmark()->parentFolderObjectId = other_node_object_id; + } +} + void BraveProfileSyncServiceImpl::CreateResolveList( const std::vector>& records, SyncRecordAndExistingList* records_and_existing_objects) { diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index 2582116779f7..6d3effe0c75c 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -53,6 +53,11 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2Migration); FORWARD_DECLARE_TEST(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId); FORWARD_DECLARE_TEST(BraveSyncServiceTestDelayedLoadModel, OnSyncReadyModelNotYetLoaded); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, IsOtherBookmarksFolder); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, ProcessOtherBookmarksFolder); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, ProcessOtherBookmarksChildren); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, CheckOtherBookmarkRecord); +FORWARD_DECLARE_TEST(BraveSyncServiceTest, CheckOtherBookmarkChildRecord); class BraveSyncServiceTest; @@ -216,6 +221,16 @@ class BraveProfileSyncServiceImpl DeviceIdV2MigrationDupDeviceId); FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTestDelayedLoadModel, OnSyncReadyModelNotYetLoaded); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, + IsOtherBookmarksFolder); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, + ProcessOtherBookmarksFolder); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, + ProcessOtherBookmarksChildren); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, + CheckOtherBookmarkRecord); + FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, + CheckOtherBookmarkChildRecord); friend class ::BraveSyncServiceTest; @@ -249,6 +264,16 @@ class BraveProfileSyncServiceImpl void SaveSyncEntityInfo(const jslib::SyncRecord* record); void LoadSyncEntityInfo(jslib::SyncRecord* record); + bool IsOtherBookmarksFolder(const jslib::SyncRecord* record) const; + // Handling "Other Bookmarks" remote records + void ProcessOtherBookmarksFolder(const jslib::SyncRecord* record, + bool* pass_to_syncer); + // Handling direct children of "Other Bookmarks" remote records + void ProcessOtherBookmarksChildren(jslib::SyncRecord* record); + // Check and correct info before sending records + void CheckOtherBookmarkRecord(jslib::SyncRecord* record); + void CheckOtherBookmarkChildRecord(jslib::SyncRecord* record); + void CreateResolveList( const std::vector>& records, SyncRecordAndExistingList* records_and_existing_objects); diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 92bf3431d621..ebb005a15a6d 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -96,9 +96,18 @@ using brave_sync::BraveSyncService; using brave_sync::BraveSyncServiceObserver; using brave_sync::MockBraveSyncClient; using brave_sync::RecordsList; +using brave_sync::RecordsListPtr; using brave_sync::SimpleBookmarkSyncRecord; using brave_sync::SimpleDeviceRecord; +using brave_sync::SimpleFolderSyncRecord; +using brave_sync::jslib_const::kBookmarks; +using brave_sync::jslib_const::kPreferences; using brave_sync::jslib::SyncRecord; +using brave_sync::tools::AsMutable; +using brave_sync::tools::GenerateObjectId; +using brave_sync::tools::GenerateObjectIdForOtherNode; +using brave_sync::tools::kOtherNodeName; +using brave_sync::tools::kOtherNodeOrder; using testing::_; using testing::AtLeast; @@ -146,6 +155,20 @@ MATCHER_P3(ContainsDeviceRecord, return false; } +MATCHER_P(MatchBookmarksRecords, + records, + "Match bookmark sync records") { + if (arg.size() != records->size()) + return false; + for (size_t i = 0; i < arg.size(); ++i) { + if (!arg.at(i)->has_bookmark() || !records->at(i)->has_bookmark()) + return false; + if (!arg.at(i)->Matches(*records->at(i).get())) + return false; + } + return true; +} + base::TimeDelta g_overridden_time_delta; base::Time g_overridden_now; @@ -470,7 +493,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { EXPECT_TRUE(DevicesContains(devices.get(), "3", "beef03", "device3")); EXPECT_CALL(*sync_client(), - SendSyncRecords("PREFERENCES", + SendSyncRecords(kPreferences, ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device3", "beef03"))) .Times(1); @@ -572,7 +595,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { EXPECT_TRUE(DevicesContains(devices.get(), "2", "beef02", "device2")); EXPECT_CALL(*sync_client(), - SendSyncRecords("PREFERENCES", + SendSyncRecords(kPreferences, ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device1", "beef01"))) .Times(1); @@ -643,7 +666,6 @@ void BraveSyncServiceTest::VerifyResetDone() { } TEST_F(BraveSyncServiceTest, OnResetSync) { - using brave_sync::jslib_const::kPreferences; EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())) .Times(AtLeast(3)); @@ -698,7 +720,6 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { } TEST_F(BraveSyncServiceTest, OnResetSyncWhenOffline) { - using brave_sync::jslib_const::kPreferences; EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())) .Times(AtLeast(3)); @@ -811,7 +832,7 @@ TEST_F(BraveSyncServiceTest, OnSaveBookmarksBaseOrder) { EXPECT_EQ(order, "1.1.1"); order.clear(); model()->other_node()->GetMetaInfo("order", &order); - EXPECT_EQ(order, "1.1.2"); + EXPECT_EQ(order, kOtherNodeOrder); EXPECT_EQ(brave_sync_prefs()->GetMigratedBookmarksVersion(), 2); } @@ -836,7 +857,7 @@ TEST_F(BraveSyncServiceTest, SetThisDeviceCreatedTime) { brave_sync::GetRecordsCallback on_get_records = base::BindOnce(&OnGetRecordsStub); base::WaitableEvent we; - EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES", _)); + EXPECT_CALL(*sync_client(), SendSyncRecords(kPreferences, _)); EXPECT_CALL(*sync_client(), SendFetchSyncRecords); sync_service()->OnPollSyncCycle(std::move(on_get_records), &we); @@ -881,7 +902,7 @@ TEST_F(BraveSyncServiceTest, OnGetExistingObjects) { EXPECT_CALL(*sync_client(), SendResolveSyncRecords).Times(1); auto records = std::make_unique(); - sync_service()->OnGetExistingObjects(brave_sync::jslib_const::kBookmarks, + sync_service()->OnGetExistingObjects(kBookmarks, std::move(records), base::Time(), false); } @@ -941,7 +962,7 @@ TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode_Reset_SetupAgain) { ASSERT_EQ(devices->size(), 1u); EXPECT_TRUE(DevicesContains(devices.get(), "0", "beef00", "this_device")); EXPECT_CALL(*sync_client(), - SendSyncRecords("PREFERENCES", + SendSyncRecords(kPreferences, ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "this_device", "beef00"))) .Times(1); @@ -979,7 +1000,6 @@ TEST_F(BraveSyncServiceTest, OnSetupSyncHaveCode_Reset_SetupAgain) { } TEST_F(BraveSyncServiceTest, ExponentialResend) { - using brave_sync::jslib_const::kBookmarks; bookmarks::AddIfNotBookmarked(model(), GURL("https://a.com/"), base::ASCIIToUTF16("A.com")); // Explicitly set sync_timestamp, object_id and order because it is supposed @@ -1070,8 +1090,6 @@ TEST_F(BraveSyncServiceTest, ExponentialResend) { } TEST_F(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords) { - using brave_sync::jslib_const::kPreferences; - EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(1); EXPECT_CALL(*observer(), OnSyncStateChanged); brave_sync_prefs()->SetSyncEnabled(true); @@ -1136,7 +1154,6 @@ TEST_F(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords) { } TEST_F(BraveSyncServiceTest, SendCompact) { - using brave_sync::jslib_const::kBookmarks; EXPECT_EQ(brave_sync_prefs()->GetLastCompactTimeBookmarks(), base::Time()); EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1); EXPECT_CALL(*sync_client(), SendFetchSyncRecords).Times(1); @@ -1170,7 +1187,6 @@ TEST_F(BraveSyncServiceTest, MigratePrevSeed) { } TEST_F(BraveSyncServiceTest, InitialFetchesStartWithZero) { - using brave_sync::jslib_const::kBookmarks; EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1); EXPECT_CALL(*sync_client(), SendFetchSyncRecords(_, base::Time(), _)) .Times(1); @@ -1310,3 +1326,277 @@ TEST_F(BraveSyncServiceTest, DeviceIdV2MigrationDupDeviceId) { base::BindOnce(&OnGetRecordsStub); sync_service()->OnPollSyncCycle(std::move(on_get_records), &we); } + +TEST_F(BraveSyncServiceTest, IsOtherBookmarksFolder) { + AsMutable(model()->other_node()) + ->SetMetaInfo("object_id", GenerateObjectIdForOtherNode(std::string())); + auto record_matches_id = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode(std::string()), + "", "", "", "", false, ""); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record_matches_id.get())); + + auto record_matches_traits = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); + EXPECT_TRUE(sync_service() + ->IsOtherBookmarksFolder(record_matches_traits.get())); + + auto record_not_match1 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + kOtherNodeName, "", "", "", false, kOtherNodeName); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_match1.get())); + + auto record_not_match2 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + "What Bookmarks", kOtherNodeOrder, "", "", false, kOtherNodeName); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_match2.get())); + + auto record_not_match3 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + kOtherNodeName, kOtherNodeOrder, "", "", false, "No Bookmarks"); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_match3.get())); + + auto record_not_folder = SimpleBookmarkSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode(std::string()), + "", "", "", "", "", false); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_folder.get())); +} + +TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksFolder) { + const std::string object_id_iter1 = + GenerateObjectIdForOtherNode(std::string()); + auto record1 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter1, + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record1.get())); + + bool pass_to_syncer = false; + sync_service()->ProcessOtherBookmarksFolder(record1.get(), &pass_to_syncer); + EXPECT_FALSE(pass_to_syncer); + std::string other_node_object_id; + model()->other_node()->GetMetaInfo("object_id", &other_node_object_id); + EXPECT_EQ(other_node_object_id, object_id_iter1); + + const std::string object_id_iter2 = + GenerateObjectIdForOtherNode(object_id_iter1); + // Now we emulate our object id is out-dated, need to catch up with current + // one + auto record2 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter2, + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record2.get())); + + pass_to_syncer = false; + sync_service()->ProcessOtherBookmarksFolder(record2.get(), &pass_to_syncer); + EXPECT_FALSE(pass_to_syncer); + other_node_object_id = ""; + model()->other_node()->GetMetaInfo("object_id", &other_node_object_id); + EXPECT_EQ(other_node_object_id, object_id_iter2); + + // Prepare children of other_node() + const bookmarks::BookmarkNode* folder_a = + model()->AddFolder(model()->other_node(), 0, base::ASCIIToUTF16("A")); + const bookmarks::BookmarkNode* bookmark_a1 = + model()->AddURL(model()->other_node(), 1, base::ASCIIToUTF16("A1"), + GURL("https://a1.com")); + const std::string folder_a_object_id = GenerateObjectId(); + const std::string bookmark_a1_object_id = GenerateObjectId(); + AsMutable(folder_a)->SetMetaInfo("object_id", folder_a_object_id); + AsMutable(bookmark_a1)->SetMetaInfo("object_id", bookmark_a1_object_id); + AsMutable(folder_a)->SetMetaInfo("order", + std::string(kOtherNodeOrder) + ".1"); + AsMutable(bookmark_a1)->SetMetaInfo("order", + std::string(kOtherNodeOrder) + ".2"); + AsMutable(folder_a) + ->SetMetaInfo("sync_timestamp", + std::to_string(base::Time::Now().ToJsTime())); + AsMutable(bookmark_a1) + ->SetMetaInfo("sync_timestamp", + std::to_string(base::Time::Now().ToJsTime())); + + // Prepare a folder for "Other Bookmarks" to move to + const bookmarks::BookmarkNode* folder_b = + model()->AddFolder(model()->bookmark_bar_node(), 0, + base::ASCIIToUTF16("B")); + const std::string folder_b_object_id = GenerateObjectId(); + AsMutable(folder_b)->SetMetaInfo("object_id", folder_b_object_id); + AsMutable(folder_b)->SetMetaInfo("order", "1.0.1.1"); + AsMutable(folder_b) + ->SetMetaInfo("sync_timestamp", + std::to_string(base::Time::Now().ToJsTime())); + // ========================================================================== + // Emulate MOVE + const std::string object_id_iter3 = + GenerateObjectIdForOtherNode(object_id_iter2); + auto record3 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter2, + kOtherNodeName, "1.0.1.1.1", folder_b_object_id, "", + false, kOtherNodeName); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record3.get())); + + auto record_a = SimpleFolderSyncRecord( + SyncRecord::Action::A_UPDATE, folder_a_object_id, + "A", "1.0.1.1.1.1", object_id_iter2, "", + false, "A"); + + auto record_a1 = SimpleBookmarkSyncRecord( + SyncRecord::Action::A_UPDATE, bookmark_a1_object_id, "https://a1.com/", + "A1", "1.0.1.1.1.2", object_id_iter2, "", false); + + // Expect sending updates + auto record_a_to_send = SyncRecord::Clone(*record_a); + auto record_a1_to_send = SyncRecord::Clone(*record_a1); + RecordsListPtr records_to_send = std::make_unique(); + records_to_send->push_back(std::move(record_a_to_send)); + records_to_send->push_back(std::move(record_a1_to_send)); + EXPECT_CALL(*sync_client(), + SendSyncRecords(kBookmarks, + MatchBookmarksRecords( + std::move(records_to_send.get())))) + .Times(1); + + pass_to_syncer = false; + sync_service()->ProcessOtherBookmarksFolder(record3.get(), &pass_to_syncer); + EXPECT_TRUE(pass_to_syncer); + other_node_object_id = ""; + model()->other_node()->GetMetaInfo("object_id", &other_node_object_id); + EXPECT_EQ(other_node_object_id, object_id_iter3); + + // Move folder A to "Other Bookmark" folder under folder B + EXPECT_EQ(sync_service()->pending_received_records_->size(), 2u); + EXPECT_TRUE(sync_service()->pending_received_records_->at(0) + ->GetBookmark().Matches(record_a->GetBookmark())); + EXPECT_TRUE(sync_service()->pending_received_records_->at(1) + ->GetBookmark().Matches(record_a1->GetBookmark())); + // ========================================================================== + // Emulate RENAME + sync_service()->pending_received_records_->clear(); + const std::string object_id_iter4 = + GenerateObjectIdForOtherNode(object_id_iter3); + const std::string new_other_node_name = "Mother Bookmarks"; + auto record4 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter3, + new_other_node_name, kOtherNodeOrder, "", "", + false, new_other_node_name); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record4.get())); + + record_a = SimpleFolderSyncRecord( + SyncRecord::Action::A_UPDATE, folder_a_object_id, + "A", std::string(kOtherNodeOrder) + ".1", object_id_iter3, "", + false, "A"); + record_a1 = SimpleBookmarkSyncRecord( + SyncRecord::Action::A_UPDATE, bookmark_a1_object_id, "https://a1.com/", + "A1", std::string(kOtherNodeOrder) + ".2", object_id_iter3, "", false); + + // Expect sending updates + record_a_to_send = SyncRecord::Clone(*record_a); + record_a1_to_send = SyncRecord::Clone(*record_a1); + records_to_send = std::make_unique(); + records_to_send->push_back(std::move(record_a_to_send)); + records_to_send->push_back(std::move(record_a1_to_send)); + EXPECT_CALL(*sync_client(), + SendSyncRecords(kBookmarks, + MatchBookmarksRecords( + std::move(records_to_send.get())))) + .Times(1); + + pass_to_syncer = false; + sync_service()->ProcessOtherBookmarksFolder(record4.get(), &pass_to_syncer); + EXPECT_TRUE(pass_to_syncer); + other_node_object_id = ""; + model()->other_node()->GetMetaInfo("object_id", &other_node_object_id); + EXPECT_EQ(other_node_object_id, object_id_iter4); + + // Move folder A to "Mother Bookmark" folder + EXPECT_EQ(sync_service()->pending_received_records_->size(), 2u); + EXPECT_TRUE(sync_service()->pending_received_records_->at(0) + ->GetBookmark().Matches(record_a->GetBookmark())); + EXPECT_TRUE(sync_service()->pending_received_records_->at(1) + ->GetBookmark().Matches(record_a1->GetBookmark())); + // ========================================================================== + // Emulate REORDER (which will be ignored) + sync_service()->pending_received_records_->clear(); + auto record5 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter4, + kOtherNodeName, "1.0.1.2", "", "", + false, kOtherNodeName); + EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record5.get())); + + EXPECT_CALL(*sync_client(), SendSyncRecords(kBookmarks, _)).Times(0); + + pass_to_syncer = false; + sync_service()->ProcessOtherBookmarksFolder(record5.get(), &pass_to_syncer); + EXPECT_FALSE(pass_to_syncer); + other_node_object_id = ""; + model()->other_node()->GetMetaInfo("object_id", &other_node_object_id); + EXPECT_EQ(other_node_object_id, object_id_iter4); + + EXPECT_EQ(sync_service()->pending_received_records_->size(), 0u); +} + +TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksChildren) { + AsMutable(model()->other_node()) + ->SetMetaInfo("object_id", GenerateObjectIdForOtherNode(std::string())); + auto record_a = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, "", + "A", std::string(kOtherNodeOrder) + ".1", + GenerateObjectIdForOtherNode(std::string()), "", + false, "A"); + EXPECT_FALSE(record_a->GetBookmark().hideInToolbar); + sync_service()->ProcessOtherBookmarksChildren(record_a.get()); + EXPECT_TRUE(record_a->GetBookmark().hideInToolbar); + + auto record_a1 = SimpleBookmarkSyncRecord( + SyncRecord::Action::A_CREATE, "", "https://a1.com/", + "A1", "1.1.1.1", "", "", false); + EXPECT_FALSE(record_a1->GetBookmark().hideInToolbar); + sync_service()->ProcessOtherBookmarksChildren(record_a1.get()); + EXPECT_FALSE(record_a1->GetBookmark().hideInToolbar); +} + +TEST_F(BraveSyncServiceTest, CheckOtherBookmarkRecord) { + const std::string object_id_iter1 = + GenerateObjectIdForOtherNode(std::string()); + const std::string object_id_iter2 = + GenerateObjectIdForOtherNode(object_id_iter1); + // No object id for other_node yet + auto record = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, "", + kOtherNodeName, kOtherNodeOrder, "", "", + false, kOtherNodeName); + std::string other_node_object_id; + EXPECT_FALSE(model()->other_node()->GetMetaInfo("object_id", + &other_node_object_id)); + sync_service()->CheckOtherBookmarkRecord(record.get()); + EXPECT_TRUE(model()->other_node()->GetMetaInfo("object_id", + &other_node_object_id)); + EXPECT_EQ(other_node_object_id, object_id_iter1); + EXPECT_EQ(record->objectId, other_node_object_id); + + // Object id is out-dated + record = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter1, + kOtherNodeName, kOtherNodeOrder, "", "", + false, kOtherNodeName); + AsMutable(model()->other_node()) + ->SetMetaInfo("object_id", object_id_iter2); + sync_service()->CheckOtherBookmarkRecord(record.get()); + EXPECT_EQ(record->objectId, object_id_iter2); +} + +TEST_F(BraveSyncServiceTest, CheckOtherBookmarkChildRecord) { + const std::string object_id_iter1 = + GenerateObjectIdForOtherNode(std::string()); + AsMutable(model()->other_node()) + ->SetMetaInfo("object_id", object_id_iter1); + auto record_a1 = SimpleBookmarkSyncRecord( + SyncRecord::Action::A_CREATE, "", "https://a1.com/", + "A1", std::string(kOtherNodeOrder) + ".1" , "", "", true); + EXPECT_TRUE(record_a1->GetBookmark().parentFolderObjectId.empty()); + sync_service()->CheckOtherBookmarkChildRecord(record_a1.get()); + EXPECT_EQ(record_a1->GetBookmark().parentFolderObjectId, object_id_iter1); +} diff --git a/components/brave_sync/jslib_messages.cc b/components/brave_sync/jslib_messages.cc index 19b5cae7a1d0..599bd8075813 100644 --- a/components/brave_sync/jslib_messages.cc +++ b/components/brave_sync/jslib_messages.cc @@ -5,8 +5,10 @@ #include "brave/components/brave_sync/jslib_messages.h" +#include #include +#include "brave/components/brave_sync/jslib_const.h" #include "brave/components/brave_sync/values_conv.h" #include "base/logging.h" #include "base/values.h" @@ -31,6 +33,15 @@ std::unique_ptr Site::Clone(const Site& site) { return std::make_unique(site); } +bool Site::Matches(const Site& site) const { + if (location == site.location && + title == site.title && + customTitle == site.customTitle && + favicon == site.favicon) + return true; + return false; +} + std::string Site::TryGetNonEmptyTitle() const { return !title.empty() ? title : customTitle; } @@ -66,6 +77,16 @@ std::unique_ptr Bookmark::Clone(const Bookmark& bookmark) { return std::make_unique(bookmark); } +bool Bookmark::Matches(const Bookmark& bookmark) const { + if (site.Matches(bookmark.site) && + isFolder == bookmark.isFolder && + parentFolderObjectId == bookmark.parentFolderObjectId && + hideInToolbar == bookmark.hideInToolbar && + order == bookmark.order) + return true; + return false; +} + SiteSetting::SiteSetting() : zoomLevel(1.0f), shieldsUp(true), adControl(SiteSetting::AdControl::ADC_INVALID), cookieControl(SiteSetting::CookieControl::CC_INVALID), @@ -171,6 +192,20 @@ Bookmark* SyncRecord::mutable_bookmark() { return bookmark_.get(); } +bool SyncRecord::Matches(const SyncRecord& record) const { + if (action == record.action && deviceId == record.deviceId && + objectId == record.objectId && objectData == record.objectData && + has_bookmark() == record.has_bookmark() && + has_historysite() == record.has_historysite() && + has_sitesetting() == record.has_sitesetting() && + has_device() == record.has_device()) { + if (objectData == jslib_const::SyncObjectData_BOOKMARK) + return GetBookmark().Matches(record.GetBookmark()); + return true; + } + return false; +} + void SyncRecord::SetBookmark(std::unique_ptr bookmark) { DCHECK(!has_bookmark() && !has_historysite() && !has_sitesetting() && !has_device()); @@ -195,6 +230,15 @@ void SyncRecord::SetDevice(std::unique_ptr device) { device_ = std::move(device); } +std::ostream& operator<<(std::ostream& out, const Site& site) { + out << "location=" << site.location << ", "; + out << "title=" << site.title << ", "; + out << "customTitle=" << site.customTitle << ", "; + out << "creationTime=" << site.creationTime << ", "; + out << "favicon=" << site.favicon; + return out; +} + } // namespace jslib } // namespace brave_sync diff --git a/components/brave_sync/jslib_messages.h b/components/brave_sync/jslib_messages.h index 02db20352685..0dd67991dc1c 100644 --- a/components/brave_sync/jslib_messages.h +++ b/components/brave_sync/jslib_messages.h @@ -34,6 +34,9 @@ class Site { ~Site(); static std::unique_ptr Clone(const Site& site); + // Ignore creationTime + bool Matches(const Site& site) const; + std::string TryGetNonEmptyTitle() const; std::string location; @@ -62,6 +65,9 @@ class Bookmark { ~Bookmark(); static std::unique_ptr Clone(const Bookmark& bookmark); + // Ignore fields and metaInfo + bool Matches(const Bookmark& bookmark) const; + Site site; bool isFolder; std::string parentFolderObjectId; // bytes @@ -149,6 +155,9 @@ class SyncRecord { const Device& GetDevice() const; Bookmark* mutable_bookmark(); + // Ignore syncTimestamp, history, site_setting and device + bool Matches(const SyncRecord& record) const; + void SetBookmark(std::unique_ptr bookmark); void SetHistorySite(std::unique_ptr history_site); void SetSiteSetting(std::unique_ptr site_setting); @@ -163,6 +172,8 @@ class SyncRecord { std::unique_ptr device_; }; +std::ostream& operator<<(std::ostream& out, const Site& site); + } // namespace jslib } // namespace brave_sync diff --git a/components/brave_sync/syncer_helper.cc b/components/brave_sync/syncer_helper.cc index 391eb8460491..e3f5ca609ab6 100644 --- a/components/brave_sync/syncer_helper.cc +++ b/components/brave_sync/syncer_helper.cc @@ -13,18 +13,12 @@ namespace brave_sync { namespace { -// Get mutable node to prevent BookmarkMetaInfoChanged from being triggered -bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) { - return const_cast(node); -} - void SetOrder(const bookmarks::BookmarkNode* node, const std::string& parent_order) { DCHECK(!parent_order.empty()); int index = node->parent()->GetIndexOf(node); - bookmarks::BookmarkNode* parent = - const_cast(node->parent()); + bookmarks::BookmarkNode* parent = tools::AsMutable(node->parent()); auto* prev_node = index == 0 ? nullptr : parent->children()[index - 1].get(); auto* next_node = static_cast(index) == parent->children().size() - 1 @@ -41,7 +35,7 @@ void SetOrder(const bookmarks::BookmarkNode* node, std::string order = brave_sync::GetOrder(prev_order, next_order, parent_order); - AsMutable(node)->SetMetaInfo("order", order); + tools::AsMutable(node)->SetMetaInfo("order", order); } } // namespace @@ -92,19 +86,26 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) { if (object_id.empty()) { object_id = tools::GenerateObjectId(); } - AsMutable(node)->SetMetaInfo("object_id", object_id); + tools::AsMutable(node)->SetMetaInfo("object_id", object_id); std::string parent_object_id; + // other_node object id will be empty for the first time, it will be + // generated before sending commits node->parent()->GetMetaInfo("object_id", &parent_object_id); - AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id); + tools::AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id); std::string sync_timestamp; node->GetMetaInfo("sync_timestamp", &sync_timestamp); if (sync_timestamp.empty()) { sync_timestamp = std::to_string(base::Time::Now().ToJsTime()); - AsMutable(node)->SetMetaInfo("sync_timestamp", sync_timestamp); + tools::AsMutable(node)->SetMetaInfo("sync_timestamp", sync_timestamp); } DCHECK(!sync_timestamp.empty()); + // Set other_node to have same sync_timestamp as least added child + if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE) { + tools::AsMutable(node->parent())->SetMetaInfo("sync_timestamp", + sync_timestamp); + } } } // namespace brave_sync diff --git a/components/brave_sync/test_util.cc b/components/brave_sync/test_util.cc index e130bc65871b..8d2ce12e9f0e 100644 --- a/components/brave_sync/test_util.cc +++ b/components/brave_sync/test_util.cc @@ -58,7 +58,8 @@ SyncRecordPtr SimpleBookmarkSyncRecord(const int action, const std::string& title, const std::string& order, const std::string& parent_object_id, - const std::string& device_id) { + const std::string& device_id, + const bool hide_in_toolbar) { auto record = std::make_unique(); record->action = ConvertEnum(action, brave_sync::jslib::SyncRecord::Action::A_MIN, @@ -76,11 +77,12 @@ SyncRecordPtr SimpleBookmarkSyncRecord(const int action, bookmark->isFolder = false; // empty parentFolderObjectId means child of some permanent node bookmark->parentFolderObjectId = parent_object_id; - bookmark->hideInToolbar = true; + bookmark->hideInToolbar = hide_in_toolbar; bookmark->order = order; bookmark->site.location = location; bookmark->site.title = title; + bookmark->site.customTitle = title; record->SetBookmark(std::move(bookmark)); @@ -89,9 +91,11 @@ SyncRecordPtr SimpleBookmarkSyncRecord(const int action, SyncRecordPtr SimpleFolderSyncRecord( const int action, + const std::string& object_id, const std::string& title, const std::string& order, const std::string& parent_object_id, + const std::string& device_id, const bool hide_in_toolbar, const std::string& custom_title) { auto record = std::make_unique(); @@ -100,8 +104,8 @@ SyncRecordPtr SimpleFolderSyncRecord( brave_sync::jslib::SyncRecord::Action::A_MAX, brave_sync::jslib::SyncRecord::Action::A_INVALID); - record->deviceId = "3"; - record->objectId = tools::GenerateObjectId(); + record->deviceId = device_id; + record->objectId = object_id.empty() ? tools::GenerateObjectId() : object_id; record->objectData = "bookmark"; record->syncTimestamp = base::Time::Now(); diff --git a/components/brave_sync/test_util.h b/components/brave_sync/test_util.h index eb75bf762977..ceca7bb73405 100644 --- a/components/brave_sync/test_util.h +++ b/components/brave_sync/test_util.h @@ -68,13 +68,16 @@ SyncRecordPtr SimpleBookmarkSyncRecord(const int action, const std::string& title, const std::string& order, const std::string& parent_object_id, - const std::string& device_id = "3"); + const std::string& device_id, + const bool hide_in_toolbar = true); SyncRecordPtr SimpleFolderSyncRecord( const int action, + const std::string& object_id, const std::string& title, const std::string& order, const std::string& parent_object_id, + const std::string& device_id, const bool hide_in_toolbar, const std::string& custom_title); diff --git a/components/brave_sync/tools.cc b/components/brave_sync/tools.cc index 8671df9a157c..73e1bbd0bba4 100644 --- a/components/brave_sync/tools.cc +++ b/components/brave_sync/tools.cc @@ -1,55 +1,82 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ +/* Copyright 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + #include "brave/components/brave_sync/tools.h" -#include #include +#include -#include "crypto/random.h" #include "base/strings/string_util.h" #include "base/time/time.h" #include "build/build_config.h" +#include "crypto/random.h" +#include "crypto/sha2.h" namespace brave_sync { namespace tools { -std::string GenerateObjectId() { - //16 random 8-bit unsigned numbers - const size_t length = 16; - uint8_t bytes[length]; - crypto::RandBytes(bytes, sizeof(bytes)); +const char kOtherNodeOrder[] = "255.255.255"; +const char kOtherNodeName[] = "Other Bookmarks"; +const size_t kIdSize = 16; +const char kOtherBookmarksObjectIdSeed[] = "other_bookmarks_object_id"; + +namespace { +std::string PrintObjectId(uint8_t* bytes) { std::stringstream ss; - for (size_t i = 0; i < length; ++i) { - const uint8_t &byte = bytes[i]; - ss << std::dec << (int)byte; - if (i != length - 1) { + for (size_t i = 0; i < kIdSize; ++i) { + const uint8_t& byte = bytes[i]; + ss << std::dec << static_cast(byte); + if (i != kIdSize - 1) { ss << ", "; } } return ss.str(); } +} // namespace + +std::string GenerateObjectId() { + // 16 random 8-bit unsigned numbers + uint8_t bytes[kIdSize]; + crypto::RandBytes(bytes, sizeof(bytes)); + return PrintObjectId(bytes); +} + +std::string GenerateObjectIdForOtherNode(const std::string old_id) { + uint8_t bytes[kIdSize]; + const std::string input = + old_id.empty() ? kOtherBookmarksObjectIdSeed : old_id; + // Take first 16 bytes + crypto::SHA256HashString(input, bytes, sizeof(bytes)); + return PrintObjectId(bytes); +} std::string GetPlatformName() { - #if defined(OS_ANDROID) - const std::string platform = "android"; - #elif defined(OS_WIN) - const std::string platform = "windows"; - #elif defined(OS_LINUX) - const std::string platform = "linux"; - #elif defined(OS_MACOSX) - const std::string platform = "macosx"; - #elif defined(OS_IOS) - const std::string platform = "ios"; - #endif +#if defined(OS_ANDROID) + const std::string platform = "android"; +#elif defined(OS_WIN) + const std::string platform = "windows"; +#elif defined(OS_LINUX) + const std::string platform = "linux"; +#elif defined(OS_MACOSX) + const std::string platform = "macosx"; +#elif defined(OS_IOS) + const std::string platform = "ios"; +#endif return platform; } -bool IsTimeEmpty(const base::Time &time) { +bool IsTimeEmpty(const base::Time& time) { return time.is_null() || base::checked_cast(time.ToJsTime()) == 0; } -} // namespace tools +// Get mutable node to prevent BookmarkMetaInfoChanged from being triggered +bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) { + return const_cast(node); +} + +} // namespace tools -} // namespace brave_sync +} // namespace brave_sync diff --git a/components/brave_sync/tools.h b/components/brave_sync/tools.h index 537e94215dac..b7a56960a06c 100644 --- a/components/brave_sync/tools.h +++ b/components/brave_sync/tools.h @@ -1,26 +1,43 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_ -#define BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_ +/* Copyright 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_ +#define BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_ #include namespace base { - class Time; -} // namespace base +class Time; +} // namespace base + +namespace bookmarks { +class BookmarkNode; +} namespace brave_sync { namespace tools { +extern const char kOtherNodeOrder[]; +extern const char kOtherNodeName[]; + std::string GenerateObjectId(); + +// If old_id is empty, it would use default seed as first iteration, in future +// iteration, caller has to provide previous used id so we can have determined +// generated object id +std::string GenerateObjectIdForOtherNode(const std::string old_id); + std::string GetPlatformName(); bool IsTimeEmpty(const base::Time &time); -} // namespace tools +bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node); + +} // namespace tools -} // namespace brave_sync +} // namespace brave_sync -#endif // BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_ +#endif // BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_ diff --git a/patches/chrome-browser-ui-bookmarks-recently_used_folders_combo_model.h.patch b/patches/chrome-browser-ui-bookmarks-recently_used_folders_combo_model.h.patch deleted file mode 100644 index 7f6c58d8358c..000000000000 --- a/patches/chrome-browser-ui-bookmarks-recently_used_folders_combo_model.h.patch +++ /dev/null @@ -1,12 +0,0 @@ -diff --git a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h -index 73b769ef73a562b55da2844b77265b1f9d788d83..48794994e7f8dd3afce69d5be076f580fb13c22c 100644 ---- a/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h -+++ b/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h -@@ -73,6 +73,7 @@ class RecentlyUsedFoldersComboModel : public ui::ComboboxModel, - void MaybeChangeParent(const bookmarks::BookmarkNode* node, - int selected_index); - -+ BRAVE_RECENTLY_USED_FOLDERS_COMBO_MODEL_H_ - private: - // Returns the node at the specified |index|. - const bookmarks::BookmarkNode* GetNodeAt(int index); diff --git a/patches/chrome-browser-ui-views-bookmarks-bookmark_editor_view.cc.patch b/patches/chrome-browser-ui-views-bookmarks-bookmark_editor_view.cc.patch deleted file mode 100644 index 7a253217a31a..000000000000 --- a/patches/chrome-browser-ui-views-bookmarks-bookmark_editor_view.cc.patch +++ /dev/null @@ -1,13 +0,0 @@ -diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc -index 64121910b194aa23f2bdee0697a2dc3ef885e235..1feba675ee7bdf70361f708d4d74a9bb6dd20eea 100644 ---- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc -+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc -@@ -493,7 +493,7 @@ BookmarkEditorView::CreateRootNode() { - std::make_unique(base::string16(), 0); - const BookmarkNode* bb_root_node = bb_model_->root_node(); - CreateNodes(bb_root_node, root_node.get()); -- DCHECK_GE(root_node->children().size(), 2u); -+ DCHECK_GE(root_node->children().size(), 1u); - DCHECK_LE(root_node->children().size(), 4u); - DCHECK_EQ(BookmarkNode::BOOKMARK_BAR, bb_root_node->children()[0]->type()); - DCHECK_EQ(BookmarkNode::OTHER_NODE, bb_root_node->children()[1]->type()); diff --git a/test/BUILD.gn b/test/BUILD.gn index 75bf06fdc0a8..b31d0be90134 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -231,7 +231,6 @@ test("brave_unit_tests") { "//brave/browser/extensions/api/brave_extensions_api_client_unittest.cc", "//chrome/browser/extensions/extension_service_test_base.cc", "//chrome/browser/extensions/extension_service_test_base.h", - "//chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc", ] deps += [ @@ -518,7 +517,6 @@ test("brave_browser_tests") { "//brave/browser/autocomplete/brave_autocomplete_provider_client_browsertest.cc", "//brave/browser/brave_scheme_load_browsertest.cc", "//brave/browser/autoplay/autoplay_permission_context_browsertest.cc", - "//brave/browser/bookmarks/brave_bookmark_client_browsertest.cc", "//brave/browser/brave_content_browser_client_browsertest.cc", "//brave/browser/brave_profile_prefs_browsertest.cc", "//brave/browser/brave_first_run_browsertest.h", @@ -540,6 +538,7 @@ test("brave_browser_tests") { "//brave/browser/net/brave_network_delegate_hsts_fingerprinting_browsertest.cc", "//brave/browser/net/brave_system_request_handler_browsertest.cc", "//brave/browser/policy/brave_policy_browsertest.cc", + "//brave/browser/profiles/brave_bookmark_model_loaded_observer_browsertest.cc", "//brave/browser/profiles/brave_profile_manager_browsertest.cc", "//brave/browser/renderer_context_menu/brave_mock_render_view_context_menu.cc", "//brave/browser/renderer_context_menu/brave_mock_render_view_context_menu.h",