Update devices each 1 second until chain is not created #1188

AlexeyBarabash · 2018-12-26T13:47:26Z

Fixes brave/brave-browser#2742 , brave/brave-browser#2782 .

Improves response time on sync creation.

Submitter Checklist:

Test Plan:

On Device1 go to chrome://sync/
Press "Start new chain" => "Computer" => "Copy code"
On device 2 go to chrome://sync/
"Enter sync chain code" button
Paste the code
"Confirm sync code" button
Device1 gets created a sync chain not so late (<5 sec) after Device2

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

darkdh

doesn't this cause two LoopProc running with different interval simultaneously?

components/brave_sync/brave_sync_service_impl.cc

darkdh

brave/brave-browser#2742 (comment)

AlexeyBarabash · 2018-12-28T11:19:06Z

Pushed the fix. Will squash and rebase after approval.

components/brave_sync/brave_sync_service_impl.cc

bridiver · 2018-12-28T18:56:46Z

components/brave_sync/brave_sync_service_impl.cc

@@ -463,6 +464,7 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
  bool contains_only_one_device = false;

  auto sync_devices = sync_prefs_->GetSyncDevices();
+  bool waiting_for_second_device = sync_devices->size() < 2;


can you add a method for this? There are two checks that both check the same condition, but with different values <= 1 vs < 2 and it's confusing to read like that

components/brave_sync/brave_sync_service_impl.cc

bridiver · 2018-12-28T18:57:54Z

@AlexeyBarabash can you please add a test for this?

AlexeyBarabash · 2018-12-31T13:05:26Z

@darkdh @bridiver
Thanks for the review.
Added a test and fixed lint issues.
Tried to run npm run lint but then understand brave/brave-browser#2743 is not merged yet. In any way looking forward to start use it.

AlexeyBarabash · 2019-01-01T22:13:36Z

Pushed a fix for #1188 (comment) comment .

AlexeyBarabash · 2019-01-02T11:49:22Z

Rebased and squashed commits.

bridiver · 2019-01-02T15:55:54Z

components/brave_sync/brave_sync_service_impl.cc

@@ -484,7 +487,7 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
        (record->deviceId == this_device_id &&
          record->action == jslib::SyncRecord::Action::A_DELETE &&
          actually_merged);
-      contains_only_one_device = sync_devices->size() < 2 &&
+      contains_only_one_device = sync_devices->have_less_than_two() &&


I'm confused about this, if there's only one device and it has been deleted, then there are no devices, not one device

bridiver · 2019-01-02T16:09:39Z

components/brave_sync/brave_sync_service_impl.cc

@@ -496,6 +499,11 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
    ResetSyncInternal();
  if (contains_only_one_device)
    OnResetSync();
