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

Allow auto-upgrade from browser-laptop via argument #1545

Closed
garrettr opened this issue Oct 11, 2018 · 15 comments
Closed

Allow auto-upgrade from browser-laptop via argument #1545

garrettr opened this issue Oct 11, 2018 · 15 comments

Comments

@garrettr
Copy link
Contributor

garrettr commented Oct 11, 2018

Test plan (run on Win32/Win64/macOS/Linux)

Test: first run

  1. Have an install of browser-laptop setup and configured (Release channel) with
    • bookmarks
    • history
    • cookies
    • saved passwords
  2. Download/install the brave-core release candidate
  3. Delete the brave-core profile (if one was created; sometimes installer will launch Brave after install)
  4. Launch brave-core via CLI and make sure to pass --upgrade-from-muon
  5. Verify items called out in Step 1 were imported into brave-core

Test: only runs once

After completing steps 1-5 of the first run test:

  1. Quit brave-core if it is open
  2. Launch brave-core via CLI and make sure to pass --upgrade-from-muon
  3. Verify that the auto-import was not repeated. For example, there should not be a second set of identical imported bookmarks.

Test notes

Brave Payments is not covered with this issue. That work is taking place in brave/brave-core#736 (which fixes issue #1215)

Description

brave-browser should recognize an optional argument, e.g. --upgrade-from-muon or --upgrade-from-browser-laptop, such that if the argument is provided, it should:

  1. Check the system for an existing installation of browser-laptop.
  2. If an existing installation is found, automatically import all supported data types from it silently and without user interaction.
  3. Allow startup to continue.

We may also want to:

  1. Automatically shut down or restart the browser once the import is complete. This could be the default behavior, or optionally triggered by an additional argument (e.g. --quit-after-import).
  2. We should consider additionally checking to see if browser-laptop is the user's current default browser, and only auto-import if it is.
    • For example, if I used browser-laptop for a few weeks last year, then switched to a different browser up until the present moment when I've decided to give brave-browser a try, I might be confused/unenthused if a bunch of out-of-date browsing data is automatically included with my first-run experience.
@garrettr garrettr added this to the Muon => Brave Core migration milestone Oct 11, 2018
@garrettr garrettr self-assigned this Oct 11, 2018
@bsclifton
Copy link
Member

@garrettr I know we've talked about it before: does import handle the referral promo? (importing the downloadId, etc)

@garrettr
Copy link
Contributor Author

@bsclifton Not yet, but isn't that being tracked in #1294?

@bsclifton
Copy link
Member

@garrettr yes- that is correct 😄 Totally forgot about that (nothing to see here!) 😅

@garrettr
Copy link
Contributor Author

While exploring options for implementing this feature, I discovered that Chromium already has a first-run auto-import feature. Here's how it works:

  1. The feature is configured on a per-profile basis via prefs, which we can set in either the default preferences or in master_preferences.
  2. The list of specific data types to be imported can be configured via prefs.
  3. The selected data types are automatically imported from the user's current default browser, which is assumed to be the first browser in the list returned by DetectSourceProfiles.

