394687 - need to import settings from Windows Mail on Vista and Windows Live Mail on XP/Vista/Windows 7

Status: RESOLVED FIXED

(MailNews Core :: Import, enhancement, P2)

Description

[Ronald Schaefer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: 2.0.0.0.6 ThunderBird

I would like to switch from Vista Windows Mail to Thunderbird, I would like to import messages from Vista Mail to Thunderbird.  

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Magnus Melin [:mkmelin]]

Confirming RFE. This really should be on the tb3 feature list if possible...

Comment 3

[WADA:World Anti-bad-Duping Agency]

FYI.
ImportExportTools extension is available.
> http://kb.mozillazine.org/Importing_folders
> http://nic-nac-project.de/~kaosmos/mboximport-en.html
Nice to have feature of Tb3, but I don't think mandatory for Tb3.

Comment 4

[steele_116]

When I installed the MBOX import export tool, all of the functions in the menu were grayed out except options.

http://nic-nac-project.de/~kaosmos/mboximport-en.html

Comment 5

[juan becerra [:juanb]]

Currently we are not able to import mail or mail settings from Windows Mail (on Vista).

Comment 6

[Magnus Melin [:mkmelin]]

Phil, is this something you'd be able to take on?

Comment 7

[Phil Lacy]

Not having Vista, it may be too long a curve for me to figure it out.

Comment 8

[Phil Lacy]

I see Vista may have put settings and email in text files and have their own index file so it may be something I can look at.  But I would need a Vista user to send me their files and do the testing. I can't commit yet because it may still be too much for me without being able to test as I go. Vista went to a comAPI for access to the data but I'd dread having to go through that to do the import.

Comment 9

[Rain [:rain1] (inactive)]

Phil: can you use Windows Live Mail on XP to test? AFAIK the format is the same as that for Windows Mail on Vista, therefore getting one thing to work should get the other, too.

Comment 10

[Rain [:rain1] (inactive)]

Changing summary to be more descriptive of what a fix for this should do.

Comment 11

[WADA:World Anti-bad-Duping Agency]

FYI.
Another way to transfer mails of an mailer to another mailer - via IMAP.
("via. IMAP" is one of official recommendations by Apple for importing mail data to Apple Mail 2)

(1) Get Gmail account, enable Gmail IMAP. (free Gmail account can use 6GB area)
(2) Upload mails to Gmail IMAP by mailer-1
(3) Download mails from Gmail IMAP by mailer-2
(4) If mailer-2 is Thunderbird, create X-Mozilla-Keys: header for tags
    by "Compact folder".
    - Move a mail to other mail folder, then move back to original folder
    - Compact folder

Comment 12

[Phil Lacy]

Attachment: information

incomplete.
documentation on WLM

Comment 13

[Phil Lacy]

Attachment: information

added xml used for nntp and imap

Comment 14

[Phil Lacy]

Attachment: Settings background document

Should be final doc for the background of windows live mail settings import. Background on file import should be straightforward as well

Comment 15

[Rain [:rain1] (inactive)]

> %userprofile$\Local Settings\Application Data\Microsoft\Windows Live Mail\accountnames

Please use "%LOCALAPPDATA%\Microsoft\Windows Live Mail" for Vista and Windows 7, and "%LOCALAPPDATA%\Microsoft\Windows Mail" for Vista's Windows Mail. (LOCALAPPDATA should ideally be there in XP too, but isn't. :( )

Comment 16

[Phil Lacy]

The location of the correct unexpanded string should be in the same place as is noted in the file
HKEY_CURRENT_USER\Software\Microsoft\Windows Live Mail:"Store Root"
However I do need to find out if Vista calls the key 'Windows Mail'. import/oexpress has a utility for expanding this string correctly.
Sid I can handle the settings but at my speed we may need to enlist someone to do the mail and address book or it may not get done before TB6.

Comment 17

[Rain [:rain1] (inactive)]

