Skip to content

Avoid importing stats multiple times from multiple imports #318

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

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

garrettr
Copy link
Contributor

Resolves brave/brave-browser#728.

Uses a simple heuristic which should work in the common case: a new brave-browser user wants to import their browser-laptop stats, and may run the import multiple times. The heuristic will fail (and stats import will fail, silently) if the brave-browser user's stats already exceed those from their browser-laptop profile.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

  1. Import stats from Brave, and open a new tab page to check the current values (should be updated from the import).
  2. Import stats from Brave again, then switch back to the new page (it updates automatically). The values for the stats should not have changed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@garrettr garrettr requested a review from bbondy August 10, 2018 04:09
// stats; intended to prevent incorrectly updating the stats multiple
// times from multiple imports.
if (ads_blocked < uint64_t{stats.adblock_count}) {
prefs->SetUint64(kAdsBlocked, ads_blocked + stats.adblock_count);
Copy link
Member

Choose a reason for hiding this comment

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

I originally meant just take the larger of the 2 values, but I think I prefer this way you came up with better. So going with it.

@bbondy bbondy merged commit a921ba4 into master Aug 10, 2018
@bbondy bbondy deleted the avoid-multiple-stats-imports branch August 23, 2018 13:40
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.

2 participants