Last Comment Bug 737381 - The Firefox Migrator should use the new migration system
: The Firefox Migrator should use the new migration system
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 14
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
Depends on: 718608
Blocks: 738263 748047 749931
  Show dependency treegraph
 
Reported: 2012-03-20 04:25 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2012-05-19 19:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (21.17 KB, patch)
2012-03-24 03:18 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
l10n: feedback-
Details | Diff | Splinter Review
well, ok (22.90 KB, patch)
2012-03-25 12:42 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
l10n: feedback-
Details | Diff | Splinter Review
final? (30.86 KB, patch)
2012-04-07 06:42 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review-
Details | Diff | Splinter Review
patch (86.20 KB, patch)
2012-04-13 12:06 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Splinter Review
as checked in (32.22 KB, patch)
2012-04-14 02:57 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-20 04:25:35 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-24 03:17:23 PDT
Since you've opened bug 738263, I'll fix that part there, but it still depends on this one.
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-24 03:18:05 PDT
Created attachment 608979 [details] [diff] [review]
patch
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-24 03:31:14 PDT
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] 2012-03-25 09:20:08 PDT
Yes, sadly, there's no other way than to hack a key change into 4_firefox.
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-25 12:42:31 PDT
Created attachment 609146 [details] [diff] [review]
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] 2012-03-27 16:41:49 PDT
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] 2012-03-28 04:00:02 PDT
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] 2012-03-28 04:46:00 PDT
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] 2012-03-28 06:32:05 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-07 06:42:02 PDT
Created attachment 613099 [details] [diff] [review]
final?
Comment 11 Marco Bonardo [::mak] 2012-04-11 13:24:50 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-13 12:06:06 PDT
Created attachment 614870 [details] [diff] [review]
patch
Comment 13 Marco Bonardo [::mak] 2012-04-13 12:28:38 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 02:57:19 PDT
Created attachment 615020 [details] [diff] [review]
as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/4af624156257
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:35:29 PDT
https://hg.mozilla.org/mozilla-central/rev/4af624156257

Note You need to log in before you can comment on or make changes to this bug.