From aa5daa2cbf66167e4a71a071177c053698cee471 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 21 Jan 2020 11:25:20 -0800 Subject: [PATCH 01/15] Revert "Merge pull request #3778 from brave/bookmark-api" This reverts commit edd63cf18f5384f1caa2dad5cf713560026af15a, reversing changes made to c7202dcbad3fd05f0554bddef0c5530c0ce3a083. --- .../api/bookmarks/bookmarks_api_unittest.cc | 64 ------------------- .../bookmarks/browser/bookmark_utils.cc | 26 -------- test/BUILD.gn | 1 - 3 files changed, 91 deletions(-) delete mode 100644 chromium_src/chrome/browser/extensions/api/bookmarks/bookmarks_api_unittest.cc delete mode 100644 chromium_src/components/bookmarks/browser/bookmark_utils.cc 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/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/test/BUILD.gn b/test/BUILD.gn index 75bf06fdc0a8..2bb176128e69 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 += [ From eaf5dd9af517666d30068b2eaf2b05629b3d7ef1 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 21 Jan 2020 15:09:25 -0800 Subject: [PATCH 02/15] Unhide other bookmarks node https://github.com/brave/brave-core/pull/3620 --- browser/BUILD.gn | 2 -- browser/bookmarks/brave_bookmark_client.cc | 23 --------------- browser/bookmarks/brave_bookmark_client.h | 26 ----------------- .../brave_bookmark_client_browsertest.cc | 28 ------------------- browser/ui/BUILD.gn | 2 -- .../recently_used_folders_combo_model.cc | 17 ----------- .../recently_used_folders_combo_model.h | 22 --------------- .../bookmarks/bookmark_model_factory.cc | 2 -- .../recently_used_folders_combo_model.h | 16 ----------- .../views/bookmarks/bookmark_bubble_view.cc | 10 ------- ...-recently_used_folders_combo_model.h.patch | 12 -------- ...ws-bookmarks-bookmark_editor_view.cc.patch | 13 --------- test/BUILD.gn | 1 - 13 files changed, 174 deletions(-) delete mode 100644 browser/bookmarks/brave_bookmark_client.cc delete mode 100644 browser/bookmarks/brave_bookmark_client.h delete mode 100644 browser/bookmarks/brave_bookmark_client_browsertest.cc delete mode 100644 browser/ui/bookmark/recently_used_folders_combo_model.cc delete mode 100644 browser/ui/bookmark/recently_used_folders_combo_model.h delete mode 100644 chromium_src/chrome/browser/ui/bookmarks/recently_used_folders_combo_model.h delete mode 100644 chromium_src/chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc delete mode 100644 patches/chrome-browser-ui-bookmarks-recently_used_folders_combo_model.h.patch delete mode 100644 patches/chrome-browser-ui-views-bookmarks-bookmark_editor_view.cc.patch 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/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/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/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 2bb176128e69..a4f5f82d5a1d 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -517,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", From bee58c5e0b867f4464bae35f62468507ad78adaf Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 21 Jan 2020 15:11:03 -0800 Subject: [PATCH 03/15] Migrate other bookmarks folder if it is still there (at the end of bookmarks bar and still has the same name as other bookmarks node) --- .../brave_bookmark_model_loaded_observer.cc | 4 +- .../bookmarks/browser/bookmark_model.cc | 25 ++++--- .../bookmarks/browser/bookmark_model.h | 2 +- .../browser/bookmark_model_unittest.cc | 73 +++++++++++++------ .../brave_profile_sync_service_impl.cc | 2 +- 5 files changed, 69 insertions(+), 37 deletions(-) diff --git a/browser/profiles/brave_bookmark_model_loaded_observer.cc b/browser/profiles/brave_bookmark_model_loaded_observer.cc index 59e4f191809a..717b57cf5a3b 100644 --- a/browser/profiles/brave_bookmark_model_loaded_observer.cc +++ b/browser/profiles/brave_bookmark_model_loaded_observer.cc @@ -30,9 +30,9 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded( // 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); + BraveMigrateOtherNodeFolder(model); #else - BraveMigrateOtherNode(model); + BraveMigrateOtherNodeFolder(model); #endif BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned); } 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/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 5b24a7df4cdb..95ed065d0bd9 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -513,7 +513,7 @@ void BraveProfileSyncServiceImpl::OnSyncReadyBookmarksModelLoaded() { ProfileSyncService::GetUserSettings()->SetSyncRequested(true); } - BraveMigrateOtherNode(model_); + BraveMigrateOtherNodeFolder(model_); } syncer::ModelTypeSet BraveProfileSyncServiceImpl::GetPreferredDataTypes() From 4a7b0eb8b73ca5b9846fa30dea07182efcb113ce Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 21 Jan 2020 21:43:18 -0800 Subject: [PATCH 04/15] Map virtual "Other Bookmarks" folder on mobile to other node (permanent node) on dekstop --- .../components/sync/engine_impl/commit.cc | 37 +++++++- .../sync/engine_impl/get_updates_processor.cc | 12 ++- components/brave_sync/BUILD.gn | 1 + .../brave_profile_sync_service_impl.cc | 85 ++++++++++++++++-- .../brave_profile_sync_service_impl.h | 4 + components/brave_sync/syncer_helper.cc | 25 +++--- components/brave_sync/tools.cc | 88 +++++++++++++------ components/brave_sync/tools.h | 38 +++++--- 8 files changed, 226 insertions(+), 64 deletions(-) diff --git a/chromium_src/components/sync/engine_impl/commit.cc b/chromium_src/components/sync/engine_impl/commit.cc index e085ed4e077a..b6357ccb9bc0 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::GetOtherNodeName(); + bookmark->site.customTitle = brave_sync::tools::GetOtherNodeName(); + // 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/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index f17afead0445..e9040448b1f3 100644 --- a/components/brave_sync/BUILD.gn +++ b/components/brave_sync/BUILD.gn @@ -148,6 +148,7 @@ source_set("core") { "//components/bookmarks/browser", "//crypto", "//extensions/buildflags", + "//ui/base", ] } diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 95ed065d0bd9..3447efdf9b6b 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -579,6 +579,13 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords( OnResolvedPreferences(*records.get()); } else if (category_name == kBookmarks) { for (auto& record : *records) { + if (IsOtherBookmarksFolder(record.get())) { + ProcessOtherBookmarksFolder(record.get()); + // We don't process "Other Bookmarks" folder in syncer + 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 @@ -713,13 +720,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", base_order + "2"); } brave_sync_prefs_->SetMigratedBookmarksVersion(2); } @@ -741,10 +749,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 +793,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 +820,65 @@ void BraveProfileSyncServiceImpl::LoadSyncEntityInfo( } } +bool BraveProfileSyncServiceImpl::IsOtherBookmarksFolder( + const jslib::SyncRecord* record) const { + 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; + + auto bookmark = record->GetBookmark(); + if (bookmark.order == tools::kOtherNodeOrder && + bookmark.site.title == tools::GetOtherNodeName() && + bookmark.site.customTitle == tools::GetOtherNodeName()) { + return true; + } + + return false; +} + +void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( + const jslib::SyncRecord* record) { + std::string other_node_object_id; + // Save object_id for late joined desktop to catch up with currecnt id + // iteration + if (model_->other_node()->GetMetaInfo("object_id", &other_node_object_id)) { + // 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 + // Update will be resolved as Create because [UPDATE, null] => [CREATE] + auto bookmark = record->GetBookmark(); + if (bookmark.order != tools::kOtherNodeOrder || + bookmark.site.title != tools::GetOtherNodeName() || + bookmark.site.customTitle != tools::GetOtherNodeName()) { + // This is to compensate mobile couldn't make "Other Bookmarks" a + // read-only folder. + // Remove remote "Other Bookmarks" folder + RecordsListPtr records = std::make_unique(); + records->push_back( + CreateDeleteBookmarkByObjectId(brave_sync_prefs_.get(), + record->objectId)); + brave_sync_client_->SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, + *records); + // Remove all local children of other_node + while (model_->other_node()->children().size()) { + model_->Remove(model_->other_node()->children().front().get()); + } + } + } +} + +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::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..646c155ce4ed 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -249,6 +249,10 @@ class BraveProfileSyncServiceImpl void SaveSyncEntityInfo(const jslib::SyncRecord* record); void LoadSyncEntityInfo(jslib::SyncRecord* record); + bool IsOtherBookmarksFolder(const jslib::SyncRecord* record) const; + void ProcessOtherBookmarksFolder(const jslib::SyncRecord* record); + void ProcessOtherBookmarksChildren(jslib::SyncRecord* record); + void CreateResolveList( const std::vector>& records, SyncRecordAndExistingList* records_and_existing_objects); diff --git a/components/brave_sync/syncer_helper.cc b/components/brave_sync/syncer_helper.cc index 391eb8460491..22cea1304689 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,17 +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; node->parent()->GetMetaInfo("object_id", &parent_object_id); - AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id); + // Setting object_id for other_node when we are about to send CREATE "Other + // Bookmarks" + if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE && + parent_object_id.empty()) { + // first iteration + parent_object_id = tools::GenerateObjectIdForOtherNode(std::string()); + tools::AsMutable(node->parent())->SetMetaInfo("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()); } diff --git a/components/brave_sync/tools.cc b/components/brave_sync/tools.cc index 8671df9a157c..1e6c810da087 100644 --- a/components/brave_sync/tools.cc +++ b/components/brave_sync/tools.cc @@ -1,55 +1,87 @@ -/* 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 "components/strings/grit/components_strings.h" +#include "crypto/random.h" +#include "crypto/sha2.h" +#include "ui/base/l10n/l10n_util.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 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); +} + +std::string GetOtherNodeName() { + return l10n_util::GetStringUTF8(IDS_BOOKMARK_BAR_OTHER_FOLDER_NAME); +} + +} // namespace tools -} // namespace brave_sync +} // namespace brave_sync diff --git a/components/brave_sync/tools.h b/components/brave_sync/tools.h index 537e94215dac..87a6ae4ae05f 100644 --- a/components/brave_sync/tools.h +++ b/components/brave_sync/tools.h @@ -1,26 +1,44 @@ -/* 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[]; + 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); + +std::string GetOtherNodeName(); + +} // namespace tools -} // namespace brave_sync +} // namespace brave_sync -#endif // BRAVE_COMPONENTS_BRAVE_SYNC_BRAVE_SYNC_TOOLS_H_ +#endif // BRAVE_COMPONENTS_BRAVE_SYNC_TOOLS_H_ From 9a24d39c706fb45605c31d029226cd91566b0c20 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 28 Jan 2020 10:17:27 -0800 Subject: [PATCH 05/15] Perform other bookmarks migration only once --- browser/brave_profile_prefs.cc | 2 ++ .../brave_bookmark_model_loaded_observer.cc | 18 ++++++++++++------ common/pref_names.cc | 1 + common/pref_names.h | 1 + 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 1e4e57f4d129..4f76092b974a 100644 --- a/browser/brave_profile_prefs.cc +++ b/browser/brave_profile_prefs.cc @@ -221,6 +221,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { registry->SetDefaultPrefValue( password_manager::prefs::kPasswordLeakDetectionEnabled, base::Value(false)); + // Restore "Other Bookmarks" migration + registry->RegisterBooleanPref(kOtherBookmarksMigrated, false); RegisterProfilePrefsForMigration(registry); } diff --git a/browser/profiles/brave_bookmark_model_loaded_observer.cc b/browser/profiles/brave_bookmark_model_loaded_observer.cc index 717b57cf5a3b..5e9465b15968 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,19 @@ 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()) - BraveMigrateOtherNodeFolder(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->IsBraveSyncEnabled()) + BraveMigrateOtherNodeFolder(model); #else - BraveMigrateOtherNodeFolder(model); + BraveMigrateOtherNodeFolder(model); #endif + profile_->GetPrefs()->SetBoolean(kOtherBookmarksMigrated, true); + } BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned); } 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[]; From bb803e29103a2c6ec72aad62a9d7609405bf2ba9 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Wed, 29 Jan 2020 11:12:35 -0800 Subject: [PATCH 06/15] Add browser test for BraveBookmarkModelLoadedObserver --- ...kmark_model_loaded_observer_browsertest.cc | 75 +++++++++++++++++++ test/BUILD.gn | 1 + 2 files changed, 76 insertions(+) create mode 100644 browser/profiles/brave_bookmark_model_loaded_observer_browsertest.cc 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/test/BUILD.gn b/test/BUILD.gn index a4f5f82d5a1d..b31d0be90134 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -538,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", From e530aefd805fa280d50a2fcd6a7c7d6bfaf37376 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Thu, 30 Jan 2020 18:50:38 -0800 Subject: [PATCH 07/15] Remap "Other Bookmarks" folder with new iteration of object id when "Other Bookmarks" is moved, renamed or reordered. When remapping happens, we also move existing children of "Other Bookmarks" node to new folder we are about to create --- .../brave_profile_sync_service_impl.cc | 58 +++++++++++++------ .../brave_profile_sync_service_impl.h | 3 +- components/brave_sync/syncer_helper.cc | 19 ++++-- 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 3447efdf9b6b..a7706b45e6e4 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -580,10 +580,14 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords( } else if (category_name == kBookmarks) { for (auto& record : *records) { if (IsOtherBookmarksFolder(record.get())) { - ProcessOtherBookmarksFolder(record.get()); - // We don't process "Other Bookmarks" folder in syncer - std::move(record); - continue; + 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()); @@ -594,6 +598,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( @@ -838,11 +843,16 @@ bool BraveProfileSyncServiceImpl::IsOtherBookmarksFolder( } void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( - const jslib::SyncRecord* record) { + 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 currecnt id // iteration - if (model_->other_node()->GetMetaInfo("object_id", &other_node_object_id)) { + 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 { // 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. @@ -853,18 +863,30 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( if (bookmark.order != tools::kOtherNodeOrder || bookmark.site.title != tools::GetOtherNodeName() || bookmark.site.customTitle != tools::GetOtherNodeName()) { - // This is to compensate mobile couldn't make "Other Bookmarks" a - // read-only folder. - // Remove remote "Other Bookmarks" folder - RecordsListPtr records = std::make_unique(); - records->push_back( - CreateDeleteBookmarkByObjectId(brave_sync_prefs_.get(), - record->objectId)); - brave_sync_client_->SendSyncRecords(jslib_const::SyncRecordType_BOOKMARKS, - *records); - // Remove all local children of other_node - while (model_->other_node()->children().size()) { - model_->Remove(model_->other_node()->children().front().get()); + // 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 + 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()); + + if (!pending_received_records_) + pending_received_records_ = std::make_unique(); + pending_received_records_->push_back(std::move(sync_record)); } } } diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index 646c155ce4ed..429a604f53ed 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -250,7 +250,8 @@ class BraveProfileSyncServiceImpl void LoadSyncEntityInfo(jslib::SyncRecord* record); bool IsOtherBookmarksFolder(const jslib::SyncRecord* record) const; - void ProcessOtherBookmarksFolder(const jslib::SyncRecord* record); + void ProcessOtherBookmarksFolder(const jslib::SyncRecord* record, + bool* pass_to_syncer); void ProcessOtherBookmarksChildren(jslib::SyncRecord* record); void CreateResolveList( diff --git a/components/brave_sync/syncer_helper.cc b/components/brave_sync/syncer_helper.cc index 22cea1304689..cda49f6e2ac1 100644 --- a/components/brave_sync/syncer_helper.cc +++ b/components/brave_sync/syncer_helper.cc @@ -91,13 +91,17 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) { std::string parent_object_id; node->parent()->GetMetaInfo("object_id", &parent_object_id); // Setting object_id for other_node when we are about to send CREATE "Other - // Bookmarks" + // Bookmarks" for the first time if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE && parent_object_id.empty()) { - // first iteration - parent_object_id = tools::GenerateObjectIdForOtherNode(std::string()); - tools::AsMutable(node->parent())->SetMetaInfo("object_id", - parent_object_id); + if (!node->parent()->GetMetaInfo("object_id", &parent_object_id)) { + // first iteration + parent_object_id = tools::GenerateObjectIdForOtherNode(std::string()); + tools::AsMutable(node->parent())->SetMetaInfo("object_id", + parent_object_id); + } + tools::AsMutable(node->parent())->SetMetaInfo("order", + tools::kOtherNodeOrder); } tools::AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id); @@ -108,6 +112,11 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) { 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 From 1e700d1c3a50dd9060782fa394c29d60e5c0dc4c Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Thu, 30 Jan 2020 21:19:59 -0800 Subject: [PATCH 08/15] Ignore REORDER of "Other Bookmarks" on mobile --- components/brave_sync/brave_profile_sync_service_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index a7706b45e6e4..716f833937dd 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -857,10 +857,12 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( // 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 + // 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 || + if ((bookmark.order != tools::kOtherNodeOrder && + !bookmark.parentFolderObjectId.empty()) || bookmark.site.title != tools::GetOtherNodeName() || bookmark.site.customTitle != tools::GetOtherNodeName()) { // Generate next iteration object id from current object_id which will be From 43e450f33eb97a6c5863a5cb26a0592927150c3b Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Mon, 3 Feb 2020 16:57:05 -0800 Subject: [PATCH 09/15] Generate first itration of other_node object id at BraveProfileSyncServiceImpl --- .../brave_profile_sync_service_impl.cc | 41 ++++++++++++++++++- .../brave_profile_sync_service_impl.h | 2 + components/brave_sync/syncer_helper.cc | 15 +------ 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 716f833937dd..bbb7b219bb24 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -219,6 +219,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)); @@ -731,8 +733,8 @@ void BraveProfileSyncServiceImpl::SetPermanentNodesOrder( order.clear(); model_->other_node()->GetMetaInfo("order", &order); if (order.empty()) { - tools::AsMutable(model_->other_node()) - ->SetMetaInfo("order", base_order + "2"); + tools::AsMutable(model_->other_node())->SetMetaInfo("order", + tools::kOtherNodeOrder); } brave_sync_prefs_->SetMigratedBookmarksVersion(2); } @@ -853,6 +855,14 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( tools::AsMutable(model_->other_node())->SetMetaInfo("object_id", record->objectId); } else { + // If late joined desktop has bookmarks in other_node before joing sync + // chain + if (other_node_object_id != record->objectId && + other_node_object_id == + tools::GenerateObjectIdForOtherNode(std::string())) { + 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. @@ -902,6 +912,33 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksChildren( 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, diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index 429a604f53ed..c61f39946bb8 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -253,6 +253,8 @@ class BraveProfileSyncServiceImpl void ProcessOtherBookmarksFolder(const jslib::SyncRecord* record, bool* pass_to_syncer); void ProcessOtherBookmarksChildren(jslib::SyncRecord* record); + void CheckOtherBookmarkRecord(jslib::SyncRecord* record); + void CheckOtherBookmarkChildRecord(jslib::SyncRecord* record); void CreateResolveList( const std::vector>& records, diff --git a/components/brave_sync/syncer_helper.cc b/components/brave_sync/syncer_helper.cc index cda49f6e2ac1..e3f5ca609ab6 100644 --- a/components/brave_sync/syncer_helper.cc +++ b/components/brave_sync/syncer_helper.cc @@ -89,20 +89,9 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) { 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); - // Setting object_id for other_node when we are about to send CREATE "Other - // Bookmarks" for the first time - if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE && - parent_object_id.empty()) { - if (!node->parent()->GetMetaInfo("object_id", &parent_object_id)) { - // first iteration - parent_object_id = tools::GenerateObjectIdForOtherNode(std::string()); - tools::AsMutable(node->parent())->SetMetaInfo("object_id", - parent_object_id); - } - tools::AsMutable(node->parent())->SetMetaInfo("order", - tools::kOtherNodeOrder); - } tools::AsMutable(node)->SetMetaInfo("parent_object_id", parent_object_id); std::string sync_timestamp; From eb12743ebc4b7b2f4f6c430d3c8c0565f9325d2d Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Mon, 3 Feb 2020 18:47:09 -0800 Subject: [PATCH 10/15] Send other_node children moving records for other desktops to catch up --- components/brave_sync/brave_profile_sync_service_impl.cc | 9 +++++++++ components/brave_sync/brave_profile_sync_service_impl.h | 3 +++ 2 files changed, 12 insertions(+) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index bbb7b219bb24..09a5dd63afef 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -886,6 +886,7 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( // 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()); @@ -896,10 +897,18 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( bookmark.order + "." + std::to_string(i + 1); LoadSyncEntityInfo(sync_record.get()); + auto record_to_send = SyncRecord::Clone(*sync_record); + + // Append chnages 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)); } } } diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index c61f39946bb8..fa8fdb4aaa58 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.h +++ b/components/brave_sync/brave_profile_sync_service_impl.h @@ -250,9 +250,12 @@ class BraveProfileSyncServiceImpl 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); From 769c336451ed99a193438b9685221ddb59fa3cbc Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Wed, 5 Feb 2020 17:15:09 -0800 Subject: [PATCH 11/15] Add tests for Virtual "Other Bookmarks" mapping --- .../brave_profile_sync_service_impl.cc | 11 +- .../brave_profile_sync_service_impl.h | 15 + .../brave_sync/brave_sync_service_unittest.cc | 316 +++++++++++++++++- components/brave_sync/jslib_messages.cc | 44 +++ components/brave_sync/jslib_messages.h | 11 + components/brave_sync/test_util.cc | 12 +- components/brave_sync/test_util.h | 5 +- 7 files changed, 392 insertions(+), 22 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 09a5dd63afef..81227cdb3487 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -829,12 +829,15 @@ 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; - auto bookmark = record->GetBookmark(); if (bookmark.order == tools::kOtherNodeOrder && bookmark.site.title == tools::GetOtherNodeName() && bookmark.site.customTitle == tools::GetOtherNodeName()) { @@ -848,14 +851,14 @@ 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 currecnt 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 { - // If late joined desktop has bookmarks in other_node before joing sync + // If late joined desktop has bookmarks in other_node before joining sync // chain if (other_node_object_id != record->objectId && other_node_object_id == @@ -899,7 +902,7 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( auto record_to_send = SyncRecord::Clone(*sync_record); - // Append chnages to remote records + // Append changes to remote records if (!pending_received_records_) pending_received_records_ = std::make_unique(); pending_received_records_->push_back(std::move(sync_record)); diff --git a/components/brave_sync/brave_profile_sync_service_impl.h b/components/brave_sync/brave_profile_sync_service_impl.h index fa8fdb4aaa58..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; diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 92bf3431d621..4387f02387e8 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::GetOtherNodeName; +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"), + GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + EXPECT_TRUE(sync_service() + ->IsOtherBookmarksFolder(record_matches_traits.get())); + + auto record_not_match1 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + GetOtherNodeName(), "", "", "", false, GetOtherNodeName()); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_match1.get())); + + auto record_not_match2 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + "What Bookmarks", kOtherNodeOrder, "", "", false, GetOtherNodeName()); + EXPECT_FALSE(sync_service() + ->IsOtherBookmarksFolder(record_not_match2.get())); + + auto record_not_match3 = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), + GetOtherNodeName(), 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, + GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + 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, + GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + 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, + GetOtherNodeName(), "1.0.1.1.1", folder_b_object_id, "", + false, GetOtherNodeName()); + 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, + GetOtherNodeName(), "1.0.1.2", "", "", + false, GetOtherNodeName()); + 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, "", + GetOtherNodeName(), kOtherNodeOrder, "", "", + false, GetOtherNodeName()); + 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); + + // Obejct id is out-dated + record = SimpleFolderSyncRecord( + SyncRecord::Action::A_CREATE, object_id_iter1, + GetOtherNodeName(), kOtherNodeOrder, "", "", + false, GetOtherNodeName()); + 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/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); From 87a0a3860d90690d2c696ac9042a0b72322b7a40 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Fri, 7 Feb 2020 11:11:11 -0800 Subject: [PATCH 12/15] Check and set kOtherBookmarksMigrated in BraveProfileSyncServiceImpl --- components/brave_sync/BUILD.gn | 1 + components/brave_sync/brave_profile_sync_service_impl.cc | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index e9040448b1f3..7fe7820417c5 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 81227cdb3487..60a38fd6be57 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" @@ -515,7 +517,10 @@ void BraveProfileSyncServiceImpl::OnSyncReadyBookmarksModelLoaded() { ProfileSyncService::GetUserSettings()->SetSyncRequested(true); } - BraveMigrateOtherNodeFolder(model_); + if (!sync_client_->GetPrefService()->GetBoolean(kOtherBookmarksMigrated)) { + BraveMigrateOtherNodeFolder(model_); + sync_client_->GetPrefService()->SetBoolean(kOtherBookmarksMigrated, true); + } } syncer::ModelTypeSet BraveProfileSyncServiceImpl::GetPreferredDataTypes() From fbf40d43d96dffc6eff89f43897bc2a3eeb7c6c2 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Tue, 18 Feb 2020 12:54:01 -0800 Subject: [PATCH 13/15] 1. Always send and check other_node name in English, "Other Bookmarks" 2. Add a missing dep 3. Put Other Bookmark migration in RegisterProfilePrefsForMigration --- browser/brave_profile_prefs.cc | 5 ++-- browser/profiles/BUILD.gn | 1 + .../components/sync/engine_impl/commit.cc | 4 +-- components/brave_sync/BUILD.gn | 1 - .../brave_profile_sync_service_impl.cc | 8 ++--- .../brave_sync/brave_sync_service_unittest.cc | 30 +++++++++---------- components/brave_sync/tools.cc | 7 +---- components/brave_sync/tools.h | 3 +- 8 files changed, 27 insertions(+), 32 deletions(-) diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 4f76092b974a..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) { @@ -221,8 +224,6 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { registry->SetDefaultPrefValue( password_manager::prefs::kPasswordLeakDetectionEnabled, base::Value(false)); - // Restore "Other Bookmarks" migration - registry->RegisterBooleanPref(kOtherBookmarksMigrated, false); RegisterProfilePrefsForMigration(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/chromium_src/components/sync/engine_impl/commit.cc b/chromium_src/components/sync/engine_impl/commit.cc index b6357ccb9bc0..4f1c16788a7f 100644 --- a/chromium_src/components/sync/engine_impl/commit.cc +++ b/chromium_src/components/sync/engine_impl/commit.cc @@ -60,8 +60,8 @@ 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::GetOtherNodeName(); - bookmark->site.customTitle = brave_sync::tools::GetOtherNodeName(); + 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 diff --git a/components/brave_sync/BUILD.gn b/components/brave_sync/BUILD.gn index 7fe7820417c5..8cb90e479992 100644 --- a/components/brave_sync/BUILD.gn +++ b/components/brave_sync/BUILD.gn @@ -149,7 +149,6 @@ source_set("core") { "//components/bookmarks/browser", "//crypto", "//extensions/buildflags", - "//ui/base", ] } diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index 60a38fd6be57..f05280cf273b 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -844,8 +844,8 @@ bool BraveProfileSyncServiceImpl::IsOtherBookmarksFolder( return true; if (bookmark.order == tools::kOtherNodeOrder && - bookmark.site.title == tools::GetOtherNodeName() && - bookmark.site.customTitle == tools::GetOtherNodeName()) { + bookmark.site.title == tools::kOtherNodeName && + bookmark.site.customTitle == tools::kOtherNodeName) { return true; } @@ -881,8 +881,8 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( auto bookmark = record->GetBookmark(); if ((bookmark.order != tools::kOtherNodeOrder && !bookmark.parentFolderObjectId.empty()) || - bookmark.site.title != tools::GetOtherNodeName() || - bookmark.site.customTitle != tools::GetOtherNodeName()) { + 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( diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 4387f02387e8..9c0971aa301e 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -106,7 +106,7 @@ using brave_sync::jslib::SyncRecord; using brave_sync::tools::AsMutable; using brave_sync::tools::GenerateObjectId; using brave_sync::tools::GenerateObjectIdForOtherNode; -using brave_sync::tools::GetOtherNodeName; +using brave_sync::tools::kOtherNodeName; using brave_sync::tools::kOtherNodeOrder; using testing::_; using testing::AtLeast; @@ -1337,25 +1337,25 @@ TEST_F(BraveSyncServiceTest, IsOtherBookmarksFolder) { auto record_matches_traits = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), - GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); EXPECT_TRUE(sync_service() ->IsOtherBookmarksFolder(record_matches_traits.get())); auto record_not_match1 = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, GenerateObjectIdForOtherNode("123"), - GetOtherNodeName(), "", "", "", false, GetOtherNodeName()); + 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, GetOtherNodeName()); + "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"), - GetOtherNodeName(), kOtherNodeOrder, "", "", false, "No Bookmarks"); + kOtherNodeName, kOtherNodeOrder, "", "", false, "No Bookmarks"); EXPECT_FALSE(sync_service() ->IsOtherBookmarksFolder(record_not_match3.get())); @@ -1371,7 +1371,7 @@ TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksFolder) { GenerateObjectIdForOtherNode(std::string()); auto record1 = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter1, - GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record1.get())); bool pass_to_syncer = false; @@ -1387,7 +1387,7 @@ TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksFolder) { // one auto record2 = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter2, - GetOtherNodeName(), kOtherNodeOrder, "", "", false, GetOtherNodeName()); + kOtherNodeName, kOtherNodeOrder, "", "", false, kOtherNodeName); EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record2.get())); pass_to_syncer = false; @@ -1434,8 +1434,8 @@ TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksFolder) { GenerateObjectIdForOtherNode(object_id_iter2); auto record3 = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter2, - GetOtherNodeName(), "1.0.1.1.1", folder_b_object_id, "", - false, GetOtherNodeName()); + kOtherNodeName, "1.0.1.1.1", folder_b_object_id, "", + false, kOtherNodeName); EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record3.get())); auto record_a = SimpleFolderSyncRecord( @@ -1522,8 +1522,8 @@ TEST_F(BraveSyncServiceTest, ProcessOtherBookmarksFolder) { sync_service()->pending_received_records_->clear(); auto record5 = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter4, - GetOtherNodeName(), "1.0.1.2", "", "", - false, GetOtherNodeName()); + kOtherNodeName, "1.0.1.2", "", "", + false, kOtherNodeName); EXPECT_TRUE(sync_service()->IsOtherBookmarksFolder(record5.get())); EXPECT_CALL(*sync_client(), SendSyncRecords(kBookmarks, _)).Times(0); @@ -1566,8 +1566,8 @@ TEST_F(BraveSyncServiceTest, CheckOtherBookmarkRecord) { // No object id for other_node yet auto record = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, "", - GetOtherNodeName(), kOtherNodeOrder, "", "", - false, GetOtherNodeName()); + kOtherNodeName, kOtherNodeOrder, "", "", + false, kOtherNodeName); std::string other_node_object_id; EXPECT_FALSE(model()->other_node()->GetMetaInfo("object_id", &other_node_object_id)); @@ -1580,8 +1580,8 @@ TEST_F(BraveSyncServiceTest, CheckOtherBookmarkRecord) { // Obejct id is out-dated record = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter1, - GetOtherNodeName(), kOtherNodeOrder, "", "", - false, GetOtherNodeName()); + kOtherNodeName, kOtherNodeOrder, "", "", + false, kOtherNodeName); AsMutable(model()->other_node()) ->SetMetaInfo("object_id", object_id_iter2); sync_service()->CheckOtherBookmarkRecord(record.get()); diff --git a/components/brave_sync/tools.cc b/components/brave_sync/tools.cc index 1e6c810da087..73e1bbd0bba4 100644 --- a/components/brave_sync/tools.cc +++ b/components/brave_sync/tools.cc @@ -11,16 +11,15 @@ #include "base/strings/string_util.h" #include "base/time/time.h" #include "build/build_config.h" -#include "components/strings/grit/components_strings.h" #include "crypto/random.h" #include "crypto/sha2.h" -#include "ui/base/l10n/l10n_util.h" namespace brave_sync { namespace tools { const char kOtherNodeOrder[] = "255.255.255"; +const char kOtherNodeName[] = "Other Bookmarks"; const size_t kIdSize = 16; const char kOtherBookmarksObjectIdSeed[] = "other_bookmarks_object_id"; @@ -78,10 +77,6 @@ bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node) { return const_cast(node); } -std::string GetOtherNodeName() { - return l10n_util::GetStringUTF8(IDS_BOOKMARK_BAR_OTHER_FOLDER_NAME); -} - } // namespace tools } // namespace brave_sync diff --git a/components/brave_sync/tools.h b/components/brave_sync/tools.h index 87a6ae4ae05f..b7a56960a06c 100644 --- a/components/brave_sync/tools.h +++ b/components/brave_sync/tools.h @@ -21,6 +21,7 @@ namespace brave_sync { namespace tools { extern const char kOtherNodeOrder[]; +extern const char kOtherNodeName[]; std::string GenerateObjectId(); @@ -35,8 +36,6 @@ bool IsTimeEmpty(const base::Time &time); bookmarks::BookmarkNode* AsMutable(const bookmarks::BookmarkNode* node); -std::string GetOtherNodeName(); - } // namespace tools } // namespace brave_sync From b5e896a2173d4903672d4d8075b6678cfadac98a Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Wed, 19 Feb 2020 18:04:54 -0800 Subject: [PATCH 14/15] Fix BraveMigrateOtherNodeFolder is not called when sync is disabled by flag --- browser/profiles/brave_bookmark_model_loaded_observer.cc | 3 ++- components/brave_sync/brave_sync_service_unittest.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/browser/profiles/brave_bookmark_model_loaded_observer.cc b/browser/profiles/brave_bookmark_model_loaded_observer.cc index 5e9465b15968..553141df0cb4 100644 --- a/browser/profiles/brave_bookmark_model_loaded_observer.cc +++ b/browser/profiles/brave_bookmark_model_loaded_observer.cc @@ -33,7 +33,8 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded( 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()) + if (!brave_profile_service || + (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled())) BraveMigrateOtherNodeFolder(model); #else BraveMigrateOtherNodeFolder(model); diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 9c0971aa301e..ebb005a15a6d 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -1577,7 +1577,7 @@ TEST_F(BraveSyncServiceTest, CheckOtherBookmarkRecord) { EXPECT_EQ(other_node_object_id, object_id_iter1); EXPECT_EQ(record->objectId, other_node_object_id); - // Obejct id is out-dated + // Object id is out-dated record = SimpleFolderSyncRecord( SyncRecord::Action::A_CREATE, object_id_iter1, kOtherNodeName, kOtherNodeOrder, "", "", From e0d275cf0fb4de738dfc295b85dec6fb4e1f96ad Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Thu, 20 Feb 2020 10:34:17 -0800 Subject: [PATCH 15/15] Always take remote other_node id when catching up --- .../brave_sync/brave_profile_sync_service_impl.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/components/brave_sync/brave_profile_sync_service_impl.cc b/components/brave_sync/brave_profile_sync_service_impl.cc index f05280cf273b..68e40caa6401 100644 --- a/components/brave_sync/brave_profile_sync_service_impl.cc +++ b/components/brave_sync/brave_profile_sync_service_impl.cc @@ -863,11 +863,11 @@ void BraveProfileSyncServiceImpl::ProcessOtherBookmarksFolder( tools::AsMutable(model_->other_node())->SetMetaInfo("object_id", record->objectId); } else { - // If late joined desktop has bookmarks in other_node before joining sync - // chain - if (other_node_object_id != record->objectId && - other_node_object_id == - tools::GenerateObjectIdForOtherNode(std::string())) { + // 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); }