Closed
Bug 715099
Opened 13 years ago
Closed 13 years ago
Convert nsProfileMigrator to JS so we can use JS modules on migration
Categories
(Firefox :: Migration, defect, P1)
Firefox
Migration
Tracking
()
RESOLVED
FIXED
Firefox 12
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(1 file, 3 obsolete files)
24.13 KB,
patch
|
Details | Diff | Splinter Review |
We need to convert nsProfileMigraor to JS asap so both new async-import and new async-migrators can land.
Blocks: 482911
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
Comment on attachment 586954 [details] [diff] [review]
like this
Works on first try, weird.
Attachment #586954 -
Flags: review?(mak77)
Assignee | ||
Updated•13 years ago
|
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
The patch seems to be lacking the relevant removals from nsModule.cpp.
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #586954 -
Attachment is obsolete: true
Attachment #586954 -
Flags: review?(mak77)
Attachment #587262 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #587262 -
Flags: review? → review?(mak77)
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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?
Assignee | ||
Comment 8•13 years ago
|
||
> @@ +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.
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #588028 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #588028 -
Flags: review? → review?(mak77)
Assignee | ||
Updated•13 years ago
|
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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #588028 -
Attachment is obsolete: true
Assignee | ||
Comment 13•13 years ago
|
||
Summary: Convert nsProfileMigrator to JS so we can JS modules on migration → Convert nsProfileMigrator to JS so we can use JS modules on migration
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → Firefox 12
Comment 14•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 15•13 years ago
|
||
This has potentially caused a regression in that the First Run Import Wizard no longer appears for Firefox 12+ (bug 724808).
Updated•13 years ago
|
Whiteboard: [snappy]
Updated•13 years ago
|
Whiteboard: [snappy]
You need to log in
before you can comment on or make changes to this bug.
Description
•