(In reply to comment #16)
> The location of the correct unexpanded string should be in the same place as is
> noted in the file
> HKEY_CURRENT_USER\Software\Microsoft\Windows Live Mail:"Store Root"

Oh, I see. This should be fine then.

> However I do need to find out if Vista calls the key 'Windows Mail'.

Windows Mail on Vista uses "Windows Mail", Windows Live Mail uses "Windows Live Mail". It'd be great to have two new entries, "Windows Mail" on Vista only, and "Windows Live Mail" on XP, Vista and 7.

> Sid I can handle the settings but at my speed we may need to enlist someone to
> do the mail and address book or it may not get done before TB6.

OK, the settings will surely be a start.

Comment 18

[Phil Lacy]

Attachment: settings code work-in-progress

Comment 19

[Phil Lacy]

Attachment: imports windows live mail settings only

This will import settings only from windows Live Mail or Windows Mail. tried on XP Windows Live Mail. Other settings can be added if users request them.

Comment 20

[Phil Lacy]

Attachment: unused code removed. non-bloat stuff for other import left in.

Removed code for address and mail import. 'not_implemented' placeholder interfaces left in. Addresses and mail may be imported later. 'Windows Live Mail' messages are in *.eml files which may be importable without a module.

Comment 21

[Phil Lacy]

Bug 171907 fix would import messages. Anyway to tie that one to this one?

Comment 23

[Vincent (caméléon)]

(In reply to comment #22)
> *** Bug 486830 has been marked as a duplicate of this bug. ***
Sorry for the duplicate. I have made a search before posting it, but only on the "Thunderbird" product, not "mailnewsCore" (I don't really understand what is the difference).

Comment 24

[Phil Lacy]

Unless this bug is expected to be picked up in extensions, it would e nice to get it on a beta.

Comment 25

[Magnus Melin [:mkmelin]]

Neil, any chance to get this reviewed for beta3?

Comment 26

[Kohei Yoshino [:kohei]]

More and more people are asking how to switch from Windows Live Mail. Is there any chance the patch will be landed?

Comment 27

[David Ascher (:davida)]

It would be awesome, but I don't know if we can fit in 3.0.

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #27)
> It would be awesome, but I don't know if we can fit in 3.0.

It has strings, so we can't land it for 3.0. Putting on the query list for 3.1.

Comment 29

[Phil Lacy]

1. This is only for import of settings which should help since we can drag and drop messages now. I don't anticipate getting to the message import in foreseeable future. And I haven't looked into LiveMail's handling of addressbooks.

2. This is my first upload with tree changes, new files, new folders, configs etc. so maybe a review of that by someone else would help Neil moving the review along.

Comment 30

[Joshua Cranmer [:jcranmer]]

(In reply to comment #29)
> 1. This is only for import of settings which should help since we can drag and
> drop messages now. I don't anticipate getting to the message import in
> foreseeable future. And I haven't looked into LiveMail's handling of
> addressbooks.

AIUI, Windows Mail doesn't do address books any more, that's now Windows Contacts (and there's also a Windows Calendar to be complete). There's a bug on Windows Contacts, but I think that's asking for a MAPI replacement as opposed to an import feature.

Comment 31

[Ben Bucksch (:BenB)]

Why does Windows Mail need its own import code instead of sharing the one of OExpress? Windows Mail is the successor, and the code looks a lot like the oexpress code. Code duplication (even if it's just 'large parts', not all) is never good, as it makes maintenance harder, e.g. I had to change the oexpress code for bug 525238, and I'd have to redo all these changes to your new code.

Comment 32

[Ben Bucksch (:BenB)]

Spoke with Phil. He pointed out that Windows Live Mail uses XML to store the config (and OE uses the registry), that's a significant enough difference to justify the fork ;-). (It was that  DoIMAPServer(), SetIdentities(), and even "IMAP Use Sicily" sounded so familar, but I didn't look closely enough.)

Suggestion: call the dir "winlivemail" instead of "winmail". The latter suggests to me that it's something generic like MAPI.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Since we already have a patch here, and this is likely to have significant value in attracting new users to Thunderbird, I believe this does need to block 3.1.  I'm setting it block beta2, since that's string/feature freeze as we currently understand it.

Neil has said he might have a chance to look at this weekend, though, so I'm still harboring crazy fantasies that we might be able to get this for beta1 (code freeze: Tues Feb 23), though I have no idea how practical they really are.

Comment 34

[[e-mail]]

So, sorry for forgetting this, but I've now had a quick scan, and basically this looks a bit like a copy of outlook express import, am I right? In which case, using hg copy (--after) would greatly simplify the diffs! It would also prevent me from complaining about all the style nits you copied (and there are unfortunately far too many). I'll still review nsWMSettings.cpp as if it wasn't a copy, since it's most likely to be different.

Comment 35

[[e-mail]]

Comment on attachment 362484 [details] [diff] [review]
unused code removed. non-bloat stuff for other import left in.

>+static PRInt32 checkNewMailTime;   // OE global setting, let's default to 30
>+static PRBool  checkNewMail;  // OE global setting, let's default to PR_FALSE
[Still says OE]

>+nsresult nsWMSettings::Create(nsIImportSettings** aImport)
>+{
>+    NS_PRECONDITION(aImport != nsnull, "null ptr");
>+    if (! aImport)
>+        return NS_ERROR_NULL_POINTER;
Please choose an indent level and stick to it.

>+  const nsCString acctname(".oeaccount");
Don't do this. If you can't use ".oeaccount" then use NS_LITERAL_CSTRING.

>+      nsCOMPtr<nsILocalFile> fileY = do_QueryInterface(fileX);
fileX is already an nsILocalFile...

>+      if (name.RFind(acctname,PR_TRUE,-1,1) != kNotFound)
Do you just need StringEndsWith here?

>+  rv = list->Item(0, getter_AddRefs(domNode));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (domNode){
This probably always succeeds, so just use if (!domNode)
  return NS_ERROR_FAILURE;

>+    rv = domNode->GetFirstChild(getter_AddRefs(domNode));
Don't reuse variables like this.

>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return domNode->GetNodeValue(value);
But consider using GetTextContent instead anyway, which saves a step.

>+nsresult WMSettings::MakeXMLdoc(nsCOMPtr<nsIDOMDocument>* xmlDoc,
>+                                nsILocalFile* file)
MakeXMLdoc(nsILocalFile* aFile, nsIDOMDocument** aXMLDoc) and move the getter_AddRefs to the caller.

>+  nsresult rv;
>+  nsCOMPtr<nsIFileInputStream> stream =
>+    do_CreateInstance(NS_LOCALFILEINPUTSTREAM_CONTRACTID);
>+  NS_ENSURE_STATE(stream);
Could use the , &rv version of do_CreateInstance instead.

>+  PRInt64 filesize;
>+  file->GetFileSize(&filesize);
Might as well do this later, once you have the parser.

>+  parser->ParseFromStream(stream, nsnull, PRInt32(filesize), "application/xml",
>+                          getter_AddRefs(*xmlDoc));
>+  NS_ENSURE_STATE(xmlDoc);
Probably don't need this, just return parser->ParseFromStream directly.

>+    nsCAutoString name;
>+    if(NS_FAILED(file->GetNativeLeafName(name)))
>+      continue;
Where do you use the name?

>+    nsIMsgAccount  *anAccount = nsnull;
nsCOMPtr<nsIMsgAccount> anAccount; (automatically releases; use getter_AddRefs)

>+    if (NS_SUCCEEDED(GetValueForTag(xmlDoc, NS_LITERAL_STRING("IMAP_Server"),
>+        value)))
>+      if (DoIMAPServer(accMgr, xmlDoc, value, &anAccount))
>+        accounts++;
>+    if(NS_SUCCEEDED(GetValueForTag(xmlDoc, NS_LITERAL_STRING("NNTP_Server"),
Nit: space after if

>+                                nsAutoString& serverName,
const nsAString& (or failing that, nsString&)

>+      GetValueForTag(xmlDoc, NS_LITERAL_STRING("IMAP_Root_Folder"), value);
>+      if (!value.IsEmpty())
>+        imapServer->SetServerDirectory(NS_ConvertUTF16toUTF8(value));
Does OE import not do this?

>+      GetValueForTag(xmlDoc, NS_LITERAL_STRING("IMAP_Secure_Connection"), value);
>+      if (!value.IsEmpty())
>+        in->SetSocketType((PRBool)value.ToInteger(&errorCode, 16) && !errorCode ?
>+        nsIMsgIncomingServer::useSSL : nsIMsgIncomingServer::defaultSocket);
Looks overcomplex, since ToInteger always returns 0 when there's an error, so
if (value.ToInteger(&errorCode, 16))
  in->SetSocketType(nsIMsgIncomingServer::useSSL);

>+          account->QueryInterface(NS_GET_IID(nsIMsgAccount),
>+            (void **)ppAccount);
Use account.forget(ppAccount);

>+  nsresult rv = pMgr->FindServer(nsDependentCString(""),
EmptyCString()

>+      GetValueForTag(xmlDoc, NS_LITERAL_STRING("NNTP__Use_Sicily"), value);
Two _s?

Comment 36

[Phil Lacy]

(In reply to comment #35)
> >+    NS_PRECONDITION(aImport != nsnull, "null ptr");
> >+    if (! aImport)
> >+        return NS_ERROR_NULL_POINTER;

I'm going to ignore the hangover nits like this until you have a chance to review with the 'hg copy' removing them. I wanted to comment here lest I forget to mention it when I get the revised patch up.

Comment 37

[Phil Lacy]

At this point should I bother changing the folder name to windowslivemail per comment 32. Not for just clarifying the name but 'Windows Mail is what MS is calling OExpress and they refer to this product as Windows Live Mail.

Comment 38

[Rain [:rain1] (inactive)]

(In reply to comment #37)
> At this point should I bother changing the folder name to windowslivemail per
> comment 32. Not for just clarifying the name but 'Windows Mail is what MS is
> calling OExpress and they refer to this product as Windows Live Mail.

I think so -- Windows Mail was only part of Windows Vista, and is no longer available (as a program) in Windows 7.

Comment 39

[Phil Lacy]

(In reply to comment #35)
> 
> >+      if (name.RFind(acctname,PR_TRUE,-1,1) != kNotFound)
> Do you just need StringEndsWith here?
> 

Docs say external use. I tried it and it does give an error that it's not an internal access API. Is there a way around that?

Comment 40

[[e-mail]]

(In reply to comment #39)
> (In reply to comment #35)
> > >+      if (name.RFind(acctname,PR_TRUE,-1,1) != kNotFound)
> > Do you just need StringEndsWith here?
> Docs say external use. I tried it and it does give an error that it's not an
> internal access API. Is there a way around that?
Try including nsStringGlue.h instead of nsString.h

Comment 41

[Phil Lacy]

(In reply to comment #35)
> >+      GetValueForTag(xmlDoc, NS_LITERAL_STRING("IMAP_Root_Folder"), value);
> >+      if (!value.IsEmpty())
> >+        imapServer->SetServerDirectory(NS_ConvertUTF16toUTF8(value));
> Does OE import not do this?

OE has this same setting. But I'm not sure if I can even test this part. I don't think gmail, my only testing venue, supports different root folders.

Comment 42

[Phil Lacy]

I see where if you use [Gmail] as the root folder then the folders are consolidated under the account root folder rather than having a separate [gmail] folder listed. It appears this code is ok then.

Comment 43

[Phil Lacy]

Attachment: with corrections from r=neil

Should be good to go. I had limited testing since I only have a pop3 accounts and NNTP accounts left over from my atrocious previous install of WLM. IMAP should be good to go.

Comment 44

[Phil Lacy]

Attachment: with corrections from r=neil

The only change here from previous, I omitted the change to jar.mn for suite

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

This includes fairly significant code changes without any automated test coverage.  If this were new code, we definitely wouldn't allow it to land without significant test writing.  However, I want to be sensitive to the fact that Phil submitted his patch LONG before the automated testing policy went into effect, and the reviewing system has done very poorly in rewarding him for his hard work (apologies for that, Phil!).

Phil, would you be willing to write at least some basic automated tests for this code in the next week or so?  (Current schedule has tree closure for 3.1b2 & string/feature freeze for 3.1 scheduled for 3/23).

Comment 46

[Phil Lacy]

I'll look into it to see what I can do. I was hurrying to get the code ready in hopes it would have landed in b1 because I'm sure we could have expected a lot of beta testing of importing windows live mail.

It's something that should be in beta as early as possible.

Comment 47

[Mark Banner (:standard8)]

(In reply to comment #45)
> Phil, would you be willing to write at least some basic automated tests for
> this code in the next week or so?

I'm not 100% convinced automated tests are possible here. I've only scanned the patch briefly, but it seem to me that the tests would require:

a) Windows Mail (or something appropriate installed)
b) Setting up of Windows Mail to a specific installation/profile
c) being able to modify that installation for different tests.

So if I'm right, I'm not sure that this is entirely possible within our current scope of automated tests. We may need to consider some other method for import, but I'll let Phil comment first.

Comment 48

[Phil Lacy]

Yes it would be limited to something that only tests import of an xml document. Locating the xml data file depends on windows registry entries which we we would need and have them point to our test data store.

Import of winmail  probably could be done with an extension. But import is a onetime use feature and the scenario would be, someone downloads TB, goes to import, sees it can't import win live mail, uninstalls TB. Like I done with OE many times since I had many identities/servers/etc.

I don't know what our 'test required' spec is, hopefully not only that.

The rule should state:
 
tests not required for autonomous code or code that only can be broken by breaking upstream code that affects more then one module. This patch would fall under that stipulation since import changes that break this one would break other import modules. 

Code as such should require the patcher to state what tests were done with the patch to ensure it works within itself.

Code should not require testing if it is in beta branch and needs to be beta tested as soon as possible, except litmus tests should be made at earliest chance and bug not closed without it or some other test.

The rule should further clarify that tests would be required if it is remotely possible future code could break it, but 'required' would still be override-able if not doable.

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #48)
> Yes it would be limited to something that only tests import of an xml document.
> Locating the xml data file depends on windows registry entries which we we
> would need and have them point to our test data store.
> 
> Import of winmail  probably could be done with an extension. But import is a
> onetime use feature and the scenario would be, someone downloads TB, goes to
> import, sees it can't import win live mail, uninstalls TB. Like I done with OE
> many times since I had many identities/servers/etc.
> 
> I don't know what our 'test required' spec is, hopefully not only that.

Given that this code is high value, was submitted long before the policy went into place, and is hard to test, I'd suggest that it's entirely reasonable for us to land as-is, with whatever automated test work you're able to get in between now and feature freeze.  You could even land the code once you have review soonish, and land the tests as they get finished and reviewed.

Our policy isn't defined nearly so specifically as to nail down your questions.  The reality is that we almost always require tests, except when the costs outweigh the benefits at the discretion of the reviewer.  I'm not convinced developing a tightly defined policy at this point gets us a lot of benefit, but I could be wrong about that.

That said, some thoughts:

> Code as such should require the patcher to state what tests were done with the
> patch to ensure it works within itself.

This sounds about right.
 
> Code should not require testing if it is in beta branch and needs to be beta
> tested as soon as possible, except litmus tests should be made at earliest
> chance and bug not closed without it or some other test.

I think that's too liberal to be applied generally to new code going forward.  I think reviewers should feel free to make exceptions of that nature if the code is exceedingly high value, low risk, hard-to-test, and/or is being grandfathered in.  A couple of those things apply to this patch.

> The rule should further clarify that tests would be required if it is remotely
> possible future code could break it, but 'required' would still be
> override-able if not doable.

Right, I think the existing "exceptions can be requested by the patcher and granted by the reviewer based on cost-benefit" covers that nicely.

Comment 50

[Phil Lacy]

(In reply to comment #48)
> tests not required for autonomous code or code that only can be broken by
> breaking upstream code that affects more then one module.

The one 'test required' qualifier you didn't address is really the one applicable here. I don't see a way this code can be broke without someone breaking import code that would break all the other import modules which implies there is no benefit to a test.

Besides, I really don't know how to do a test on it ;). Maybe Neil can add some suggestions as to what can be tested to prevent future code.

Maybe our build window boxes will allow us to write to the registry.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #50)
>
> The one 'test required' qualifier you didn't address is really the one
> applicable here. I don't see a way this code can be broke without someone
> breaking import code that would break all the other import modules

Just skimming the patch (and not knowing the import code very well), it certainly looks to me as though most of the methods in nsOESettings.cpp could actually break entirely independently of upstream code.  

I could imagine writing some combination of C++ and JavaScript unit tests to exercise that code.  

Whether that's actually the highest value thing you could devote testing effort to over the next few weeks, I'm unsure.  But more on that in a bit...

> which implies there is no benefit to a test.

I don't agree.  Unless we have unit tests for the upstream code or other importers (which I suspect that don't), we would be likely not to notice that the upstream code is broken.  That said, you might well be onto something that the highest value thing to test write tests for would be the general importing code that's upstream from this.

> Besides, I really don't know how to do a test on it ;). Maybe Neil can add some
> suggestions as to what can be tested to prevent future code.
>
> Maybe our build window boxes will allow us to write to the registry.

Indeed, let's see if we can figure that out.  Earlier, Standard8 said:

> I'm not 100% convinced automated tests are possible here. I've only scanned the
> patch briefly, but it seem to me that the tests would require:
>
> a) Windows Mail (or something appropriate installed)
> b) Setting up of Windows Mail to a specific installation/profile
> c) being able to modify that installation for different tests.

Would taking a few files from an existing install and writing a script to put them into some place appropriate be enough?  Maybe in combination with linking against a tiny stub library containing mock registry reading/writing routines that simply return something hardcoded, from a table, or from some user-specific subtree of the registry that is writable?

Note that I'm completely willing to believe that the exact changes in this patch are hard to test.  But I'm hoping we can think of some sort of low-hanging automated test fruit in or upstream from this code that would help us notice if things went squirrelly in the future.

Comment 52

[Phil Lacy]

I don't know why writing to the registry wouldn't be possible. Someone needs to decide if that is what we want to do to our test machines. If not the next thing I can think of is to do a patch to enable users to select the location of the win live mail file store and use that in a test. This is something they ask for in OE as well.

This patch only addresses settings which all I bothered with since the drag and drop of files is now in place. But users will be complaining that all the read and starred settings are gone, I couldn't find documentation on where winlivemail keeps that data.

Most likely this bug feature won't go far if it depends on me doing a test first. I'm not one to 'knock out' code, so Neil ought to review my changes here so it's ready if you decide to go in without the test.

Comment 53

[Phil Lacy]

And, windows may change the way the data is stored in the registry which would break this code like happened in OE. That was the principle fix I did last year to OE.

Comment 54

[Phil Lacy]

But we can't test for that

Comment 55

[Ben Bucksch (:BenB)]

Phil, dmose already agreed to have this in without tests. I would suggest that you ping Neil to review this without tests, and then file a followup bug to create tests for 1) import in general and for 2) a test of this importer, by importing certain registry keys for various settings from plaintext reg files.

Comment 56

[Joshua Cranmer [:jcranmer]]

With respect to automated testing, some of my thoughts:

1. The last I was told (which was admittedly some time ago) was that automated tests for system-integration components was problematic because results could change based on how the developer sets up their system.

2. It is possible that import code is not fully testable with xpcshell tests for now because it tries to use things from multiple threads which aren't intended to be used in such a manner.

3. A possible way to test this would be to make a hidden pref that controls the registry root you look at. This is more or less what I did when I tried to make tests for an Evolution address book importer.

Comment 57

[Phil Lacy]

Neil's been flagged on the re-review. But I'll remind him tonight on IRC

Comment 58

[Phil Lacy]

I'm don't know how our tests run, is there a #ifdef that can be used to replace windows registry code with an equivalent?

Comment 59

[Ben Bucksch (:BenB)]

Phil, I recommend that you file a new bug to discuss test code.

As for Windows Registry reading, it's trivial to make a test: Just create a file named e.g. foo.reg with the following content:

Windows Registry Editor Version 5.00

[HKEY_CURRENT_USER\SOFTWARE\Policies\Microsoft\Windows\AppCompat]
"VDMDisallowed"=dword:00000001

Save it (as foo.reg). Double-click it or "run" it. You'll have that key in the registry. For your test, you'd of course take the registry keys that Windows Mail writes. I think regedit can even export them (in the plaintext format).
So, when your test starts, you run regedit with that file, using nsIProcess, and then run your importer, and then use regedit to delete that key.
In fact, Mozilla even has an XPCOM interface to the Windows registry, if you want to do the deletion (or even importing) this way.
This way, you need neither Windows Mail installed nor fake anything, and you can test different account settings.

Comment 60

[Phil Lacy]

(In reply to comment #59)
> [HKEY_CURRENT_USER\SOFTWARE\Policies\Microsoft\Windows\AppCompat]
> "VDMDisallowed"=dword:00000001

Ben, is that just an example for your comment or is that a setting that needs to be executed for our test boxes along with my own win live mail settings.

Comment 61

[Ben Bucksch (:BenB)]

That was just an example of a random .reg file. Of course you'd use the values that Windows Mail needs. (The line before that is needed too.) As said, regedit can export that, IIRC, but make sure that you use the plaintext variant, not the binary export.

Comment 62

[Ben Bucksch (:BenB)]

In fact, I explictly wrote:
> For your test, you'd of course take the registry keys that Windows
> Mail writes.

Comment 63

[Phil Lacy]

(In reply to comment #52)
> I don't know why writing to the registry wouldn't be possible. Someone needs to
> decide if that is what we want to do to our test machines.

A simple 'yes' here would have set me free, but thanks for long version, I appreciate the answer either way.

I see what I can do. If I'm spinning my wheels because of the threading in import I'd appreciate a heads up on that as well.

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

The main issue I know of re writing to the registry is that if developers find that they've broken a test, they need to be able to run the test on their own machine repeatedly until they fix it, and we don't want to be inadvertently leaving useless crud in the registry or, worse, actually breaking the machine's functionality somehow.

In an ideal world, I suspect we'd use mock registry reading/writing routines to avoid this issue.

That said, if that turns out to be an impractical amount of work for the real world, we need to be very careful to write into some temporary sub-tree of the registry and not into some place that will mess up regular operation of the machine, and to clean everything up afterwards.

Comment 65

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #63)
>
> I see what I can do. If I'm spinning my wheels because of the threading in
> import I'd appreciate a heads up on that as well.

Can you elaborate on what specifically you're concerned about here?

Comment 66

[Phil Lacy]

Attachment: wip test

test import prefs written my import. needs registry code and data files and cross checks.
uploading so I don't lose it.

Comment 67

[Phil Lacy]

alert! Neil

will be uploading a new patch using nsIWindowsRegKey so a non invasive test can be done.

Should be ready tonight feel free to bump this in your queue.

Comment 68

[Phil Lacy]

Attachment: version to remove native registry functions.

This passes my tests which import my local registry info.  Test should be doable with the nsIWindowsRegKey in place instead of windows API.

Comment 69

[Phil Lacy]

Comment on attachment 432966 [details] [diff] [review]
version to remove native registry functions.

SOME NITS


>+  static void SetSmtpServer(nsIDOMDocument *xmlDoc, nsIMsgIdentity *id,
>+                               nsAutoString& inUserName, PRBool useSecAuth);

line up params, (typ)

>+    return(NS_ERROR_NULL_POINTER);

remove return parentheses (typ)

>+PRBool WMSettings::DoImport(nsIMsgAccount **ppAccount)
>+{
>+  nsresult  rv;

extra space

>+      if (!imapServer){
>+        IMPORT_LOG1("*** Failed to create nsIImapIncomingServer for %S!\n",
>+          serverName.get());

line up

>+  }
>   else if (NS_SUCCEEDED(rv) && in) {
>-    IMPORT_LOG2("Existing POP3 server named: %s, userName: %s\n",
>-                pServerName, pUserName);
>+    IMPORT_LOG2("Existing POP3 server named: %S, userName: %S\n",
>+                 serverName.get(), userName.get());

 
>-        IMPORT_LOG0("Created an account and set the NNTP server as the incoming server\n");
>+        IMPORT_LOG0("Created an account and set the NNTP server"
>+                     " as the incoming server\n");
> 

line up (typ)

Comment 71

[Phil Lacy]

Attachment: test to check import patch, not for checkin

this dumps the prefs imported by the import code.
This may help review on import code as it verifies the import of accounts.

It need finalization where it checks the prefs to a known data set. But in meantime hope this helps.
Also it probably need more inclusive datafiles, such as a imap account or additional prefs. The microsoft site has example data xml files. These should be copyable without an infringement.
--
later

Comment 72

[Phil Lacy]

OESettings.cpp may have gone unchanged for 5 years, nevertheless it is now bitrotted. Will have to fix and resubmit patch or it won't build.

Comment 73

[Phil Lacy]

Attachment: builds to trunk

some bitrot over weekend added to this patch

Comment 74

[Phil Lacy]

Attachment: tests with dump to log showing prefs added by import

still needs actual tests, but this will dump prefs to log for viewing import is working.
mail.account.account1.identities--id1
mail.account.account1.server--server1
mail.account.account2.identities--id2
mail.account.account2.server--server2
mail.account.account3.server--server4
mail.account.account4.identities--id3
mail.account.account4.server--server3
mail.accountmanager.accounts--account1
mail.accountmanager.accounts--account1,account2
mail.accountmanager.accounts--account1,account2,account3
mail.accountmanager.accounts--account1,account2,account3,account4
mail.accountmanager.defaultaccount--account1
mail.accountmanager.localfoldersserver--server4
mail.identity.id2.fullName--test
[e-mail]
mail.identity.id3.fullName--popdisplayname
mail.identity.id3.smtpServer--smtp1
[e-mail]
mail.newsrc_root--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News
mail.newsrc_root-rel--[ProfD]News
mail.root.nntp--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News
mail.root.nntp-rel--[ProfD]News
mail.root.none--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\Mail
mail.root.none-rel--[ProfD]Mail
mail.root.pop3--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\Mail
mail.root.pop3-rel--[ProfD]Mail
mail.server.server1.directory--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News\testmsnews.microsoft.com
mail.server.server1.directory-rel--[ProfD]News/testmsnews.microsoft.com
mail.server.server1.hostname--testmsnews.microsoft.com
mail.server.server1.name--Microsoft Communities Test
mail.server.server1.newsrc.file--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News\testmsnews.microsoft.com.rc
mail.server.server1.newsrc.file-rel--[ProfD]News/testmsnews.microsoft.com.rc
mail.server.server1.type--nntp
mail.server.server2.directory--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News\testnews.mozilla.org
mail.server.server2.directory-rel--[ProfD]News/testnews.mozilla.org
mail.server.server2.hostname--testnews.mozilla.org
mail.server.server2.name--accountnamemozillanews
mail.server.server2.newsrc.file--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\News\testnews.mozilla.org.rc
mail.server.server2.newsrc.file-rel--[ProfD]News/testnews.mozilla.org.rc
mail.server.server2.type--nntp
mail.server.server3.check_new_mail--true
mail.server.server3.defer_get_new_mail--false
mail.server.server3.deferred_to_account--account3
mail.server.server3.directory--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\Mail\pop3.test.test
mail.server.server3.directory-rel--[ProfD]Mail/pop3.test.test
mail.server.server3.hostname--pop3.test.test
mail.server.server3.name--testpopaccountname
mail.server.server3.type--pop3
mail.server.server3.userName--testpopusername
mail.server.server4.directory--f:\mozilla\objdir-debug_tb\mozilla\_tests\mailtest\Mail\Local Folders
mail.server.server4.directory-rel--[ProfD]Mail/Local Folders
mail.server.server4.hostname--Local Folders
mail.server.server4.name--Local Folders
mail.server.server4.type--none
mail.server.server4.userName--nobody
mail.smtpserver.smtp1.hostname--smtp.pop.test
mail.smtpservers--
mail.smtpservers--smtp1

Comment 75

[Phil Lacy]

the import of imap accounts leaks. I can't isolate to this code and the only difference I see so far is imap uses nsIImapIncomingServer in the import code.

Comment 76

[[e-mail]]

Comment on attachment 427975 [details] [diff] [review]
with corrections from r=neil

Would you mind if I reviewed this version first, and then once this is checked in I can review the test changes?

Comment 77

[[e-mail]]

Comment on attachment 427975 [details] [diff] [review]
with corrections from r=neil

>+EXPORTS     = \
>+          nsOERegUtil.h \
>+		  $(NULL)
Nit: please be consistent with tabs.

>-DEPTH		= ../../..
>-topsrcdir	= @top_srcdir@
>-srcdir		= @srcdir@
>-VPATH		= @srcdir@
>+DEPTH       =   ../../..
>+topsrcdir   =   @top_srcdir@
>+srcdir      =   @srcdir@
>+VPATH       =   @srcdir@
Nit: no actual changes here, right?

> 
> include $(DEPTH)/config/autoconf.mk
> 
>-MODULE		= importOE
>-LIBRARY_NAME   = importOE_s
>-ifndef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
>-MOZILLA_INTERNAL_API = 1
>-endif
>-META_COMPONENT	= mail
>+MODULE      =           importWM
>+LIBRARY_NAME   =        importWM_s
>+MOZILLA_INTERNAL_API =  1
>+META_COMPONENT  =       mail
You need to keep the MOZ_INCOMPLETE_EXTERNAL_LINKAGE ifdef.

>-REQUIRES	= xpcom \
...
>-		  $(NULL)
>+REQUIRES    = \
>+                        xpcom \
>+                        $(NULL)
Nit: tabs again, as per the copied file.

>-CPPSRCS		= \
...
>-		$(NULL)
>+CPPSRCS     = \
...
>+                        $(NULL)
Ditto.

>diff --git a/mailnews/import/oexpress/nsOEImport.cpp b/mailnews/import/winlivemail/nsWMImport.cpp
>diff --git a/mailnews/import/oexpress/nsOESettings.cpp b/mailnews/import/winlivemail/nsWMSettings.cpp
Not reviewed yet.

Comment 78

[Phil Lacy]

(In reply to comment #76)
> (From update of attachment 427975 [details] [diff] [review])
> Would you mind if I reviewed this version first, and then once this is checked
> in I can review the test changes?

NP. I seem to have problems with my getOEacctFiles apparently not picking up all the .oeaccount files.

I think you can get the review out of the way but I need to look at that before check-in.

Comment 79

[Ben Bucksch (:BenB)]

WinMail docs say: "IMAP_Secure_Connection    DWORD   0   Specifies whether to use a Secure Sockets Layer (SSL) connection to the IMAP server."
http://msdn.microsoft.com/en-us/library</ms715237%28VS.85%29.aspx#ww_accounts_data>

Does that refer only to SSL or also STARTTLS? I assume only SSL. What about STARTTLS? Does OE/WinLive try that automatically (when offered) always? Or how would it be configured?

Comment 80

[Phil Lacy]

I added a microsoft's sample files and the importer seems to have problem with it's imap oeaccount so it isn't my getOEacctFiles(). 

This problem shouldn't affect review.

Comment 81

[Phil Lacy]

Attachment: added more sample account files

adding the sample files microsoft posts on their site.

earlier problem with data files was due to missing '<' in a closing tag.

only dumps the prefs, still doesn't do tests at this point. will add later. Once users have their chance at import we may have more sample files to add to the test. Just need to add to the resource directory.

Comment 82

[Phil Lacy]

Attachment: actual test

this test does a check but it has the data in the code file which is a little clunky to say the least. I added a comment it needs a better check.
Probably a binary data file in case future files added to the test have invalid string characters.

But it's not entirely unreadable with the data string included... and it passes.

Comment 83

[Phil Lacy]

(In reply to comment #77)
> (From update of attachment 427975 [details] [diff] [review])
> >+EXPORTS     = \
> >+          nsOERegUtil.h \
> >+		  $(NULL)
> Nit: please be consistent with tabs.

nsOERegUtils is replaced and removed here. The latest is attachment 433806 [details] [diff] [review]

Comment 84

[[e-mail]]

(In reply to comment #79)
> Does that refer only to SSL or also STARTTLS?
I don't suppose Windows Mail is that much different from Outlook Express which only supports STARTTLS for SMTP and only SSL for POP3/IMAP.

Comment 85

[[e-mail]]

Comment on attachment 433806 [details] [diff] [review]
builds to trunk

>-  return( NS_ERROR_NOT_AVAILABLE);
>+  return NS_ERROR_NOT_IMPLEMENTED; // TODO: ? return( NS_ERROR_NOT_AVAILABLE);
[Might as well not change this.]

>+{return NS_ERROR_NOT_IMPLEMENTED;
Nit: please put the { and return on separate lines

>+  value.Truncate();
[Don't need to do this.]

>+  nsCOMPtr<nsIWindowsRegKey> key;
>+  if (NS_FAILED(FindWMKey(key))) {
I don't like the way FindWMKey writes to its parameter. Can you create the registry key object here and just pass it as an nsIWindowsRegKey* please.

>+  if (key->OpenChild(NS_LITERAL_STRING("mail"), nsIWindowsRegKey::ACCESS_QUERY_VALUE,
>+                      getter_AddRefs(subKey)) == ERROR_SUCCESS) {
Hmm, does nsIWindowsRegKey return Windows or nsresult codes?

>+  rv = key->ReadStringValue(NS_LITERAL_STRING("Store Root"), storeRoot); // ReadStringValue(NS_LITERAL_STRING("Store Root"), storeRoot, aExpand = PR_TRUE);
>+                                                                         //  <<<<<<<<<<<<<<<<     NEIL. WOULD BE NICE TO HAVE THIS AS OPT FLAG IN nsIWindowsRegKey  
You'll need to file an XPCOM bug, I'm afraid.

>+  if (temp.GetMutableData(&buffer, size - 1 ) == 0)      // <<<<<<<<<<<<<<<<     NEIL.  NOT AS UNWIELDLY AS BeginWriting. Not aware of external portability issues
Not portable to the external API, unfortunately. SetLength(size - 1) and BeginWriting is the only portable way I know.

>+  nsCOMPtr<nsIMutableArray> fileArray(do_CreateInstance(NS_ARRAY_CONTRACTID));
This should be an nsCOMArray<nsILocalFile>

>+  nsCOMPtr<nsISimpleEnumerator> entries;
>+  fileArray->Enumerate(getter_AddRefs(entries));
This should be a simple for loop.

>-  return( accounts != 0);
>+  return(accounts != 0);
Nit: "noise" changes make the patch look bigger than it needs to.

Comment 86

[Phil Lacy]

Attachment: changes per review

should have the nits fixed... plus a few more I noticed. I used the BeginWriting that uses iterators. Let me know if there is a later version we are using.

Comment 87

[Mark Banner (:standard8)]

Comment on attachment 439814 [details] [diff] [review]
changes per review

> # Short name of import module
>-## @name OEIMPORT_NAME
>+## @name WMIMPORT_NAME
> ## @loc None
> ## LOCALIZATION NOTE (2000): DONT_TRANSLATE
>-2000=Outlook Express
>+2000=Windows Live Mail

You need to give all these strings completely different numbers, otherwise L10n won't pick up the changes. You also need to update the localization note numbers as well.

Is this breaking import from Outlook Express or is that still possible? If so, I think you should be doing a combined name rather than just windows live mail.

Comment 88

[Mark Banner (:standard8)]

(In reply to comment #87)
> (From update of attachment 439814 [details] [diff] [review])
> > # Short name of import module
> >-## @name OEIMPORT_NAME
> >+## @name WMIMPORT_NAME
> > ## @loc None
> > ## LOCALIZATION NOTE (2000): DONT_TRANSLATE
> >-2000=Outlook Express
> >+2000=Windows Live Mail
> 
> You need to give all these strings completely different numbers, otherwise L10n
> won't pick up the changes. You also need to update the localization note
> numbers as well.

Sorry, I missed the fact that the patch has this file being copied and then updated. You can ignore that.

Comment 89

[David :Bienvenu]

Neil, is there any chance you can finish this review today? We have a string freeze for 3.1 tonight...

Comment 90

[Phil Lacy]

Comment on attachment 439814 [details] [diff] [review]
changes per review

>+  PRUnichar *buffer = nsnull;
remnant from last review comments. needs to be removed

Comment 91

[[e-mail]]

If the strings are a sticking point, they could be checked in now, if you like.

Comment 92

[David :Bienvenu]

(In reply to comment #91)
> If the strings are a sticking point, they could be checked in now, if you like.

That only makes sense if we're pretty sure we can get the rest of the patch in in the next couple days...

Comment 93

[Simon Paquet [:sipaq]]

This patch didn't make it for the string freeze. Adding late-l10n keyword and informing Thunderbird localizers.

Developers should be aware of the process that we have for this, which is detailed at https://developer.mozilla.org/en/Thunderbird_Localization#Breaking_the_string_freeze

Comment 94

[Simon Paquet [:sipaq]]

Comment on attachment 439814 [details] [diff] [review]
changes per review

Many localizers will not notice string changes if the string identifier isn't changed. That may be a bit less likely here, since you also rename the string file, but we should still rename the string identifiers in the three cases, where they are affected.

>diff --git a/mail/locales/en-US/chrome/messenger/oeImportMsgs.properties b/mail/locales/en-US/chrome/messenger/wmImportMsgs.properties
>
> # Short name of import module
>-## @name OEIMPORT_NAME
>+## @name WMIMPORT_NAME
> ## @loc None
> ## LOCALIZATION NOTE (2000): DONT_TRANSLATE
>-2000=Outlook Express
>+2000=Windows Live Mail

Here.

> # Default name of imported addressbook
>-## @name OEIMPORT_DEFAULT_NAME
>+## @name WMIMPORT_DEFAULT_NAME
> ## @loc None
>-2006=Outlook Express Address Book
>+2006=Windows Live Mail Address Book

Here.

> # Autofind description
>-## @name OEIMPORT_AUTOFIND
>+## @name WMIMPORT_AUTOFIND
> ## @loc None
>-2007=Outlook Express address book (windows address book)
>+2007=Windows Live Mail address book (windows address book)

And here.

Comment 95

[Pavel Cvrcek [:JasnaPaka]]

(In reply to comment #93)
> This patch didn't make it for the string freeze. Adding late-l10n keyword and
> informing Thunderbird localizers.

Looks like you didn't add keyword. Fixed.

Comment 96

[Simon Paquet [:sipaq]]

(In reply to comment #95)
> Looks like you didn't add keyword. Fixed.

Thanks Pavel!

Comment 97

[[e-mail]]

(In reply to comment #94)
> That may be a bit less likely here, since you also rename the string file
Actually we copy it. Will the l10n tools just see it as a new file in en-US?

Comment 98

[Simon Paquet [:sipaq]]

Ok, then I've misread the patch.

If you copy it, it will look to localizers and their l10n tools as a new file, so please ignore my previous comments.

Comment 99

[[e-mail]]

Comment on attachment 439814 [details] [diff] [review]
changes per review

>-  return( NS_ERROR_NOT_AVAILABLE);
>+  return( NS_ERROR_NOT_AVAILABLE);
Nit: stray DOS line ending

>+  nsAString::iterator begin;
>+  nsString expandedStoreRoot;
>+  expandedStoreRoot.SetLength(size - 1);
>+  expandedStoreRoot.BeginWriting(begin);
>+  if (begin.size_forward() != size - 1)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  ::ExpandEnvironmentStringsW((LPCWSTR)storeRoot.get(), (LPWSTR)begin.get(), size);
Sorry, I forgot there are several BeginWriting methods. I should have made it clear that you need to pass (LPWSTR)expandedStoreRoot.BeginWriting() to ExpandEnvironmentStringsW. (Actually BeginWriting(iterator) is deprecated.) r=me with that fixed. If you still want to check that SetLength worked then the most portable way is to check the return value of Length().

Comment 100

[David :Bienvenu]

Comment on attachment 433850 [details] [diff] [review]
actual test

I can review the test...

Comment 101

[David :Bienvenu]

And I'd be happy to sr the final patch, once you address Neil's last comments, Phil.

Comment 102

[Phil Lacy]

won't be at computer until 8h from now but it should be a quick fix and upload.
(reminder to see comment 90)

Comment 103

[David :Bienvenu]

Phil, I'm going to try to get this in the next hour or two...if I understand Neil's second comment correctly...

Comment 104

[David :Bienvenu]

Attachment: tweaked patch

this fixes Neil's two nits. This diff is bigger because it treats the new files as new files instead of copies of the OE files...

Unfortunately, the unit test fails, which I'm looking into.

Comment 105

[David :Bienvenu]

Attachment: non-matching test results

these are the test results I got...does this test still work for you, Phil?

Comment 106

[Phil Lacy]

Attachment: final version r+neil

Comment 107

[David :Bienvenu]

Comment on attachment 440932 [details] [diff] [review]
final version r+neil

sr=me. But, do we need a new version of the test patch?

Comment 108

[Phil Lacy]

Attachment: tests with more data

Comment 109

[Phil Lacy]

for record, your failing test was due to me adding more mock winmail data files but not adjusting the test to suit.

New test has the test updated to the added data files.

Essentially, and per comments in test, if you add more mock winmail files then you need to recapture the output and change the comparison data in the test.

This should allow us to add any new xml files if we have bugs on the import code later.

Comment 110

[Mark Banner (:standard8)]

Attachment: Corrected test

I took a look at the test - it was still failing for me. Turns out the issue was that some of the prefs being checked were file locations which weren't relative. As tests can be run on different configurations, these would just fail as soon as we move to a different machine.

I've therefore added an exclusion set which we don't check. That is ok, as there are also relative versions of those prefs which we can check successfully.

Phil, I would also strongly suggest that you rework the matching function so that it is comparing prefs with an object, or something like that. With the current method it is extremely difficult to detect where there are differences in the preferences, if for instance, just one pref was broken on the import.

However I'm going to check this all in as-is as we need to get this bug landed.

Comment 111

[Mark Banner (:standard8)]

I noticed one issue in the strings that I fixed on checkin: WMIMPORT_MAILBOX_SUCCESS(2002) was using %S and %d, if l10n use those replacements and change the order then we'll crash if they don't change them to %1$S and %2$d. Therefore I just changed the notes and strings to use %1$S and %2$d as that means that l10n should pick them up correctly.

All checked in: http://hg.mozilla.org/comm-central/rev/d58818b1fb31

I'm just going to let this cycle before resolving as fixed and posting to md.l10n.

Comment 112

[Phil Lacy]

Thanks for catch and fix on the paths

(In reply to comment #110)
> Phil, I would also strongly suggest that you rework the matching function so
> that it is comparing prefs with an object, or something like that. With the

I wanted to test it in the loop but quicker was more important at the time. I'll look into updating it in near future and do it in another bug so we can close this one when you're ready

Comment 113

[Mark Banner (:standard8)]

Phil, I'm still having a few troubles fixing the unit test up. If you're able to can you take a look at the Thunderbird3.1 tree?

http://tinderbox.mozilla.org/Thunderbird3.1/

I've just added a bit which adds in that dump statement you had commented out - hopefully that will help us resolve the final bit once the box cycles again.

Comment 114

[Phil Lacy]

taking test problems to Bug 561422

Comment 115

[Mark Banner (:standard8)]

The test has been disabled for now, and as Phil says, we'll work on it in bug 561422. Therefore marking this bug as fixed.

Comment 116

[Ben Bucksch (:BenB)]

Thank you and congrats Phil!

Comment 117

[Phil Lacy]

goto Bug 561562 for message import