715099 - Convert nsProfileMigrator to JS so we can use JS modules on migration

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P1)

Description

[Asaf Romano (gone)]

We need to convert nsProfileMigraor to JS asap so both new async-import and new async-migrators can land.

Comment 1

[Asaf Romano (gone)]

Attachment: like this

I'll test this on Windows in a bit, correct as needed, and ask for review.

While there is a lot of room for improvement for how this works in general (in particular, the source name should be provided migrators - not the other way around), but I'm not going to do any of that in this bug. Let's keep this bug for converting the current code to js so we can land migrators+import asap.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 586954 [details] [diff] [review]
like this

Works on first try, weird.

Comment 3

[Henri Sivonen (:hsivonen)]

The patch seems to be lacking the relevant removals from nsModule.cpp.

Comment 4

[Asaf Romano (gone)]

Attachment: thanks

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 587262 [details] [diff] [review]
thanks

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

::: browser/components/migration/src/ProfileMigrator.js
@@ +18,5 @@
> +    // By opening the Migration FE with a supplied bpm, it will automatically
> +    // migrate from it.
> +    let [key, migrator] = this._getDefaultBrowserMigratorKey();
> +    if (!key)
> +      return;

The cpp code was throwing here, not bailing out, as we discussed on irc the idl says startup ignores exception, and likely that's the best way to handle. Though please update the idl comment at http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsIProfileMigrator.idl#88 to specify this is a no-op if no source is found.

