756390 - "Reset Firefox" doesn't show up for rebranding and ff-on-xr

Status: NEW

(Firefox :: Migration, defect)

Description

[Mike Hommey [:glandium]]

"Reset Firefox" relies on MOZ_APP_NAME and MOZ_BUILD_APP to build the string used to find the profile migrator component. This comes with several drawbacks:
- When building Firefox as a xulrunner application, both are set to xulrunner when building toolkit, and about:support then searches the @mozilla.org/profile/migrator;1?app=xulrunner&type=xulrunner contract.
- When building Firefox without the Firefox branding (think Iceweasel, Icecat, and many others), MOZ_APP_NAME is usually not "firefox". This means searching a contract that is not @mozilla.org/profile/migrator;1?app=browser&type=firefox.

I think an easy way out would be to use prefs instead of hardcoding the values at build time, or extend nsIXULAppInfo to contain this information. Gavin, any preference?

Comment 1

[Mike Hommey [:glandium]]

Another possibility, which I think I like better:
- change the "firefox" profile migrator key to be "$MOZ_APP_NAME"
- add a bool canMigrate(in ACString aKey) to nsIProfileMigrator
- instead of checking if @mozilla.org/profile/migrator;1?app=browser&type=firefox is in Components.classes, we could ask the profile migrator if it canMigrate(Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).name.toLowerCase())
- instead of building the migration.properties bundle url from MOZ_BUILD_APP, start from the browser.chromeURL pref.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> - instead of building the migration.properties bundle url from
> MOZ_BUILD_APP, start from the browser.chromeURL pref.

For the record, there is also the toolkit.defaultChromeURI pref, which, interestingly, browser/ doesn't set (but b2g and mobile do). That's the pref xulrunner apps are expected to set.

Comment 3

[Mike Hommey [:glandium]]

Attachment: (PoC) Make the "Reset Firefox" feature more generic

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 629706 [details] [diff] [review]
(PoC) Make the "Reset Firefox" feature more generic

This is a proof of concept.

Comment 5

[Mike Hommey [:glandium]]

Matthew (MattN), when do you think you can take a look to this?

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 629706 [details] [diff] [review]
(PoC) Make the "Reset Firefox" feature more generic

Review of attachment 629706 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should wait for bug 718280 to land since it will simplify this code a bit.

The reason I used MOZ_APP_NAME was because I thought a fork might have different toolkit components to migrate and would want their own self-migrator implementation which wouldn't conflict with Firefox's.  If there is more benefit to assume all builds of browser/ want to migrate the same data then this approach makes sense. f+ in that case for the overall approach changing Firefox/MOZ_APP_NAME references to "self". I'll want to take another look after taking the deCOM into account.

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +92,1 @@
>  FirefoxProfileMigrator.prototype.classID = Components.ID("{91185366-ba97-4438-acba-48deaca63386}");

This is going away in bug 718280.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +585,5 @@
> +   * @returns whether the migrator key is supported.
> +   */
> +  canMigrate:
> +  function MU_canMigrate(aMigratorKey) {
> +    return "@mozilla.org/profile/migrator;1?app=browser&type=" + aMigratorKey in Cc;

We should have MigrationUtils.resetSupported check getMigrator("self") instead (once bug 718280 lands). Then we don't need this helper.

::: toolkit/content/resetProfile.js
@@ +40,5 @@
>    try {
> +    if (currentProfileDir.equals(profileService.selectedProfile.rootDir) &&
> +        "@mozilla.org/toolkit/profile-migrator;1" in Cc) {
> +      let pm = Cc["@mozilla.org/toolkit/profile-migrator;1"].createInstance(Ci.nsIProfileMigrator);
> +      return ("canMigrate" in pm) && pm.canMigrate("self");

This is changing in bug 718280.