1836773 - Support a new extensions MigrationResource type in the migration wizard

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P2)

Description

[Tim Giles [:tgiles]]

Again from :mconley's initial breakdown of work, we'll need to do the following:

1. Adding the new extension resource type to the list of resource types that the migration wizard supports. This bug is a good reference for that kind of work.
2. Modify the migration wizard to allow progress to be in another state, "information" or "warning" depending on finalized designs to alert the user if extensions were partially migrated or if none could be migrated.
   • MigrationWizardParent and MigrationWizardChild would need to be modified as well in order to support this new state
3. Modify the migration wizard to allow for an informational link to appear for the extensions resource type upon completion.
   • This link might send the user to about:addons or to AMO. Refer to the designs for the destination of this link

Comment 1

[Tim Giles [:tgiles]]

Hey :mconley, I need some help wrapping my head around some of the blocks that I need to implement here.

So one of the things I'm having trouble with is the #getStringsForImportQuantity function in MigrationWizardParent.sys.mjs. I know I need to follow the pattern set by the other resource types and add the Fluent string and all that, but this is where we would need to call MigrationUtils.getImportedCount("extensions") correct? And in MigrationUtils.sys.mjs I also need to add insertExtensionsWrapper(extensions). This seems like where we need to get the number of matched and total extensions from the addons API call right? It seems like I need to add this insertExtensionsWrapper() call into the migrate function of GetExtensionsResource() but I'm not entirely sure.

Another issue is modifying MigrationWizardParent and MigrationWizardChild to handle the new info or warning state. Would this be a new state/page that is declared in MigrationWizardConstants and then used in the setState function in migration-wizard? I know that these two files aren't the ones you mentioned previously, so I'm sure I'm just missing some critical linkage information here 🙂 I can see setting the correct Fluent string in these cases is going to be an issue since we have to determine the Fluent string in getStringForImportQuantity; I don't think the solution is modifying getImportedCount for this case but I'm not having any other ideas at the moment (brain power is definitely spent for today).

Also, should the Addons.installExtensions (or whatever the API will be) call be in the callback of ChromeProfileMigrator.GetExtensionResource or in MigrationUtils.insertExtensionWrapper? It seems like the add-ons would be installed during the migrate call right? It seems like the migrate callbacks are doing privileged operations and getting data ready while the insertXYZWrapper functions actually commit that data to disk. I'm guessing that's the same separation we would want to have for extensions? Like the migrate callback just asks for the matched extensions and gets back an array (or whatever data structure) of matched and unmatched extensions, and then the insertExtensionsWrapper would call Addons.installExtensions (or whatever the actual API looks like)? Sorry if it's a bit scattered, just thinking out loud.

Appreciate any help on the matter!

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Ah, thanks for reminding me about quantities. Yes, this is all a bit awkward.

The way things have historically been done here is as you've observed - a MigratorResource does some pre-processing to get some information to migrate, puts that information into a common format, and then uses one of the insert utilities on MigrationUtils to put that information in the Firefox profile. Those insert methods are also responsible for tracking quantities.

So here's how I suggest we proceed with the quantities:

1. Expose a new method on MigrationUtils, like installExtensionsWrapper or something. It's responsibility will be to call into the AddonManager API that ultimately does the installation. We should ask the addons team to have that API return to us a count of how many extensions (if any) were imported.
2. Have the MigrationResource that you're building in ChromeProfileMigrator extract the addon ID list from the Chrome profile, and then pass those IDs to MigrationUtils.installExtensionsWrapper.
3. Add extensions to this list: https://searchfox.org/mozilla-central/rev/21a53462fa38fbb9e9f6f52105f62c25776bed06/browser/components/migration/MigrationUtils.sys.mjs#769-774
4. Have MigrationUtils.installExtensionsWrapper set _importQuantities["extensions"] with the total number of successfully installed extensions
5. Then have MigrationWizardParent call MigrationUtils.getImportedCount("extensions"); for if "EXTENSIONS" is passed as the resourceTypeStr in #getStringForImportQuantity.

Would this be a new state/page that is declared in MigrationWizardConstants and then used in the setState function in migration-wizard?

