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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Migration
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

unspecified
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

We need to convert nsProfileMigraor to JS asap so both new async-import and new async-migrators can land.
Blocks: 482911
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 on attachment 586954 [details] [diff] [review]
like this

Works on first try, weird.
Attachment #586954 - Flags: review?(mak77)
Blocks: 420938
Blocks: 357898
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
The patch seems to be lacking the relevant removals from nsModule.cpp.
Created attachment 587262 [details] [diff] [review]
thanks
Attachment #586954 - Attachment is obsolete: true
Attachment #586954 - Flags: review?(mak77)
Attachment #587262 - Flags: review?
Attachment #587262 - Flags: review? → review?(mak77)
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 on attachment 587262 [details] [diff] [review]
thanks

needs new patch
Attachment #587262 - Flags: review?(mak77)
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?
> @@ +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.
Created attachment 588028 [details] [diff] [review]
address comments
Attachment #588028 - Flags: review?
Attachment #588028 - Flags: review? → review?(mak77)
Attachment #587262 - Attachment is obsolete: true
(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 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
Attachment #588028 - Flags: review?(mak77) → review+
Created attachment 588631 [details] [diff] [review]
for checkin
Attachment #588028 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/2343aa7b0815
Summary: Convert nsProfileMigrator to JS so we can JS modules on migration → Convert nsProfileMigrator to JS so we can use JS modules on migration
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/2343aa7b0815
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
This has potentially caused a regression in that the First Run Import Wizard no longer appears for Firefox 12+ (bug 724808).
Depends on: 724808

Updated

6 years ago
Whiteboard: [snappy]
Whiteboard: [snappy]
Blocks: 739056
You need to log in before you can comment on or make changes to this bug.