Merge pull request #3749 from brave/sync-compaction

Trigger sync compaction for bookmarks every week

DEPS

@@ -15,7 +15,7 @@ deps = {
   "vendor/bip39wally-core-native": "https://github.com/brave-intl/bip39wally-core-native.git@13bb40a215248cfbdd87d0a6b425c8397402e9e6",
   "vendor/bat-native-anonize": "https://github.com/brave-intl/bat-native-anonize.git@e3742ba3e8942eea9e4755d91532491871bd3116",
   "vendor/bat-native-tweetnacl": "https://github.com/brave-intl/bat-native-tweetnacl.git@dd0c535898a645b84d6f3372b393bdad6746108c",
-  "components/brave_sync/extension/brave-sync": "https://github.com/brave/sync.git@8a9f4fefe42c25b5359c402f97f454fa333c8d48",
+  "components/brave_sync/extension/brave-sync": "https://github.com/brave/sync.git@3c443268bef1675532a472875151a5f053fee82e",
   "vendor/bat-native-usermodel": "https://github.com/brave-intl/bat-native-usermodel.git@a82acda22d8cb255d86ee28734efb8ad886e9a49",
   "vendor/challenge_bypass_ristretto_ffi": "https://github.com/brave-intl/challenge-bypass-ristretto-ffi.git@f88d942ddfaf61a4a6703355a77c4ef71bc95c35",
 }

browser/extensions/api/brave_sync_api.cc

@@ -153,6 +153,21 @@ BraveSyncSaveBookmarksBaseOrderFunction::Run() {
   return RespondNow(NoArguments());
 }
 
+ExtensionFunction::ResponseAction
+BraveSyncOnCompactCompleteFunction::Run() {
+  std::unique_ptr<brave_sync::OnCompactComplete::Params> params(
+      brave_sync::OnCompactComplete::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  BraveSyncService* sync_service = GetSyncService(browser_context());
+  DCHECK(sync_service);
+  sync_service->GetBraveSyncClient()
+      ->sync_message_handler()
+      ->OnCompactComplete(params->category_name);
+
+  return RespondNow(NoArguments());
+}
+
 ExtensionFunction::ResponseAction BraveSyncExtensionInitializedFunction::Run() {
   // Also inform sync client extension started
   BraveSyncService* sync_service = GetSyncService(browser_context());

browser/extensions/api/brave_sync_api.h

@@ -59,6 +59,12 @@ class BraveSyncSaveBookmarksBaseOrderFunction : public ExtensionFunction {
   ResponseAction Run() override;
 };
 
+class BraveSyncOnCompactCompleteFunction : public ExtensionFunction {
+  ~BraveSyncOnCompactCompleteFunction() override {}
+  DECLARE_EXTENSION_FUNCTION("braveSync.onCompactComplete", UNKNOWN)
+  ResponseAction Run() override;
+};
+
 class BraveSyncExtensionInitializedFunction : public ExtensionFunction {
   ~BraveSyncExtensionInitializedFunction() override {}
   DECLARE_EXTENSION_FUNCTION("braveSync.extensionInitialized", UNKNOWN)

browser/extensions/api/brave_sync_event_router.cc

@@ -108,6 +108,19 @@ void BraveSyncEventRouter::SendGetBookmarksBaseOrder(
   event_router_->BroadcastEvent(std::move(event));
 }
 
+void BraveSyncEventRouter::SendCompact(
+    const std::string& category_name) {
+  std::unique_ptr<base::ListValue> args(
+     extensions::api::brave_sync::SendCompact::Create(
+        category_name).release());
+  std::unique_ptr<Event> event(
+     new Event(extensions::events::FOR_TEST,
+       extensions::api::brave_sync::SendCompact::kEventName,
+       std::move(args)));
+
+  event_router_->BroadcastEvent(std::move(event));
+}
+
 void BraveSyncEventRouter::LoadClient() {
   std::unique_ptr<base::ListValue> args(
      extensions::api::brave_sync::OnLoadClient::Create()

browser/extensions/api/brave_sync_event_router.h

@@ -53,6 +53,8 @@ class BraveSyncEventRouter {
   void SendGetBookmarksBaseOrder(const std::string& device_id,
                                  const std::string& platform);
 
+  void SendCompact(const std::string& category_name);
+
   void LoadClient();
 
  private:

common/extensions/api/brave_sync.json

@@ -267,6 +267,17 @@
           }
         ]
       },
+      {
+        "name": "sendCompact",
+        "type": "function",
+        "description": "Browser sends this to compact records of a category",
+        "parameters": [
+          {
+            "type": "string",
+            "name": "categoryName"
+          }
+        ]
+      },
       {
         "name": "onLoadClient",
         "type": "function",
@@ -396,6 +407,17 @@
             "name": "order"
           }
         ]
+      },
+      {
+        "name": "onCompactComplete",
+        "type": "function",
+        "description": "Notify browser that compaction for category is done",
+        "parameters": [
+          {
+            "type": "string",
+            "name": "category_name"
+          }
+        ]
       }
     ]
   }

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -565,13 +565,21 @@ void BraveProfileSyncServiceImpl::OnResolvedSyncRecords(
   } else if (category_name == kBookmarks) {
     for (auto& record : *records) {
       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
+      // cycle to have valid get_record_cb_
+      if (!pending_received_records_)
+        pending_received_records_ = std::make_unique<RecordsList>();
+      pending_received_records_->push_back(std::move(record));
     }
     // Send records to syncer
-    if (get_record_cb_)
+    if (get_record_cb_) {
       sync_thread_->task_runner()->PostTask(
           FROM_HERE,
           base::BindOnce(&DoDispatchGetRecordsCallback,
-                         std::move(get_record_cb_), std::move(records)));
+                         std::move(get_record_cb_),
+                         std::move(pending_received_records_)));
+    }
     SignalWaitableEvent();
   } else if (category_name == kHistorySites) {
     NOTIMPLEMENTED();
@@ -594,6 +602,12 @@ void BraveProfileSyncServiceImpl::OnSaveBookmarksBaseOrder(
   OnSyncReady();
 }
 
+void BraveProfileSyncServiceImpl::OnCompactComplete(
+    const std::string& category) {
+  if (category == kBookmarks)
+    brave_sync_prefs_->SetLastCompactTimeBookmarks(base::Time::Now());
+}
+
 int BraveProfileSyncServiceImpl::GetDisableReasons() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -822,6 +836,14 @@ void BraveProfileSyncServiceImpl::FetchSyncRecords(const bool bookmarks,
   }
   if (bookmarks) {
     category_names.push_back(kBookmarks);  // "BOOKMARKS";
+
+    base::Time last_compact_time =
+        brave_sync_prefs_->GetLastCompactTimeBookmarks();
+    if (tools::IsTimeEmpty(last_compact_time) ||
+        base::Time::Now() - last_compact_time >
+            base::TimeDelta::FromDays(kCompactPeriodInDays)) {
+      brave_sync_client_->SendCompact(kBookmarks);
+    }
   }
   if (preferences) {
     category_names.push_back(kPreferences);  // "PREFERENCES";

components/brave_sync/brave_profile_sync_service_impl.h

@@ -31,6 +31,7 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnResetSync);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, ClientOnGetInitData);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetInitData);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSaveBookmarksBaseOrder);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnCompactComplete);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSyncPrefsChanged);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSyncDebug);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, StartSyncNonDeviceRecords);
@@ -40,6 +41,7 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest,
                      OnSetupSyncHaveCode_Reset_SetupAgain);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, ExponentialResend);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, SendCompact);
 
 class BraveSyncServiceTest;
 
