Merge pull request #1532 from brave/sync-revert

Revert "Merge pull request #1188 from brave/sync_frequent_initial_update"

Author: darkdh

components/brave_sync/brave_sync_service_impl.cc

@@ -290,9 +290,7 @@ void BraveSyncServiceImpl::BackgroundSyncStarted(bool startup) {
   if (startup)
     bookmark_change_processor_->Start();
 
-  const bool waiting_for_second_device =
-                            !sync_prefs_->GetSyncDevices()->has_second_device();
-  StartLoop(waiting_for_second_device);
+  StartLoop();
 }
 
 void BraveSyncServiceImpl::BackgroundSyncStopped(bool shutdown) {
@@ -468,10 +466,9 @@ BraveSyncServiceImpl::PrepareResolvedPreferences(const RecordsList& records) {
 void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
   const std::string this_device_id = sync_prefs_->GetThisDeviceId();
   bool this_device_deleted = false;
-  bool at_least_one_deleted = false;
+  bool contains_only_one_device = false;
 
   auto sync_devices = sync_prefs_->GetSyncDevices();
-  const bool waiting_for_second_device = !sync_devices->has_second_device();
   for (const auto &record : records) {
     DCHECK(record->has_device() || record->has_sitesetting());
     if (record->has_device()) {
@@ -487,20 +484,18 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
         (record->deviceId == this_device_id &&
           record->action == jslib::SyncRecord::Action::A_DELETE &&
           actually_merged);
-      at_least_one_deleted = at_least_one_deleted ||
-          (record->action == jslib::SyncRecord::Action::A_DELETE &&
-          actually_merged);
+      contains_only_one_device = sync_devices->size() < 2 &&
+        record->action == jslib::SyncRecord::Action::A_DELETE &&
+          actually_merged;
     }
   } // for each device
 
   sync_prefs_->SetSyncDevices(*sync_devices);
-  if (this_device_deleted ||
-                 (at_least_one_deleted && !sync_devices->has_second_device())) {
+
+  if (this_device_deleted)
     ResetSyncInternal();
-  } else if (waiting_for_second_device && sync_devices->has_second_device()) {
-    // Restart loop with 30 sec interval
-    StartLoop(false);
-  }
+  if (contains_only_one_device)
+    OnResetSync();
 }
 
 void BraveSyncServiceImpl::OnSyncPrefsChanged(const std::string& pref) {
@@ -615,14 +610,10 @@ void BraveSyncServiceImpl::SendDeviceSyncRecord(
 }
 
 static const int64_t kCheckUpdatesIntervalSec = 60;
-static const int64_t kCheckInitialUpdatesIntervalSec = 1;
 
-void BraveSyncServiceImpl::StartLoop(const bool use_initial_update_interval) {
+void BraveSyncServiceImpl::StartLoop() {
   timer_->Start(FROM_HERE,
-                  base::TimeDelta::FromSeconds(
-                    use_initial_update_interval ?
-                        kCheckInitialUpdatesIntervalSec :
-                        kCheckUpdatesIntervalSec),
+                  base::TimeDelta::FromSeconds(kCheckUpdatesIntervalSec),
                   this,
                   &BraveSyncServiceImpl::LoopProc);
 }
@@ -649,10 +640,6 @@ void BraveSyncServiceImpl::LoopProcThreadAligned() {
   RequestSyncData();
 }
 
-base::TimeDelta BraveSyncServiceImpl::GetLoopDelay() const {
-  return timer_->GetCurrentDelay();
-}
-
 void BraveSyncServiceImpl::NotifyLogMessage(const std::string& message) {
   DLOG(INFO) << message;
 }

components/brave_sync/brave_sync_service_impl.h

@@ -33,7 +33,6 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnSyncReadyNewToSync);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetExistingObjects);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, BackgroundSyncStarted);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, BackgroundSyncStopped);
-FORWARD_DECLARE_TEST(BraveSyncServiceTest, LoopDelayVaries);
 
 class BraveSyncServiceTest;
 
@@ -111,7 +110,6 @@ class BraveSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnGetExistingObjects);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BackgroundSyncStarted);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BackgroundSyncStopped);
-  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, LoopDelayVaries);
   friend class ::BraveSyncServiceTest;
 
   // SyncMessageHandler overrides
@@ -159,11 +157,10 @@ class BraveSyncServiceImpl
     const std::string& deviceId,
     const std::string& objectId);
 
-  void StartLoop(const bool use_initial_update_interval);
+  void StartLoop();
   void StopLoop();
   void LoopProc();
   void LoopProcThreadAligned();