To reuse this functionality to achieve auto-import from browser-laptop upon upgrade, we would need to:

  • Modify the default preferences to enable the first-run auto-import feature (easy)
  • Teach first_run::AutoImport about the new importer data types supported by BraveImporter (easy)
  • Detect whether Brave is default browser and order importer list accordingly (moderate, already tracked in Importable profiles should have additional fields set #104).

With these items implemented, Brave Browser would auto-import all supported data types from the user's current default browser, regardless of whether it is browser-laptop/Muon or something else. We should then consider whether we want to:

  1. Only auto-import if the user's current default browser is browser-laptop/Muon?
    • I am in favor of this and think it aligns well with current desired functionality
  2. Always auto-import from browser-laptop/Muon, even if it's not the user's current default browser?

It'd be nice to leverage this existing functionality, as long as it satisfies our design requirements. @bsclifton What do you think? See any potential conflicts with the functionality you require?

@bsclifton
Copy link
Member

@garrettr wow- this is pretty cool and it would be great to re-use this 😄

I guess it depends on how hard implementing #104 would be. If you think we can land this (along with the other import logic) within the next week, I'd say lets go for it. If that might be at risk, I'd say let's go for a flag-only approach for now (and then follow up with this later, if it makes sense)

With regards to the auto-import from browser-laptop/Muon... could we implement both? For example, we can (by default) implement your option 1 (only import if it's the default browser). We could then override this by providing the flag specified here (--import-from-muon for example) if we want to do option 2

So far, all stakeholders have agreed on always importing on first run. We can definitely revisit that though- cc: @rebron @davidtemkin @bbondy @kjozwiak

@davidtemkin
Copy link

When we auto-import from Muon following an upgrade, that is a version upgrade from a user POV. It's like going from Chrome 69 to Chrome 70, except that there are, ahem, substantial changes. But the process we are modeling is that of upgrading a browser to the latest version, not that of installing a new browser and importing from a pre-existing browser. So the expectation is that a user's data will be carried over.

A manual download, I think, is different. I don't think we'd want to auto-import from any browser (Muon or other) when a user has manually downloaded/installed brave core. In that case, they need to decide what to do, as the process we are modeling in that case is one of installing a new browser. In that situation, it isn't a given that they'd want to import from Muon, or from their default browser, or from any browser.

@garrettr
Copy link
Contributor Author

Good point, @davidtemkin—it's important to distinguish the "auto upgrade from muon to brave-core" and "manually installed brave-core" cases, and a command line set on the first run of the new browser by the upgrader is definitely the simplest way to accomplish that.

We could tie reusing the existing first_run::AutoImport functionality together with a command line arg by adding a pref that can be set on the command line, e.g. via CommandLinePrefStore. The only potential downside is it would limit Brave auto-import to running only when both conditions—brave-core is on first run, and pref is set (by command line arg, or another mechanism)—are true. This seems to exactly fit our use case, but I can imagine it might make manual testing somewhat cumbersome because you would have to clear brave-core's state every time so it would think it was on its first run (but devs should probably do that anyway?)

@bsclifton
Copy link
Member

@garrettr I think that's pretty reasonable - and you're right, that's the only use-case where we want this logic to fire 😄 So this approach would be perfect

To address the manual import process, #104 captures the work we'd need to do (which we could do at a later time)

@kjozwiak
Copy link
Member

his seems to exactly fit our use case, but I can imagine it might make manual testing somewhat cumbersome because you would have to clear brave-core's state every time so it would think it was on its first run (but devs should probably do that anyway?)

@garrettr as long as it's manually testable, no worries about needing to nuke profiles/states. QA nukes states/profiles on a daily basis so no worries there 👍I personally do it at least 10-15 times sometimes.

@garrettr
Copy link
Contributor Author

Worth noting that some Chromium devs appear to have been considering the removal of first_run::AutoImport semi-recently; unfortunately, the most current relevant issue, 555550, is not publicly visible at the time of this writing.

@LaurenWags
Copy link
Member

LaurenWags commented Nov 6, 2018

Using test plan from description and

Brave 0.56.8 Chromium: 70.0.3538.77 (Official Build) (64-bit)
Revision 0f6ce0b0cd63a12cb4eccea3637b1bc9a29148d9-refs/branch-heads/3538@{#1039}
OS Mac OS X

I found that:

  • bookmarks, history, passwords are brought over. I am prompted once during this auto import to allow access to my keychain.
  • cookies are not imported automatically.
  • however, running manual import of cookies, (Brave > Import Bookmarks and Settings > choose Brave, choose only Cookies) cookies are then imported. If I do this, I am prompted to allow access to my keychain.

Verification passed on

Brave 0.56.8 Chromium: 70.0.3538.77 (Official Build) (64-bit)
Revision 0f6ce0b0cd63a12cb4eccea3637b1bc9a29148d9-refs/branch-heads/3538@{#1039}
OS Windows
  • Verified bookmarks/passwords/history imported with arguments
  • Verified relaunch with command line params not duplicate entries which are already imported
  • Verified manually importing cookie worked but not via the command line launch

Verification passed on

Brave 0.56.8 Chromium: 70.0.3538.77 (Official Build) (64-bit)
Revision 0f6ce0b0cd63a12cb4eccea3637b1bc9a29148d9-refs/branch-heads/3538@{#1039}
OS Windows 7 x64
  • imported bookmarks, history and passwords
  • failed to import cookies

Verification Passed on

Brave 0.56.8 Chromium: 70.0.3538.77 (Official Build) (64-bit)
Revision 0f6ce0b0cd63a12cb4eccea3637b1bc9a29148d9-refs/branch-heads/3538@{#1039}
OS Windows 10 x64
  • Verified BM's, history and saved passwords imported correctly in b-c
  • Failed to import cookies
  • Verified relaunch with command line params not duplicate entries which are already imported
  • Verified manually importing cookie worked but not via the command line launch

@LaurenWags
Copy link
Member

Logged follow up issue #2013 for cookies not being imported

@srirambv
Copy link
Contributor

srirambv commented Nov 6, 2018

@garrettr @bsclifton should launching b-c with the command line paramenter when b-l is open should it import ? It does ask the browser to be closed when you try to manually import from Brave

@bsclifton
Copy link
Member

@srirambv you'll get a chance to try that with the browser-laptop release, but as soon as brave-core is launched, browser-laptop quits

@garrettr
Copy link
Contributor Author

garrettr commented Nov 6, 2018

Good question, @srirambv, I will test to verify the behavior. The b-l importer cannot run correctly if b-l is open. When a b-c user initiates b-l import, the status of b-l is detected and the user is prompted to close b-l before continuing the import. It is likely that this check is getting ignored or bypassed when the importer is run automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment