Last Comment Bug 329744 - Write migrator for moving to Toolkit-based profiles
: Write migrator for moving to Toolkit-based profiles
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Startup & Profiles (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: ---
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
Depends on: 385668 384083 396033
Blocks: 342659 350695 350696 381336 382870 384175 306175 suiterunner 342666 381338 381339 381483 382109 382168 382459 382495 382939 383051 383070 383481 385059 399802 399811
  Show dependency treegraph
 
Reported: 2006-03-08 01:13 PST by Mark Banner (:standard8)
Modified: 2008-05-08 21:33 PDT (History)
31 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP Patch v1 (148.27 KB, patch)
2006-06-06 13:43 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Create a central build location in /suite (14.90 KB, patch)
2006-06-08 10:13 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Create a central build location in /suite v2 (checked in) (15.06 KB, patch)
2006-06-25 03:05 PDT, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Implement the migrator v1 (154.06 KB, patch)
2006-07-13 10:45 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v1a (146.94 KB, patch)
2006-07-13 10:50 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v1b (152.57 KB, patch)
2006-07-14 05:20 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v2 (155.31 KB, patch)
2006-07-17 05:55 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v2a (157.34 KB, patch)
2006-07-26 13:09 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v2b (158.23 KB, patch)
2006-07-31 12:39 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v3 (185.36 KB, patch)
2006-08-03 13:53 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v3a (184.89 KB, patch)
2006-08-05 02:35 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Chrome changes for review (40.89 KB, patch)
2006-08-05 02:45 PDT, Mark Banner (:standard8)
kairo: review+
neil: superreview-
Details | Diff | Splinter Review
Add build structure in for profile migration (checked in) (20.26 KB, patch)
2006-10-31 13:52 PST, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Chrome changes for review v2 (29.50 KB, patch)
2006-11-07 14:14 PST, Mark Banner (:standard8)
neil: review-
neil: superreview-
Details | Diff | Splinter Review
Implement the migrator v4 (183.86 KB, patch)
2006-11-07 14:24 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v4a (167.37 KB, patch)
2006-12-02 10:52 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v5 (170.45 KB, patch)
2007-02-07 14:01 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v5a (174.26 KB, patch)
2007-02-07 14:05 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Chrome changes for review v3 (checked in) (31.46 KB, patch)
2007-02-07 14:09 PST, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Implement the migrator v6 (142.13 KB, patch)
2007-04-04 11:34 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v7 (143.09 KB, patch)
2007-04-05 11:58 PDT, Mark Banner (:standard8)
neil: review-
Details | Diff | Splinter Review
Implement the migrator v8 (137.32 KB, patch)
2007-04-23 12:41 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v8a (144.67 KB, patch)
2007-05-10 14:24 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v9 (151.50 KB, patch)
2007-05-13 14:35 PDT, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review
Implement the migrator v10 (151.43 KB, patch)
2007-05-14 14:06 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Implement the migrator v10a (151.98 KB, patch)
2007-05-18 10:46 PDT, Mark Banner (:standard8)
mnyromyr: review+
Details | Diff | Splinter Review
Change strings to reference SeaMonkey 1.x (checked in) (2.01 KB, patch)
2007-05-21 14:03 PDT, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Fix writing of the target prefs.js (5.99 KB, patch)
2007-05-22 14:29 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Fix writing of the target prefs.js v2 (6.20 KB, patch)
2007-05-24 12:45 PDT, Mark Banner (:standard8)
neil: review-
Details | Diff | Splinter Review
Fix writing of the target prefs.js v3 (checked in) (6.31 KB, patch)
2007-05-25 10:08 PDT, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Make the profile migrator dialog slightly bigger (771 bytes, patch)
2007-06-05 14:25 PDT, Mark Banner (:standard8)
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
Rework the #defines for strings (20.62 KB, patch)
2007-06-21 12:30 PDT, Mark Banner (:standard8)
neil: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2006-03-08 01:13:03 PST
As we move over to the toolkit profile manager, we're also going to need to write a migrator for the profile now being in .mozilla/seamonkey (I think) rather than just .mozilla.

There is some migrator code in firefox that we should be able to re-use for this.
Comment 1 Mark Banner (:standard8) 2006-05-17 05:11:57 PDT
Just to note I have the startings of a patch for this. It'll probably be another week before I have anything ready for review.
Comment 2 Mark Banner (:standard8) 2006-06-06 13:43:33 PDT
Created attachment 224602 [details] [diff] [review]
WIP Patch v1

This is a WIP patch. There is a lot to it, and its not ready for review yet. I'm going with keeping it simple (?) to being with and only implementing a suite based migrator. It'll probably cope with Netscape migration, though I'm not totally sure yet.

At this stage, I'd mainly like to get feedback on the locations of the files/build methods (this uses a suite/build directory as otherwise we'd have to move the files from suite/profile into suite/profile/src if we want to build a component in just suite/profile).

If anyone wants to try building this on Windows or Mac and maybe see what it is missing in terms of pref migration, then feel free, any feedback will be useful.
Comment 3 Robert Kaiser 2006-06-08 09:27:49 PDT
Comment on attachment 224602 [details] [diff] [review]
WIP Patch v1

>Index: suite/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/suite/Makefile.in,v
>retrieving revision 1.10
>diff -u -p -r1.10 Makefile.in
>--- suite/Makefile.in	19 May 2006 18:33:17 -0000	1.10
>+++ suite/Makefile.in	6 Jun 2006 20:31:26 -0000
>@@ -57,6 +57,7 @@ ifdef MOZ_XUL_APP
> DIRS += \
> 		browser \
> 		profile \
>+		build \
> 		app \
> 		$(NULL)

This looks good to me - if we want to link our components statically or stuff like that, we might need such a common build dir anyways...


>Index: suite/locales/jar.mn
>===================================================================
>+% locale migration @AB_CD@ %locale/@AB_CD@/migration/
>+* locale/@AB_CD@/migration/migration.properties                       (%chrome/migration/migration.properties)
>+  locale/@AB_CD@/migration/migration.dtd                              (%chrome/migration/migration.dtd)

Could you please align the braces with the communcator lines below? This would look tidier, I think.

I still don't completely understand why we're inventing a separate "migration" chrome package though...
Comment 4 Mark Banner (:standard8) 2006-06-08 10:13:11 PDT
Created attachment 224875 [details] [diff] [review]
Create a central build location in /suite

This patch moves the current library creation for the suite directory profile provider from being created in /suite/profile to /suite/build. This can then be used for other things (like migration and anything else we want to build under /suite).

I've included a few things that are currently excluded that will be enabled in the part 2 patch as it makes it easier for me currently.
Comment 5 Mark Banner (:standard8) 2006-06-08 10:14:28 PDT
Comment on attachment 224875 [details] [diff] [review]
Create a central build location in /suite

Get the review's address right.
Comment 6 Benjamin Smedberg [:bsmedberg] 2006-06-14 13:19:58 PDT
Comment on attachment 224875 [details] [diff] [review]
Create a central build location in /suite

