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

Import browser laptop stats #315

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Import browser laptop stats #315

merged 2 commits into from
Aug 10, 2018

Conversation

garrettr
Copy link
Contributor

@garrettr garrettr commented Aug 9, 2018

Closes brave/brave-browser#630.

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:

Manual

  1. Have a browser-laptop profile with some non-zero values for the new tab stats. You can verify this quickly with jq '.adblock, .httpsEverywhere, .trackingProtection | .count' ~/Library/Application\ Support/brave/session-store-1.
  2. Delete your Brave-Browser-Development directory to reset stats values to 0; alternatively, check the current values so you can compare before/after import.
  3. yarn start
  4. Open the main menu (e.g. Brave on macOS) and choose Import Bookmarks and Settings...
  5. Choose the Brave profile from the dropdown menu of browser profiles.
  6. Make sure the Stats checkbox is activated.
  7. Click Import.
  8. Open a new tab and confirm that the stats have the expected imported values.

Automated

yarn test brave_unit_tests. BraveImporter.ImportStats should pass.

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 review from bbondy and darkdh August 9, 2018 02:19
@garrettr
Copy link
Contributor Author

garrettr commented Aug 9, 2018

Built and tested successfully on macOS and Linux.

@garrettr garrettr force-pushed the import-browser-laptop-stats branch from dcd6652 to ee36252 Compare August 9, 2018 04:30
@garrettr
Copy link
Contributor Author

garrettr commented Aug 9, 2018

Rebased on latest master to include c3d0cb0 (fixes Windows builds) so I can test on Windows.

@garrettr
Copy link
Contributor Author

garrettr commented Aug 9, 2018

Built and tested successfully on Windows.

darkdh
darkdh requested changes Aug 9, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

Only one typo and overall looks good.
I know we have brave/brave-browser#511 recorded so I didn't ask patch reduction here.

app/brave_generated_resources.grd Outdated
@@ -89,6 +89,9 @@
<message name="IDS_SETTINGS_IMPORT_COOKIES_CHECKBOX" desc="Checkbox for importing cookies">
Cookies
</message>
<message name="IDS_SETTINGS_IMPORT_STATS_CHECKBOX" desc="Checkvbox for importing stats">
Copy link
Member

Choose a reason for hiding this comment

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

s/Checkvbox/Checkbox/

@garrettr
Copy link
Contributor Author

garrettr commented Aug 9, 2018

I know we have brave/brave-browser#511 recorded so I didn't ask patch reduction here.

I appreciate that! I did spend some time looking for ways to minimize the patches, but didn't see any quick wins. The patches this adds are so straightforward and in tandem with patches from previous importer PR's that I agree it makes sense to try to minimize them as part of a unified effort.

@darkdh
Copy link
Member

darkdh commented Aug 9, 2018

lgtm, can you squash c5bdf7ae61eb36a7853708121fabdb224b63c521 into 8d16a19e93ab31f790d6ff683ec4aaba166e0dee? And I will approve, thanks.

@garrettr garrettr force-pushed the import-browser-laptop-stats branch from c5bdf7a to fdc4cb1 Compare August 9, 2018 17:57
darkdh
darkdh approved these changes Aug 9, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

bbondy
bbondy reviewed Aug 10, 2018
View reviewed changes
browser/importer/brave_profile_writer.cc
void BraveProfileWriter::UpdateStats(const BraveStats& stats) {
PrefService* prefs = profile_->GetOriginalProfile()->GetPrefs();

prefs->SetUint64(kAdsBlocked,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not going to block on this @garrettr but if you have time for a follow up I'd prefer to have this take the greater of the 2 stats and use that. That way someone can't as easily game it by importing multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbondy Blocking on that is no problem, it's a quick fix! I'll do it right now

Copy link
Member

Choose a reason for hiding this comment

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

I was too fast for you, so you'll have to PR again, but can be from the same branch obviously.

Copy link
Member

Choose a reason for hiding this comment

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

actually I wasn't too fast for you but I was too slow to read your reply and too fast to merge 😆

@bbondy bbondy merged commit 1581d1f into master Aug 10, 2018
@garrettr garrettr mentioned this pull request Aug 10, 2018
Closed
@bbondy bbondy deleted the import-browser-laptop-stats branch August 23, 2018 13:39
petemill added a commit that referenced this pull request Dec 11, 2018
@petemill
Update 0.59.x to latest brave-ui 0.59.x
8642d0f 
Fix brave/brave-browser#2439 - Clock AM / PM font size
Use brave-ui welcome images instead of brave-core

```
*   4e07f89 - (59 minutes ago) Merge pull request #315 from brave/revert-sync-59 — Pete Miller (HEAD -> brave-core-0.59.x, origin/brave-core-0.59.x)
|\
| * fb7deb8 - (69 minutes ago) Revert "sync v2" — Cezar Augusto
| * c5e16c7 - (69 minutes ago) Revert "Merge pull request #308 from brave/sync_img" — Cezar Augusto
|/
* c4694e2 - (20 hours ago) Merge pull request #247 from brave/c71-clock-period — Pete Miller
* a2443d8 - (4 days ago) do not count first letter if space in textareaclipboard — Cezar Augusto
* 2c16ae2 - (5 days ago) Merge pull request #308 from brave/sync_img — Cezar Augusto
* 7d9811c - (3 weeks ago) sync v2 — Cezar Augusto
* 3e3be2f - (4 days ago) update snapshot from walletSummary — Cezar Augusto
* a4c4dbd - (5 days ago) Merge pull request #309 from brave/welcome_img — Pete Miller
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy left review comments

@darkdh darkdh darkdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garrettr @darkdh @bbondy