@@ -93,6 +95,7 @@ class BraveProfileSyncServiceImpl
   void OnDeletedSyncUser() override;
   void OnDeleteSyncSiteSettings() override;
   void OnSaveBookmarksBaseOrder(const std::string& order) override;
+  void OnCompactComplete(const std::string& category_name) override;
 
   // syncer::SyncService implementation
   int GetDisableReasons() const override;
@@ -135,6 +138,7 @@ class BraveProfileSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnResetSync);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, ClientOnGetInitData);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSaveBookmarksBaseOrder);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnCompactComplete);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnGetInitData);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSyncPrefsChanged);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSyncDebug);
@@ -146,6 +150,7 @@ class BraveProfileSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, ExponentialResend);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest,
                            GetDevicesWithFetchSyncRecords);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, SendCompact);
   friend class ::BraveSyncServiceTest;
 
   void SignalWaitableEvent();
@@ -196,6 +201,10 @@ class BraveProfileSyncServiceImpl
   static std::vector<unsigned> GetExponentialWaitsForTests();
   static const std::vector<unsigned> kExponentialWaits;
   static const int kMaxSendRetries;
+  static const int kCompactPeriodInDays = 7;
+  static constexpr int GetCompactPeriodInDaysForTests() {
+    return kCompactPeriodInDays;
+  }
 
   std::unique_ptr<brave_sync::prefs::Prefs> brave_sync_prefs_;
   // True when is in active sync chain
@@ -224,6 +233,7 @@ class BraveProfileSyncServiceImpl
 
   base::Time chain_created_time_;
   std::vector<RecordsListPtr> pending_send_records_;
+  std::unique_ptr<RecordsList> pending_received_records_;
 
   // Used to ensure that certain operations are performed on the sequence that
   // this object was created on.

components/brave_sync/brave_sync_prefs.cc

@@ -29,6 +29,8 @@ const char kSyncLatestRecordTime[] = "brave_sync.latest_record_time";
 const char kSyncLatestDeviceRecordTime[] =
     "brave_sync.latest_device_record_time";
 const char kSyncLastFetchTime[] = "brave_sync.last_fetch_time";
