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

NTP: Prevent data loss in topSites after topSites update #5097

Merged
merged 3 commits into from
Mar 31, 2020
Merged

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Mar 30, 2020

Reviewers: Please focus review on commit ce9e621 since it's a fix for the reason the first commit 3203693 was reverted.

Fix brave/brave-browser#8781

Top sites (now called grid sites) now have their own storage. This change allows the migration process from a shared to a dedicated storage.

This PR also re-introduces 3203693 which was reverted in #4996

Fix brave/brave-browser#2971

Submitter Checklist:

Test Plan:

Existing profile

  1. Before checking out this PR, keep in mind your pinned top sites; if you have none, pin some tiles and keep in mind their URL and grid position.
  2. Check out this PR. Assert your pinned top sites are there, on the same position as before.

Clean profile

  1. Top sites should work as expected. As you visit new sites, grid should populate with them.
  2. Pinning/reordering/excluding should work as expected.

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.

This reverts commit b131048, reversing
changes made to 5682cae.
Fix brave/brave-browser#8781
Top sites (now called grid sites) now have their
own storage. This change allows the migration
process from a shared to a dedicated storage.
@cezaraugusto
Copy link
Contributor Author

Added some more tests cc @bsclifton

@bsclifton bsclifton added this to the 1.8.x - Nightly milestone Mar 31, 2020
@bsclifton bsclifton requested review from simonhong and removed request for petemill March 31, 2020 06:35
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Manually tested - works great! Code is nice too- thanks for adding tests 😄👍

Added @simonhong to review as he's working on the Super Referral project which also uses top site tiles 😄

@bsclifton
Copy link
Member

Reviewed w/ @simonhong - changes shouldn't cause a problem w/ spec as the color for top site tiles is mobile only I believe. Ready to merge! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lost pinned top sites on NTP upon upgrade refactor new tab top sites
2 participants