I'm not a seamonkey reviewer. I also coulda sworn I had removed support for META_COMPONENT... why is that there?
Comment 7 Mark Banner (:standard8) 2006-06-15 11:31:34 PDT
(In reply to comment #6)
> (From update of attachment 224875 [details] [diff] [review] [edit])
> I'm not a seamonkey reviewer. I also coulda sworn I had removed support for
> META_COMPONENT... why is that there?
> 
Its not actually necessary, I've removed it on my local version.
Comment 8 Mark Banner (:standard8) 2006-06-25 03:05:59 PDT
Created attachment 226969 [details] [diff] [review]
Create a central build location in /suite v2 (checked in)

Updated patch to remove the unnecessary META_COMPONENTS.
Comment 9 neil@parkwaycc.co.uk 2006-07-05 06:34:05 PDT
Comment on attachment 226969 [details] [diff] [review]
Create a central build location in /suite v2 (checked in)

Please remove all references to migration before checkin. Those references should only appear in the relevant bug.
Comment 10 Mark Banner (:standard8) 2006-07-05 10:44:32 PDT
Comment on attachment 226969 [details] [diff] [review]
Create a central build location in /suite v2 (checked in)

(In reply to comment #9)
> (From update of attachment 226969 [details] [diff] [review] [edit])
> Please remove all references to migration before checkin. Those references
> should only appear in the relevant bug.
> 

Thanks Neil, checked in with migration parts removed.
Comment 11 Eyal Rozenberg 2006-07-07 10:05:50 PDT
If people are writing an sm profile migrator, please make it flexible enough so as to 

- import a profile from an arbitrary, user-specified directory
- import only some aspects of a profile (e.g. only prefs, only mail accounts etc... maybe present a heirarchy of profile aspects to import, as you have in installation of large software packages - you either 'import all', 'import none' or expand a branch in the tree to make a finer selection) - and allow importing these from separate directories (e.g. mbox files from /some/where, address book from /altogether/elsewhere etc.)
- either merge the imported data into an existing toolkity profile or create a new profile, based on user selection, at a location of user's choice
- work in firefox and thunderbird as well (offering to import only those aspects of a profile relevant to the importing app)
- import mail folders by using their paths in the prefs rather than copying them into an alternate profile directory

Doing so will automagically resolve the following bugs (at least for imports from sm/Mozilla Suite/Netscape 4.x):

bug 013832 - merge POP accounts in multiple profiles into single profile
bug 023090 - Migrate in place with copy/diverge choice
bug 055309 - Need an option to store migrated profiles in a user's choice of location
bug 063389 - Need options for importing Mozilla/Thunderbird/Netscape6-7 mails
bug 072399 - Merge (or Synchronize) address books
bug 121725 - Import mozilla addressbook
bug 154236 - [rfe]Need ability to manually specify 4.x profile locations
bug 250160 - Password Manager needs import of lists of passwords
bug 271592 - Import of multiple profiles (from Mozilla) not possible
bug 333343 - Importing mail from Communicator 4.x in Thunderbird 1.5 is not possible

and maybe some more.
Comment 12 Robert Kaiser 2006-07-07 10:14:18 PDT
As a prerequisite for bug 328887, we _only_ need to be able to import a complete SeaMonkey 1.x profile which is refenced from the "old" default profile registry file. Everything else can be done later in the process, and I guess we'd be happy about any patches Eyal can do to improve this. :)
Comment 13 Eyal Rozenberg 2006-07-07 11:22:43 PDT
(In reply to comment #12)
> and I guess we'd be happy about any patches Eyal can do to improve this. :)

I can't make any promises, but if something nicely extensible is written, with hooks (or at least "//XXX add here"  hooks) and everything, I'll try to hack at it some.
Comment 14 Mark Banner (:standard8) 2006-07-13 10:45:01 PDT
Created attachment 229117 [details] [diff] [review]
Implement the migrator v1

This is still a work in progress patch, much of which I haven't reviewed myself. So no guarentees for not loosing data or anything. If anyone can try it out that would be useful (especially on windows or mac), comments here please but try not to duplcate other people's comments. Thanks.
Comment 15 Mark Banner (:standard8) 2006-07-13 10:50:22 PDT
Created attachment 229120 [details] [diff] [review]
Implement the migrator v1a

Patch without the changes from my tree for other bugs.
Comment 16 Mark Banner (:standard8) 2006-07-14 05:20:56 PDT
Created attachment 229236 [details] [diff] [review]
Implement the migrator v1b

Still wip, but hopefully got all the files needed this time.
Comment 17 Mark Banner (:standard8) 2006-07-17 05:55:17 PDT
Created attachment 229461 [details] [diff] [review]
Implement the migrator v2

Still wip, there is much more that needs checking before its ready. This version fixes:

- Height of migration dialog so that everything that we migrate fits on it.
- Saved Form Data, personal dictionary, mail views data, search data files now all migrated
- wallet (form data) and signon (password) prefs migrated.
Comment 18 Mark Banner (:standard8) 2006-07-26 13:09:28 PDT
Created attachment 230787 [details] [diff] [review]
Implement the migrator v2a

Changes:

- virtualFolders.dat is now migrated
- Bookmarks can now be imported from an existing profile or file via the Bookmark Manager (was just a file before), note that this should easily extend to other types of profiles.
- Various strings made a bit more consistent
- Most of the js & xul self-reviewed, in preparation for requesting reviews. Still a few XXX to sort out.
Comment 19 Mark Banner (:standard8) 2006-07-27 13:16:07 PDT
Comment on attachment 230787 [details] [diff] [review]
Implement the migrator v2a

Just a warning, this patch and some of the ones before it may have modified the source profile's pref.js - most likely the home page preference, but maybe others. Still working on a fix for it.
Comment 20 Mark Banner (:standard8) 2006-07-31 12:39:37 PDT
Created attachment 231461 [details] [diff] [review]
Implement the migrator v2b

Changes:

- Fixed the setting of the homepage so that it should work (bug 343784 had the fix for ff). Note that I haven't verified/tried multiple homepages yet (e.g. tabbed).
Comment 21 Mark Banner (:standard8) 2006-08-03 13:53:32 PDT
Created attachment 232002 [details] [diff] [review]
Implement the migrator v3

This patch:

- Should fix windows build and migration initialise problems
- may fix mac build
- Implements a thunderbird profile migration option (totally untested and probably tries to do more than it should, but the basic action seems to work).

The thunderbird migration option got implemented as I was trying to work out a better class hierarchy for re-use of code.
Comment 22 Mark Banner (:standard8) 2006-08-05 02:35:33 PDT
Created attachment 232302 [details] [diff] [review]
Implement the migrator v3a

Changes:

- Import Source strings can now be specified generically, but overriden if necessary. This means less strings required in migration.properties.
- Correct the fallback from default browser selection on windows so that it works.
- Get the progress bar working properly for mail folder copying and also share some more code via nsNetscapeProfileMigratorBase.
Comment 23 Mark Banner (:standard8) 2006-08-05 02:45:35 PDT
Created attachment 232304 [details] [diff] [review]
Chrome changes for review

This patch is a subset of the v3a patch which includes all the chrome items and the public interface (or alternately everything bar the xpfe, suite/build and suite/profile/migration/src sections).

I think this part of the patch is stable enough to be reviewed & checked in. The cpp code still needs a bit of work to verify all the relevant preferences are copied, but that can be done whilst we're reviewing this. Anything we find later can then be a simple diff to the checked in code.

Robert if you're don't want to review this patch (or think I should get someone else to), could you at least give your OK for the locale related items and file locations?
Comment 24 Mark Banner (:standard8) 2006-08-05 02:50:57 PDT
(In reply to comment #23)
> Created an attachment (id=232304) [edit]
> Chrome changes for review
> 
> This patch is a subset of the v3a patch which includes all the chrome items and
> the public interface (or alternately everything bar the xpfe, suite/build and
> suite/profile/migration/src sections).

I forgot to say - the patch won't change/interfear with anything functionally at the moment, just give us a little extra size in the chrome packages for the time being.
Comment 25 Robert Kaiser 2006-08-09 10:34:14 PDT
Comment on attachment 232304 [details] [diff] [review]
Chrome changes for review

I don't exactly understand most of the JS there, but I trust it works (and the sr will look through the js/xul code). r=me for the locale parts and locations of the files
Comment 26 Mark Banner (:standard8) 2006-08-09 10:55:23 PDT
Comment on attachment 232304 [details] [diff] [review]
Chrome changes for review

Neil, are you happy to do the rest of the r and the sr for the patch? (see Robert's comment above). 

Note also that the v3a is the full patch that this was taken from. I felt it was time to start getting things checked in.
Comment 27 neil@parkwaycc.co.uk 2006-08-24 12:25:25 PDT
Comment on attachment 232304 [details] [diff] [review]
Chrome changes for review

OK, so here are some impressions just from using it:
1. The wizard is huge. Even on the -migration, the list of things that were migrated only took up half the wizard
2. When importing bookmarks, the last option is "From File" except the header is "Import bookmarks from:"
3. When migrating the home page, the focus doesn't match the selection
4. Is there no way to choose what to migrate?
Comment 28 neil@parkwaycc.co.uk 2006-08-24 15:36:54 PDT
Comment on attachment 232302 [details] [diff] [review]
Implement the migrator v3a

>   importBookmarks: function ()
>   {
>+    // XXX: ifdef it to be non-modal (non-"sheet") on mac (see bug 259039)
>+#ifdef XP_MACOSX
>+    var features = "centerscreen,chrome,resizable=no";
>+#else
>+    var features = "modal,centerscreen,chrome,resizable=no";
>+#endif
>+    window.fromFile = false;
>+    window.openDialog("chrome://communicator/content/migration.xul", "migration", features, "bookmarks");
>+    if (window.fromFile)
>+    {
>+      this.importBookmarksFromFile();
>+    }
>+  },
The problem here is that you're using the window modally - you're expecting fromFile to be set as soon as control returns from openDialog, and the only way to achieve that is to make the window modal on all platforms.

Besides, you're using #ifdef :-P
Comment 29 neil@parkwaycc.co.uk 2006-08-24 16:42:03 PDT
Comment on attachment 232304 [details] [diff] [review]
Chrome changes for review

>+<!ENTITY importFromSeamonkey.label       "SeaMonkey 1.0/1.1, Netscape 6/7 or Mozilla 1.x">
>+<!ENTITY importFromSeamonkey.accesskey   "S">
>+<!ENTITY importFromThunderbird.label     "Thunderbird">
>+<!ENTITY importFromThunderbird.accesskey "T">
Presumably there will be further choices to follow? How come this list isn't built dynamically?

>--- /dev/null	2006-08-05 08:02:03.988112750 +0100
>+++ suite/profile/migration/jar.mn	2006-07-30 21:27:10.000000000 +0100
>@@ -0,0 +1,40 @@
>+# ***** BEGIN LICENSE BLOCK *****
We don't normally license jar.mn files, do we? I'm not sure they contain anything worth licensing, they're just list of files.

>+*  content/communicator/migration.xul
>+*  content/communicator/migration.js
Kill the *s and use proper comments please.

>+const kIMig = Components.interfaces.nsISuiteProfileMigrator;
No, that's not how we name interface convenience shortcuts.
>+const kIPStartup = Components.interfaces.nsIProfileStartup;
How come only one of these interfaces is in this patch?

>+      this._autoMigrate = window.arguments[2].QueryInterface(kIPStartup);
>+
>+      if (this._autoMigrate) {
QueryInterface always returns a value, so this can never fail.
[Alternatively you aren't always getting here when you think you are.]

>+  onImportSourcePageShow: function() {
>+    // This function is called before init, so check for importing just
>+    // bookmarks here
Is that true? This is a really sucky place to check for bookmarks.

>+        var contractID = kProfileMigratorContractIDPrefix + suffix;
>+        try {
>+          var migrator = Components.classes[contractID].createInstance(kIMig);
>+        }
Use if (contractID in Components.classes)

>+// XXX I'm not sure I really understand the phoneix bit here,
>+// even when looking at blame.
Looks like it's intended to stop you migrating the current profile?

>+  onSelectProfilePageRewound: function() {
>+    this._selectedProfile = document.getElementById("profiles").selectedItem.id;
>+  },
Why do we have to do anything at all on a Back click?

>+    for (var i = 0; i < 16; ++i) {
>+      var itemID = (items >> i) & 0x1 ? Math.pow(2, i) : 0;
var itemID = items & (1 << i);
Math.pow indeed! Which twit wrote that?

>+          checkbox.setAttribute("label",
>+                                bundle.stringBundle.
>+                                GetStringFromName(itemID + "_" + this._source));
What's wrong with bundle.getString(itemID + "_" + this._source); ?

>+        dataSources.appendChild(checkbox);
>+        if (!this._itemsFlags || this._itemsFlags & itemID)
>+          checkbox.checked = true;
Possible XBL race condition, prefer to use setAttribute instead.

>+        this._itemsFlags |= parseInt(checkbox.id);
Doesn't | automagically convert to integer anyway?

>+  onImportItemCommand: function(aEvent) {
>+    var checkboxes = document.getElementById("dataSources")
>+                             .getElementsByTagName("checkbox");
>+
>+    var oneChecked = false;
>+    for (var i = 0; i < checkboxes.length; ++i) {
>+      if (checkboxes[i].checked) {
>+        oneChecked = true;
>+        break;
>+      }
>+    }
>+
>+    this._wiz.canAdvance = oneChecked;
>+  },
Try document.getElementById("dataSources").getElementsByAttribute("checked", "true").item(0) != null;


>+      var startPages = bundle.getString("homePageOptionCount");
>+    } catch(ex) {}
>+
>+    if (!pageTitle || !pageDesc || !startPages || startPages < 1)
Hmm, does < cast to integer or string? I forget.

>+    if (startPages > 1) {
>+      numberOfChoices += startPages;
Another place where things could go wrong because startPages is a string.

>+    var source = null;
>+    switch (this._source) {
>+      case "seamonkey":
>+        source = "sourceNameSeamonkey";
>+        break;
>+      case "thunderbird":
>+        source = "sourceNameThunderbird";
>+        break;
>+    }
This isn't extensible.

>+  // 5 - Migrating
I was a bit disturbed to find that I had already begun migration without realising it.

>+    while (items.hasChildNodes())
>+      items.removeChild(items.firstChild);
Apparently removing the last child is better.

>+        label.setAttribute("style", "font-weight: bold");
This is not a nice way to display progress. Perhaps you could have an image which starts as a right arrow, then changes to spinning arrows, then ends up as a tick.

>+            var prefFile = dirSvc.get("ProfDS", Components.interfaces.nsIFile);
>+            prefFile.append("prefs.js");
>+            prefSvc.savePrefFile(prefFile);
Odd, where were you expecting the other migrated prefs to be saved?

>+    <vbox id="dataSources" style="overflow: auto; -moz-appearance: listbox"
>+          align="left" flex="1" xhtml2:role="wairole:groupbox"/>
What on earth is going on here? Make up your mind whether this is a <groupbox> or a <listbox> (with checkbox listitems) but no mutants please.
Comment 30 Worcester12345 2006-08-25 00:20:53 PDT
(In reply to comment #27)
...
> 4. Is there no way to choose what to migrate?

Checkboxes would be great here. 
Comment 31 Mark Banner (:standard8) 2006-08-25 00:39:19 PDT
(In reply to comment #27)
> 4. Is there no way to choose what to migrate?

You mean like just bookmarks & homepage from original profile to a new profile? or were you thinking of being able to import specific items into your existing profile - this latter case I was thinking could be done later as I believe it will be possible.

(In reply to comment #29)
> (From update of attachment 232304 [details] [diff] [review] [edit])
> >+<!ENTITY importFromSeamonkey.label       "SeaMonkey 1.0/1.1, Netscape 6/7 or Mozilla 1.x">
> >+<!ENTITY importFromSeamonkey.accesskey   "S">
> >+<!ENTITY importFromThunderbird.label     "Thunderbird">
> >+<!ENTITY importFromThunderbird.accesskey "T">
> Presumably there will be further choices to follow? How come this list isn't
> built dynamically?

Yes there will be further choices as we add migrators for them.

> >+const kIMig = Components.interfaces.nsISuiteProfileMigrator;
> No, that's not how we name interface convenience shortcuts.
Sorry, that's copied from browser.

> >+const kIPStartup = Components.interfaces.nsIProfileStartup;
> How come only one of these interfaces is in this patch?

The nsIProfileStartup is already defined in toolkit:

http://lxr.mozilla.org/seamonkey/source/toolkit/profile/public/nsIProfileMigrator.idl#49

> >+            var prefFile = dirSvc.get("ProfDS", Components.interfaces.nsIFile);
> >+            prefFile.append("prefs.js");
> >+            prefSvc.savePrefFile(prefFile);
> Odd, where were you expecting the other migrated prefs to be saved?

The rest of the migrator code does some pref file switching, to obtain the original profiles prefs. Its pretty horrible and I didn't want to try and fix what browser does to be better.
Comment 32 Mark Banner (:standard8) 2006-10-31 13:52:14 PST
Created attachment 244244 [details] [diff] [review]
Add build structure in for profile migration (checked in)

Basically this is just the makefile structure, the public interface and the base nsProfileMigrator class.

I'm still working on the chrome changes from the review. In the meantime, I'd like to put this patch in to cut down the size of the patch I've got in work.

Functionally it doesn't add anything except maybe one extra restart on the initial startup (where no suiterunner profiles exist), but it will at least reduce the amount of things outstanding to review.
Comment 33 Mark Banner (:standard8) 2006-11-07 13:13:34 PST
(In reply to comment #29)
> >+      var startPages = bundle.getString("homePageOptionCount");
> >+    } catch(ex) {}
> >+
> >+    if (!pageTitle || !pageDesc || !startPages || startPages < 1)
> Hmm, does < cast to integer or string? I forget.

Looks like it casts to an integer in this case. I checked both startPages as 0 and as 1 and it worked correctly both times.
Comment 34 Mark Banner (:standard8) 2006-11-07 14:14:01 PST
Created attachment 244932 [details] [diff] [review]
Chrome changes for review v2

Updated chrome patch to address Neil's comments from the first version. Full patch coming up. Note that I've moved the Makefile.in and interface file creation to the  "Add build structure in for profile migration" patch (attachment 244244 [details] [diff] [review]).

Also note that I'd like to defer "building the list of options to export from dynamically" and "improved progress display (e.g. ticks)" to later patches, in preference to getting something basic in and working.
Comment 35 Mark Banner (:standard8) 2006-11-07 14:24:29 PST
Created attachment 244934 [details] [diff] [review]
Implement the migrator v4

Updated full patch. Improvements:

- Fixed most of Neil's review comments (see comment 34).
- nsISuiteProfileMigrator.ALL now is 0xFFF rather than 0x000, this helps with making the algorithms simpler, and makes more sense.
- All javascript.* preferences now copied for both migration options.
Comment 36 Robert Kaiser 2006-11-08 04:40:49 PST
(In reply to comment #34)
> Also note that I'd like to defer "building the list of options to export from
> dynamically" and "improved progress display (e.g. ticks)" to later patches, in
> preference to getting something basic in and working.

I think it would probably be a good idea to file followup bugs on all such improvements, so we can close this bug when the basic stuff is finished.
Comment 37 neil@parkwaycc.co.uk 2006-12-01 06:20:56 PST
Comment on attachment 244934 [details] [diff] [review]
Implement the migrator v4

> SHARED_LIBRARY_LIBS = \
>         ../profile/$(LIB_PREFIX)suiteprofile_s.$(LIB_SUFFIX) \
>+	../profile/migration/src/$(LIB_PREFIX)suitemigration_s.$(LIB_SUFFIX)
>         $(NULL)
I don't know whether tabs or spaces are right here but mixing them isn't.

> EXTRA_DSO_LDOPTS += \
>+        $(LIBS_DIR) \
>+        $(EXTRA_DSO_LIBS) \
>+	$(MOZ_UNICHARUTIL_LIBS) \
>         $(LIBXUL_DIST)/../modules/libreg/src/$(LIB_PREFIX)mozreg_s.$(LIB_SUFFIX) \
>+        $(MOZ_JS_LIBS) \
>+        $(MOZ_COMPONENT_LIBS) \
>         $(NULL)
Again, a tab slipped in one of these lines...

>+<!ENTITY importFromBookmarks.label       "Import Bookmarks from:">
Should be importBookmarksFrom no?

>+homePageImport=Import your home page from %S
I noticed that the radio button didn't wrap, instead it overflowed :-(

>--- /dev/null	2006-11-07 07:50:16.436328250 +0000
>+++ suite/profile/migration/jar.mn	2006-11-06 20:33:06.000000000 +0000
>@@ -0,0 +1,3 @@
>+comm.jar:
>+  content/communicator/migration.xul
>+  content/communicator/migration.js
I'm not sure whether we want these a) from suite/profile/migration/content/ b) to content/communicator/migration/


>+    var os = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
Might want to wrap the .getService onto its own line.

>+    group.selectedItem = !this._source ?
>+      firstSelectable : document.getElementById(this._source);
Might as well switch the values thus avoiding the !

>+  onImportSourcePageAdvanced: function() {
>+    var newSource = document.getElementById("importSourceGroup").selectedItem.id;
With radio buttons the idea is that you set the value on the button and subsequently read the selected value directly from the radiogroup element.

>+    if (!this._migrator || (newSource != this._source)) {
Nit: Don't need the extra ()s.


>+        var profileName = sourceProfiles.QueryElementAt(0, Components.interfaces.nsISupportsString);
>+        this._selectedProfile = profileName.data;
Nit: could eliminate the temporary.

>+    // Disabling this for now, since we ask about import sources in automigration
>+    // too and don't want to disable the back button
>+    // if (this._autoMigrate)
>+    //   document.documentElement.getButton("back").disabled = true;
Out of interest, why might someone want to undisable it?

>+    while (profiles.hasChildNodes()) 
>+      profiles.removeChild(profiles.firstChild);
Nit: remove the last child, it's slightly faster ;-)

>+      item.id = sourceProfiles.
>+        QueryElementAt(i, Components.interfaces.nsISupportsString).data;
Nit: . at the start of the line, i.e.
item.id = sourceProfiles
    .QueryElementAt(i, ...).data;

>+      if (itemID > 0) {
Nit: if (itemID) suffices.

>+        if (!this._itemsFlags || this._itemsFlags & itemID)
I thought the flags were never zero...

>+      if (checkbox.localName == "checkbox" && checkbox.checked)
Why wouldn't it be a checkbox?

>+    // When automigrating, show all of the data that can be received from this source.
>+    if (this._autoMigrate)
>+      this._itemsFlags = this._migrator.getMigrateData(this._selectedProfile, this._autoMigrate);

>+    case "Migration:ItemBeforeMigrate":
>+      var label = document.getElementById(aData + "_migrated");
>+      if (label)
>+        label.setAttribute("style", "font-weight: bold");
>+      break;
>+    case "Migration:ItemAfterMigrate":
>+      var label = document.getElementById(aData + "_migrated");
>+      if (label)
>+        label.removeAttribute("style");
>+      break;
Should use classes for this... possibly one class for "item waiting to migrate" (dot), one for "item being migrated" (arrow) and one for "item finished" (checkmark) [I'm not expecting you to skin this today!]


>+        // We're done now.
>+        this._wiz.canAdvance = true;
>+        this._wiz.advance();
>+
>+        setTimeout(close, 5000);
>+      }
>+      else {
>+        this._wiz.canAdvance = true;
>+        var nextButton = this._wiz.getButton("next");
>+        nextButton.click();
So close, and yet so far :-P

>+      <radio id="nothing" label="&importFromNothing.label;"
>+                 accesskey="&importFromNothing.accesskey;" hidden="true"/>
Nit: wrong indent for accesskey

>+    <vbox id="dataSources" style="overflow: auto;"
>+          align="left" flex="1" xhtml2:role="wairole:groupbox"/>
Might be better to only use overflow-y here.

>+DEPTH		= ../../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@


>+  nsresult GetDefaultSuiteMigratorKey(nsACString& key,
>+                                      nsCOMPtr<nsISuiteProfileMigrator>& spm);
nsCOMPtr<>&? What's wrong with a **?

>+      key = "seamonkey";
Nit: AssignLiteral

>+#define INTERNAL_NAME_SEAMONKEY       "apprunner"
???

>+  // skip an opening quotation mark if present
>+  if (value.CharAt(1) != ':')
So why not check for one?

>+void GetProfilePath(nsIProfileStartup* aStartup, nsCOMPtr<nsIFile>& aProfileDir);
nsCOMPtr<>& again...

>+++ suite/profile/migration/src/nsSuiteProfileMigratorUtils.cpp	2006-11-06 20:33:06.000000000 +0000
I stopped reviewing here.
Comment 38 neil@parkwaycc.co.uk 2006-12-01 06:24:40 PST
Comment on attachment 244932 [details] [diff] [review]
Chrome changes for review v2

As per comment 37.
Comment 39 Robert Kaiser 2006-12-01 06:35:40 PST
(In reply to comment #37)
> I don't know whether tabs or spaces are right here but mixing them isn't.

Should be tabs - Mark, please check them in as such.
Makefiles require tab-indenting in many cases, so it's common in our code to use tabs also in those continued lines.
Comment 40 Mark Banner (:standard8) 2006-12-02 10:40:44 PST
(In reply to comment #39)
> (In reply to comment #37)
> > I don't know whether tabs or spaces are right here but mixing them isn't.
> 
> Should be tabs - Mark, please check them in as such.
> Makefiles require tab-indenting in many cases, so it's common in our code to
> use tabs also in those continued lines.
> 
Turns out I'd already fixed this in the "Add build structure" patch, but hadn't then brought the changes back to the v4 patch.
Comment 41 Mark Banner (:standard8) 2006-12-02 10:52:58 PST
Created attachment 247277 [details] [diff] [review]
Implement the migrator v4a

Same as v4 but patch regenerated now the build structure patch has been checked in - in case anyone wishes to test/compile with the full migrator WIP version.
Comment 42 Mark Banner (:standard8) 2007-02-03 12:29:58 PST
(In reply to comment #37)
> (From update of attachment 244934 [details] [diff] [review])
> >--- /dev/null	2006-11-07 07:50:16.436328250 +0000
> >+++ suite/profile/migration/jar.mn	2006-11-06 20:33:06.000000000 +0000
> >@@ -0,0 +1,3 @@
> >+comm.jar:
> >+  content/communicator/migration.xul
> >+  content/communicator/migration.js
> I'm not sure whether we want these a) from suite/profile/migration/content/ b)
> to content/communicator/migration/

I've changed to content/communicator/migration, I don't mind about moving them to content if that's what we want, I just thought it'd mean one directory less (for only two files).

> >+  nsresult GetDefaultSuiteMigratorKey(nsACString& key,
> >+                                      nsCOMPtr<nsISuiteProfileMigrator>& spm);
> nsCOMPtr<>&? What's wrong with a **?

There are saving on addrefs etc and making the code slightly simpler. I'll change it if you want me to though.
Comment 43 Mark Banner (:standard8) 2007-02-07 14:01:34 PST
Created attachment 254341 [details] [diff] [review]
Implement the migrator v5

This patch addresses most of Neil's nits from the last one. I've also passed it through jst's patch reviewer and fixed most of the issues there.

There's still a lot of work to do on the c++ side yet, so I'll post a patch with the ui stuff for separate review in a mo.
Comment 44 Mark Banner (:standard8) 2007-02-07 14:05:42 PST
Created attachment 254342 [details] [diff] [review]
Implement the migrator v5a

I missed the dtd & properties files out of the v5 patch :-(
Comment 45 Mark Banner (:standard8) 2007-02-07 14:09:35 PST
Created attachment 254343 [details] [diff] [review]
Chrome changes for review v3 (checked in)

Revised chrome for review, same as v5 patch.

I think I've classed the progress list correctly as requested. I'll raise a bug for it to be schemed when this patch is approved for check in.
Comment 46 neil@parkwaycc.co.uk 2007-02-10 16:22:51 PST
Comment on attachment 254343 [details] [diff] [review]
Chrome changes for review v3 (checked in)

>+          migrator = Components.classes[contractID]
>+            .createInstance(Components.interfaces.nsISuiteProfileMigrator);
Nit: Use your const for nsISuiteProfileMigrator so you can line up the .

>+        this._selectedProfile = sourceProfiles
>+          .QueryElementAt(0, Components.interfaces.nsISupportsString).data;
Nit: I think this one deserves a 4-space continuation indent. (I'm not sure about your other continuation indents, I just picked this one at random.)

>+        if (this._itemsFlags == nsISuiteProfileMigrator.ALL ||
>+            this._itemsFlags & itemID)
Since ALL is now 0xFFF this means that the first test always fails when the second test fails so there's no point doing it. Or to put it another way,
((p ∨ q) ∧ (p ⇒ q)) ⇒ b

>+                                .createInstance(Components.interfaces
>+                                                          .nsISupportsString);
These really needs their own const.

>+      <radio id="seamonkey" label="&importFromSeamonkey.label;"
>+             accesskey="&importFromSeamonkey.accesskey;" value="seamonkey"/>
>+      <radio id="thunderbird" label="&importFromThunderbird.label;"
>+             accesskey="&importFromThunderbird.accesskey;"
>+             value="thunderbird"/>
>+      <!-- fromfile is used for bookmark importing -->
>+      <radio id="fromFile" label="&importFromFile.label;" value="fromFile"
>+             accesskey="&importFromFile.accesskey;" hidden="true"/>
>+      <radio id="nothing" label="&importFromNothing.label;" value="nothing"
>+             accesskey="&importFromNothing.accesskey;" hidden="true"/>
Do you use these ids anywhere (so far I only spotted fromFile)?
Comment 47 neil@parkwaycc.co.uk 2007-02-11 14:06:41 PST
Comment on attachment 254343 [details] [diff] [review]
Chrome changes for review v3 (checked in)

>+homePageImport=Import your home page from %S
OK, so it turns out that the overflowing radio button is a toolkit bug.

>+        // We're done now.
>+        this._wiz.canAdvance = true;
>+        this._wiz.advance();
>+
>+        setTimeout(close, 5000);
I didn't think you were allowed to pass native methods to setTimeout.

>+        this._wiz.canAdvance = true;
>+        var nextButton = this._wiz.getButton("next");
>+        nextButton.click();
You forgot to fix this (see above!)

r=me with these and comment 46 fixed.
Comment 48 Mark Banner (:standard8) 2007-02-12 12:01:52 PST
(In reply to comment #46)
> (From update of attachment 254343 [details] [diff] [review])
> >+      <radio id="seamonkey" label="&importFromSeamonkey.label;"
> >+             accesskey="&importFromSeamonkey.accesskey;" value="seamonkey"/>
> >+      <radio id="thunderbird" label="&importFromThunderbird.label;"
> >+             accesskey="&importFromThunderbird.accesskey;"
> >+             value="thunderbird"/>
> >+      <!-- fromfile is used for bookmark importing -->
> >+      <radio id="fromFile" label="&importFromFile.label;" value="fromFile"
> >+             accesskey="&importFromFile.accesskey;" hidden="true"/>
> >+      <radio id="nothing" label="&importFromNothing.label;" value="nothing"
> >+             accesskey="&importFromNothing.accesskey;" hidden="true"/>
> Do you use these ids anywhere (so far I only spotted fromFile)?
> 

"nothing" is in the init function of the MigrationWizard. The thunderbird & seamonkey ids are indirectly referenced in the second half of the onImportSourcePageShow function.

I used setTimeout(function() {window.close();}, 5000); for the setTimeout replacement, if that isn't good enough I can change it later as the code won't actually be enabled yet.
Comment 49 Mark Banner (:standard8) 2007-02-12 12:23:26 PST
Comment on attachment 254343 [details] [diff] [review]
Chrome changes for review v3 (checked in)

Chrome changes checked in with nits addressed. I'll post an updated full patch in the next day or so as there is not too much that has changed yet.
Comment 50 Mark Banner (:standard8) 2007-04-04 11:34:47 PDT
Created attachment 260619 [details] [diff] [review]
Implement the migrator v6

(In reply to comment #49)
> I'll post an updated full patch in the next day or so as there is not too
> much that has changed yet.

I said that quite a while ago, this is the updated patch, I don't think its had too many changes since the last one, but its good enough to compile and work the basics still.
Comment 51 Mark Banner (:standard8) 2007-04-05 11:58:16 PDT
Created attachment 260747 [details] [diff] [review]
Implement the migrator v7

v7 fixes sig file copying to the same way as Thunderbird.

Note that I've still got to verify all the "simple" prefs that we need are copied across (basically the ones in the pref transform array). However because this patch is so big and reasonably complex, I'd like to start to get reviews on the functionality of what is there.
Comment 52 neil@parkwaycc.co.uk 2007-04-05 14:22:51 PDT
Comment on attachment 260747 [details] [diff] [review]
Implement the migrator v7

>+      nsCAutoString val;
>+      val = ToNewCString(NS_ConvertUTF16toUTF8(data));
>+
>+      aResult.Assign(val);
CopyUTF16toUTF8(data, aResult);
(Note: doesn't the code above leak the new string?)

>+  nsresult rv = aBranch->method(xform->sourcePrefName, value); \
>+  if (NS_SUCCEEDED(rv)) \
>+    xform->prefHasValue = PR_TRUE; \
>+  return rv;
Is it really correct to return rv in this case? Surely you would write
xform->prefHasValue = NS_SUCCEEDED(aBranch->method(xform->sourcePrefName, value));
return NS_OK;

>+    nsAutoString data;
>+    data.AssignWithConversion(aTransform->stringValue);
>+    pls->SetData(data.get());
pls->SetData(NS_ConvertASCIItoUTF16(aTransform->stringValue)).get());


>+    return aBranch->SetIntPref("network.image.imageBehavior",
>+                               aTransform->intValue == 1 ? 0 :
>+                               aTransform->intValue);
We don't need this conversion, we support a value of 1.
(What you could do is convert it to the new pref permissions.default.image
nii 1 -> pdi 3, nii 2 -> pdi 2 ndi 0/other -> pdi 1)

>+    return aBranch->SetIntPref("network.cookie.cookieBehavior",
>+                               aTransform->intValue == 3 ? 0 :
>+                               aTransform->intValue);
We currently support a value of 3, although biesi wants to kill off p3p...

>+    // Seamonkey's download manager uses a single pref to control behavior:
Presumably this depends on which download manager we end up with?

>+    case nsIPrefBranch::PREF_INVALID:
>+      {
>+        nsCOMPtr<nsIPrefLocalizedString> str;
>+        rv = branch->GetComplexValue(currPref,
>+                                     NS_GET_IID(nsIPrefLocalizedString),
>+                                     getter_AddRefs(str));
>+
>+        if (NS_SUCCEEDED(rv) && str)
>+          str->ToString(&pref->wstringValue);
Um... PREF_INVALID will never have a value... in fact I'm not even sure PREV_INVALID is possible any more (ajscult wrote a patch to delete them).

>+      nsCRT::free(pref->stringValue);
NS_Free

>+  // XXX TO_DO
>+  return NS_OK;
>+  //  nsCOMPtr<nsIPasswordManagerInternal> pmi(
>+  //    do_GetService("@mozilla.org/passwordmanager;1"));
>+  //  return pmi->ReadPasswords(seamonkeyPasswordsFile);
Yet the next function is CopyPasswords?

>+nsresult
>+nsNetscapeProfileMigratorBase::GetSchemaValueFileName(PRBool aReplace,
>+                                                      char** aFileName)
...
>+  return LocateSignonsFile(aFileName);
This looks wrong...

>+protected:
>+  // This function is designed to be overriden by derived classes so that
>+  // the required profile data for the specific application can be obtained.
>+  virtual nsresult FillProfileDataFromRegistry() { return NS_ERROR_FAILURE; }
Can this be made abstract ( = 0; instead of { return NS_ERROR_FAILURE; } )?
Comment 53 neil@parkwaycc.co.uk 2007-04-06 10:07:19 PDT
Comment on attachment 260747 [details] [diff] [review]
Implement the migrator v7

>+  nsresult rv;
...
>+  rv = GetDefaultSuiteMigratorKey(key, getter_AddRefs(spm));
nsresult rv = ...

>+  if (NS_FAILED(rv)) return rv;
if (NS_FAILED(rv))
  return rv;

>+      if (!spm) return NS_ERROR_FAILURE;
Same here, etc.

>+#ifdef XP_WIN
>+    // The "Default Browser" key in the registry was set to a browser for which
>+    // no profile data exists. On Windows, this means the Default Browser
>+    // settings in the registry are bad, and we should just fall back to IE
>+    // in this case.
>+    spm = do_CreateInstance(NS_SUITEPROFILEMIGRATOR_CONTRACTID_PREFIX "ie");
Don't we want to assign key here too?

>-  // This is purposely broken as using this would mean that we have
>-  // to use data from where profiles exist currently. We want to copy
>-  // it so that we can create a "fresh" profile. There may be a way
>-  // to do it from here, but currently we haven't found an easy one.
>   //if (ImportRegistryProfiles(NS_LITERAL_CSTRING("mozilla")))
>   //    return NS_OK;
Why remove the helpful comment?
Why provide ImportRegistryProfiles if we're not going to call it?


>+  return NS_ERROR_FAILURE;
>+#else
...
>+  return NS_OK;
>+#endif
I don't see why one should be failure and the other should be OK.

>+  // the last thing to do is to actually copy over any mail folders we have
>+  // marked for copying we want to do this last and it will be asynchronous
>+  // so the UI doesn't freeze up while we perform this potentially very long
>+  // operation.
Hmm, so importing mail is the only thing that doesn't freeze up the UI?

>+  MigrationData data[] = { { ToNewUnicode(FILE_NAME_PREFS),
Now this looks suboptimal - all of these file names are ASCII.
Yet we have to go to the trouble of copying unicode strings.

>+  // add some extra migration fields for things we also migrate
>+  *aResult |= nsISuiteProfileMigrator::ACCOUNT_SETTINGS |
>+    nsISuiteProfileMigrator::MAILDATA |
>+    nsISuiteProfileMigrator::NEWSDATA |
>+    nsISuiteProfileMigrator::ADDRESSBOOK_DATA;
Can't we init *aResult with this value in the first place?
Also, I don't like the indentation here - maybe
*aResult |=
    nsISuiteProfileMigrator::ACCOUNT_SETTINGS |
    nsISuiteProfileMigrator::MAILDATA |
    nsISuiteProfileMigrator::NEWSDATA |
    nsISuiteProfileMigrator::ADDRESSBOOK_DATA;

>+  MAKESAMETYPEPREFTRANSFORM("network.proxy.type",                      Int),
...
>+  MAKESAMETYPEPREFTRANSFORM("network.proxy.type",                      Int),
You seem to have duplicates. Maybe sorting them would help?

>+  MAKEPREFTRANSFORM("mail.pane_config","mail.pane_config.dynamic", Int, Int)
Actually we need to transform mail.pane_config.dynamic if we have it - I guess we can transform both and if we don't have .dynamic then we'll just the transformed value of mail.pane_config anyway?

>+  nsVoidArray* accounts = new nsVoidArray();
>+  nsVoidArray* identities = new nsVoidArray();
>+  nsVoidArray* servers = new nsVoidArray();
>+  nsVoidArray* smtpservers = new nsVoidArray();
>+  nsVoidArray* ldapservers = new nsVoidArray();
>+  nsVoidArray* labelPrefs = new nsVoidArray();
>+  nsVoidArray* fontPrefs = new nsVoidArray();
>+  nsVoidArray* others = new nsVoidArray();
Allocate these on the stack, rather than the heap, e.g.
nsVoidArray accounts;

>+#ifndef MOZ_PLACES
WTF?

>+  MAKEPREFTRANSFORM("mail.pane_config","mail.pane_config.dynamic", Int, Int)
Doesn't Thunderbird only use .dynamic?

>+  ReadBranch("general.startup.", psvc, others);
I don't think Thunderbird uses this.

>+struct FontPref {
Does this belong?
Comment 54 Mark Banner (:standard8) 2007-04-12 12:00:47 PDT
(In reply to comment #53)
> (From update of attachment 260747 [details] [diff] [review])
> >+  // the last thing to do is to actually copy over any mail folders we have
> >+  // marked for copying we want to do this last and it will be asynchronous
> >+  // so the UI doesn't freeze up while we perform this potentially very long
> >+  // operation.
> Hmm, so importing mail is the only thing that doesn't freeze up the UI?

I think generally copying other files is considered "quick". It's how the others currently work, not sure if its really worth changing.
Comment 55 neil@parkwaycc.co.uk 2007-04-13 04:19:50 PDT
(In reply to comment #54)
>I think generally copying other files is considered "quick". It's how the
>others currently work, not sure if its really worth changing.
So if you're not copying any mail, you'll go straight from clicking "Next" to seeing the page with all the migration complete?
Comment 56 Mark Banner (:standard8) 2007-04-23 12:41:18 PDT
Created attachment 262535 [details] [diff] [review]
Implement the migrator v8

Updated version incorporates most of Neil's comments from his review, still some to work on. I have however gone through browser-prefs.js and added migration of a bunch of prefs we were missing but need to migrate. Still got mailnews and a few other areas to do in this respect.
Comment 57 Mark Banner (:standard8) 2007-05-10 14:24:49 PDT
Created attachment 264408 [details] [diff] [review]
Implement the migrator v8a

Updated patch - I've think I've got all the prefs I need to in the SeaMonkey profile migrator. Still need to address some nits and finish off the thunderbird one.
Comment 58 Mark Banner (:standard8) 2007-05-13 14:35:03 PDT
Created attachment 264688 [details] [diff] [review]
Implement the migrator v9

I think this fixes all the previous review comments. I've also added all the prefs that I think we need to migrate.
Comment 59 Tony Mechelynck [:tonymec] 2007-05-13 15:50:54 PDT
(In reply to comment #58)
> Created an attachment (id=264688) [details]
> Implement the migrator v9
> 
> I think this fixes all the previous review comments. I've also added all the
> prefs that I think we need to migrate.
> 

I'm not sure I understand the code, but prefs "that we don't know about" in the Sm/xpfe profile (such as prefs set by extensions) will be copied verbatim to the Sm/SR profile, won't they? They won't disappear?
Comment 60 Mark Banner (:standard8) 2007-05-14 00:01:28 PDT
(In reply to comment #59)
> I'm not sure I understand the code, but prefs "that we don't know about" in the
> Sm/xpfe profile (such as prefs set by extensions) will be copied verbatim to
> the Sm/SR profile, won't they? They won't disappear?

Prefs that we don't know about won't currently be copied - we took the decision early on to use this opportunity to "clean out" profiles (some of which may have been around since 4.x days). We're also not migrating extensions so I don't think we should migrate those prefs that they set either.
Comment 61 Karsten Düsterloh 2007-05-14 00:27:09 PDT
> > Sm/xpfe profile (such as prefs set by extensions) will be copied verbatim to
> > the Sm/SR profile, won't they? They won't disappear?
> 
> Prefs that we don't know about won't currently be copied

Anything else might be hard anyway, unless you wanted to explicitly state which of those prefs we have heard about ;-) we'd want to migrate. In other words: we just can't know how $random_extension did name its prefs.

OTOH, copying over the extensions.* pref branch might be worth considering, since it's used by Chatzilla, Venkman, Reporter and some third party extensions.
Comment 62 neil@parkwaycc.co.uk 2007-05-14 02:19:12 PDT
Comment on attachment 264688 [details] [diff] [review]
Implement the migrator v9

>+    key = AssignLiteral("ie");
Should be key.AssignLiteral of course.

>+struct MigrationData {
>+  nsString fileName;
>+  PRUint32 sourceFlag;
>+  PRBool replaceOnly;
>+};
I'd still prefer fileName to be a char*, and you can use AppendNative to avoid having to convert it to Unicode and back.
Comment 63 Giacomo Magnini 2007-05-14 02:54:35 PDT
(In reply to comment #61)
> OTOH, copying over the extensions.* pref branch might be worth considering,
> since it's used by Chatzilla, Venkman, Reporter and some third party
> extensions.

As long as those prefs dont conflict with the new EM registering of the sme extension... Right?
Comment 64 neil@parkwaycc.co.uk 2007-05-14 03:45:51 PDT
Comment on attachment 264688 [details] [diff] [review]
Implement the migrator v9

>+  result =
>+    do_CreateInstance(NS_SUITEPROFILEMIGRATOR_CONTRACTID_PREFIX "seamonkey");
>+  if (result)
>+    result->GetSourceExists(&exists);
>+  if (exists) {
>+    aKey.AssignLiteral("seamonkey");
>+    result.swap(*spm);
>+    return NS_OK;
>+  }
>+
>+  result =
>+    do_CreateInstance(NS_SUITEPROFILEMIGRATOR_CONTRACTID_PREFIX "thunderbird");
>+  if (result)
>+    result->GetSourceExists(&exists);
>+  if (exists) {
>+    aKey.AssignLiteral("thunderbird");
>+    result.swap(*spm);
>+  }
>+#endif
>+  return NS_ERROR_FAILURE;
>+}
Missing an return NS_OK; after that second result.swap(*spm); ?

>+  // add some extra migration fields for things we also migrate
Slightly inaccurate. Perhaps // migration fields for things we always migrate
(note that you didn't apply the code fix to the Thunderbird migrator...)

>+  // Frees file name strings allocated above.
This comment is no longer accurate.

>+#define MAX_BRANCHES 18
I think I'd prefer NS_ARRAY_LENGTH(branchNames) to make it easier to change. Otherwise this is much better than your previous version.

>+    "mailnews.reply_",
This might be a violation of the pref service contract.
(i.e. it might stop working without warning)
Comment 65 neil@parkwaycc.co.uk 2007-05-14 03:51:24 PDT
Comment on attachment 264688 [details] [diff] [review]
Implement the migrator v9

>+  MAKEPREFTRANSFORM("browser.downloadmanager.behavior", 0, Int,
>+                    DownloadManager),
This might not be necessary ;-)

>+  MAKESAMETYPEPREFTRANSFORM("mail.pane_config",                        Int),
I'd prefer if you could transform this it mail.pane_config.dynamic - that way we could get rid of all the code that uses mail.pane_config which would be nice.

Otherwise I think this is near enough to get a + :-)
Comment 66 Mark Banner (:standard8) 2007-05-14 04:45:50 PDT
(In reply to comment #63)
> (In reply to comment #61)
> > OTOH, copying over the extensions.* pref branch might be worth considering,
> > since it's used by Chatzilla, Venkman, Reporter and some third party
> > extensions.
> As long as those prefs dont conflict with the new EM registering of the sme
> extension... Right?

I think Chatzilla, Venkman & Reporter pref migration may be ok (though I haven't looked in too much detail yet). Having thought about it a bit more, I think third party extensions may be a bad idea.

Take for instance palm sync (which is included in tree and may be a strange case, but is hopefully a useful example), it uses extensions.palmsync.conduitRegistered to determine if to run an installer on first startup after installation. If we blindly migrated extensions.* we'd migrate this pref as well and palm sync wouldn't work correctly because the conduit wouldn't be installed.

There may be other extensions like this, but I think we shouldn't get into maintaining a list of third party extension preferences to migrate - in-tree extensions that we ship by default though may be a good idea.
Comment 67 Robert Kaiser 2007-05-14 04:53:50 PDT
(In reply to comment #66)
> There may be other extensions like this, but I think we shouldn't get into
> maintaining a list of third party extension preferences to migrate - in-tree
> extensions that we ship by default though may be a good idea.

I'm with you on that - It's probably best to get in what we have now though (after fixing review comments) and then work on such improvements afterwards. This reasults in 1) no blocking of the patch while figuring out what to migrate explicitely, and 2) shorter patches for those additions - the current patch is big enough already (and this bug long enough).
Comment 68 Karsten Düsterloh 2007-05-14 04:55:02 PDT
(In reply to comment #66)
> Take for instance palm sync (which is included in tree and may be a strange
> case, but is hopefully a useful example), it uses
> extensions.palmsync.conduitRegistered to determine if to run an installer on
> first startup after installation.

This is most stupid behaviour(tm) anyway; and, yes, I know that such behaviour is induced by the EM's lack of OnInstall/OnDeinstall hooks.

> If we blindly migrated extensions.* we'd
> migrate this pref as well and palm sync wouldn't work correctly because the
> conduit wouldn't be installed.

If I run the same profile with a different build, this pref is likely to be wrongly set almost always - that palm sync usually doesn't change that much is just mere luck.

> There may be other extensions like this, but I think we shouldn't get into
> maintaining a list of third party extension preferences to migrate

Of course not, agreed. That's why I think just grabbing the whole extensions.* branch is useful despite some extensions doing stupid things...
Comment 69 Mark Banner (:standard8) 2007-05-14 14:06:07 PDT
Created attachment 264795 [details] [diff] [review]
Implement the migrator v10

Fixes Neil's comments apart from (pending Neil's further thoughts):

> I'd still prefer fileName to be a char*, and you can use AppendNative to avoid
> having to convert it to Unicode and back.

Any other changes to preference migrations apart from the review comments will be dealt with in follow-up bugs.

Karsten, can you review/check this from a mac perspective please?
Comment 70 Caio Tiago Oliveira (asrail) 2007-05-14 17:47:54 PDT
I would request the logs and settings from chatzilla...
My autojoin lists are too large that I couldn't even remember all the channels, for instance.

Would be bad to loose this data and since we ship chatzilla in every version...

Comment 71 Mark Banner (:standard8) 2007-05-18 10:46:26 PDT
Created attachment 265280 [details] [diff] [review]
Implement the migrator v10a

10a adds ifdefs so that we deal with xpfe dm pref migration rather than xpfe->toolkit dm pref migration.
Comment 72 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-20 08:42:06 PDT
(In reply to comment #62)
> I'd still prefer fileName to be a char*, and you can use AppendNative to avoid
> having to convert it to Unicode and back.

not really, AppendNative converts to unicode, at least on windows.

Comment 73 Mark Banner (:standard8) 2007-05-20 09:03:51 PDT
(In reply to comment #69)
> Created an attachment (id=264795) [details]
> Implement the migrator v10
> 
...
> Karsten, can you review/check this from a mac perspective please?

This patch has landed ahead of Karsten's review so we can get feedback and progress towards the suiterunner changeover on trunk.

Comment 74 neil@parkwaycc.co.uk 2007-05-20 09:04:10 PDT
Comment on attachment 264688 [details] [diff] [review]
Implement the migrator v9

>+struct MigrationData {
>+  nsString fileName;
>+  PRUint32 sourceFlag;
>+  PRBool replaceOnly;
>+};
Note that this won't compile on Solaris.
Comment 75 Robert Kaiser 2007-05-21 07:09:52 PDT
Comment on attachment 254343 [details] [diff] [review]
Chrome changes for review v3 (checked in)

>Index: suite/locales/en-US/chrome/common/migration/migration.dtd
>===================================================================
>+<!ENTITY importFromSeamonkey.label       "SeaMonkey 1.0/1.1, Netscape 6/7 or Mozilla 1.x">

>Index: suite/locales/en-US/chrome/common/migration/migration.properties
>===================================================================
>+sourceNameseamonkey=SeaMonkey 1.0/1.1, Netscape 6/7 or Mozilla 1.x
>+importedSeamonkeyBookmarksTitle=SeaMonkey 1.0/1.1, Netscape 6/7 or Mozilla 1.x

Given the decision to use "SeaMonkey 2" naming for suiterunner, it might be a good idea to change those strings to talk of "SeaMonkey 1.x" instead of "SeaMonkey 1.0/1.1"
Comment 76 Mark Banner (:standard8) 2007-05-21 14:03:48 PDT
Created attachment 265543 [details] [diff] [review]
Change strings to reference SeaMonkey 1.x (checked in)

Changes strings to reference SeaMonkey 1.x rather than SeaMonkey 1.0/1.1.

Note: I'm still aware of the solaris build problem, I'll try and look into that later this week.
Comment 77 Mark Banner (:standard8) 2007-05-22 14:29:09 PDT
Created attachment 265711 [details] [diff] [review]
Fix writing of the target prefs.js

I was looking at the newsgroups bug and found this significant bug as well :/

The prefs.js file that currently gets saved in the new profile is actually a copy of the source prefs.js file. Most of the "selective" migration has been lost.

The problem is that nsIPrefsService->ReadUserPrefs(nsnull) isn't able to set the default pref file at migration time because we haven't got a profile yet and so various things aren't set up.

So this patch changes things so that we select whichever prefs file we need (source/target) when we need it, but as an additional check we force a reload of the target prefs.js file just before exiting migration. This ensures we don't mess up when we're saving/setting the home page pref.

I've also tidied up a couple of bits in migration.js that I found whilst I was hacking it all around fixing the problem.
Comment 78 neil@parkwaycc.co.uk 2007-05-24 04:59:35 PDT
Comment on attachment 265711 [details] [diff] [review]
Fix writing of the target prefs.js

>+  // Now we've finished migration, ensure we're going to use the correct
>+  // prefs file (note: the migrator js code may alter the home page prefs
Doesn't TransformPreferences already do this?
Comment 79 Mark Banner (:standard8) 2007-05-24 10:00:01 PDT
(In reply to comment #78)
> (From update of attachment 265711 [details] [diff] [review])
> >+  // Now we've finished migration, ensure we're going to use the correct
> >+  // prefs file (note: the migrator js code may alter the home page prefs
> Doesn't TransformPreferences already do this?
> 
Yes, but its not called last - there's a couple of other functions which get called after TransformPreferences which set the prefs to something else. I mainly put it in like this because I wanted a catch-all case.
Comment 80 Mark Banner (:standard8) 2007-05-24 12:45:18 PDT
Created attachment 265980 [details] [diff] [review]
Fix writing of the target prefs.js v2

After discussion with Neil on irc. Revised patch now reads the prefs for wallet files from the new profile (they've already been copied) and falls back to finding a file if necessary.

Therefore we don't need the additional final check that I previously added.
Comment 81 neil@parkwaycc.co.uk 2007-05-25 06:19:43 PDT
Comment on attachment 265980 [details] [diff] [review]
Fix writing of the target prefs.js v2

>+    if (psvc) {
>+      nsCOMPtr<nsIPrefBranch> branch(do_QueryInterface(psvc));
> 
>-    nsCOMPtr<nsIPrefBranch> branch(do_QueryInterface(psvc));
do_QueryInterface is null-safe, no need to make this change.

>-    return branch->GetCharPref("wallet.SchemaValueFileName", aFileName);
>+      if (branch)
>+        return branch->GetCharPref("wallet.SchemaValueFileName", aFileName);
>+    }
I think this should return only if GetCharPref succeeded.
Comment 82 Mark Banner (:standard8) 2007-05-25 10:08:35 PDT
Created attachment 266079 [details] [diff] [review]
Fix writing of the target prefs.js v3 (checked in)

v3 -> Fix Neil's comments
Comment 83 neil@parkwaycc.co.uk 2007-05-26 08:38:39 PDT
Comment on attachment 266079 [details] [diff] [review]
Fix writing of the target prefs.js v3 (checked in)

Wallet still migrates so I guess this is OK.
Comment 84 Sven Grull 2007-05-27 06:22:33 PDT
I did another migration after the checkin and realized that the name of the SeaMonkey profile is not migrated to the suiterunner profile. Is this this intended or shouldn't suiterunner use the same profilename as SeaMonkey? 
Comment 85 Mark Banner (:standard8) 2007-05-27 09:12:39 PDT
(In reply to comment #84)
> I did another migration after the checkin and realized that the name of the
> SeaMonkey profile is not migrated to the suiterunner profile. Is this this
> intended or shouldn't suiterunner use the same profilename as SeaMonkey? 
> 
This is intended - by the time we've got to migration, the back-end has already created part of a profile (and a location for it), we can't change that easily.

Therefore the work around (and I'll try and remember to post this elsewhere) is to either do the migration and rename the profile in the profile manager (though the directory will still be the same) or to do:

./seamonkey -createProfile newprofile
./seamonkey -P newprofile -migration
Comment 86 Sven Grull 2007-05-28 02:41:19 PDT
(In reply to comment #85)
> and I'll try and remember to post this elsewhere

I think it would be useful to have a wiki site for the migration purposes.

> is to either do the migration and rename the profile in the profile manager
> (though the directory will still be the same) or to do:
> 
> ./seamonkey -createProfile newprofile
> ./seamonkey -P newprofile -migration

Thanks for the tip. This worked like a charm. Great.

Do you prefer a single bug for each pref that wasn't migrated or should we post a list of prefs that are missing while migration to this bug?
Comment 87 Mark Banner (:standard8) 2007-05-28 03:24:44 PDT
(In reply to comment #86)
> Do you prefer a single bug for each pref that wasn't migrated or should we post
> a list of prefs that are missing while migration to this bug?

Some prefs are purposely not migrated (generally obsolete/not required after transition/third-party extensions we don't know about).

However, to save clogging up bugs, please post to mozilla.dev.apps.seamonkey in the first instance about the missing prefs and we can discuss them there.
Comment 88 Sven Grull 2007-05-30 09:17:06 PDT
I just downloaded the tinderbox suiterunner zip-build 2007053005 and wanted to migrate my profile once again. Unfortunately I don't get over the "Import Settings and Data" dialog. I can click "Next" but it has no effect. "Cancel" works as expected.
Comment 89 Ruediger Lahl 2007-05-30 11:11:50 PDT
(In reply to comment #88)

Same here. My profile is outside off the standard directory. Maybe this also cause the error.
Comment 90 Mark Banner (:standard8) 2007-05-30 11:24:32 PDT
(In reply to comment #88)
> I just downloaded the tinderbox suiterunner zip-build 2007053005 and wanted to
> migrate my profile once again. Unfortunately I don't get over the "Import
> Settings and Data" dialog. I can click "Next" but it has no effect. "Cancel"
> works as expected.
> 

I'm looking into this as part of bug 382449. Please move over onto that bug.

I'd prefer it also that any other problems with profile migration are posted in new bugs. This bug is really only remaining open whilst I wait for Karsten's final mac review.
Comment 91 Karsten Düsterloh 2007-06-04 15:30:07 PDT
Comment on attachment 265280 [details] [diff] [review]
Implement the migrator v10a

I actually didn't do a thorough code review as such (I just skimmed through the patch), but I tested the migrator beast with trunk on Mac and it worked as expected (as far as I can tell and modulo already filed bugs), even with my non-standard (pro)file locations. (The overlong migration time I mentioned elsewhere was due to a forgotten 2GB mail file for bug 361730... *g*)

One nit, though:
The wizard panel showing the list of items to migrate is not tall enough. I migrated 12 items there and got a vertical scrollbar, and the progress meter was hidden completely...
Comment 92 Mark Banner (:standard8) 2007-06-05 14:25:59 PDT
Created attachment 267328 [details] [diff] [review]
Make the profile migrator dialog slightly bigger

This makes the dialog a little taller. I don't get the scroll bar I used to on linux, hopefully this should be enough for mac as well.
Comment 93 Karsten Düsterloh 2007-06-05 15:30:09 PDT
Comment on attachment 267328 [details] [diff] [review]
Make the profile migrator dialog slightly bigger

Been doing the mindreading stuff again, eh?
That was the *exact* change I tried yesterday... ;-)
Comment 94 neil@parkwaycc.co.uk 2007-06-06 03:35:38 PDT
Comment on attachment 267328 [details] [diff] [review]
Make the profile migrator dialog slightly bigger

Should the width be 50ch? (see bug 380378)
Comment 95 neil@parkwaycc.co.uk 2007-06-06 03:47:22 PDT
Comment on attachment 267328 [details] [diff] [review]
Make the profile migrator dialog slightly bigger

>-        style="width: 40em; height: 30em;"
>+        style="width: 40em; height: 35em;"
Actually 35em looks a bit tall... would 32em work?
Comment 96 Mark Banner (:standard8) 2007-06-19 09:02:58 PDT
(In reply to comment #95)
> (From update of attachment 267328 [details] [diff] [review])
> >-        style="width: 40em; height: 30em;"
> >+        style="width: 40em; height: 35em;"
> Actually 35em looks a bit tall... would 32em work?
> 
Checked in with 32em after having agreed with Karsten over irc.
Comment 97 Mark Banner (:standard8) 2007-06-21 12:30:16 PDT
Created attachment 269261 [details] [diff] [review]
Rework the #defines for strings

This patch deals with the last thing I'm going to fix on this bug. Other bugs in the profile migrator will be fixed in the related bugs or in new bugs.

This patch should resolve the string issues that Neil was concerned about - mainly the nsString in the struct, and the #defines with NS_LITERAL_STRING included. I've changed some function parameters to const char* as that avoids needing NS_LITERAL_STRING everywhere. I've also included a few tidy ups as there were a few nsDependentCString("xyz") where they should have used the NS_LITERAL_CSTRING version.
Comment 98 Mark Banner (:standard8) 2007-06-21 14:04:32 PDT
(In reply to comment #97)
> Created an attachment (id=269261) [details]
> Rework the #defines for strings
> 
> This patch deals with the last thing I'm going to fix on this bug. Other bugs
> in the profile migrator will be fixed in the related bugs or in new bugs.

This last patch is checked in, therefore as noted above, other issues with the
migrator will be dealt with in other bugs, and this bug is FIXED (yay!).
Comment 99 Philip Chee 2007-11-01 00:12:44 PDT
FYI: once Bug 394288 is fixed, don't migrate these:
314   MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.bookmarks",   Bool),
316   MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.home",        Bool),

Comment 100 Tony Mechelynck [:tonymec] 2008-05-08 21:33:02 PDT
*** Bug 342659 has been marked as a duplicate of this bug. ***

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