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.

Sign up for GitHub

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

Jump to bottom

USSBookmarks migration #4324

Merged
merged 5 commits into from
Jan 9, 2020
Merged

USSBookmarks migration #4324

merged 5 commits into from
Jan 9, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jan 8, 2020 •
edited
Loading

fix brave/brave-browser#6437

Submitter Checklist:

Test Plan:

Add (1)

  1. Establish sync chain without any bookmarks between device A and device B
  2. Add A1.com and A2.com on device A
  3. A1.com and A2.com should appear on device B shortly

Add with existing bookmarks (2)

  1. Add A1.com and A2.com on device A before sync chain creation
  2. Create sync chain on device A and connect device B
    3 . A1.com and A2.com should appear on device B instantly

Add with same order (3)

  1. Establish sync chain without any bookmarks between device A and device B
  2. Add A1.com and A2.com on device A
  3. Add B1.com and B2.com on device B
  4. Wait for A1, A2, B1, B2 appeared on both devices
  5. Add A3.com to device A and B3.com to device B almost at the same time
  6. both devices should have same bookmarks hierarchy

Move

  1. Follow Add (1)
  2. Add folder A on device A
  3. Move A1.com and A2.com into folder A
  4. Changes should be reflected on device B

Modify

  1. Follow Add (1)
  2. Edit A1.com to be A1111.com
  3. Change should be reflected on device B

Reorder (1)

  1. Follow Add (1)
  2. Move A2.com in front of A1.com
  3. Change should be reflected on device B

Reorder (2)

  1. Follow Add (1)
  2. Move A1.com behind A2.com
  3. Change should be reflected on device B

Delete

  1. Follow Move
  2. Delete folder A on device A
  3. There will soon be no folder or bookmarks on device B

Migration

  1. Do Add (1) from old Brave
  2. Quit from both devices and launch new Brave
  3. Randomly pick Move, Modify, Reorder or Delete to test

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 force-pushed the uss-bookmarks branch 3 times, most recently from f29b502 to a71745d Compare January 8, 2020 07:06
@darkdh darkdh self-assigned this Jan 8, 2020
@darkdh darkdh marked this pull request as ready for review January 8, 2020 19:27
@darkdh darkdh requested a review from bridiver as a code owner January 8, 2020 19:27
@darkdh darkdh requested a review from AlexeyBarabash January 8, 2020 19:27
bridiver
bridiver reviewed Jan 8, 2020
View reviewed changes
patches/components-sync_bookmarks-bookmark_remote_updates_handler.cc.patch Outdated
const size_t old_index = size_t{old_parent->GetIndexOf(node)};
const size_t new_index =
- ComputeChildNodeIndex(new_parent, update_entity.unique_position, tracker);
+ ComputeChildNodeIndex(new_parent, update_entity.unique_position, tracker) BRAVE_APPLY_REMOTE_UPDATE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have to change this line? Can't we check against the value of new_index ? I'm also concerned that we're using a ternary operator with an int. It's better to make a int value check there

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 9b84b78

bridiver
bridiver reviewed Jan 8, 2020
View reviewed changes
patches/components-sync_bookmarks-bookmark_remote_updates_handler.cc.patch Outdated
update_entity.specifics.bookmark(), parent_node,
ComputeChildNodeIndex(parent_node, update_entity.unique_position,
- bookmark_tracker_),
+ bookmark_tracker_) BRAVE_PROCESS_CREATE_2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 9b84b78

bridiver
bridiver reviewed Jan 9, 2020
View reviewed changes
app/brave_main_delegate.cc
@@ -173,7 +173,6 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) {
features::kWebXr.name,
features::kWebXrGamepadModule.name,
unified_consent::kUnifiedConsent.name,
switches::kSyncUSSBookmarks.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

header in this file as well? https://github.com/brave/brave-core/pull/4324/files#diff-931f170b705cce8579670910b176bdf2L16

Copy link
Member Author

Choose a reason for hiding this comment

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

it is required for switches::kSyncServiceURL

@darkdh darkdh requested a review from bridiver January 9, 2020 21:39
@darkdh darkdh merged commit b6b84be into master Jan 9, 2020
@darkdh darkdh deleted the uss-bookmarks branch January 9, 2020 23:49
@AlexeyBarabash AlexeyBarabash mentioned this pull request Jan 10, 2020
Closed
brave-builds pushed a commit that referenced this pull request Jan 22, 2020
@brave-browser-releases
Uplift of #4324 (squashed) to dev
7c2f45a
brave-builds pushed a commit that referenced this pull request Jan 22, 2020
@brave-browser-releases
Uplift of #4324 (squashed) to beta
c173561
This was referenced Jan 22, 2020
Merged
Merged
@btlechowski
Copy link

I found a serious regression in 1.5.x (brave/brave-browser#7893) and therefor we should wait with uplift till the issue is resolved.

@btlechowski btlechowski mentioned this pull request Jan 28, 2020
Closed
mkarolin pushed a commit that referenced this pull request Jan 30, 2020
@brave-browser-releases @mkarolin
Uplift of #4324 (squashed) to beta
0190034
@btlechowski btlechowski mentioned this pull request Jan 31, 2020
Closed
mkarolin pushed a commit that referenced this pull request Jan 31, 2020
@brave-browser-releases @mkarolin
Uplift of #4324 (squashed) to dev
16c516c
@btlechowski
Copy link

Verification passed on

Brave 1.5.75 Chromium: 80.0.3987.66 (Official Build) nightly (64-bit)
Revision 56ea77d65c47ecbb1180b99f860d85b51117864a-refs/branch-heads/3987@{#662}
OS Windows 7 Service Pack 1 (Build 7601.24544)
Brave 1.5.75 Chromium: 80.0.3987.66 (Official Build) nightly (64-bit)
Revision 56ea77d65c47ecbb1180b99f860d85b51117864a-refs/branch-heads/3987@{#662}
OS Ubuntu 18.04 LTS

Verified test plan from #4324 (comment)
Also test by switching the devices.
Tested on clean and upgrade profile.

[Pass] Add (1)
[Pass] Add with existing bookmarks (2)
[Pass] Add with same order (3)
[Pass] Move
[Pass] Modify
[Pass] Reorder (1)
[Pass] Reorder (2)
[Pass] Delete
[Pass] Migration

@btlechowski btlechowski mentioned this pull request Feb 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@AlexeyBarabash AlexeyBarabash Awaiting requested review from AlexeyBarabash

Assignees

@darkdh darkdh

Labels
feature/sync
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate brave sync to USS bookmarks
3 participants
@darkdh @btlechowski @bridiver