737381 - The Firefox Migrator should use the new migration system

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Asaf Romano (gone)]

In bug 718608 (MigrationUtils) I'm fixing the Chrome migraotr to adopt the new MigrationUtils module, and the upcoming Safari migrator will also use it.  

However, I don't want to block the progress on the MigrationUtils & Safari on the adoption of MigrationUtils in the Firefox migrator. That will therefore happen in this bug.

Comment 1

[Asaf Romano (gone)]

Since you've opened bug 738263, I'll fix that part there, but it still depends on this one.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Asaf Romano (gone)]

Comment on attachment 608979 [details] [diff] [review]
patch

l10n peers: I'm concerned about the following change:

 4_ie=Browsing History
 4_safari=Browsing History
 4_chrome=Browsing History
-4_firefox=Browsing History
+4_firefox=Browsing History and Bookmarks
...
+64_firefox=Bookmarks Backups

Localizers will be notified about the first change but not about the second, right?

It's quite tricky to rename the first entry, but I could introduce some hacks to workaround that.  Is that my only option though?

Comment 4

[Axel Hecht [:Pike]]

Yes, sadly, there's no other way than to hack a key change into 4_firefox.

Comment 5

[Asaf Romano (gone)]

Attachment: well, ok

Once we decide what do with this UI, I'll fix this nicely. For now, this hack will do.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 609146 [details] [diff] [review]
well, ok

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

oops, this request got lost due to some fancy bug editing

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 609146 [details] [diff] [review]
well, ok

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

