Send all history to sync server under the flag

Fixes brave/brave-browser#28062

browser/about_flags.cc

@@ -388,6 +388,12 @@ constexpr char kBraveSyncHistoryDiagnosticsName[] =
 constexpr char kBraveSyncHistoryDiagnosticsDescription[] =
     "Brave Sync History Diagnostics flag displays additional sync related "
     "information on History page";
+constexpr char kBraveSyncSendAllHistoryName[] =
+    "Send All History to Brave Sync";
+constexpr char kBraveSyncSendAllHistoryDescription[] =
+    "With Send All History flag all sync entries are sent to Sync server "
+    "including "
+    "transition ink, bookmark, reload, etc";
 
 // Blink features.
 constexpr char kFileSystemAccessAPIName[] = "File System Access API";
@@ -799,6 +805,11 @@ constexpr char kBraveChangeActiveTabOnScrollEventDescription[] =
       flag_descriptions::kBraveSyncHistoryDiagnosticsDescription,           \
       kOsAll, FEATURE_VALUE_TYPE(                                           \
           brave_sync::features::kBraveSyncHistoryDiagnostics)},             \
+    {"brave-sync-send-all-history",                                         \
+      flag_descriptions::kBraveSyncSendAllHistoryName,                       \
+      flag_descriptions::kBraveSyncSendAllHistoryDescription,                \
+      kOsAll, FEATURE_VALUE_TYPE(                                           \
+          brave_sync::features::kBraveSyncSendAllHistory)},                 \
     BRAVE_IPFS_FEATURE_ENTRIES                                              \
     BRAVE_NATIVE_WALLET_FEATURE_ENTRIES                                     \
     BRAVE_NEWS_FEATURE_ENTRIES                                              \

chromium_src/components/history/core/browser/sync/typed_url_sync_bridge.cc

@@ -0,0 +1,49 @@
+/* Copyright (c) 2023 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 https://mozilla.org/MPL/2.0/. */
+
+#include "brave/components/brave_sync/features.h"
+
+namespace {
+
+bool IsSendAllHistoryEnabled() {
+  return base::FeatureList::IsEnabled(
+      brave_sync::features::kBraveSyncSendAllHistory);
+}
+
+}  // namespace
+
+#define BRAVE_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED \
+  if (IsSendAllHistoryEnabled()) {                    \
+    return true;                                      \
+  }
+
+#define BRAVE_HAS_TYPED_URL_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED \
+  if (IsSendAllHistoryEnabled()) {                                  \
+    /* When there are no visits, we must return false. Otherwise */ \
+    /* TypedURLSyncBridge will try to send data to sync .*/         \
+    return !visits.empty();                                         \
+  }
+
+#define BRAVE_TYPED_URL_SYNC_BRIDGE_SHOULD_SYNC_VISIT_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED \
+  BRAVE_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED
+
+// At TypedURLSyncBridge::WriteToTypedUrlSpecifics when syncing all
+// history, don't ignore reload transitions
+#define BRAVE_WRITE_TO_TYPED_URL_SPECIFICS_DONT_IGNORE_RELOADS \
+  if (!IsSendAllHistoryEnabled())
+
+// TypedURLSyncBridge::ApplySyncChanges is invoked when data arrives from the
+// server into local storage. Therefore it is possible that other device in
+// chain will have kBraveSyncSendAllHistory feature enabled, but the current
+// device won't. So skip DCHECK below without respect to the feature.
+#define BRAVE_TYPED_URL_SYNC_BRIDGE_APPLY_SYNC_CHANGES_SKIP_DCHECK if (false)
+
+#include "src/components/history/core/browser/sync/typed_url_sync_bridge.cc"
+
+#undef BRAVE_TYPED_URL_SYNC_BRIDGE_APPLY_SYNC_CHANGES_SKIP_DCHECK
+#undef BRAVE_WRITE_TO_TYPED_URL_SPECIFICS_DONT_IGNORE_RELOADS
+#undef BRAVE_HAS_TYPED_URL_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED
+#undef BRAVE_TYPED_URL_SYNC_BRIDGE_SHOULD_SYNC_VISIT_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED
+#undef BRAVE_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED

chromium_src/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc

