Merge pull request #4404 from brave/restore-other-bookmarks

Restore other bookmarks

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",

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) {

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",

browser/profiles/brave_bookmark_model_loaded_observer.cc

@@ -5,8 +5,11 @@
 
 #include "brave/browser/profiles/brave_bookmark_model_loaded_observer.h"
 
+#include "brave/common/pref_names.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/prefs/pref_service.h"
 
 #include "brave/components/brave_sync/buildflags/buildflags.h"
 #if BUILDFLAG(ENABLE_BRAVE_SYNC)
@@ -23,16 +26,20 @@ BraveBookmarkModelLoadedObserver::BraveBookmarkModelLoadedObserver(
 void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded(
     BookmarkModel* model,
     bool ids_reassigned) {
+  if (!profile_->GetPrefs()->GetBoolean(kOtherBookmarksMigrated)) {
 #if BUILDFLAG(ENABLE_BRAVE_SYNC)
-  BraveProfileSyncServiceImpl* brave_profile_service =
+    BraveProfileSyncServiceImpl* brave_profile_service =
       static_cast<BraveProfileSyncServiceImpl*>(
           ProfileSyncServiceFactory::GetForProfile(profile_));
-  // When sync is enabled, we need to send migration records to other devices so
-  // it is handled in BraveProfileSyncServiceImpl::OnSyncReady
-  if (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled())
-    BraveMigrateOtherNode(model);
+    // When sync is enabled, we need to send migration records to other devices
+    // so it is handled in BraveProfileSyncServiceImpl::OnSyncReady
+    if (!brave_profile_service ||
+        (brave_profile_service && !brave_profile_service->IsBraveSyncEnabled()))
+      BraveMigrateOtherNodeFolder(model);
 #else
-  BraveMigrateOtherNode(model);
+    BraveMigrateOtherNodeFolder(model);
 #endif
+    profile_->GetPrefs()->SetBoolean(kOtherBookmarksMigrated, true);
+  }
   BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned);
 }

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);
+}

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",

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