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

BraveProfileLock #245

Merged
merged 2 commits into from
Jul 12, 2018
Merged

BraveProfileLock #245

merged 2 commits into from
Jul 12, 2018

Conversation

garrettr
Copy link
Contributor

Resolves brave/brave-browser#423.

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:

Adapted from test plan in #101.

Manual testing

  1. You will need a Brave browser-laptop profile with some profile data saved in it.
  2. Start Brave browser-laptop in the background.
  3. yarn start
  4. Open the main menu (e.g. Brave on macOS) and choose Import Bookmarks and Settings...
  5. Choose your test Brave profile from the dropdown menu of browser profiles and click Import.
  6. You should see an alert with the title "Close Brave," the text "To finish importing, close all Brave windows," and two buttons: Try again and Cancel.
  7. Cancel should return you to the Settings page without importing anything from the Brave profile.
  8. Try again should return you to the same alert repeatedly, until the background Brave process is quit.
  9. Quit the background Brave process, then click Try again.
  10. Your Brave profile data should be successfully imported.

Cross-platform testing

I have verified that this PR builds and passes the manual tests described above on macOS. I am waiting on builds for Windows and Linux, and will update this issue with the results of those builds and manual tests once they're available.

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 July 12, 2018 00:53
@garrettr garrettr force-pushed the brave-profile-lock branch from 4e16043 to c8f2c7e Compare July 12, 2018 01:30
darkdh
darkdh reviewed Jul 12, 2018
View reviewed changes
browser/importer/brave_external_process_importer_host.cc
base::FilePath user_data_dir = source_profile_.source_path.DirName();
browser_lock_.reset(new ChromeProfileLock(user_data_dir));
} else { // source_profile_.importer_type == importer::TYPE_BRAVE
browser_lock_.reset(new BraveProfileLock(source_profile_.source_path));
Copy link
Member

Choose a reason for hiding this comment

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

why this can't be user_data_dir just like ChromeProfileLock uses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chrome supports profiles, so for Chrome source_path is a subdirectory containing the profile data. Brave doesn't support profiles, so Brave's source_path is already the user_data_dir.

browser/importer/brave_external_process_importer_host.cc
bool BraveExternalProcessImporterHost::CheckForChromeLock(
const importer::SourceProfile& source_profile) {
if (source_profile.importer_type != importer::TYPE_CHROME)
bool BraveExternalProcessImporterHost::CheckForChromeOrBraveLock() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the name to more general naming like CheckForBrowserLock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was going for, but I don't think it should have the fully generic CheckForBrowserLock name unless it also subsumes CheckForFirefoxLock. I could do do that (e.g. by making FirefoxProfileLock a subclass of BrowserProfileLock, etc.) but that would require patching and since it isn't necessary I left it out.

darkdh
darkdh approved these changes Jul 12, 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.

++

@darkdh darkdh merged commit 7673133 into master Jul 12, 2018
@garrettr
Copy link
Contributor Author

Thanks for the merge, @darkdh! I'm still doing manual testing of this PR on Linux and Windows, so if I find any issues there I'll fix them in follow-ups.

@garrettr
Copy link
Contributor Author

garrettr commented Jul 12, 2018 •
edited
Loading

Confirmed BraveProfileLock builds and passes manual tests on Linux.

@garrettr
Copy link
Contributor Author

Confirmed BraveProfileLock builds and passes manual tests on Windows.

@garrettr garrettr deleted the brave-profile-lock branch July 12, 2018 15:32
@garrettr garrettr mentioned this pull request Dec 13, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@bbondy bbondy Awaiting requested review from bbondy

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

Successfully merging this pull request may close these issues.

Prompt user to close Brave before importing from a Brave profile
2 participants
@garrettr @darkdh