@@ -0,0 +1,99 @@
+// Copyright (c) 2023 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 https://mozilla.org/MPL/2.0/.
+
+
+#include <vector>
+
+#include "base/test/scoped_feature_list.h"
+#include "brave/components/brave_sync/features.h"
+
+
+#define DeleteLocalTypedUrlVisit DISABLED_DeleteLocalTypedUrlVisit
+#define ExpireLocalTypedVisit DISABLED_ExpireLocalTypedVisit
+#define MaxVisitLocalTypedUrl DISABLED_MaxVisitLocalTypedUrlt
+#define NoTypedVisits DISABLED_NoTypedVisits
+#define ThrottleVisitLocalTypedUrl DISABLED_ThrottleVisitLocalTypedUrl
+
+#include "src/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc"
+
+#undef ThrottleVisitLocalTypedUrl
+#undef NoTypedVisits
+#undef MaxVisitLocalTypedUrl
+#undef ExpireLocalTypedVisit
+#undef DeleteLocalTypedUrlVisit
+
+namespace history {
+
+// This is re-done TypedURLSyncBridgeTest.NoTypedVisits test for 
+// the case when Brave's kBraveSyncSendAllHistory feature is enabled.
+TEST_F(TypedURLSyncBridgeTest, BraveSendAllVisitOverrideNoTypedVisits) {
+  base::test::ScopedFeatureList scoped_feature_list(
+      {brave_sync::features::kBraveSyncSendAllHistory});
+
+  std::vector<VisitRow> visits;
+  URLRow url(MakeTypedUrlRow(kURL, kTitle, 0, 1000, false, &visits));
+
+  sync_pb::TypedUrlSpecifics typed_url;
+  EXPECT_TRUE(WriteToTypedUrlSpecifics(url, visits, &typed_url));
+  // Below is difference from TypedURLSyncBridgeTest.NoTypedVisits:
+  // URLs without typed URL visits, but with link transition should be written
+  // to specifics.
+  EXPECT_EQ(1, typed_url.visits_size());
+}
+
+// Equals to TypedURLSyncBridgeTest.ExpireLastLocalVisit plus enabling
+// kBraveSyncSendAllHistory feature
+TEST_F(TypedURLSyncBridgeTest, BraveExpireLastLocalVisit) {
+  base::test::ScopedFeatureList scoped_feature_list(
+      {brave_sync::features::kBraveSyncSendAllHistory});
+  StartSyncing(std::vector<TypedUrlSpecifics>());
+
+  // Add two URLs into the history db and notify the bridge to get it synced up
+  // and thus also metadata written into the DB.
+  std::vector<VisitRow> visits1, visits2;
+  URLRow row1 = MakeTypedUrlRow(kURL, kTitle, /*typed_count=*/1,
+                                /*last_visit=*/1, /*hidden=*/false, &visits1);
+  URLRow row2 = MakeTypedUrlRow(kURL2, kTitle2, /*typed_count=*/1,
+                                /*last_visit=*/3, /*hidden=*/false, &visits2);
+  fake_history_backend_->SetVisitsForUrl(&row1, visits1);
+  fake_history_backend_->SetVisitsForUrl(&row2, visits2);
+
+  URLRow row1_original;
+  ASSERT_TRUE(fake_history_backend_->GetURL(GURL(kURL), &row1_original));
+
+  EXPECT_CALL(mock_processor_,
+              Put(IsValidStorageKey(), Pointee(HasTypedUrlInSpecifics()), _))
+      .Times(2u)
+      .WillRepeatedly(StoreMetadata);
+  bridge()->OnURLsModified(fake_history_backend_.get(), {row1, row2},
+                           /*is_from_expiration=*/false);
+
+  std::string storage_key1 = GetStorageKey(kURL);
+  std::string storage_key2 = GetStorageKey(kURL2);
+  EXPECT_THAT(GetAllSyncMetadataKeys(),
+              UnorderedElementsAre(storage_key1, storage_key2));
+
+  // Simulate expiration of all visits before time 2. Simulate kURL is
+  // bookmarked so that it does not get deleted despite there's no visit left.
+  EXPECT_CALL(*history_backend_client_, IsPinnedURL(GURL(kURL)))
+      .WillOnce(Return(true));
+  fake_history_backend_->ExpireHistoryBeforeForTesting(SinceEpoch(2));
+  URLRow row1_updated;
+  ASSERT_TRUE(fake_history_backend_->GetURL(GURL(kURL), &row1_updated));
+  EXPECT_EQ(row1_updated.typed_count(), 0);
+  EXPECT_EQ(row1_updated.last_visit(), base::Time());
+
+  // Notify typed url sync service of these URLs getting expired (it does not
+  // matter that we pass in the old version of row1, the bridge will fix it up).
+  EXPECT_CALL(mock_processor_, Delete).Times(0);
+  EXPECT_CALL(mock_processor_, UntrackEntityForStorageKey(storage_key1));
+  bridge()->OnURLsModified(fake_history_backend_.get(), {row1_updated},
+                           /*is_from_expiration=*/true);
+
+  // The urls are removed from the metadata store.
+  EXPECT_THAT(GetAllSyncMetadataKeys(), UnorderedElementsAre(storage_key2));
+}
+
+}  // namespace history    