I suggest that we aim to re-use the PROGRESS page, and try to make it more flexible so that it can show more variations. Right now it just shows a progress or success state - and if it's succeeded, an optional message under the resource name. We're going to need to make it slightly more flexible for the extensions case.

Does that help?

Comment 3

[Tim Giles [:tgiles]]

I think that clears up my questions for now, thanks for the help!

Comment 4

[Tim Giles [:tgiles]]

Hey :mconley, another question for you. How are we going to handle the various UI states for the full match/partial match/no match cases? Since getImportedCount only returns a single value, I'm still not sure how we're going to get the "partial matched" number. Should the extensions entry in _importQuantities actually be an object with two key-value pairs, one for successfully imported and one for total imported? Seems not ideal since importQuantities is just key:number at the moment and would introduce some inconsistencies. It then seems reasonable that we need to introduce some conditionals in the lazy.MigrationWizardConstants.DISPLAYED_RESOURCE_TYPES.EXTENSIONS case.

Should I just add two entries to _importQuantities, something like:

  _importQuantities = {
    bookmarks: 0,
    logins: 0,
    history: 0,
    cards: 0,
    total_extensions: 0,
    matched_extensions: 0,
  };

Then the #getStringForImportQuantity case for extensions can just call getImportedCount twice, once for total imported extensions and once for matched extensions. Does that seem reasonable or do you have something better in mind?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Then the #getStringForImportQuantity case for extensions can just call getImportedCount twice, once for total imported extensions and once for matched extensions. Does that seem reasonable or do you have something better in mind?

Yep, that seems like the right call to me!

Comment 6

[Tim Giles [:tgiles]]

Attachment: Bug 1836773 - Support the new MigrationResource in the migration wizard. r=mconley!,willdurand!

Enables support for migrating extensions from Chrome once we have the
API to match and install extensions from the add-ons store.

Depends on D180005

Comment 7

[Tim Giles [:tgiles]]

Hi Mike, got another question for you.

Should GetExtensionsResource's migrate function return one of these new MigrationWizardConstants.PROGRESS_VALUE values? I'm just looking at MigrationWizardParent.#doBrowserMigration and seeing that we're using the callback these GetFooResource to determine success or not. Since the extensions import should support the four new progress values, LOADING, SUCCESS, ERROR, and INFO, am I correct in assuming we'll need to update #doBrowserMigration to handle this new information? I'm not seeing a way to use the expanded ProgressState object properties (linkURL and linkText) without making this change. It also seems like we'll need to import the MigrationWizardConstants in MigrationUtils which I'm not entirely sure about, because it appears that I'll need to return a MigrationWizardConstants.PROGRESS_VALUE from the installExtensionsWrapper function (which would be a deviation from the other wrappers).

There's a lot of moving parts and message passing going on here, so I want to make sure I'm understanding the structure and how this data needs to be passed around properly. Appreciate the help as always!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Yeah, you're right - I think we're going to want a slight alteration to how #doBrowserMigration works. Well spotted - I think this is going to require a slight refactoring of our plan. Here's what I suggest:

1. Have the extensions resource migrate call the callback with both the success state as the first argument, and a new second argument - a "details" object. That details object can, for example, contain the information about how many extensions were found, and how many were matched. I think that means we can rely less on _importQuantities there.
2. The callback is defined here, as resourceDone: https://searchfox.org/mozilla-central/rev/fb55a4ee44a9a95d099aa806ca494eb988252ded/browser/components/migration/MigratorBase.sys.mjs#430. We'll need to add a second details argument there for it to handle, and then I suggest passing those details along to the onProgressCallback.
3. Then, in MigrationWizardParent, we'd finally get our optional details object in here: https://searchfox.org/mozilla-central/rev/fb55a4ee44a9a95d099aa806ca494eb988252ded/browser/components/migration/MigrationWizardParent.sys.mjs#398. We can then check to see if the resource type happens to match MigrationUtils.resourceTypes.EXTENSIONS, and if so, examine the details object. Here we can determine which string we want to use for the message instead of using #getStringForImportQuantity, and also determine if we want to show the support link (and if so, supply those properties in the progress update object).

