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

Prompt user to close Chrome before importing from a Chrome profile #101

Merged
merged 2 commits into from
May 15, 2018

Conversation

garrettr
Copy link
Contributor

@garrettr garrettr commented May 7, 2018

The UX and implementation are based on how Chrome's Firefox importer prompts the user to close Firefox before importing from a Firefox profile. ChromeProfileLock provides the same interface as FirefoxProfileLock, and the implementation is simpler because we are able to reuse Chromium's ProcessSingleton to check if Chrome is running at the time of import.

Resolves brave/brave-browser#103.

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 testing

  1. You will need a Chrome profile with some profile data saved in it.
  2. Start Chrome 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 Chrome profile from the dropdown menu of browser profiles and click Import.
  6. You should see an alert with the title "Close Chrome," the text "To finish importing, close all Chrome windows," and two buttons: Try again and Cancel.
  7. Cancel should return you to the Settings page without importing anything from the Chrome profile.
  8. Try again should return you to the same alert repeatedly, until the background Chrome process is quit.
  9. Quit the background Chrome process, then click Try again.
  10. Your Chrome 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, Windows, and Linux.

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 darkdh May 7, 2018 20:55
@garrettr garrettr force-pushed the chrome-profile-lock branch from 52637cf to 9d7060a Compare May 7, 2018 20:56
@darkdh darkdh force-pushed the chrome-profile-lock branch from 9d7060a to a44a433 Compare May 15, 2018 04:44
darkdh
darkdh previously approved these changes May 15, 2018
void ChromeProfileLock::Unlock() {
if (!HasAcquired())
return;
process_singleton_.Cleanup();
Copy link
Member

Choose a reason for hiding this comment

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

we should set lock_acquired_ to false here and notice once this singleton has been cleanup, it can't be used again. Just in case we will use Unlock else where other than destructor in the future and also have a correct behavior in unittest.
addressed in a44a433

@darkdh darkdh force-pushed the chrome-profile-lock branch from a44a433 to d6890a3 Compare May 15, 2018 04:56
darkdh
darkdh previously approved these changes May 15, 2018
@darkdh darkdh force-pushed the chrome-profile-lock branch from d6890a3 to 9f3644d Compare May 15, 2018 05:04
darkdh
darkdh previously approved these changes May 15, 2018
@darkdh darkdh force-pushed the chrome-profile-lock branch from 9f3644d to 3e68ed7 Compare May 15, 2018 18:18
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 88525af into master May 15, 2018
@garrettr garrettr deleted the chrome-profile-lock branch May 16, 2018 18:33
@garrettr garrettr mentioned this pull request Jul 12, 2018
9 tasks
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
User changing default monthly value is now indicated in state
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Use default config so ninja is in PATH
petemill pushed a commit that referenced this pull request Jul 27, 2020
Use default config so ninja is in PATH
petemill pushed a commit that referenced this pull request Jul 28, 2020
Use default config so ninja is in PATH
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.

Importing from Chrome should work if Chrome is open when user initiates import
2 participants