+
+  if (waiting_for_second_device && !sync_devices->have_less_than_two()) {


I don't understand why we need this second check and the extra var. Why do we care what the previous size of the sync_devices was? It's also possible that both this_device_deleted and contains_only_one_device can both be true. The logic here just doesn't make sense to me

couldn't we just do something like this?

diff --git a/components/brave_sync/brave_sync_service_impl.cc b/components/brave_sync/brave_sync_service_impl.cc index cb76609e..09fb02e7 100644 --- a/components/brave_sync/brave_sync_service_impl.cc +++ b/components/brave_sync/brave_sync_service_impl.cc @@ -464,9 +464,7 @@ 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 contains_only_one_device = false; auto sync_devices = sync_prefs_->GetSyncDevices(); for (const auto &record : records) { @@ -480,22 +478,21 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) { record->syncTimestamp.ToJsTime()), record->action, &actually_merged); - this_device_deleted = this_device_deleted || - (record->deviceId == this_device_id && + this_device_deleted = + record->deviceId == sync_prefs_->GetThisDeviceId() && 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) + if (this_device_deleted || sync_devices->size() == 0) { ResetSyncInternal(); - if (contains_only_one_device) - OnResetSync(); + } else if (sync_devices->size() == 1) { + StartLoop(true); + } } void BraveSyncServiceImpl::OnSyncPrefsChanged(const std::string& pref) {

I'm confused about this, if there's only one device and it has been deleted, then there are no devices, not one device

Yes this is a naming issue.

I don't understand why we need this second check and the extra var. Why do we care what the previous size of the sync_devices was?

Because we want to set update interval from 1 to 60 sec if devices size was changed from (0|1) to (2|3|...); and we dont want to touch update interval when devices list was changed from in other cases, like (2|3|4 => 3|4|5) or (3|4|5 => 2|3|4).

The idea is: we cannot have less than two devices and if we have less than two devices we should fetch devices list 1 time/sec to improve the response time.

It's also possible that both this_device_deleted and contains_only_one_device can both be true. The logic here just doesn't make sense to me

There are three things which does BraveSyncServiceImpl::OnResolvedPreferences:

We received this device is being deleted, someone on other device in chain pressed the cross near this device.
We mark preferences to set sync state to not enabled.

When devices list size gets to 1, this means sync chain is no longer exists about to be destroyed. We also mark preferences as sync not enabled.

ResetSyncInternal() and OnResetSync() do the same if (sync_devices->size() <= 1).

set fetch sync records interval from 1 sec to 60 sec when sync chain created, devices size was changed from (0|1) to (2|3|...).

Modifying.

bridiver · 2019-01-02T16:23:56Z

components/brave_sync/brave_sync_service_impl.cc


-void BraveSyncServiceImpl::StartLoop() {
+void BraveSyncServiceImpl::StartLoop(const bool waiting_for_second_device) {


maybe use_initial_update_interval?

agree, sounds more self-explaining.

AlexeyBarabash · 2019-01-04T13:12:24Z

Pushed fixes.

AlexeyBarabash · 2019-01-14T10:45:57Z

Rebased to current master.

darkdh

lgtm

bridiver · 2019-01-14T16:56:49Z

components/brave_sync/sync_devices.h

@@ -43,6 +43,7 @@ class SyncDevices {
   std::unique_ptr<base::Value> ToValueArrOnly() const;
   std::string ToJson() const;
   size_t size() const { return devices_.size(); }
+   bool have_less_than_two() const { return size() < 2; }


just a nit, but can we make this has_second_device and reverse the logic? I think the purpose would be more intuitive and have_less_than_two just sounds weird to me

Modified this.

AlexeyBarabash · 2019-01-14T21:28:09Z

Put the fix according to the last notice, squashed fix commits and force pushed the branch.
So I need again your approval @bridiver @darkdh .

Update devices each 1 second until chain is not created

…ate" This reverts commit f005c59, reversing changes made to 9c45f2f. fix brave/brave-browser#3182

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

AlexeyBarabash requested a review from darkdh December 26, 2018 13:47

AlexeyBarabash self-assigned this Dec 26, 2018

AlexeyBarabash added enhancement feature/sync labels Dec 26, 2018

darkdh reviewed Dec 27, 2018

View reviewed changes

darkdh reviewed Dec 28, 2018

View reviewed changes

components/brave_sync/brave_sync_service_impl.cc Outdated Show resolved Hide resolved

components/brave_sync/brave_sync_service_impl.cc Outdated Show resolved Hide resolved

bridiver reviewed Dec 28, 2018

View reviewed changes

components/brave_sync/brave_sync_service_impl.cc Outdated Show resolved Hide resolved

AlexeyBarabash force-pushed the sync_frequent_initial_update branch from d157365 to 2b2286a Compare January 2, 2019 11:47

AlexeyBarabash force-pushed the sync_frequent_initial_update branch from 2b2286a to 190aff1 Compare January 2, 2019 12:08

AlexeyBarabash mentioned this pull request Jan 2, 2019

Adding a device to sync chain doesn't enable sync on Linux instantly brave/brave-browser#2782

Closed

bridiver reviewed Jan 2, 2019

View reviewed changes

AlexeyBarabash added the PR/Pending Review label Jan 8, 2019

AlexeyBarabash force-pushed the sync_frequent_initial_update branch from 2e5f82c to 0fd2666 Compare January 14, 2019 10:45

darkdh previously approved these changes Jan 14, 2019

View reviewed changes

bridiver reviewed Jan 14, 2019

View reviewed changes

AlexeyBarabash dismissed darkdh’s stale review via f293eaa January 14, 2019 21:16

AlexeyBarabash added 2 commits January 14, 2019 23:21

Update devices each 1 second until chain is not created

1789cd2

Unit test for variable loop interval

ca1da86

AlexeyBarabash force-pushed the sync_frequent_initial_update branch from f293eaa to ca1da86 Compare January 14, 2019 21:23

bridiver approved these changes Jan 14, 2019

View reviewed changes

AlexeyBarabash merged commit f005c59 into master Jan 14, 2019

AlexeyBarabash added a commit that referenced this pull request Jan 14, 2019

Merge pull request #1188 from brave/sync_frequent_initial_update

9c80f41

Update devices each 1 second until chain is not created

AlexeyBarabash removed the PR/Pending Review label Jan 14, 2019

AlexeyBarabash added a commit that referenced this pull request Jan 14, 2019

Merge pull request #1188 from brave/sync_frequent_initial_update

101b132

Update devices each 1 second until chain is not created

bsclifton deleted the sync_frequent_initial_update branch January 24, 2019 21:15

darkdh mentioned this pull request Feb 1, 2019

Sync chain creator does not get some bookmarks on initial sync brave/brave-browser#3182

Closed

darkdh added a commit that referenced this pull request Feb 1, 2019

Revert "Merge pull request #1188 from brave/sync_frequent_initial_upd…

1cf3ec9

…ate" This reverts commit f005c59, reversing changes made to 9c45f2f. fix brave/brave-browser#3182

darkdh added a commit that referenced this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert

02850d6

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

darkdh added a commit that referenced this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert

a4f9c3f

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

darkdh mentioned this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert #1533

Merged

18 tasks

darkdh added a commit that referenced this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert

63c32a5

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

darkdh mentioned this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert #1534

Merged

18 tasks

darkdh added a commit that referenced this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert

b2bf9bc

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

darkdh mentioned this pull request Feb 1, 2019

Merge pull request #1532 from brave/sync-revert #1536

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update devices each 1 second until chain is not created #1188

Update devices each 1 second until chain is not created #1188

AlexeyBarabash commented Dec 26, 2018 •

edited

Loading

darkdh left a comment

darkdh left a comment

AlexeyBarabash commented Dec 28, 2018

bridiver Dec 28, 2018

bridiver commented Dec 28, 2018

AlexeyBarabash commented Dec 31, 2018

AlexeyBarabash commented Jan 1, 2019

AlexeyBarabash commented Jan 2, 2019

bridiver Jan 2, 2019

bridiver Jan 2, 2019

bridiver Jan 2, 2019 •

edited

Loading

AlexeyBarabash Jan 4, 2019

bridiver Jan 2, 2019

AlexeyBarabash Jan 4, 2019

AlexeyBarabash commented Jan 4, 2019

AlexeyBarabash commented Jan 14, 2019

darkdh left a comment

bridiver Jan 14, 2019

AlexeyBarabash Jan 14, 2019

AlexeyBarabash commented Jan 14, 2019


		void BraveSyncServiceImpl::StartLoop() {
		void BraveSyncServiceImpl::StartLoop(const bool waiting_for_second_device) {

Update devices each 1 second until chain is not created #1188

Update devices each 1 second until chain is not created #1188

Conversation

AlexeyBarabash commented Dec 26, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

darkdh left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

AlexeyBarabash commented Dec 28, 2018

Choose a reason for hiding this comment

bridiver commented Dec 28, 2018

AlexeyBarabash commented Dec 31, 2018

AlexeyBarabash commented Jan 1, 2019

AlexeyBarabash commented Jan 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash commented Jan 4, 2019

AlexeyBarabash commented Jan 14, 2019

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash commented Jan 14, 2019

AlexeyBarabash commented Dec 26, 2018 •

edited

Loading

bridiver Jan 2, 2019 •

edited

Loading