The Firefox Migrator should use the new migration system

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Migration
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

unspecified
Firefox 14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Summary: The Firefox Migration should use the new migration system → The Firefox Migrator should use the new migration system
Assignee: nobody → mano
Status: NEW → ASSIGNED

Updated

5 years ago
Blocks: 738263
Since you've opened bug 738263, I'll fix that part there, but it still depends on this one.
Created attachment 608979 [details] [diff] [review]
patch
Attachment #608979 - Flags: review?(mak77)
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?
Attachment #608979 - Flags: feedback?(gandalf)
Attachment #608979 - Flags: feedback?(axel)

Comment 4

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

Updated

5 years ago
Attachment #608979 - Flags: feedback?(gandalf)
Attachment #608979 - Flags: feedback?(axel)
Attachment #608979 - Flags: feedback-
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.
Attachment #608979 - Attachment is obsolete: true
Attachment #608979 - Flags: review?(mak77)
Attachment #609146 - Flags: review?
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
Attachment #609146 - Flags: review? → review?(mak77)
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
Attachment #609146 - Flags: review?(mak77) → review+

Comment 8

5 years ago
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.
Attachment #609146 - Flags: feedback-
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
Created attachment 613099 [details] [diff] [review]
final?
Attachment #609146 - Attachment is obsolete: true
Attachment #613099 - Flags: review?(mak77)
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
Attachment #613099 - Flags: review?(mak77) → review-
Created attachment 614870 [details] [diff] [review]
patch
Attachment #614870 - Flags: review?(mak77)
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.
Attachment #614870 - Flags: review?(mak77) → review+
Created attachment 615020 [details] [diff] [review]
as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/4af624156257
Attachment #613099 - Attachment is obsolete: true
Attachment #614870 - Attachment is obsolete: true
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/4af624156257
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 748047
Blocks: 749931

Updated

5 years ago
No longer blocks: 748047

Updated

5 years ago
Blocks: 748047
You need to log in before you can comment on or make changes to this bug.