@@ +38,5 @@
> +                           params);
> +  },
> +
> +  _getDefaultBrowserMigratorKey: function PM_getDefaultBrowserMigratorKey() {
> +    // We don't yet support checking of the default browser on all platforms,

"checking for"

@@ +39,5 @@
> +  },
> +
> +  _getDefaultBrowserMigratorKey: function PM_getDefaultBrowserMigratorKey() {
> +    // We don't yet support checking of the default browser on all platforms,
> +    // and, of course, we don't have a migrators for all browsers.  Thus,

s/a migrators/migrators/

@@ +41,5 @@
> +  _getDefaultBrowserMigratorKey: function PM_getDefaultBrowserMigratorKey() {
> +    // We don't yet support checking of the default browser on all platforms,
> +    // and, of course, we don't have a migrators for all browsers.  Thus,
> +    // for each platform, we have a fallback list (win: ie, chrome, safari;
> +    // mac: safari, chrome; linux: chrome), which is used in these cases.

rather than specifying the list in the comment, causing us to eventually having to update the comment each time, would be nice to have it in a const (that may be ifdef per platform)

@@ +53,5 @@
> +      regKey.open(regKey.ROOT_KEY_LOCAL_MACHINE,
> +                  "SOFTWARE\\Classes\\HTTP\\shell\\open\\command",
> +                  regKey.ACCESS_READ);
> +      value = regKey.readStringValue("").toLowerCase();
> +      let pathMatches = value.match(/^"?(.+\.exe)"?/);

make it non greedy as /^"?(.+?\.exe)"?/, so we stop at the first .exe... I suppose some malware may add .exe later in the command line

@@ +103,5 @@
> +#ifdef XP_MACOSX
> +    migratorKeys.push("safari");
> +#endif
> +    migratorKeys.push("chrome");
> +#endif

may we have the default lists in a per-platform const and just merge the arrays (discarding dupes) for all platforms here?
An alternative may be to start with the default list, and just move the default browser at the beginning if we have one (sort with a custom comparator?)
Should be more readable than having to walk the code to build the default list.

@@ +109,5 @@
> +    for (let i = 0; i < migratorKeys.length; i++) {
> +      let migrator = Cc[this._getMigratorContractID(migratorKeys[i])].
> +                     createInstance(Ci.nsIBrowserProfileMigrator);
> +      if (migrator.sourceExists)
> +        return [migratorKeys[i], migrator];

shouldn't you catch exceptions here?

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 587262 [details] [diff] [review]
thanks

needs new patch

Comment 7

[Henri Sivonen (:hsivonen)]

Are profile migrators supposed to show up on the first run with this patch applied? Or will this need the migrator rewrites in order to behave as expected?

Comment 8

[Asaf Romano (gone)]

> @@ +109,5 @@
> > +    for (let i = 0; i < migratorKeys.length; i++) {
> > +      let migrator = Cc[this._getMigratorContractID(migratorKeys[i])].
> > +                     createInstance(Ci.nsIBrowserProfileMigrator);
> > +      if (migrator.sourceExists)
> > +        return [migratorKeys[i], migrator];

> shouldn't you catch exceptions here?

All migrators should be implemented, and none of them should throw for sourceExists, so I think it's find not to catch exceptions here.

>> Are profile migrators supposed to show up on the first run with this patch applied?
>> Or will this need the migrator rewrites in order to behave as expected?

No changes required.

Comment 9

[Asaf Romano (gone)]

Attachment: address comments

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Mano from comment #8)
> >> Are profile migrators supposed to show up on the first run with this patch applied?
> >> Or will this need the migrator rewrites in order to behave as expected?
> 
> No changes required.

Could you test the patch from bug 482911, please, and help me find out what's wrong with it as far as the profile migrator integration goes?

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 588028 [details] [diff] [review]
address comments

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

::: browser/components/migration/src/BrowserProfileMigrators.manifest
@@ +1,5 @@
> +component {6F8BB968-C14F-4D6F-9733-6C6737B35DCE} ProfileMigrator.js
> +contract @mozilla.org/toolkit/profile-migrator;1 {6F8BB968-C14F-4D6F-9733-6C6737B35DCE}
> +
> +component {4cec1de4-1671-4fc3-a53e-6c539dc77a26} ChromeProfileMigrator.js
> +contract @mozilla.org/profile/migrator;1?app=browser&type=chrome {4cec1de4-1671-4fc3-a53e-6c539dc77a26}

fwiw, this probably bitrotted with the landing of the Firefox migrator. And while I'm here, you may take a look at that too, to check if it or your new shared module need updating.

::: browser/components/migration/src/ProfileMigrator.js
@@ +17,5 @@
> +
> +ProfileMigrator.prototype = {
> +  migrate: function PM_migrate(aStartup) {
> +    // By opening the Migration FE with a supplied bpm, it will automatically
> +    // migrate from it.

"migrator FE with a supplied bpm"... did we pay a cent for each letter in the past? :)

@@ +34,5 @@
> +                           "chrome,dialog,modal,centerscreen,titlebar",
> +                           params);
> +  },
> +
> +  _wrapCSTR: function PM__wrapCSTR(aStr) {

what about _toCString?

@@ +56,5 @@
> +  _PLATFORM_FALLBACK_LIST:
> +#ifdef XP_WIN
> +     ["ie", "chrome", /* safari */],
> +#else
> +#ifdef XP_MACOSX

could you use #elifdef here?

@@ +64,5 @@
> +#endif
> +#endif
> +
> +  _getDefaultMigrator: function PM__getDefaultMigrator() {
> +    let migratorsOrdered = this._PLATFORM_FALLBACK_LIST.slice(0);

Not a specific reason, but for style consistency we usually Array.slice(this._PLATFORM_FALLBACK_LIST) when slicing to copy

@@ +76,5 @@
> +                  regKey.ACCESS_READ);
> +      let value = regKey.readStringValue("").toLowerCase();      
> +      let pathMatches = value.match(/^"?(.+?\.exe)"?/);
> +      if (!pathMatches)
> +        throw "Could not extract path from " + REG_KEY + "(" + value + ")";

throw new Error

@@ +121,5 @@
> +    // of the array.
> +    if (defaultBrowser) {
> +      let currentIndex = migratorsOrdered.indexOf(defaultBrowser);
> +      [migratorsOrdered[0], migratorsOrdered[currentIndex]] =
> +        [defaultBrowser, migratorsOrdered[0]];

doesn't this mess up ordering? if I have ie,chrome,safari, and defaultBrowser is safari, the expected order should be safari,ie,chrome, while this returns safari,chrome,ie
Wouldn't be simpler something like migratorsOrdered.sort(function (a, b) b == defaultBrowser ? 1 : 0); I'm not much worried by sorting a so small array.

@@ +122,5 @@
> +    if (defaultBrowser) {
> +      let currentIndex = migratorsOrdered.indexOf(defaultBrowser);
> +      [migratorsOrdered[0], migratorsOrdered[currentIndex]] =
> +        [defaultBrowser, migratorsOrdered[0]];
> +    }

This may be applied to any other default browser. while it's true we just have have win support for now, I'd rather have this ready for future, by leaving only the try/catch in the ifdef

Comment 12

[Asaf Romano (gone)]

Attachment: for checkin

Comment 13

[Asaf Romano (gone)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2343aa7b0815

Comment 14

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/2343aa7b0815

Comment 15

[u279076]

This has potentially caused a regression in that the First Run Import Wizard no longer appears for Firefox 12+ (bug 724808).