+const char kSyncLastCompactTimeBookmarks[] =
+    "brave_sync.last_compact_time.bookmarks";
 const char kSyncDeviceList[] = "brave_sync.device_list";
 const char kSyncApiVersion[] = "brave_sync.api_version";
 const char kSyncMigrateBookmarksVersion[]
@@ -53,6 +55,8 @@ void Prefs::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterTimePref(prefs::kSyncLatestRecordTime, base::Time());
   registry->RegisterTimePref(prefs::kSyncLatestDeviceRecordTime, base::Time());
   registry->RegisterTimePref(prefs::kSyncLastFetchTime, base::Time());
+  registry->RegisterTimePref(prefs::kSyncLastCompactTimeBookmarks,
+                             base::Time());
 
   registry->RegisterStringPref(prefs::kSyncDeviceList, std::string());
   registry->RegisterStringPref(prefs::kSyncApiVersion, std::string("0"));
@@ -174,6 +178,14 @@ base::Time Prefs::GetLastFetchTime() {
   return pref_service_->GetTime(kSyncLastFetchTime);
 }
 
+void Prefs::SetLastCompactTimeBookmarks(const base::Time &time) {
+  pref_service_->SetTime(kSyncLastCompactTimeBookmarks, time);
+}
+
+base::Time Prefs::GetLastCompactTimeBookmarks() {
+  return pref_service_->GetTime(kSyncLastCompactTimeBookmarks);
+}
+
 std::unique_ptr<SyncDevices> Prefs::GetSyncDevices() {
   auto existing_sync_devices = std::make_unique<SyncDevices>();
   std::string json_device_list = pref_service_->GetString(kSyncDeviceList);

components/brave_sync/brave_sync_prefs.h

@@ -102,6 +102,8 @@ class Prefs {
   base::Time GetLatestDeviceRecordTime();
   void SetLastFetchTime(const base::Time &time);
   base::Time GetLastFetchTime();
+  void SetLastCompactTimeBookmarks(const base::Time &time);
+  base::Time GetLastCompactTimeBookmarks();
 
   std::unique_ptr<SyncDevices> GetSyncDevices();
   void SetSyncDevices(const SyncDevices& sync_devices);

components/brave_sync/brave_sync_service_unittest.cc

@@ -873,8 +873,7 @@ TEST_F(BraveSyncServiceTest, ExponentialResend) {
   // emulate the wait time after reaching maximum retry
   EXPECT_TRUE(contains(should_sent_at, 230));
   for (size_t i = 0; i <= 231 + 1; ++i) {
-    auto time_override =
-        OverrideForTimeDelta(base::TimeDelta::FromMinutes(i));
+    auto time_override = OverrideForTimeDelta(base::TimeDelta::FromMinutes(i));
     bool is_send_expected = contains(should_sent_at, i);
     int expect_call_times = is_send_expected ? 1 : 0;
     EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", _))
@@ -976,3 +975,25 @@ TEST_F(BraveSyncServiceTest, GetDevicesWithFetchSyncRecords) {
     sync_service()->FetchDevices();
   }
 }
+
+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);
+  sync_service()->FetchSyncRecords(true, false, true, 1000);
+  // timestamp is not writtend until we get CompactedSyncCategory
+  EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1);
+  sync_service()->FetchSyncRecords(true, false, true, 1000);
+  sync_service()->OnCompactComplete(kBookmarks);
+  EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(0);
+  sync_service()->FetchSyncRecords(true, false, true, 1000);
+  {
+    auto time_override = OverrideForTimeDelta(base::TimeDelta::FromDays(
+        sync_service()->GetCompactPeriodInDaysForTests()));
+    EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(1);
+    sync_service()->FetchSyncRecords(true, false, true, 1000);
+    sync_service()->OnCompactComplete(kBookmarks);
+  }
+  EXPECT_CALL(*sync_client(), SendCompact(kBookmarks)).Times(0);
+  sync_service()->FetchSyncRecords(true, false, true, 1000);
+}

components/brave_sync/client/brave_sync_client.h

@@ -51,6 +51,8 @@ class SyncMessageHandler {
   virtual void OnDeleteSyncSiteSettings() = 0;
   // SAVE_BOOKMARKS_BASE_ORDER
   virtual void OnSaveBookmarksBaseOrder(const std::string& order) = 0;
+  // COMOACTED_SYNC_CATEGORY
+  virtual void OnCompactComplete(const std::string& category_name) = 0;
 };
 
 class BraveSyncClient {
@@ -78,6 +80,8 @@ class BraveSyncClient {
   virtual void SendDeleteSyncCategory(const std::string &category_name) = 0;
   virtual void SendGetBookmarksBaseOrder(const std::string &device_id,
                                          const std::string &platform) = 0;
+  // COMPACT_SYNC_CATEGORY
+  virtual void SendCompact(const std::string& category_name) = 0;
 
   virtual void OnExtensionInitialized() = 0;