components/brave_sync/features.cc

@@ -21,5 +21,12 @@ BASE_FEATURE(kBraveSyncHistoryDiagnostics,
              "BraveSyncHistoryDiagnostics",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// When this feature is enabled, Sync sends to remote server all the history
+// entries, including page transition beyond typed url (link, bookmark, reload,
+// etc).
+BASE_FEATURE(kBraveSyncSendAllHistory,
+             "BraveSyncSendAllHistory",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+
 }  // namespace features
 }  // namespace brave_sync

components/brave_sync/features.h

@@ -15,6 +15,8 @@ BASE_DECLARE_FEATURE(kBraveSync);
 
 BASE_DECLARE_FEATURE(kBraveSyncHistoryDiagnostics);
 
+BASE_DECLARE_FEATURE(kBraveSyncSendAllHistory);
+
 }  // namespace features
 }  // namespace brave_sync
 

patches/components-history-core-browser-history_backend.cc.patch

@@ -0,0 +1,13 @@
+diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
+index 21dc7a8b1f94c67c3ebe183547cdb5dedca948e0..0af590e6480a70ef77cb24a80bb13c9fdb29ee0e 100644
+--- a/components/history/core/browser/history_backend.cc
++++ b/components/history/core/browser/history_backend.cc
+@@ -3,7 +3,7 @@
+ // found in the LICENSE file.
+
+ #include "components/history/core/browser/history_backend.h"
+-
++    #include "brave/components/brave_sync/features.h"
+ #include <algorithm>
+ #include <functional>
+ #include <iterator>

patches/components-history-core-browser-sync-typed_url_sync_bridge.cc.patch