-  base::TimeDelta GetLoopDelay() const;  // For tests only
 
   void GetExistingHistoryObjects(
     const RecordsList &records,

components/brave_sync/brave_sync_service_unittest.cc

@@ -526,7 +526,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) {
   auto resolved_record = SyncRecord::Clone(*records.at(0));
   resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
   resolved_records.push_back(std::move(resolved_record));
-  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(AtLeast(3));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(5);
   sync_service()->OnResolvedPreferences(resolved_records);
 
   auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
@@ -698,94 +698,3 @@ TEST_F(BraveSyncServiceTest, BackgroundSyncStopped) {
   sync_service()->BackgroundSyncStopped(false);
   EXPECT_FALSE(sync_service()->timer_->IsRunning());
 }
-
-TEST_F(BraveSyncServiceTest, LoopDelayVaries) {
-  // This test is almost the same as BraveSyncServiceTest::OnDeleteDevice
-  // Loop delay should be:
-  // 1 sec when there is no sync chain yet
-  // 60 sec when 2 or more devices
-  // after sync chain destroy the loop should not be running
-
-  sync_service()->BackgroundSyncStarted(false);
-
-  EXPECT_TRUE(sync_service()->timer_->IsRunning());
-  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 1u);
-
-  RecordsList records;
-  records.push_back(SimpleDeviceRecord(
-      jslib::SyncRecord::Action::A_CREATE,
-      "1", "device1"));
-  records.push_back(SimpleDeviceRecord(
-      jslib::SyncRecord::Action::A_CREATE,
-      "2", "device2"));
-  records.push_back(SimpleDeviceRecord(
-      jslib::SyncRecord::Action::A_CREATE,
-      "3", "device3"));
-  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
-  sync_service()->OnResolvedPreferences(records);
-
-  sync_service()->sync_prefs_->SetThisDeviceId("1");
-  auto devices = sync_service()->sync_prefs_->GetSyncDevices();
-
-  // Have 3 devices now
-  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 60u);
-
-  EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1"));
-  EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2"));
-  EXPECT_TRUE(DevicesContains(devices.get(), "3", "device3"));
-
-  using brave_sync::jslib::SyncRecord;
-  EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES",
-      ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device3"))).Times(1);
-  sync_service()->OnDeleteDevice("3");
-
-  RecordsList resolved_records;
-  auto resolved_record = SyncRecord::Clone(*records.at(2));
-  resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
-  resolved_records.push_back(std::move(resolved_record));{
-  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
-  sync_service()->OnResolvedPreferences(resolved_records);}
-
-  auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
-  EXPECT_TRUE(DevicesContains(devices_final.get(), "1", "device1"));
-  EXPECT_TRUE(DevicesContains(devices_final.get(), "2", "device2"));
-  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
-
-  // Have 2 devices now, still expecting 60 sec delay
-  EXPECT_EQ(sync_service()->GetLoopDelay().InSeconds(), 60u);
-
-  // Delete all remaining devices
-  resolved_records.clear();
-
-  auto resolved_record_0 = SyncRecord::Clone(*records.at(0));
-  resolved_record_0->action = jslib::SyncRecord::Action::A_DELETE;
-  resolved_records.push_back(std::move(resolved_record_0));
-
-  auto resolved_record_1 = SyncRecord::Clone(*records.at(1));
-  resolved_record_1->action = jslib::SyncRecord::Action::A_DELETE;
-  resolved_records.push_back(std::move(resolved_record_1));
-
-  {
-    // Expecting call of OnSyncStateChanged at least 3 times:
-    // delete "device1"
-    // delete "device2"
-    // brave_sync.enabled to false, several times
-    EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(AtLeast(3));
-    EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1));
-    sync_service()->OnResolvedPreferences(resolved_records);
-  }
-
-  devices_final = sync_service()->sync_prefs_->GetSyncDevices();
-  EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1"));
-  EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2"));
-  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
-
-  // We have mock client, so emulate these steps when all devices removed
-  // 1) brave_sync.enabled goes to false
-  // 2) BraveSyncClientImpl::OnSyncEnabledChanged
-  // 3) BraveSyncClientImpl::LoadOrUnloadExtension(false)
-  // 4) BraveSyncClientImpl::OnExtensionUnloaded
-  // 5) BraveSyncServiceImpl::BackgroundSyncStopped
-  sync_service()->BackgroundSyncStopped(true);
-  EXPECT_FALSE(sync_service()->timer_->IsRunning());
-}

components/brave_sync/sync_devices.h

@@ -43,7 +43,6 @@ class SyncDevices {
    std::unique_ptr<base::Value> ToValueArrOnly() const;
    std::string ToJson() const;
    size_t size() const { return devices_.size(); }
-   bool has_second_device() const { return size() >= 2; }
    void FromJson(const std::string &str_json);
    void Merge(const SyncDevice& device, int action, bool* actually_merged);