Mark Banner (:standard8) Assignee
	Description • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 1 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 2 • 19 years ago

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.

	Robert Kaiser
	Comment 3 • 19 years ago

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...

	Mark Banner (:standard8) Assignee
	Comment 4 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 5 • 19 years ago

Comment on attachment 224875 [details] [diff] [review]
Create a central build location in /suite

Get the review's address right.

	Benjamin Smedberg
	Comment 6 • 19 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 7 • 19 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 8 • 19 years ago

Updated patch to remove the unnecessary META_COMPONENTS.

	neil@parkwaycc.co.uk
	Comment 9 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 10 • 19 years ago

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.

	Eyal Rozenberg
	Comment 11 • 19 years ago

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.

	Robert Kaiser
	Comment 12 • 19 years ago

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. :)

	Eyal Rozenberg
	Comment 13 • 19 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 14 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 15 • 18 years ago

Patch without the changes from my tree for other bugs.

	Mark Banner (:standard8) Assignee
	Comment 16 • 18 years ago

Still wip, but hopefully got all the files needed this time.

	Mark Banner (:standard8) Assignee
	Comment 17 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 18 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 19 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 20 • 18 years ago

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).

	Mark Banner (:standard8) Assignee
	Comment 21 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 22 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 23 • 18 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 24 • 18 years ago

(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.

	Robert Kaiser
	Comment 25 • 18 years ago

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

	Mark Banner (:standard8) Assignee
	Comment 26 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 27 • 18 years ago

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?

	neil@parkwaycc.co.uk
	Comment 28 • 18 years ago

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

	neil@parkwaycc.co.uk
	Comment 29 • 18 years ago

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.

	Worcester12345
	Comment 30 • 18 years ago

(In reply to comment #27)
...
> 4. Is there no way to choose what to migrate?

Checkboxes would be great here. 

	Mark Banner (:standard8) Assignee
	Comment 31 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 32 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 33 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 34 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 35 • 18 years ago

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.

	Robert Kaiser
	Comment 36 • 18 years ago

(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.

	neil@parkwaycc.co.uk
	Comment 37 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 38 • 18 years ago

Comment on attachment 244932 [details] [diff] [review]
Chrome changes for review v2

As per comment 37.

	Robert Kaiser
	Comment 39 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 40 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 41 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 42 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 43 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 44 • 18 years ago

I missed the dtd & properties files out of the v5 patch :-(

	Mark Banner (:standard8) Assignee
	Comment 45 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 46 • 18 years ago

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)?

	neil@parkwaycc.co.uk
	Comment 47 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 48 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 49 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 50 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 51 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 52 • 18 years ago

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; } )?

	neil@parkwaycc.co.uk
	Comment 53 • 18 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 54 • 18 years ago

(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.

	neil@parkwaycc.co.uk
	Comment 55 • 18 years ago

(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?

	Mark Banner (:standard8) Assignee
	Comment 56 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 57 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 58 • 18 years ago

I think this fixes all the previous review comments. I've also added all the prefs that I think we need to migrate.

	Tony Mechelynck [:tonymec]
	Comment 59 • 18 years ago

(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?

	Mark Banner (:standard8) Assignee
	Comment 60 • 18 years ago

(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.

	Karsten Düsterloh
	Comment 61 • 18 years ago

> > 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.

	neil@parkwaycc.co.uk
	Comment 62 • 18 years ago

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.

	Giacomo Magnini
	Comment 63 • 18 years ago

(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?

	neil@parkwaycc.co.uk
	Comment 64 • 18 years ago

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)

	neil@parkwaycc.co.uk
	Comment 65 • 18 years ago

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 + :-)

	Mark Banner (:standard8) Assignee
	Comment 66 • 18 years ago

(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.

	Robert Kaiser
	Comment 67 • 18 years ago

(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).

	Karsten Düsterloh
	Comment 68 • 18 years ago

(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...

	Mark Banner (:standard8) Assignee
	Comment 69 • 18 years ago

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?

	Caio Tiago Oliveira (asrail)
	Comment 70 • 18 years ago

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...

	Mark Banner (:standard8) Assignee
	Comment 71 • 18 years ago

10a adds ifdefs so that we deal with xpfe dm pref migration rather than xpfe->toolkit dm pref migration.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 72 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 73 • 18 years ago

(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.

	neil@parkwaycc.co.uk
	Comment 74 • 18 years ago

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.

	Robert Kaiser
	Comment 75 • 18 years ago

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"

	Mark Banner (:standard8) Assignee
	Comment 76 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 77 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 78 • 18 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 79 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 80 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 81 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 82 • 18 years ago

v3 -> Fix Neil's comments

	neil@parkwaycc.co.uk
	Comment 83 • 18 years ago

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.

	Sven Grull
	Comment 84 • 18 years ago

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? 

	Mark Banner (:standard8) Assignee
	Comment 85 • 18 years ago

(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

	Sven Grull
	Comment 86 • 18 years ago

(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?

	Mark Banner (:standard8) Assignee
	Comment 87 • 18 years ago

(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.

	Sven Grull
	Comment 88 • 18 years ago

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.

	Ruediger Lahl
	Comment 89 • 18 years ago

(In reply to comment #88)

Same here. My profile is outside off the standard directory. Maybe this also cause the error.

	Mark Banner (:standard8) Assignee
	Comment 90 • 18 years ago

(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.

	Karsten Düsterloh
	Comment 91 • 18 years ago

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...

	Mark Banner (:standard8) Assignee
	Comment 92 • 18 years ago

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.

	Karsten Düsterloh
	Comment 93 • 18 years ago

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... ;-)

	neil@parkwaycc.co.uk
	Comment 94 • 18 years ago

Comment on attachment 267328 [details] [diff] [review]
Make the profile migrator dialog slightly bigger

Should the width be 50ch? (see bug 380378)

	neil@parkwaycc.co.uk
	Comment 95 • 18 years ago

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?

	Mark Banner (:standard8) Assignee
	Comment 96 • 18 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 97 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 98 • 18 years ago

(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!).

	Philip Chee
	Comment 99 • 17 years ago

FYI: once Bug 394288 is fixed, don't migrate these:
314   MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.bookmarks",   Bool),
316   MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.home",        Bool),