Last Comment Bug 715099 - Convert nsProfileMigrator to JS so we can use JS modules on migration
: Convert nsProfileMigrator to JS so we can use JS modules on migration
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 12
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 724808
Blocks: 357898 420938 482911 739056
  Show dependency treegraph
 
Reported: 2012-01-04 04:21 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2012-03-25 06:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
like this (17.37 KB, patch)
2012-01-09 04:30 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
thanks (21.65 KB, patch)
2012-01-10 01:48 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
address comments (18.84 KB, patch)
2012-01-12 07:43 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Splinter Review
for checkin (24.13 KB, patch)
2012-01-14 06:18 PST, 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-01-04 04:21:11 PST
We need to convert nsProfileMigraor to JS asap so both new async-import and new async-migrators can land.
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-09 04:30:47 PST
Created attachment 586954 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-09 04:43:48 PST
Comment on attachment 586954 [details] [diff] [review]
like this

Works on first try, weird.
Comment 3 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-01-10 00:56:14 PST
The patch seems to be lacking the relevant removals from nsModule.cpp.
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-10 01:48:30 PST
Created attachment 587262 [details] [diff] [review]
thanks
Comment 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-10 06:44:29 PST
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] (Away 6-20 Aug) 2012-01-11 03:54:15 PST
Comment on attachment 587262 [details] [diff] [review]
thanks

needs new patch
Comment 7 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-01-11 07:29:33 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-12 07:31:24 PST
> @@ +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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-12 07:43:00 PST
Created attachment 588028 [details] [diff] [review]
address comments
Comment 10 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-01-13 03:30:48 PST
(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] (Away 6-20 Aug) 2012-01-13 14:40:06 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-14 06:18:55 PST
Created attachment 588631 [details] [diff] [review]
for checkin
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-15 23:29:00 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/2343aa7b0815
Comment 14 Justin Wood (:Callek) 2012-01-16 19:40:19 PST
https://hg.mozilla.org/mozilla-central/rev/2343aa7b0815
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-08 08:05:29 PST
This has potentially caused a regression in that the First Run Import Wizard no longer appears for Firefox 12+ (bug 724808).

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