It might be cleaner to have a separate helper function for the extensions case in that progress callback, but we can deal with that when we get there.

How does that sound?

Comment 9

[Tim Giles [:tgiles]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)

How does that sound?

Plan sounds good to me, thanks for the additional clarification. This makes the job more manageable. Another question for you however, since we're extending the migrate function to handle this new details parameter in the ChromeProfileMigrator, should we also update the MigratorBase migrate function and documentation? Or is this implicitly handled by the fact that we're overriding the migrate function in the ChromeProfileMigrator? Trying to keep things consistent where I can.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Yes - we'll need to update MigratorBase function and documentation to plumb the details through. It's a little confusing because we have similar functions called migrate here - but the migrate on MigratorBase is the one that all migrators use, and the migrate on the MigratorResource that you're building is called from MigratorBase here: https://searchfox.org/mozilla-central/rev/9ac4c7d32fa21ddef5cdf281939228ecfedabf64/browser/components/migration/MigratorBase.sys.mjs#470

So, to sum, yes - we'll need to update MigratorBase (both the function and the docstring) to plumb the details through.

Comment 11

[Tim Giles [:tgiles]]

Thanks for all the help, wouldn't be possible to finish this without your help! I've got another question/favor to ask though. I'd like your help on the telemetry, haven't added too much telemetry before and not sure how to handle FX_MIGRATION_EXTENSIONS_QUANTITY from the import wizard telemetry snapshot sheet. I'm also not too sure on how to handle the migration_finished case either since by the time we call this.#sendTelemetryEvent("migration_finished", extraArgs) we don't have access to the extension success state that was present in the parent as far as I understand it. I think the FX_MIGRATION_EXTENSIONS_QUANTITY will be straightforward, just a Services.telemetry.foo call or something...but yeah really not sure how to get the correct extra key in the "migration_finished" case. Any ideas on how to move forward?

Comment 12

[Tim Giles [:tgiles]]

Attachment: Bug 1836773 - Disable importing extensions in the migration wizard by default. r=mconley!

Depends on D180876

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tim Giles [:tgiles] from comment #11)

Thanks for all the help, wouldn't be possible to finish this without your help! I've got another question/favor to ask though. I'd like your help on the telemetry, haven't added too much telemetry before and not sure how to handle FX_MIGRATION_EXTENSIONS_QUANTITY from the import wizard telemetry snapshot sheet. I'm also not too sure on how to handle the migration_finished case either since by the time we call this.#sendTelemetryEvent("migration_finished", extraArgs) we don't have access to the extension success state that was present in the parent as far as I understand it. I think the FX_MIGRATION_EXTENSIONS_QUANTITY will be straightforward, just a Services.telemetry.foo call or something...but yeah really not sure how to get the correct extra key in the "migration_finished" case. Any ideas on how to move forward?

We'll need to do a few things here:

1. Add the FX_MIGRATION_EXTENSIONS_QUANTITY histogram definition somewhere in here: https://searchfox.org/mozilla-central/rev/ddd233d195a5e4b1c75a972a7fd322947fa1d5e3/toolkit/components/telemetry/Histograms.json#8886. Presuming there's an extensions entry in _importQuantities, then it should get recorded. Note that adding this means doing a new data review request, like the one in bug 1834545.
2. Hm, tricky about the event telemetry. What we might need to do is make it so that the Migrate message sent from the child here: https://searchfox.org/mozilla-central/rev/ddd233d195a5e4b1c75a972a7fd322947fa1d5e3/browser/components/migration/MigrationWizardChild.sys.mjs#294-295 resolves with information that the child can then use to set the extra args properly.

I can try to take a look at this telemetry piece, if you can focus on getting the UI working.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/372cfbbc50ea
Support the new MigrationResource in the migration wizard. r=mconley,willdurand,fluent-reviewers,desktop-theme-reviewers,flod
https://hg.mozilla.org/integration/autoland/rev/ca5b9474d66b
Disable importing extensions in the migration wizard by default. r=mconley

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/372cfbbc50ea
https://hg.mozilla.org/mozilla-central/rev/ca5b9474d66b