Add "--upgrade-from-muon" flag to auto-import from Muon #729

garrettr · 2018-10-24T21:55:27Z

I'm looking for feedback on this WIP PR for brave/brave-browser#1545, specifically:

Is the choice of BraveBrowserMainExtraParts::PreMainMessageLoopRun as hook point appropriate, or is there perhaps a better location?
Is there a better way to obtain the target profile than g_browser_process and GetLastUsedProfile?
I ended up copying a significant amount of code from first_run::AutoImport, mostly boilerplate for setting up the importer and handling callbacks. Is this ok, or would it be preferable for me to switch to an approach that patches first_run::AutoImport to minimize duplicated code, as I proposed in comments starting with Allow auto-upgrade from browser-laptop via argument brave-browser#1545 (comment))?

Any other feedback or suggestions very welcome! Feel free to untag/tag in other reviewers if necessary.

garrettr · 2018-10-25T16:59:17Z

@bridiver Might you have feedback on the first two bullet points listed in the top comment?

bridiver · 2018-10-25T17:39:39Z

browser/brave_browser_main_extra_parts.cc

@@ -15,3 +16,7 @@ BraveBrowserMainExtraParts::~BraveBrowserMainExtraParts() {
 void BraveBrowserMainExtraParts::PreCreateThreads() {
  brave::InitializeResourceBundle();
 }
+
+void BraveBrowserMainExtraParts::PreMainMessageLoopRun() {


this is fine because the profile is initialized in PreMainMessageLoopRun and this will run after the chrome extra parts, but I think we want to guard this with local_state pref that is set after import instead of using a command line flag

Do you think guarding with a local_state pref would be preferable to conditioning auto-import on first run, e.g. via first_run::IsChromeFirstRun?

instead of using a command line flag

We need a command line flag to distinguish "Brave-Core is running for the first time after being installed by the Muon upgrader" and "Brave-Core is running for the first time after being downloaded and installed de novo". See brave/brave-browser#1545 (comment) for discussion.

shouldn't we just upgrade by default if we detect a previous browser-laptop install? Then no command line flag is necessary and a pref would work fine

shouldn't we just upgrade by default if we detect a previous browser-laptop install?

We discussed this in a meeting last week and consensus was that auto-import should only happen in the case where the user is running browser-laptop and performs the update that migrates them to brave-core. We need a way to distinguish this from the case where the user downloads and installs brave-core separately, but may also have an existing browser-laptop installation (which could be their current/default browser, or defunct and unused and therefore probably not desirable to auto-import from). We agreed that passing a command line argument to the brave-core binary from the auto-update was the best way to distinguish these cases.

cc @bsclifton @davidtemkin

@bridiver What do you think of the local state pref I added in 51c68d4? With this addition,

auto-import from Muon only occurs if triggered by passing --upgrade-from-muon command line flag

auto-import from Muon status is tracked in a local_state pref, so auto-import can occur at most once

the local_state pref may be useful to other components (e.g. if Brave Rewards wants to convert pins -> donations, it can check the pref to see if there are any imported pins that need conversion)

garrettr · 2018-10-25T17:39:53Z

One issue that I noticed while testing this yesterday: on some platforms (at least macOS and Linux), some of the supported importer data types (cookies and saved passwords) are encrypted by Muon with the "Chromium Safe Storage" key in the system keyring. In order to import these data types, the importer automatically prompts the user to provide their password so it can access the required key from the system keyring.

When import from Muon is triggered by a user action, the keychain password prompt is slightly weird (the choice of the "Chromium Safe Storage" key name by Muon is unfortunate, but there's nothing that can be done about that now), but it sort of makes sense and at least there's something of a cause-effect relationship evident to the user.

I am concerned that the keychain password prompt will be more confusing, or even alarming, for users when the Muon importer is automatically run during the Muon -> Brave-Core upgrade (as requested in brave/brave-browser#872). In this case, all the user sees is a blank browser window, followed by a keychain password prompt with no additional context.

Our options here are:

Continue to attempt to import cookies and saved passwords from Muon, triggering the keychain password prompt on some platforms.
1. If the user provides their password and clicks "Allow"/"Always allow", importing cookies and saved passwords will succeed.
2. If the user declines to provide their password (by clicking "Deny"), the import will succeed for some data types (e.g. history, bookmarks) but will silently fail for others (cookies, saved passwords) that require keychain access for decryption.
  - There is currently no error message shown to users when some or all of the import process fails. In the auto-import use case, the browser launch will continue and some or none of the imported data will be available once the initial load finishes.
Don't attempt to import cookies and saved passwords from Muon, which will avoid triggering the key chain password prompt. Naturally, this provides for a significantly less seamless auto-upgrade experience, so I do not favor this approach.

cc @bsclifton @davidtemkin

browser/auto_import_muon.cc

browser/BUILD.gn

browser/auto_import_muon.cc

browser/auto_import_muon.h

davidtemkin · 2018-10-26T16:06:31Z

@garrettr Well, that's a bummer! Is there a way for us to provide an additional prompt to the user before showing the unfortunately-named "chromium safe storage" system prompt? Otherwise it will be inexplicable. (I'm not sure what it would say; that's an interesting challenge right there).

garrettr · 2018-10-26T16:24:41Z

@davidtemkin We can pop a system dialog at that point, but it's hard for me to imagine a good UX. All I can think of is displaying a modal dialog that says "We've detected that you're auto-upgrading from a previous version of Brave browser. To automatically import all of your settings, you will need to Allow the subsequent keychain access prompt" with a single button: Ok/Continue.

I despise both modal dialogs and dialogs that don't actually offer the user choices, but can't think of any other options.

davidtemkin · 2018-10-26T16:26:21Z

@garrettr @bsclifton wait, I have a different idea... since the actual upgrade process will look like this (as far as I know)

Muon browser gets an update
That version of Muon knows how to install and run Brave Core

--> Can we "export" the salient data within Muon (do this silently) to an area that can be read without user permission by Brave-Core?

bsclifton · 2018-10-26T18:08:10Z

@garrettr I personally think it makes sense to try the import, even if it shows the prompt. We can address this by showing more details in the banner that comes up when an update is available in browser-laptop (before the restart happens). @bradleyrichter can help me with that messaging

garrettr · 2018-10-27T00:07:55Z

Thanks for the reviews and feedback, everyone. I'll try to incorporate everything and get a final PR ready for review by Monday.

garrettr · 2018-10-29T16:16:36Z

Can I get another comprehensive review from any of the currently assigned reviewers? Other than a final squash before merge, I think this PR is ready or close to being ready to merge.

Re: #729 (comment) and subsequent discussion, if we want to do something about it I think it should be tracked in a follow-up because

@bsclifton needs this PR merged ASAP to unblock his work on Muon -> Brave-Core migration
all proposed solutions so far have been somewhat complex or controversial

darkdh

can we do first_run.h and first_run.cc overrides to add AutoImportMuon so we don't have to copy ImportEndedObserver and ImportFromSourceProfile?

darkdh

lgtm

Resolves brave/brave-browser#1545.

garrettr · 2018-10-29T22:50:41Z

Rebasing to prep for merge dismissed @darkdh's review. Can I get another quick review? Nothing has changed since @darkdh's approval other than squashing everything into one commit for merge.

Add "--upgrade-from-muon" flag to auto-import from Muon

bbondy · 2018-10-30T14:51:03Z

master: 61865f4
0.57.x: 39afb14
0.56.x: f9e3279

garrettr added the pr/work-in-progress label Oct 24, 2018

garrettr requested review from petemill and bbondy October 24, 2018 21:55

bbondy requested a review from darkdh October 24, 2018 23:12

bridiver reviewed Oct 25, 2018

View reviewed changes

browser/auto_import_muon.cc Outdated Show resolved Hide resolved

bridiver reviewed Oct 25, 2018

View reviewed changes

browser/BUILD.gn Outdated Show resolved Hide resolved

bridiver reviewed Oct 25, 2018

View reviewed changes

browser/auto_import_muon.cc Outdated Show resolved Hide resolved

bridiver reviewed Oct 25, 2018

View reviewed changes

browser/auto_import_muon.h Outdated Show resolved Hide resolved

garrettr mentioned this pull request Oct 25, 2018

Import Brave Payments data from Muon #736

Merged

garrettr force-pushed the 1545-auto-upgrade-arg branch from 89a0263 to 51c68d4 Compare October 29, 2018 15:51

darkdh reviewed Oct 29, 2018

View reviewed changes

bsclifton assigned garrettr Oct 29, 2018

darkdh previously approved these changes Oct 29, 2018

View reviewed changes

Add "--upgrade-from-muon" flag to auto-import from Muon

b151293

Resolves brave/brave-browser#1545.

garrettr dismissed darkdh’s stale review via b151293 October 29, 2018 22:48

garrettr force-pushed the 1545-auto-upgrade-arg branch from 9ff9a7b to b151293 Compare October 29, 2018 22:48

garrettr added PR/Pending Review and removed pr/work-in-progress labels Oct 29, 2018

darkdh approved these changes Oct 29, 2018

View reviewed changes

garrettr removed the PR/Pending Review label Oct 30, 2018

garrettr merged commit 61865f4 into master Oct 30, 2018

NejcZdovc deleted the 1545-auto-upgrade-arg branch October 30, 2018 07:03

bbondy pushed a commit that referenced this pull request Oct 30, 2018

Merge pull request #729 from brave/1545-auto-upgrade-arg

39afb14

Add "--upgrade-from-muon" flag to auto-import from Muon

bbondy pushed a commit that referenced this pull request Oct 30, 2018

Merge pull request #729 from brave/1545-auto-upgrade-arg

f9e3279

Add "--upgrade-from-muon" flag to auto-import from Muon

bbondy added the merged/0.56.x-beta label Oct 30, 2018

crazy-max mentioned this pull request Oct 30, 2018

Upgrade to the next generation of Brave browser (brave-core) portapps/brave-portable#4

Closed

garrettr mentioned this pull request Nov 7, 2018

cookies not imported - follow up to 1545 brave/brave-browser#2013

Closed

bbondy added this to the 0.56.x - Release milestone Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "--upgrade-from-muon" flag to auto-import from Muon #729

Add "--upgrade-from-muon" flag to auto-import from Muon #729

garrettr commented Oct 24, 2018

garrettr commented Oct 25, 2018

bridiver Oct 25, 2018 •

edited

Loading

garrettr Oct 25, 2018

garrettr Oct 25, 2018

bridiver Oct 26, 2018

garrettr Oct 27, 2018

garrettr Oct 29, 2018

garrettr commented Oct 25, 2018

davidtemkin commented Oct 26, 2018

garrettr commented Oct 26, 2018

davidtemkin commented Oct 26, 2018

bsclifton commented Oct 26, 2018

garrettr commented Oct 27, 2018

garrettr commented Oct 29, 2018

darkdh left a comment

darkdh left a comment

garrettr commented Oct 29, 2018

bbondy commented Oct 30, 2018

Add "--upgrade-from-muon" flag to auto-import from Muon #729

Add "--upgrade-from-muon" flag to auto-import from Muon #729

Conversation

garrettr commented Oct 24, 2018

garrettr commented Oct 25, 2018

bridiver Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

garrettr Oct 25, 2018

Choose a reason for hiding this comment

garrettr Oct 25, 2018

Choose a reason for hiding this comment

bridiver Oct 26, 2018

Choose a reason for hiding this comment

garrettr Oct 27, 2018

Choose a reason for hiding this comment

garrettr Oct 29, 2018

Choose a reason for hiding this comment

garrettr commented Oct 25, 2018

davidtemkin commented Oct 26, 2018

garrettr commented Oct 26, 2018

davidtemkin commented Oct 26, 2018

bsclifton commented Oct 26, 2018

garrettr commented Oct 27, 2018

garrettr commented Oct 29, 2018

darkdh left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

garrettr commented Oct 29, 2018

bbondy commented Oct 30, 2018

bridiver Oct 25, 2018 •

edited

Loading