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

Try to import chrome data w/o warning dialog #5336

Merged
merged 3 commits into from
May 14, 2020
Merged

Try to import chrome data w/o warning dialog #5336

merged 3 commits into from
May 14, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Apr 23, 2020 •
edited
Loading

Even if chrome is still running, importing will be done because
we will import from copied db file.

Resolves brave/brave-browser#2049

Submitter Checklist:

Test Plan:

Refer to issue.

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.

@simonhong simonhong added this to the 1.10.x - Nightly milestone Apr 23, 2020
@simonhong simonhong self-assigned this Apr 23, 2020
@simonhong
Copy link
Member Author

@bsclifton @rebron WDYT about this changed UX?

@simonhong simonhong force-pushed the fix_profile_importing_failure branch from 7a4e2a9 to 13c0db1 Compare April 24, 2020 00:24
@simonhong
Copy link
Member Author

@rebron @bsclifton Or how about using another sentence something like Please make sure that chrome doesn't run now?

@bsclifton
Copy link
Member

Restarted CI after brave/brave-browser#9431 was merged 😄

@simonhong
Copy link
Member Author

Got intermittent build error only on macOS. Re-run on macOS.

18:17:35  In file included from ../../brave/browser/brave_wallet/brave_wallet_utils.cc:10:
18:17:35  ../../brave/common/pref_names.h:10:10: fatal error: 'components/gcm_driver/gcm_buildflags.h' file not found
18:17:35  #include "components/gcm_driver/gcm_buildflags.h"
18:17:35           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:17:35  1 error generated.

@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Apr 24, 2020
@simonhong
Copy link
Member Author

Got only unrelated BraveRewardsBrowserTest.NotVerifiedWallet test failure.

@simonhong simonhong removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Apr 24, 2020
@rebron
Copy link
Collaborator

rebron commented Apr 28, 2020 •
edited
Loading

@simonhong Let's not have a dialog at all and import data while Chrome is running matching Firefox behavior, and what we do for macOS and Linux (no warning dialog).

@simonhong simonhong force-pushed the fix_profile_importing_failure branch from 13c0db1 to 3e20745 Compare April 28, 2020 06:47
@simonhong simonhong requested a review from bridiver as a code owner April 28, 2020 06:47
@simonhong simonhong changed the title Just warning if browser detect chrome is running while importing Try to import chrome data w/o warning dialog Apr 28, 2020
@simonhong simonhong force-pushed the fix_profile_importing_failure branch 2 times, most recently from 554c92e to 189be2a Compare April 28, 2020 09:22
@simonhong
Copy link
Member Author

Kindly ping to reviewers :)

bsclifton
bsclifton approved these changes May 1, 2020
View reviewed changes
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.

Tested this out- it works great 😄 Really nice approach

@simonhong
Copy link
Member Author

Kindly ping to @bridiver :)

bridiver
bridiver reviewed May 5, 2020
View reviewed changes
utility/importer/chrome_importer.cc Outdated
@@ -108,6 +108,27 @@ bool SetEncryptionKeyForPasswordImporting(
}
#endif

class CreateCopyFile {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe ScopedTempFile is what you want here

Copy link
Member Author

@simonhong simonhong May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want copying ScopedTempFile to here and use it in this file?
Unfortunately, ScopedTempFile could be only used by chromecast module's test file.
And, copying to temp file is another common pattern in this PR.
So, I think using CreateCopyFile is appropriate.

Copy link
Collaborator

@bridiver bridiver May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't notice that, but then I think this should be ScopedCopyFile

@simonhong simonhong requested a review from bridiver May 12, 2020 00:50
bridiver
bridiver reviewed May 12, 2020
View reviewed changes
utility/importer/chrome_importer.cc Outdated
explicit CreateCopyFile(const base::FilePath& original_file_path) {
DCHECK(base::PathExists(original_file_path));
if (base::CreateTemporaryFile(&copied_file_path_))
base::CopyFile(original_file_path, copied_file_path_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if this fails? You need proper error handling and can't just assume the copy worked

bridiver
bridiver reviewed May 12, 2020
View reviewed changes
utility/importer/chrome_importer.cc
sql::Database db;
if (!db.Open(history_path))
if (!db.Open(copy_history_file.copied_file_path())) {
Copy link
Collaborator

@bridiver bridiver May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

re: https://github.com/brave/brave-core/pull/5336/files#r423416068 this could crash if either the temp file creation or the copy fails

bridiver
bridiver reviewed May 12, 2020
View reviewed changes
utility/importer/chrome_importer.cc
@@ -196,7 +218,9 @@ void ChromeImporter::ImportBookmarks() {
base::FilePath bookmarks_path =
source_path_.Append(
base::FilePath::StringType(FILE_PATH_LITERAL("Bookmarks")));
base::ReadFileToString(bookmarks_path, &bookmarks_content);
CreateCopyFile copy_bookmark_file(bookmarks_path);
base::ReadFileToString(copy_bookmark_file.copied_file_path(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, but I'm not sure what happens if you pass an empty string to JSONReader

simonhong added 3 commits May 12, 2020 11:35
@simonhong
Just warning if browser detect chrome is running while importing
c26d326 
With this warning dialog, user will close chrome windows.
After that user clicks continue button, browser will not check lock again aind
will try to import chrome's settings.
Even if chrome is still running, importing will be done because
importing is done with temporarily copied db files.
@simonhong
Don't show warning dialog while importing
ded84b5 
Browser will try to import data w/o warning about closing currently
running chrome instance.
@simonhong
Handle temp file copy failure
38b53f3
@simonhong simonhong force-pushed the fix_profile_importing_failure branch from 189be2a to 38b53f3 Compare May 12, 2020 02:58
@simonhong
Copy link
Member Author

@bridiver Handled file copy failure.

@simonhong
Copy link
Member Author

Merged because CI only has pep8 error and it's not related with this PR. It's already fixed by #5522.

@simonhong simonhong merged commit 15e6598 into master May 14, 2020
@simonhong simonhong deleted the fix_profile_importing_failure branch May 14, 2020 00:12
@simonhong simonhong mentioned this pull request May 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@bridiver bridiver bridiver approved these changes

@rebron rebron Awaiting requested review from rebron

@mkarolin mkarolin Awaiting requested review from mkarolin

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.10.x - Release
Development

Successfully merging this pull request may close these issues.

Can't import from Chrome while Chrome is running
4 participants
@simonhong @bsclifton @rebron @bridiver