Closed Bug 1271775 Opened 4 years ago Closed 3 years ago

Create an initial implementation of auto-migration of the most-useful browser data on initial startup of a clean (non-reset) profile behind a pref

Categories

(Firefox :: Migration, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [migration-needs-uplift])

Attachments

(1 file)

This should avoid showing the modal dialog and/or the user having to make any choices. Because all the migration dialog really does is call into component-ized migrator implementations per browser, assuming we have a decision for the things that need to be decided, this should just be a matter of calling the same component-ized migrator implementations automatically.

The initial implementation should live behind a pref so we can develop it while also working on things like the "undo" functionality orthogonally. We can turn it on in favour of the current implementation when we think it's "ready".

We'll need decisions from bug 1271774 and bug 1271766 in order to implement this.
Blocks: 1271799
Depends on: 1271778
Blocks: 1271800
Priority: -- → P2
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Blocks: 1277294
I may be slow serving this request, since I'll leave for London on Thursday and I have quite a few requests to handle already.
(In reply to Marco Bonardo [::mak] from comment #2)
> I may be slow serving this request, since I'll leave for London on Thursday
> and I have quite a few requests to handle already.

OK. Matt, are you in a better position?
Flags: needinfo?(MattN+bmo)
Attachment #8760310 - Flags: review?(mak77) → review?(jaws)
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Just realized this needs some more telemetry so that we can evaluate whether this is actually doing something useful.
Flags: needinfo?(MattN+bmo)
Attachment #8760310 - Flags: review?(jaws)
You may want to do some bugs cleanup if you're starting on automatic migration.
bug 737804 is our previous attempt at this. It may also contain some interesting ideas.
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57940/diff/1-2/
Attachment #8760310 - Flags: review?(jaws)
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57940/diff/2-3/
(In reply to Marco Bonardo [::mak] - Away 9-13 June from comment #5)
> You may want to do some bugs cleanup if you're starting on automatic
> migration.
> bug 737804 is our previous attempt at this. It may also contain some
> interesting ideas.

Yes. That bug seems more concerned with continuous migration. This is purely about the initial startup procedure. Right now if you install Firefox on a computer with no previous trace of Firefox on it, we will show you the migration dialog on startup. That's likely not a great initial experience, and this is about changing that.

Most of the concerns in bug 737804 comment 4 I don't think apply here. The only thing I can see that might crop up here is the automatic migration breaking because other browsers are open, in which case (per the current patch) we fall back on the dialog. We can use the telemetry that's in the patch to see how common that case is. I'm not too worried about it right now.
Depends on: 1279240
Attachment #8760310 - Flags: review?(jaws) → review+
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

https://reviewboard.mozilla.org/r/57940/#review55780

::: browser/components/migration/tests/unit/test_automigration.js:97
(Diff revision 3)
> + * and actual migration (which implies startup)
> + */
> +add_task(function* checkIntegration() {
> +  gShimmedMigrator = {
> +    get sourceProfiles() {
> +      dump("Read sourceProfiles");

Can you use info() here instead of dump? dump gets written to the console at some later stage and isn't always in order of output, in my experience.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/63b15ddaadae
allow bypassing the initial migration dialog, r=jaws
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Comment on attachment 8760310 [details]
> Bug 1271775 - allow bypassing the initial migration dialog,
> 
> https://reviewboard.mozilla.org/r/57940/#review55780
> 
> ::: browser/components/migration/tests/unit/test_automigration.js:97
> (Diff revision 3)
> > + * and actual migration (which implies startup)
> > + */
> > +add_task(function* checkIntegration() {
> > +  gShimmedMigrator = {
> > +    get sourceProfiles() {
> > +      dump("Read sourceProfiles");
> 
> Can you use info() here instead of dump? dump gets written to the console at
> some later stage and isn't always in order of output, in my experience.

do_print instead, because xpcshell, and Assert.jsm doesn't have an equivalent for 'info'. Thanks for the quick turnaround!

remote:   https://hg.mozilla.org/integration/fx-team/rev/63b15ddaadae
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/9c0dce75ed0c
shut up eslint warning about return value, rs=bustage
https://hg.mozilla.org/mozilla-central/rev/63b15ddaadae
https://hg.mozilla.org/mozilla-central/rev/9c0dce75ed0c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Benjamin, I just realized I forgot to request a privacy review for the telemetry probe this adds. The code is preffed off, so I don't think it needs backing out (though let me know if you disagree!) - could you post-review the telemetry probe, please? Thank you!
Attachment #8760310 - Flags: review?(benjamin)
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

https://reviewboard.mozilla.org/r/57940/#review56952

::: toolkit/components/telemetry/Histograms.json:4462
(Diff revision 3)
> +    "alert_emails": ["gijs@mozilla.com"],
> +    "expires_in_version": "53",
> +    "kind": "count",
> +    "keyed": true,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Where automatic migration was attempted, indicates to what degree we succeeded."

Since this is a keyed histogram, you need to document the key. Is a keyed histogram really necessary? They are more expensive to process than an enumerated histogram.
Attachment #8760310 - Flags: review?(benjamin) → review-
https://reviewboard.mozilla.org/r/57940/#review56952

> Since this is a keyed histogram, you need to document the key. Is a keyed histogram really necessary? They are more expensive to process than an enumerated histogram.

Re-reading this, I can't tell what this records either. Does it record a "1" value when migration succeeds? Are you going to compare this to another histogram?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> https://reviewboard.mozilla.org/r/57940/#review56952
> 
> > Since this is a keyed histogram, you need to document the key. Is a keyed histogram really necessary? They are more expensive to process than an enumerated histogram.
> 
> Re-reading this, I can't tell what this records either. Does it record a "1"
> value when migration succeeds?

Yes, on a number of different keys to understand how far into the automigration we got. I could use an enum, it'd just be less readable. Doing "magical constant to user-readable string" in your head when reading the output is annoying (which is something I've noticed trying to use some of the other histograms). It would be nice if our dashboards provided a way to make this better.

> Are you going to compare this to another
> histogram?

I effectively want to have an easy way to see how many users make it from start to finish on the automatic migration path. I guess from that perspective, no, although in principle it might be interesting to see how many people make it through this compared to how many people make it through the manual dialog-clicking (but we don't have perfect telemetry for that right now, I believe).

At this stage, if I want to redo this as an enumerated histogram, do I need to come up with a new ID, or not?
Flags: needinfo?(benjamin)
Yes, to change the type you need a new histogram name. We're talking about a simpler way to do enumerations in bug 1188888 which doesn't help you now but could in the future. If you want to keep the keying now, you still need to document in some detail the possible values. I had assumed that the keys would be the browser you were migrating from, but your comment makes me think it is something far different.
Flags: needinfo?(benjamin)
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1281602
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1283542
Whiteboard: [migration-needs-uplift]
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Approval Request Comment
[Feature/regressing bug #]: automigration, used in funnelcake builds 88 (bug 1295873)
[User impact if declined]: no funnelcake. :-(
[Describe test coverage new/current, TreeHerder]: comes with automated xpcshell test, which has run without issue for the last 2.5 months.
[Risks and why]: low risk. Preffed off from the start, so the code in this patch effectively doesn't run at all unless preffed on. Comes with some telemetry that only produces data when the feature is preffed on. Note that because I messed up, the telemetry here got data-r- from bsmedberg after the fact and was redone in bug 1281602, which I'll also request uplift for.
[String/UUID change made/needed]: no.
Attachment #8760310 - Flags: approval-mozilla-beta?
Benjamin, does the fix in bug 1281602 address your concerns from your r- here? 
If so we can go ahead with uplift. The beta 7 build will likely start later than usual tomorrow afternoon PST (because of our infrastructure issues yesterday and today)
Flags: needinfo?(benjamin)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> Benjamin, does the fix in bug 1281602 address your concerns from your r-
> here? 
> If so we can go ahead with uplift. The beta 7 build will likely start later
> than usual tomorrow afternoon PST (because of our infrastructure issues
> yesterday and today)

The fix in bug 1281602 was r+'d by bsmedberg. :-)
Comment on attachment 8760310 [details]
Bug 1271775 - allow bypassing the initial migration dialog,

Automigration feature for test pilot, will be preffed off by default on release, please uplift.
Attachment #8760310 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Ah, I meant funnelcake not test pilot.
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.