@@ -0,0 +1,44 @@
+diff --git a/components/history/core/browser/sync/typed_url_sync_bridge.cc b/components/history/core/browser/sync/typed_url_sync_bridge.cc
+index 487d267c3a423c613fd41c74893933355f1d2a97..155bac5fee7111cddfba202c30d66008c26e0739 100644
+--- a/components/history/core/browser/sync/typed_url_sync_bridge.cc
++++ b/components/history/core/browser/sync/typed_url_sync_bridge.cc
+@@ -68,6 +68,7 @@ std::string GetStorageKeyFromURLRow(const URLRow& row) {
+ }
+
+ bool HasTypedUrl(const std::vector<VisitRow>& visits) {
++  BRAVE_HAS_TYPED_URL_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED
+   return base::ranges::any_of(visits, [](const VisitRow& visit) {
+     return ui::PageTransitionCoreTypeIs(visit.transition,
+                                         ui::PAGE_TRANSITION_TYPED);
+@@ -239,6 +240,7 @@ absl::optional<syncer::ModelError> TypedURLSyncBridge::ApplySyncChanges(
+       continue;
+     }
+
++    BRAVE_TYPED_URL_SYNC_BRIDGE_APPLY_SYNC_CHANGES_SKIP_DCHECK
+     DCHECK(specifics.visits_size());
+     sync_pb::TypedUrlSpecifics filtered_url = FilterExpiredVisits(specifics);
+     if (filtered_url.visits_size() == 0) {
+@@ -527,6 +529,7 @@ bool TypedURLSyncBridge::WriteToTypedUrlSpecifics(
+     // Walk the passed-in visit vector and count the # of typed visits.
+     for (const VisitRow& visit : visits) {
+       // We ignore reload visits.
++      BRAVE_WRITE_TO_TYPED_URL_SPECIFICS_DONT_IGNORE_RELOADS
+       if (PageTransitionCoreTypeIs(visit.transition,
+                                    ui::PAGE_TRANSITION_RELOAD)) {
+         continue;
+@@ -554,6 +557,7 @@ bool TypedURLSyncBridge::WriteToTypedUrlSpecifics(
+
+   for (const VisitRow& visit : visits) {
+     // Skip reload visits.
++    BRAVE_WRITE_TO_TYPED_URL_SPECIFICS_DONT_IGNORE_RELOADS
+     if (PageTransitionCoreTypeIs(visit.transition,
+                                  ui::PAGE_TRANSITION_RELOAD)) {
+       continue;
+@@ -1136,6 +1140,7 @@ bool TypedURLSyncBridge::ShouldIgnoreVisits(
+
+ bool TypedURLSyncBridge::ShouldSyncVisit(int typed_count,
+                                          ui::PageTransition transition) {
++  BRAVE_TYPED_URL_SYNC_BRIDGE_SHOULD_SYNC_VISIT_RETURN_TRUE_IF_SEND_ALL_HISTORY_ENABLED
+   // Just use an ad-hoc criteria to determine whether to ignore this
+   // notification. For most users, the distribution of visits is roughly a bell
+   // curve with a long tail - there are lots of URLs with < 5 visits so we want

patches/components-history-core-browser-sync-typed_url_sync_bridge_unittest.cc.patch

@@ -0,0 +1,15 @@
+diff --git a/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc b/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc
+index cc1f6024e05d8138bc2b12e9023db51bb6b57504..35cb3b6ac75994b80dc92705a842b85a98f17885 100644
+--- a/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc
++++ b/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc
+@@ -160,7 +160,10 @@ URLRow MakeTypedUrlRow(const std::string& url,
+   history_url.set_hidden(hidden);
+
+   Time last_visit_time = SinceEpoch(last_visit);
++LOG(ERROR) << "[BraveSync] " << __func__ << " last_visit_time=" <<  last_visit_time;
++LOG(ERROR) << "[BraveSync] " << __func__ << " last_visit_time.is_null()=" <<  last_visit_time.is_null();
+   history_url.set_last_visit(last_visit_time);
++LOG(ERROR) << "[BraveSync] " << __func__ << " history_url.last_visit()=" <<  history_url.last_visit();  
+
+   ui::PageTransition transition = ui::PAGE_TRANSITION_RELOAD;
+   bool incremented_omnibox_typed_score = false;

patches/components-sync-model-processor_entity.cc.patch

@@ -0,0 +1,15 @@
+diff --git a/components/sync/model/processor_entity.cc b/components/sync/model/processor_entity.cc
+index 59c6b68f73487fa3e4fba7f5269dddef8896257d..e89cceaf2e278ac02623ff239d54dbab80ab6dce 100644
+--- a/components/sync/model/processor_entity.cc
++++ b/components/sync/model/processor_entity.cc
+@@ -147,8 +147,8 @@ bool ProcessorEntity::IsVersionAlreadyKnown(int64_t update_version) const {
+
+ void ProcessorEntity::RecordIgnoredRemoteUpdate(
+     const UpdateResponseData& update) {
+-  DCHECK(metadata_.server_id().empty() ||
+-         metadata_.server_id() == update.entity.id);
++//  DCHECK(metadata_.server_id().empty() ||
++//         metadata_.server_id() == update.entity.id);
+   metadata_.set_server_id(update.entity.id);
+   metadata_.set_server_version(update.response_version);
+   // Either these already matched, acked was just bumped to squash a pending

test/BUILD.gn

@@ -154,6 +154,8 @@ test("brave_unit_tests") {
     "//components/sync/driver/sync_auth_manager_unittest.cc",
     "//components/sync/engine/sync_scheduler_impl_unittest.cc",
     "//components/sync_device_info/device_info_sync_bridge_unittest.cc",
+
+    "//components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc",
   ]
 
   deps = [