Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force to refresh sync cycle(poll/nudge) when transiting from CONFIGURATION to NORMAL mode. #6363

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Aug 6, 2020

This is to mitigate the chance of bookmarks duplication when first time
sync which is caused by first device send it local bookmarks only after
a poll interval(60s). When second device wants to do
BookmarkModelMerger::Merge(), it can't get anything from server because
first device hasn't sent any. So it commit its own bookmarks which will
cause duplication.

Note: Chrome has FCM invalidation ready so its first device will receive
notification within ~10s

Resolves brave/brave-browser#11116

Submitter Checklist:

Test Plan:

STR in brave/brave-browser#11116

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

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@darkdh darkdh self-assigned this Aug 6, 2020
@darkdh darkdh changed the title Force to refresh poll/nudge when transiting from CONFIGURATION to NORMAL mode. Force to refresh sync cycle(poll/nudge) when transiting from CONFIGURATION to NORMAL mode. Aug 6, 2020
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darkdh
Copy link
Member Author

darkdh commented Aug 6, 2020

follow-up issue created brave/brave-browser#11118

…ATION to NORMAL mode.

This is to mitigate the chance of bookmarks duplication when first time
sync which is caused by first device send it local bookmarks only after
a poll interval(60s). When second device wants to do
BookmarkModelMerger::Merge(), it can't get anything from server because
first device hasn't sent any. So it commit its own bookmarks which will
cause duplication.

Note: Chrome has FCM invalidation ready so its first device will receive
notification within ~10s
@darkdh
Copy link
Member Author

darkdh commented Aug 6, 2020

unrelated CI failure

4:32:01  [  FAILED  ] GreaselionServiceTest.ScriptInjectionWithPrecondition, where TypeParam =  and GetParam() =  (1358 ms)
14:32:01  [604/604] GreaselionServiceTest.ScriptInjectionWithPrecondition (1597 ms)
14:32:01  1 test failed:
14:32:01      GreaselionServiceTest.ScriptInjectionWithPrecondition (../../brave/browser/greaselion/greaselion_browsertest.cc:265)

https://ci.brave.com/job/pr-brave-browser-sync-v2-initial-merge/4/execution/node/648/log/

@btlechowski
Copy link

LGTM to uplift

Brave 1.14.25 Chromium: 84.0.4147.111 (Official Build) nightly (64-bit)
Revision 6efb4d9a67167fa201878f23b57e131cc5189e2d-refs/branch-heads/4147@{#967}
OS Linux

I am still able to reproduce if I create the sync chain within few(2) seconds, but it is still a big improvement.

darkdh pushed a commit that referenced this pull request Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Desktop] Sync v2 initial bookmarks duplication
5 participants