::: browser/components/migration/content/migration.js
@@ +229,5 @@
> +          itemName = bundle.getString(itemID + "_" + this._source);
> +        }
> +        catch(ex) {
> +          // Temporary hack to notify localizers about the change to 4_firefox
> +          // This will be removed once all localizaions are fixed.

typo: localizaions

I don't think we should remove this, since removing this means we notify again a string change. on the other side we can cycle forever between id_source and id_source_2, basically solving the notification issue with l10n.

I'd just say:
// Temporary hack to be able to notify localizers about changes to resources names.

@@ +390,5 @@
>        if (itemID > 0) {
> +        let itemName;
> +        try {
> +          // Temporary hack to notify localizers about the change to 4_firefox
> +          // This will be removed once all localizaions are fixed.

typo: localizaions
btw, see above.

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +25,5 @@
>  
> +FirefoxMigrator.prototype.getResources = function() {
> +  // Only allow migrating from the default (selected) profile since this will
> +  // be the only one returned by the toolkit profile service after bug 214675
> +  // has landed.

nit: may we remove the "has landed" from here? not really adding anything to the comment.

@@ +37,2 @@
>  
> +  let currentProfileDir = MigrationUtils.profileStartup.directory;

add a comment above that this migrator can be used only in the startup path, so we can assume profileStartup is defined. Otherwise it's not obvious.

@@ +37,4 @@
>  
> +  let currentProfileDir = MigrationUtils.profileStartup.directory;
> +  if (sourceProfileDir.equals(currentProfileDir))
> +    return null;

Add a comment to clarify this too.

@@ +44,5 @@
> +    for (let fileName of aFileNames) {
> +      let file = sourceProfileDir.clone();
> +      file.append(fileName);
> +      if (!file.exists())
> +        return null;

Add comment stating file resources are monolithic and we don't make partial copies since they are not expected to work alone.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +257,5 @@
> +    FORMDATA:   Ci.nsIBrowserProfileMigrator.FORMDATA,
> +    PASSWORDS:  Ci.nsIBrowserProfileMigrator.PASSWORDS,
> +    BOOKMARKS:  Ci.nsIBrowserProfileMigrator.BOOKMARKS,
> +    OTHERDATA:  Ci.nsIBrowserProfileMigrator.OTHERDATA
> +  },

I'm surprised we didn't do this before. Do you plan to replace various usages we have in the code, like in ChromeProfileMigrator?

I'd probably call it resourceTypes rather than resourcesTypes both for coherence with naming we did in the past, and cause it's easier to read an pronounce

::: browser/locales/en-US/chrome/browser/migration/migration.properties
@@ +12,5 @@
>  importedSearchUrlDesc=Type "%S <search query>" in the Location Bar to perform a search on %S.
>  
> +# Safari Reading List
> +importedSafariReadingListNoSuffix=Reading List
> +importedSafariReadingListSuffixed=Reading List (from Safari)

looks like leftover from another patch

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 609146 [details] [diff] [review]
well, ok

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

What I have suggested in another bug that has computed keys was to create a mapping that let's you optionally use a different key.

In this case, something like 
{
  '4_firefox': '4_firefox_history_and_bookmarks'
}
would be good.

I also think that trying and only using the new key on error is wrong, as old localizations might still have the wrong string around for a while. I'm usually not all that paranoid about asking localizers to remove obsolete strings.

Thus, I'd rather have you check a redirector, and go for the renamed key right away.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 609146 [details] [diff] [review]
well, ok

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

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +16,4 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> +Components.utils.import("resource://gre/modules/MigrationUtils.jsm");

Forgot to point out, MigrationUtils is not part of gre

Comment 10

[Asaf Romano (gone)]

Attachment: final?

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 613099 [details] [diff] [review]
final?

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

sorry, wrong bundle

::: browser/components/migration/content/migration.js
@@ +224,5 @@
>        if (itemID > 0) {
>          var checkbox = document.createElement("checkbox");
>          checkbox.id = itemID;
> +        checkbox.setAttribute("label",
> +          MigrationUtils.migrationBundle

nit: Is repeating migration actually useful? couldn't we just make it MigrationUtils.bundle and in future special case other bundles with more specific names? I mean, this is likely the more commonly used one.

@@ +322,5 @@
>      if (oldHomePageURL && source) {
> +      var appName = MigrationUtils.migrationBundle.GetStringFromName(source);
> +      var oldHomePageLabel =
> +        MigrationUtils.migrationBundle.formatStringFromName("homePageImport",
> +                                                            [appName]);

This (or better, the second part) is wrong, we have 2 bundles, the "bundle" element is MigrationUtils.migrationBundle, but homePageImport is part of the "brandbundle" element.
Unfortunately someone named the var bundle instead of brandBundle, that is quite easier to confuse with bundle2 (sigh). So please rename bundle to brandBundle and bundle2 to MigrationUtils.bundle.

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +86,4 @@
>  
> +Object.defineProperty(FirefoxProfileMigrator.prototype, "startupOnlyMigrator", {
> +  get: function() true
> +});

nit: I'd probably move this above the component stuff, but not important.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +321,5 @@
>  
>    /**
> +   * Proxy for the migration bundle.  It's purpose is mapping strings' keys to
> +   * their current "bumped" overrides, which are used for notifying localizers
> +   * on changes.

s/on/of/ maybe?

@@ +336,5 @@
> +      }
> +      return gMigrationBundle;
> +    },
> +
> +    __noSuchMethod__: function() this._bundle(arguments),

I'm partially worried by this fake-bundle magic, cause other methods won't obey the mapping (they don't need to for now, just worried of future additions).  Maybe we should just go far from the bundle API, and rather provide a MigrationUtils.getLocalizedString(aName, [optional]aReplacements), that internally does the right calls? That may also simplify some indentation.
I suppose you made this cause one day this could be removed without any code change required.
Oh well, considered the code here is quite self-contained, will probably be fine both ways.

@@ +340,5 @@
> +    __noSuchMethod__: function() this._bundle(arguments),
> +
> +    GetStringFromName: function(aName) {
> +      if (aName in this._OVERRIDES)
> +        aName = this._OVERRIDES[aName];

aName = this._OVERRIDES[aName] || aName;

@@ +346,5 @@
> +    },
> +
> +    formatStringFromName: function(aName, params, length) {
> +      if (aName in this._OVERRIDES)
> +        aName = this._OVERRIDES[aName];

ditto

Comment 12

[Asaf Romano (gone)]

Attachment: patch

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 614870 [details] [diff] [review]
patch

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

r=me ignoring the unrelated safari removal :)

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +32,2 @@
>  
> +  const Cu = Components.utils;

hm? looks like leftover from debug

@@ +50,5 @@
> +      let file = sourceProfileDir.clone();
> +      file.append(fileName);
> +
> +      // File resources are monolithic: We don't make partial copies since they
> +      // are not expected to work alone.

nit: either s/:/./ or s/We/we/

::: browser/components/migration/src/MigrationUtils.jsm
@@ +327,5 @@
>      }
>    },
>  
>    /**
> +   * Gets a string from the migration bundle, shorthand for

period instead of comma.

@@ +342,5 @@
> +   * @return the retrieved string.
> +   *
> +   * @see nsIStringBundle
> +   */
> +  getLocalizedString: function(aKey, aReplacements) {

name the function, plz I suppose MU_getLocalizedString

::: toolkit/content/Services.jsm
@@ +93,5 @@
>    ["startup", "@mozilla.org/toolkit/app-startup;1", "nsIAppStartup"],
>    ["sysinfo", "@mozilla.org/system-info;1", "nsIPropertyBag2"],
>    ["clipboard", "@mozilla.org/widget/clipboard;1", "nsIClipboard"],
> +  ["DOMRequest", "@mozilla.org/dom/dom-request-service;1", "nsIDOMRequestService"],
> +  ["toolkitProfile", "@mozilla.org/toolkit/profile-service;1", "nsIToolkitProfileService"]

not sure about this, probably not worth it, depending on how many uses are there in the codebase. I have no idea which rules we followed so far for these additions.

Comment 14

[Asaf Romano (gone)]

Attachment: as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/4af624156257

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4af624156257