User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12 Build Identifier: Current Thunderbird versions do not allow the user to select a port when using the Account Wizard. More POP3 and IMAP4 servers are running on secure ports (993, 995). This extension checks based on the host name whether the account is IMAP4 or POP3 and detects the Port and Security settings (TLS, SSL). The extension is a proof of concept and would be a good starting point for a real implementation. -Brian Reproducible: Always Steps to Reproduce: 1. Launch the Account Wizard 2. Enter an IMAP4 or POP3 server that only runs on secure ports. 3. Complete the Account Wizard 4. Try to download mail . Actual Results: An error will be raised unable to connect to server. The user must then manually edit the Account Settings to set the correct security and port information.

Here is the source code for the example Account Wizard Auto Discovery XPI.

David, this is the extension we were talking about.

My first inclination was to add these capabilities to the core protocol handlers, who know how to parse capabilities, etc, but I'm warming to the approach in this bug of just doing it from js, since it doesn't touch the core code. I'll try to make a patch where this is integrated into the account wizard, and test it out.

Hi Brian K, didn't notice it was you at first :-) I've tried the xpi, and it seems to install and work, though I haven't tried it with a real server. It adds an "auto discover" button to the page where you give the server type (imap or pop3) and name. I would think we would do auto-discovery automatically (perhaps when the user clicks the next button?) but cc'ing Bryan for his UE input.

related to bug 342242?

related but not the same, since this xpi, at least, does port probing and capability testing, not auto-config via dns

This moves the autoconfigure stuff into the core account wizard. It works like the extension, in that you click an autoconfigure button to make us do port and capability probing for imap/pop3. I've got it so it works in TB, but I have a little work to do to get it working for SM (mostly string changes). Hacking the account wizard is never fun, and I may not be putting things in all the right places. I'm going to make a stab at making a patch that will work with SM and then run it past Neil.

Neil, I've basically taken the xpi code and moved it directly into the account manager. I've tried to put things in reasonable places. I didn't create a whole new dtd for the AutoConfigureDialog code (I'm not sure that should even be a separate dialog, but it does allow for "Cancel" and "Apply"). The patch seems to work and is a start on doing more autoconfig stuff. It should probably be extended for SMTP as well.

Not having reviewed the UI or the cod, I think this is extremely valuable work, and setting flags to indicate such.

Comment on attachment 328483 [details] [diff] [review] take a stab at working w/ suite OK, so I took this patch for a spin, and while I like the idea, I think it needs some serious UI love, so I'm disappointed that Bryan hasn't commented yet ;-) The first thing I noticed was that autoconfigure doesn't work while you're offline, but there's no feedback for that. It also doesn't work when there's no hostname, instead presenting an alert; I suggest both conditions should simply disable the button (which by the way needs an accesskey). I don't like the way that autoconfigure results are static, but there's work to allow manual entry of the port and socket type, so we should coordinate. I don't like the design of the autoconfigure dialog, but maybe we can redesign or eliminate it. I don't really understand the networking code.

(In reply to comment #10) > (From update of attachment 328483 [details] [diff] [review]) > OK, so I took this patch for a spin, and while I like the idea, I think it > needs some serious UI love, so I'm disappointed that Bryan hasn't commented yet > ;-) Yes, I'm a little reluctant to spend a lot more time on it w/o getting that UI love :-) > > The first thing I noticed was that autoconfigure doesn't work while you're > offline, but there's no feedback for that. It also doesn't work when there's no > hostname, instead presenting an alert; I suggest both conditions should simply > disable the button (which by the way needs an accesskey). Those I can do. > > I don't like the way that autoconfigure results are static, but there's work to > allow manual entry of the port and socket type, so we should coordinate. > > I don't like the design of the autoconfigure dialog, but maybe we can redesign > or eliminate it. That's something I'm interested in Bryan's opinion on. I'm torn about making it a dialog or not (i.e., automatic or explicit). I believe mail.app does it automatically. It would be a bit more work to make it happen automatically in the account wizard...we'd want to have a way for the user to cancel the process, for example, which makes doing it on "Next" a little odd. > > I don't really understand the networking code. > Conceptually, all the network code does is connect to different ports on the server, and if successful, sends the appropriate command to get the server to respond with capabilities, to see if STARTTLS is enabled, and then quits/logs out. But looking at it a bit more, I could clean it up a little...

I'm going to try this extension out to see how it works, we do have a wiki page for improvements to the account wizard. See page 3 especially: http://wiki.mozilla.org/MailNews:Account_Wizard:Email#Page_3

We should probe 587 as SMTP port too if not doing so already.

Yes, see #c8 - but I am thinking of taking a quick whack at that.

I've updated my account wizard mockups to match the wiki page, take a look at that in general and let me know what you think. This may need to be broken into other bugs, but I think it is all connected to get the user experience right. I've made the auto-detect more inline/automatic about checking the hostname, but we should discuss issues related to network timeouts or other problems. It seems like we should be able to keep checking (on input) for valid hostnames and then probe the hostname. Note the spinner in there so the person can have some idea of network activity going on. There's a possibility of a person losing entered information when the probe results return (perhaps taking a while). One option could be to make the entries insensitive, but that would likely require us to let the person cancel the probing. Instead I think we can just create an undo-scheme where probed results return and we show an undo link besides the spinner for getting back whatever was in the entries before we auto-filled them. (still need to work on this - not in mockups) Weave uses a system very similar to this for password checking in their wizard code. http://hg.mozilla.org/labs/weave/index.cgi/file/c8bce0724360/chrome/content/ Mockups are in my account wizard folder here, with relevant links below; feedback appreciated. http://clarkbw.net/designs/account-wizard/auto-config/ IMAP w/ auto-detection http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-pre-auto-detection.png http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected.png POP w/ auto-detection http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-pop-pre-auto-detection.png http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-pop-auto-detected.png

Note, I seemed to have gotten my spinners mixed up in the pre vs. post detection and that's fixed now. Also added some prototype mockups with 'undo'. This would revert the settings back to whatever they were before the auto-detect changed them. http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected%20(with%20undo).png As well as one with a 'try again'. This action would appear after any attempt to auto-detect failed (like offline) or after an undo. We could try to have both actions available as there is a choice, but I'd like to be a bit minimal and the actions will appear in the same location so you could click twice for undo, try again. http://clarkbw.net/designs/account-wizard/auto-config/accont-wizard-p3-imap-auto-detected%20(with%20try%20again).png

Oh, and bug 326076 and bug 221030 have some code for using a menu list or radio group, though not exactly what is specified on the wiki page.

I took a stab at extending the auto discovery code to handle smtp. I haven't hooked it up to the account wizard, so it's completely untested. I won't be able to test it too much, since my ISP blocks SMTP servers other than its own, and doesn't support STARTTLS.

Brian, what would trigger the initial autoconfigure in the proposed UI? I assume the try again link is for subsequent autoconfigure attempts. Would it start off as a "configure" link or button? Apolgies if it's already in a screen shot... I'm not sure undo is going to be that useful

I was suggesting we watch the text input box for a valid hostname and when the person has paused/finished typing. Though there can be lots of valid looking hostnames unintentionally created, for instance when a person starts entering the initial imap of imap.gmail.com; imap could be considered valid. So there might be some spinning during typing. The 'try again' would only appear after failure to successfully return information and if the person hadn't typed anymore information. (for a network down case)

I don't think we easily know when the user has finished typing (Neil can correct me if I'm wrong). I suppose we could use timers and check if the text field value has not changed in a few seconds...We know when the text widget loses focus, but it's very likely that it will lose focus because the user has clicked the "Next" button. I worry about the spinning when typing, because I know when I'm adding a new account, I'm usually referring back and forth between the account wizard and an e-mail or web page that tells me the host name, which means it looks like I've stopped typing. Completely unrelated comment - we also want to check for secure auth capabilities, and set that for the user if the server supports a secure authentication mechanism.

I suggest start detecting after user press next button and show of page of what we detected (if any) and in case user misspells name of server it can always come back at retype name of server press next again.

I think it would have to be based on a timer a second or two after typing. Likely the real value can only be determined with some testing. Not that user correction are going to be that likely since we should be detecting for the most secure possible connection and defaulting to that. (as a side note I wondered if we should be keeping all possible valid connections for the person to choose) However we want to show that we've found the right setup before they leave the page. I think bienvenu's comment about how people enter this information is completely correct, and what we're targeting. A person is probably looking at something else for the information and so after typing in the hostname has to read the port / security connection type from the same place. During the time they are reading t-bird should have auto-detected the settings.

Over irc, we discussed taking advantage of autoconfig to simplify the process - Bryan posted this for a first page - http://pastebin.mozilla.org/498008 The idea is that we'd do auto-config when the user presses next from this screen, and figure out if the server is imap or pop3, supports tls or ssl, and whether it supports secure authentication. If authentication fails, we tell the user and allow them to correct the password or server name. Bryan also suggested on the final page, we give the user the opportunity to remember the password, with a check box, turned on by default. One issue is that we need the user name to logon with. We could try both the full e-mail address and the part before the @ as user names, but guessing about user name isn't ideal, when we'll be sending a password along with it. And there are situations where the username is neither. If we ask for the username explicitly, this page starts to overflow. We could make the account wizard a little bigger. An other possibility is to break auto config into two steps - in the first, the user enters their identity name, e.g., John Smith, along with the incoming server name and outgoing server name. Then we autoconfig the servers, making sure we can connect to them, figuring out a port and corresponding server type (IMAP vs. POP3), and connection type (SSL/TLS, etc). Then, on the next screen, the user enters their email address, user name, and password, and we verify that all works, and figure out if we should use secure auth. Then, the user would go to the verification page. There might be edge cases, e.g., we can connect to the server on the imap port, but the user is only enabled for pop3 access, and we've preferred imap over pop3. If we have a way for the user to go into edit settings despite errors, that would be one work around. Or we could detect that both protocols work and let the user pick. What we need to enable is account setup even when autoconfig fails. Or a way to bypass autoconfig, perhaps to go straight to account settings - power users have asked for that, so we could solve two problems at once.

Can we also detect identity name of current logged in user (John Doe) on Windows platform too? I know we had this functionality on Linux at least but never seen such thing on Windows platform.

This starts in the direction Bryan has proposed - ask for person's name and e-mail address, and try to figure out mail server name, type (IMAP or POP3) and connection type (TLS, SSL, plain). It shows a progress button, and tells you which host its trying. The dialog is messy, and I left in the autoconfigure button for ease of testing. I'm going to hand this off to DavidA, and go work on the second part of the backend for this; once we've discovered a server, type, and port, verify that the user can logon to the server with the password they give us, and determine the most secure authentication mechanism that will work with that server. I'll probably add a method to nsIMsgIncomingServer that does this, so the front end code would create an incoming server object and call the logon method. In order to make it easier to communicate results back to the front end, I may make the calling js poke the use secure auth property on the server object directly, try that logon, if it fails, fall back to insecure auth. The logon method will take a url listener, so the calling code will get called back with success or failure, once we've tried to logon. That's all speculation at this point; I need to start implementing it to see what's going to work in the backend...

Attaching this for sharing w/ ben bucksch and bienvenu, for discussion. Not ready for review.

As promised, here's the JS class I propose to hold the account data, within the dialog. One instance would hold the values shown in the UI, and various autoconfig functions would return instances, and there'll be an createAccount function which takes that and creates the account in the backend.

this doesn't quite work yet, and I had to actually create the imap server object w/ the account manager in order to get the logon verification url to run (it doesn't create the account, but we will need a way to clean up the server object after we're done with it - I'll probably need to add a method to the account manager to remove the server object, and we will need to be very dilligent about cleaning up after ourselves). I have to write similar code for POP3, and SMTP, as well as getting this to work correctly for IMAP (I think the problem is in my test harness, so converting over to David's code would probably help)

Cool, I'll try to merge my front-end code with the back-end part of this patch, hopefully tomorrow. I might even try it with an hg clone of comm-central, which is probably where we need to end up anyway.

TODO (to myself) from that last bug: figure out where port 587 fits in.

Port 587, as described in RFC 4409, is the standard port for mail submission. Traditionally, port 25 was used for mail submission, but it is now good practice for ISPs to block outbound port 25 at their firewalls as a spam prevention mechanism, so 25 just isn't viable for traveling users or users on a random WiFi network. There is also use of port 465 for SSL submission although this port isn't even registered for that purpose. RFC 5068, an Internet Best Current Practice states: MUAs SHOULD use the SUBMISSION port for message submission. So my advice would be to use 587 if it works, and fall back to port 25 if it doesn't. Also, if you ever see STARTTLS advertised in response to EHLO, attempt to negotiate it and if it succeeds, require STARTTLS for all future connections. I don't think it's worth probing 465 since it's non-standard (and could be something else).

FYI, I've created a c-c clone at http://hg.mozilla.org/users/dascher_mozilla.com/autoconfig/, and committed the backend parts of davidb's last two patches to it. I'll be doing my front-end work there, but most likely won't have anything committed until early next week. (I don't care where we do this, but I wanted to learn a bit about hg clones, so this seemed a good practice area) Also FYI, rough front-end code is currently in an extension at http://hg.mozilla.org/users/dascher_mozilla.com/emailconfig/ but it's not worth looking at yet.

(In reply to comment #35) > Traditionally, port 25 was used for mail submission, but it is now good > practice for ISPs to block outbound port 25 at their firewalls as a spam > prevention mechanism I wouldn't call blocking port 25 by providers "good practice" as it prevents use cases for people running their own legitimate mail servers, which may be forced to go with (higher priced, but otherwise identical) business accounts then to avoid that block. Anyway, that's beyond the scope of this RFE... > I don't think it's worth probing 465 since it's non-standard (and could be > something else). While port 587 is now used by many if not most providers, some providers offer 465 for SSL. Thus, it should be probed if 587 and 25 fail or if neither of them offers TLS for encryption, and 465 with SSL tested as a fallback otherwise.

this shows how to create a server object, init it, and call verifyLogon on it.

I will land this in your/our repository, David, once I've figured out a way of testing it a little. Normal message sending still works. I've mostly restrained myself from cleaning up the code around the stuff I added, but I did change the password stuff to use (ns)ACString, and I cleaned up whitespace in places

I should also mention that I did land an additional change to the imap code in the repository to suppress error messages during logon verification.

- I refactored the wizard dialog's JS code a lot - Added AccountConfig - Rewrote most of emailWizard.js - Wrapped guessConfig to return an AccountConfig and be UI-less. - Rewrote most of verifyLogon() (now in verifyLogin.js) to be UI-less and take an AccountConfig. - Moved accountCreation.xul/js to emailWizard.xul/js and autoconfig to guessConfig.js I left the old files in place, so you can just change mailnews/base/prefs/resources/content/accountUtils.js - window.openDialog("chrome://messenger/content/accountcreation/emailWizard.xul", + window.openDialog("chrome://messenger/content/accountcreation/accountCreation.xul", to get the "old" dialog and code back. My changes are mostly dry-coded and not much tested, DOES NOT FULLY WORK YET. Outstanding: - Test, fix - onblur is called twice - Re-add gussing of username and secure auth (see verifyLogin.js, move to guessConfig.js) - fetchConfig.js implementations I hope to get the code fixed and working soon. In the meantime, David A., can you take a look at it and see whether you like it? bienvenu, if you want to continue work on autoConfig.js / guessConfig.js, go ahead, either file will be fine, as I didn't touch the autoConfig.js code, just copied it and added a wrapper at the top, so merging should be trivial. Note: - log4moz.js is not built by default and breaks the whole dialog when it's not there - I get verifyLogon is not a function(). I guess I need to update and rebuild.

setting target milestone to b1

Switching for b1 flags to target milestones, to avoid flag churn.

These are the set of backend changes we've made for auto config. They fall into three main areas: 1. Adding verifyLogon methods for imap,pop3, and smtp. These methods verify that with the passed in password, the user can logon to the server. The process should not alert the user or failure, nor put up a password prompt. 2. Don't put up a password if the url is running without a msgWindow, i.e., is a biff url. (I may need to make sure we don't forget passwords when biff urls are run, but I think that's going to be part of a rework of how we forget passwords). 3. Add the ability to remove a server from the account manager, since the new account wizard has to create temporary server objects. Apologies in advance if I've messed this patch up - I had to edit the diff by hand quite a bit to get a diff between the two repositories.

Comment on attachment 337116 [details] [diff] [review] backend changes for logon verification >+ /* dead code. */ > readonly attribute boolean isSecureServer; Is there a bug to remove it then? >+ void verifyLogon(in ACString aPassword, in nsIUrlListener aUrlListener); All the implementations of verifyLogon simply set the password immediately. IMHO it makes more sense for the caller to set the password separately. >+ // invalidate the FindServer() cache if we are removing the cached server >+ if (m_lastFindServerResult) { >+ nsCString cachedServerKey; >+ rv = m_lastFindServerResult->GetKey(cachedServerKey); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ if (serverKey.Equals(cachedServerKey)) { Is there any reason why we can't compare the pointers? >- // clear out all identity information if no other account uses it. >- if (!identityStillUsed) >- identity->ClearAllValues(); >+ identity->ClearAllValues(); // clear out all identity information. I think there are people who tweak their prefs to share identities... >+ void verifyLogon(in ACString aPassword, in nsIUrlListener aUrlListener); (Same as above) > if (incomingServerToUse) > { > nsCString tmpPassword; > nsresult rv = incomingServerToUse->GetPassword(tmpPassword); >- *aPassword = ToNewCString(tmpPassword); >+ aPassword = tmpPassword; > return rv; > } Will this simplify to return incomingServerToUse->GetPassword(aPassword); ? >+ rv = NS_MsgBuildSmtpUrl(aFilePath, smtpServer, > aRecipients, aSenderIdentity, aUrlListener, aStatusFeedback, > aNotificationCallbacks, &urlToRun, aRequestDSN); // this ref counts urlToRun > if (NS_SUCCEEDED(rv) && urlToRun) > { > nsCOMPtr<nsISmtpUrl> smtpUrl = do_QueryInterface(urlToRun, &rv); > if (NS_SUCCEEDED(rv)) > smtpUrl->SetSmtpServer(smtpServer); Now you're passing the smtpServer to NS_MsgBuildSmtpUrl, it should set it directly on the created smtpUrl so that the callers don't have to. >- if (aRecipientsList) >- *aRecipientsList = ToNewCString(m_toPart); >- return NS_OK; >+ if (aRecipientsList) >+ *aRecipientsList = ToNewCString(m_toPart); >+ return NS_OK; You forgot to diff -w ;-) >+ else if (!PL_strcasecmp(m_urlidSubString, "verifyLogon")) Nit: you appended it as "verifylogon" >+ nsCString escapedUsername; >+ *((char**)getter_Copies(escapedUsername)) = nsEscape(popUser.get(), url_XAlphas); Aren't we supposed to use the extenal escape API? Anyway, you mean .Adopt >+ char * urlSpec = PR_smprintf("pop3://%s@%s:%d/?verifyLogon", >+ escapedUsername.get(), popHost.get(), popPort); >+ if (!urlSpec) >+ return NS_ERROR_OUT_OF_MEMORY; >+ nsCOMPtr<nsIURI> url; >+ rv = BuildPop3Url(urlSpec, nsnull, popServer, aUrlListener, >+ getter_AddRefs(url), nsnull); >+ PR_Free(urlSpec); Nit: PR_smprintf_free (or whatever it's called)

I've filed Bug 453979 to remove isSecureServer, which BenB noted as dead code in the idl, and I verified earlier that it is indeed not used. >All the implementations of verifyLogon simply set the password immediately. >IMHO it makes more sense for the caller to set the password separately. I was trying to maintain a bit of flexibility about how the password gets set but if we need that, I can always put it back the way it is. >>- // clear out all identity information if no other account uses it. >>- if (!identityStillUsed) >>- identity->ClearAllValues(); >>+ identity->ClearAllValues(); // clear out all identity information. >I think there are people who tweak their prefs to share identities... This is a bad diff on my part. I should have diffed against a completely clean tree...the code that looks like its getting removed is actually getting added, so that people who tweak their prefs to share identities won't have them cleaned out :-) I'll attach a new, I hope, clean, patch in a bit...

This attempts to address Neil's comments. I also made nsMsgAccountManager::SetLastServerFound void, since it never returned an error, and that let me cleanup a little bit of code.

Comment on attachment 337493 [details] [diff] [review] logon verification backend changes addressing Neil's comments >+NS_IMETHODIMP >+nsMsgAccountManager::RemoveIncomingServer(nsIMsgIncomingServer *aServer, >+ PRBool aCleanupFiles) >+{ >+ nsCString serverKey; >+ nsresult rv = aServer->GetKey(serverKey); >+ NS_ENSURE_SUCCESS(rv,rv); nit: space missing (there are several of these throughout the patch). >+ m_incomingServers.Remove(serverKey); >+ >+ nsCOMPtr<nsIMsgFolder> rootFolder; >+ aServer->GetRootFolder(getter_AddRefs(rootFolder)); >+ nsCOMPtr<nsISupportsArray> allDescendents; >+ NS_NewISupportsArray(getter_AddRefs(allDescendents)); >+ rootFolder->ListDescendents(allDescendents); Should we be checking these for failure / null pointers? >+ PRUint32 cnt =0; nit: space missing >+ rv = allDescendents->Count(&cnt); >+ NS_ENSURE_SUCCESS(rv, rv); >+ for (PRUint32 i = 0; i < cnt;i++) nit: space missing. >+ { >+ nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(allDescendents, i, &rv); >+ folder->ForceDBClosed(); You've got the value of rv, but you're not checking it... > NS_IMETHODIMP >diff -uwr ./base/util/nsMsgMailNewsUrl.cpp /autoclone/mailnews/base/util/nsMsgMailNewsUrl.cpp >--- ./base/util/nsMsgMailNewsUrl.cpp Fri Aug 29 08:26:35 2008 >+++ /autoclone/mailnews/base/util/nsMsgMailNewsUrl.cpp Thu Aug 28 12:50:27 2008 >@@ -202,11 +202,9 @@ > NS_ENSURE_ARG_POINTER(aMsgWindow); > *aMsgWindow = nsnull; > >- // note: it is okay to return a null msg window and not return an error >- // it's possible the url really doesn't have msg window > nsCOMPtr<nsIMsgWindow> msgWindow(do_QueryReferent(m_msgWindowWeak)); > msgWindow.swap(*aMsgWindow); >- return NS_OK; >+ return *aMsgWindow ? NS_OK : NS_ERROR_FAILURE; NS_ERROR_NULL_POINTER would seem slightly more appropriate. >@@ -72,7 +73,7 @@ > * It can be specified/saved here to avoid prompting the user constantly for > * the sending password. > */ >- attribute string password; >+ attribute ACString password; For the record, I was going to suggest changing this (and the others) to AUTF8String to help with using non-ASCII characters in passwords. However I looked into things a bit more, and came across items like base 64 encoding, and the specs for SMTP and decided we need to resolve this elsewhere with more thought. > nsIURI GetNewMail(in nsIMsgWindow aMsgWindow, in nsIUrlListener aUrlListener, >- in nsIMsgFolder aInbox, >- in nsIPop3IncomingServer popServer); >+ in nsIMsgFolder aInbox, in nsIPop3IncomingServer popServer); Its a bit hard to tell with -w but this looks the wrong indentation. > nsIURI CheckForNewMail(in nsIMsgWindow aMsgWindow, in nsIUrlListener aUrlListener, >- in nsIMsgFolder inbox, >- in nsIPop3IncomingServer popServer); >+ in nsIMsgFolder inbox, in nsIPop3IncomingServer popServer); ditto wrt the indentation - nsCString escapedUsername; - *((char **)getter_Copies(escapedUsername)) = - nsEscape(username.get(), url_XAlphas); + nsCString escapedUsername.Adopt(nsEscape(username.get(), url_XAlphas); This doesn't compile (nsSmtpServer), and please could you use MsgEscapeString version so we're ready for frozen API. nsPop3Service.cpp seems to be missing an #include "nsINetUtil.h" (In reply to comment #45) > 2. Don't put up a password if the url is running without a msgWindow, i.e., is > a biff url. (I may need to make sure we don't forget passwords when biff urls > are run, but I think that's going to be part of a rework of how we forget > passwords). I think checking this would be a good idea before submitting it. We don't want our forget password cases to get even worse. r=me with the comments addressed, bustages fixed and the not forgetting passwords on biff checked. Also, I think we could really do with some unit tests for this. Certainly we have fake servers for both pop and smtp and I think writing some tests around those would be a good idea. However, don't wait on writing them to get this code in, as it is probably going to conflict with some of the password manager stuff I am working on.

carrying forward standard8's r+, requesting sr from Neil. I've made it so we only forget imap passwords if there's a msg window (we were already doing this for pop3). I fixed the spaces in NS_ENSURE_SUCCESS(rv, rv) if they were lines I added, but in the interest of keeping this patch from being even bigger, I left the other ones alone. I addressed all the other comments, I believe.

3.0b1 flag is going away in favour of 3.0 flag and milestone combination.

Comment on attachment 337672 [details] [diff] [review] patch addressing Standard8's comments - checked in >+ else if (!PL_strcasecmp(m_urlidSubString, "verifyLogon")) Bah, that wasn't my idea when I made the comment... it looked inconsistent with all the other imap url flags although I admit that I hadn't seen the POP3 code's consistent use of verifyLogon by then.

This isn't ready for beta 1, so we're moving it out to beta 2.

In recent comments to Bug 112356, Bug 231541 and Bug 312431 Bryan Clark claims that this patch will alter the default "leave on server" behaviour (which is rather contentious). I can't see any discussion above regarding a change to the default "leave on server" behaviour and Bryan's comments in the other bugs (particularly Bug 231451 comment #91) are somewhat vague. Could someone please explain just exactly what is the intention in this patch with regard to the default leave on server behaviour?

bug 231541 comment 91 is where I was describing an alternate method for managing mails on a POP server post account creation. I also created bug 455681 to track the inclusion of this new way of warning and managing server space for POP3 users.

Today I created an email account with Apples Mail (out of curiosity). This was easy, because Apple Mail sets the encryption to SSL per default (this is good for the normal, inexperienced user). You can alter this after the account creation is complete (but I don't find a TLS option). In my first experiences with e-mail clients, the hardest thing in configuration was to find the configuration informations (name of the POP3/IMAP and SMTP server, Ports, best encryption). So I think a sophisticated method and helpful for inexperienced users would be a list in the Account Wizard that lists the popularest webmail providers. So you can choose your provider and the best settings will be done automatically. After that you can edit it (for the experts) or use this default settings. This would make the account creation extremely easy for everyone. But I don't know if this idea is feasible. One thing I think is very important is not to get rid of terms like "SSL" or "TLS" for simplification reasons. Because every webmail provider uses this terms on there websites. And this terms are established. And one thing I like on Thunderbird is the potential to have the full control of your account settings.

(In reply to comment #56) > [...] So I think a sophisticated method and helpful > for inexperienced users would be a list in the Account Wizard that lists the > popularest webmail providers. So you can choose your provider and the best > settings will be done automatically. [...] Most popular where? In my country, the most popular email providers are @belgacom.net and @skynet.be, but there are other national servers, and some international servers like @hotmail.com and @gmail.com, and some servers in neighbouring countries like @chello.fr or @yahoo.fr, are also popular. (My own accounts are with @belgacom.net, @skynet.be, @yahoo.co.uk and @gmail.com.) I'll concede that most of my country's citizens will prefer French or Dutch interfaces for their browsers and mailers, but I'll also bet that there are people all over the world (even in non-English-speaking countries) who, like me, prefer en-US menus and messages. I think that if you want to list all servers with a non-negligible number of users you'll get such a long list that it won't be practical, especially for an inexperienced user. I believe it would be better to have the user type the desired server name (as mentioned on whatever document he got from his server when signing up for an account), perhaps as part of the desired email address. This would also allow maintaining a single list of servers with their respective settings, as opposed with a different list of "popular servers" for each localized version of Thunderbird.

> I think a sophisticated method and helpful for inexperienced users would > be a list in the Account Wizard that lists the popularest webmail > providers. That's already implemented on the autoconfig branch. See https://wiki.mozilla.org/Thunderbird:Autoconfiguration .

(More precisely, it fetches the config, depending on the email address domain.)

(In reply to comment #57) I think that if you want to list all > servers with a non-negligible number of users you'll get such a long list that > it won't be practical, especially for an inexperienced user. Yes, I'm aware that this is an complex to realize idea. But Thunderbird would be the first program with this feature. :D Hm, maybe you can handle this, that you only type your email adress in and than an automated mechanism will compare the part after the "@" with an internal list of all providers to find the settings. (In reply to comment #58) > That's already implemented on the autoconfig branch. > See https://wiki.mozilla.org/Thunderbird:Autoconfiguration . OH, wow, I didn't know about that bevor. Nice to know. I think this is that what I'm talking about, but I have to read it exactly first. :)

In order to stop loading from BenB's web server, I've made the url we fetch http config info configurable by setting "mailnews.auto_config_url". If we get this set up, we'd set this pref in mailnews.js (or all-thunderbird.js, if it's TB-only)

moving to b2

I've made a pass through the code in the autoconfig repo to make the strings localizable, updated it to use the newer log4moz in gloda, and moved the duplicate account detection to before we verify the password, since there's no sense verifying the password for a duplicate account. I've updated the TODO list somewhat as well. I'll come up with a plan of attack for the remaining work to get the code into some sort of reviewable shape so we can get it into the tree sooner rather than later, and the localizers can start translating all the strings, and people can start banging on it, and I hope help out with some of the issues.

David, great changes, thanks. Also thanks for doing the localization pass. I can help with getting this finished. Let's talk about it. Please ping me when you start, if I haven't contacted you before.

this is a cumulative wip patch - I've made a pass cleaning up the js formatting, got rid of the dump statements, etc, and got things working in a comm-central tree, among other things. I'm going to try to start getting reviews on parts of this, while I continue to work on some remaining issues.

Phil, do you might taking a jaundiced look at this? I realize it's an insanely large patch but I'm trying to get it into the tree so that we can get it in the hands of users. Right now, it adds a new file menu, File | New | Mail Account (Quick Setup), which brings up the autoconfig wizard that davida and benb worked on. The wizard seems to be working relatively well. I haven't hooked up any pre-canned xml files yet; Ideally they'd be on our momo server somewhere. Right now some are on BenB's server, but we don't want to check in with that in the code :-) The windows theme is pretty far behind the mac theme since Davida works on the mac. I haven't dove into that yet. Picking the smtp server isn't fully implemented yet either.

> The wizard seems to be working relatively well It doesn't for me. It breaks easily in lots of cases for me. That's why I'd want to work on it still, but I don't want to hold anybody up.

Comment on attachment 356817 [details] [diff] [review] more wip Perhaps Magnus has some time to review some or all of this...

My after-work reviewing didn't get past wondering whether you were going to attach another patch that didn't have the "blah blah blah, XXX" strings in /locales/, or you were going to attach another patch that put the strings in content until you got l10n-ready strings.

There are a few strings that the code passes around that afaik are not actually displayed to the user. The ones that are I put in a/mail/locales/en-US/chrome/messenger/accountCreation.properties though I may have missed some...

Lest I forget, or don't get the whole thing done: + const CC = Components.classes; + const CI = Components.interfaces; r-, CC should only be Components.Constructor (CI doesn't have that confusion problem, but *will* needlessly lead to someone trying to use Ci, currently at 270 uses in the tree, instead of CI, currently at 9).

+ try { + let prefs = CC["@mozilla.org/preferences-service;1"] + .getService(CI.nsIPrefBranch); + prefs.setBoolPref(leaveOnServerPref, + config.incoming.leaveMessagesOnServer); + prefs.setBoolPref(deleteFromServerPref, + config.incoming.deleteOnServerWhenLocalDelete); + } catch (ex) {dump(ex);} What error are we expecting, why aren't we catching it and re-throwing anything else, and why do we think we can continue creating an account when we can't save prefs?

Comment on attachment 356817 [details] [diff] [review] more wip String nitting: >+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd >+<!ENTITY none.label "None"> >+ >+<!ENTITY gotoadvanced.label "Go to advanced settings"> One extra space starting here >+<!ENTITY accountcreation.title "Account Setup"> Except here it's a bit more than one >+<!ENTITY insecureCleartext.description >+'Warning! This is an insecure server. Email is sent in >+clear-text, so your email could be read by attackers, etc. Thunderbird >+will let you get to your mail, but you should really get your email >+provider to configure the server with a secure connection. >+More details are available on this , >+blah blah blah'> Single quotes look odd; if I've seen multiline entities wrapped in column zero like that I don't remember it (and if I really haven't, then l10n tool authors maybe haven't, and may break); blah blah blah >+<!ENTITY incoming_settings.label "Incoming settings:"> >+<!ENTITY outgoing_settings.label "Outgoing settings:"> >+<!ENTITY incoming_is_checked.label "I've double-checked the incoming settings."> Are those aligned -2 from 0, or -2 from the ones that were +1? >+<!ENTITY understood.label "I know what I'm doing."> >+<!ENTITY getmeout.label "This is scary, let me out."> >+ >+<!ENTITY getcertinfo.label "Get information on securing email servers."> Then the alignment wheels fell clear off. Personally, I don't actually care much between align and just one space after the entity name, but the variety of alignments is distracting. >+<!ENTITY certinfo.url "http://www.mozillamessaging.com/XXX/about/certs"> I very strongly doubt that you want that URL in a localized entity rather than in a pref, but if you do (you don't), you don't want it XXXed >+<!ENTITY insecureSelfsigned.description >+"Warning! This is an insecure server. The server uses >+a certificate that we can't trust, so we can't be sure that someone >+isn't intercepting the traffic between Thunderbird and your server. >+Thunderbird will let you get to your mail, but you should really get your >+email provider to configure the server with a trusted certificate. >+More details are available on this URL..., >+blah blah blah"> >+ >+<!ENTITY secureServer.description >+"Congratulations! This is a secure server, blah blah blah"> Double quotes, but again the wrapping looks surprising, and blah blah blah. >diff --git a/mail/locales/en-US/chrome/messenger/accountCreation.properties b/mail/locales/en-US/chrome/messenger/accountCreation.properties >new file mode 100644 >--- /dev/null >+++ b/mail/locales/en-US/chrome/messenger/accountCreation.properties >@@ -0,0 +1,39 @@ >+# accountCreation.properties >+ >+cleartext_incoming=This is an insecure incoming server. Data is sent in clear-text, so your password and email you receive could be read by third parties. For all of these, don't need two spaces between sentences, don't need the trailing space that's after several of them. >+selfsigned_incoming=This is an mis-configured incoming server. The certificate used by the server can't be trusted, so while the connection is encrypted, we can't be sure that the server is controlled by who it claims to be. Either "misconfigured" (go go gadget spellchecker, saying that's wrong but misconfiguration is right), or perhaps better "incorrectly configured" or "incorrectly set up" - all of which still fail to adequately get across "You didn't do anything wrong, but your mail server was set up by someone who didn't know what they were doing." >+cleartexturl=http://www.mozillamessaging.com/help/about/certs And you certainly don't want to have that URL in both a localized entity without any guidance about how to localize it and also in this localized string, slightly different (help does look better than XXX) but still unlocalized and unnoted. You're much more likely to want to have it, once, in a pref like mailnews.start_page.welcome_url, including the probably-want of having the version as well as the locale in the URL, depending on what content's going to be there. >+check_preconfig=Checking for preconfiguration... >+checking_config=Checking for configuration of your account... >+checking_mozilla_config=Checking for configuration provided by Mozilla community... >+probing_config=Probing configuration... >+checking_password=Checking password... Real ellipses, rather than three dots, please… >+incoming_server_exists=Incoming server already exists! >+outgoing_server_exists=Outgoing server already exists! Exciting, but not exciting enough for an exclamation point. I didn't (yet) check for which strings are actually unused.

+function RememberPassword(server, password) +{ + CC["@mozilla.org/passwordmanager;1"] + .getService(Components.interfaces.nsIPasswordManager) + .addUser(server.serverURI, "", password); +} Should be rememberPassword, CC is undefined (as it should be, for a single-use at two spaces indent where we don't really need the space-savings), and I doubt we want to save the password in wallet anymore :)

+function sslLabel(val) +{ + switch(val) + { + case 1: + return gStringsBundle.getString("no_encryption"); + case 2: + return "SSL"; // SSL and TLS shouldn't need to be translated + case 3: + return "TLS"; + default: + gEmailWizardLogger.error("Asked to create an sslLabel for: " + val + '\n'); + return "Unknown"; + } +} It puts the START before the TLS, or it gets the hose again. But if they don't need to be translated, then we shouldn't already have them in multiple localized files, none of which have l10n notes saying not to localize them, some of which are nothing but "SSL" or "TLS" or "STARTTLS", and some of which *are* localized (ССЛ in sr, எஸ்எஸ்எல் in ta). Localize 'em.

philor wrote: > >+<!ENTITY certinfo.url "http://www.mozillamessaging.com/XXX/about/certs"> > I very strongly doubt that you want that URL in a localized entity rather than > in a pref This is just a "more info" URL. It contains text that would have been too much to put in the dialog, but otherwise is just a long explanation of the dialog text. As such, it's inherently part of the localization (or help). It's not a pref in that it would ever be changed by a user, does not affect how the system works etc.. I agree that it should appear only in one place, though.

No, it should be a pref, because that's how partner builds and corporate deployments decide to have things like updates, or addons, or crash reports, or explanations of just how badly their certs suck, go to their server rather than ours. And while it should be localized in the sense of http://www.mozillamessaging.com/ja-JP/about/certs (or certs?locale=ja-JP, depending mostly on what gozer likes), that shouldn't be done by localizers in a localized string file, it should be done by code fetching a pref and using the URLFormatterService to replace %LOCALE% and probably %VERSION%, if it now or later will say different things for different versions.

philor, I once was a redistributor. I wouldn't want to change such a URL. If so, I'd want it to be part of help or online help and use the same base URL.

(In reply to comment #79) > [...] > And while it should be localized in the sense of > http://www.mozillamessaging.com/ja-JP/about/certs (or certs?locale=ja-JP, > depending mostly on what gozer likes), that shouldn't be done by localizers in > a localized string file, it should be done by code fetching a pref and using > the URLFormatterService to replace %LOCALE% and probably %VERSION%, if it now > or later will say different things for different versions. The only general rule I care about is to separate all URLs that the client will be talking to in their own separate URL-space: http://live.mozillamessaging.com/[...] And whatever is there can redirect things all over the place, it doesn't matter, but the idea is to keep all URLs that will be baked into the client in the same root, so we can treat it extra carefully. Apart from that, how content is organized under there, not that important to me. Right now, we have: http://live.mozillamessaging.com/thundebird/start http://live.mozillamessaging.com/thunderbird/firstfun http://live.mozillamessaging.com/thunderbird/whatsnew Each get passed in a bunch of url arguments, i.e. ?locale=[xx]&version=[xx]&platform=[xx]

(In reply to comment #80) > philor, I once was a redistributor. I wouldn't want to change such a URL. That's fine, as long as you don't confuse your own desires with those of other people. Corporate deployments don't _have_ to run their own AUS, but they can. Partner builds don't _have_ to provide their own help site, but they can. > If so, I'd want it to be part of help or online help and use the same base URL. Fine by me, if you can figure out a non-horrid system for making it clear that anyone who uses a different pref for app.support.baseURL is then required to support URLs x (y and z as time goes on), and can work out the website details with the way the MoMo support.baseURL currently sends everything off to mozilla.org. Not sure whether that would be twice as hard as just putting it in its own pref, or ten times, though.

(In reply to comment #75) > I didn't (yet) check for which strings are actually unused. Only unused ones are +thisurl=on this web page. +cleartexturl=http://www.mozillamessaging.com/help/about/certs

+var emailRE = /^(("[\w-\s]+")|([\w-]+(?:\.[\w-]+)*)|("[\w-\s]+")([\w-]+(?:\.[\w-]+)*))(@((?:[\w-]+\.)*\w[\w-]{0,66})\.([a-z]{2,6}(?:\.[a-z]{2})?)$)|(@\[?((25[0-5]\.|2[0-4][0-9]\.|1[0-9]{2}\.|[0-9]{1,2}\.))((25[0-5]|2[0-4][0-9]|1[0-9]{2}|[0-9]{1,2})\.){2}(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[0-9]{1,2})\]?$)/i; If we're going to go with the current very frustrating UI of just silently sitting there with no feedback or guidance when we don't like an email address, we need The Ultimate Email Regex. This one failed on the second address I tried, philringnalda+foo@gmail.com.

> > If we're going to go with the current very frustrating UI of just silently > sitting there with no feedback or guidance when we don't like an email address, we should give some sort of feedback when we don't like the e-mail address > we need The Ultimate Email Regex. This one failed on the second address I > tried, philringnalda+foo@gmail.com. Is there such a regex? A quick Google search didn't turn up anything. I think we want to err on the side of forgiveness since we're basically just using the e-mail address to get the host domain and the user name, and the user can override both.

(In reply to comment #85) > > we need The Ultimate Email Regex. This one failed on the second address I > > tried, philringnalda+foo@gmail.com. > > Is there such a regex? A quick Google search didn't turn up anything. I think > we want to err on the side of forgiveness since we're basically just using the > e-mail address to get the host domain and the user name, and the user can > override both. The fellow who authored this page compared 11 different e-mail-based regexes and put each of them up against a test suite of 17 hypothetically-valid and 18 invalid e-mail addresses: http://fightingforalostcause.net/misc/2006/compare-email-regex.php In the end, he did find a "winner" that triggered a couple false positives within the test suite but was able to avoid triggering any false negatives. That regex is listed first on the page and, for convenience, I've also pasted it below: /^[-_a-z0-9\'+*$^&%=~!?{}]++(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*+@(?:(?![-.])[-a-z0-9.]+(?<![-.])\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d++)?$/iD

davida, clarkbw: how committed are we to keeping this "guess when the user is done typing their email address instead of having a [Set me up] button" UI? Even assuming someone rewrites Alex's regex to not need look-behind assertions, we're still absolutely certain to get false positives on the user being done, and go out and hit the network for something other than what they intend. Having that actually do evil _probably_ requires a pretty contrived thing, like you're setting up your foo@evil.com.com CNet account, get as far as entering your password, remember it's now evil.cnet.com, backspace through the last .com and then an alert in another program blurs us, and we wind up sending your password to evil.com while you're dealing with that, but having it surprise the user by going off to do something they didn't ask for or want is nearly certain.

In case somebody doesn't know: I implemented this together with davida and bienvenu. Given that you seem to be progressing to review, and I still considered this unfinished, I am looking into it a bit. The state is still scary, code quality is still "experimental". There are dozens of bugs still, on all levels, starting from design and logic bugs. Whatever I try falls on its nose. About half of these are logged in TODO, but there's much more. I don't think this is ready for review. There are also UI changes that I discussed with Bryan in Barcelona. I'd like to fix this, but it'll probably take me a week of concentrated work.

BenB, thx for your offer of help. I don't think this feature has a chance of making it into the product unless we we get it reviewed and landed in the next week or two, so that the strings can be localized, and we can get feedback from users. The current patch makes it trivial to "remove" this feature from the UI since there's only a single menu item point of access. I would like to fix the worst of the bugs without reworking the whole thing and putting it back into an unreviewable state. I'd also like to get a directory setup on the mozilla message site with .xml files for the most popular e-mail accounts so that we don't have to do the guessing at all for most users.

> The only general rule I care about is to separate all URLs that the client will > be talking to in their own separate URL-space: > > http://live.mozillamessaging.com/[...] > > And whatever is there can redirect things all over the place, it doesn't > matter, > but the idea is to keep all URLs that will be baked into the client in the same > root, so we can treat it extra carefully. > > Apart from that, how content is organized under there, not that important to > me. Right now, we have: > > http://live.mozillamessaging.com/thundebird/start > http://live.mozillamessaging.com/thunderbird/firstfun > http://live.mozillamessaging.com/thunderbird/whatsnew > > Each get passed in a bunch of url arguments, i.e. > ?locale=[xx]&version=[xx]&platform=[xx] Gozer, I assume this also goes for the xml config files that we will be fetching for the common isp's like gmail, at&t, yahoo, etc? Eventually, we'd like to be able to populate these files from contributors or something like gerv's database, but it will take time to set up a process for that. I'd like to get started, however, with some of the basic xml files - do you have any preference for what directory we put them in? I'm thinking something like http://live.mozillamessaging.com/emailconfig - I don't think these files are particularly Thunderbird-specific (e.g., SM could use them as well). I don't know if locales are important - I know gmail does things a little differently in Europe, for example, but I think they use a different host domain name, which would just be an extra file. We would probably still pass in the full url, and let redirects handling things on the server-side?

(In reply to comment #90) > > [...] > > > > http://live.mozillamessaging.com/thundebird/start > > http://live.mozillamessaging.com/thunderbird/firstfun > > http://live.mozillamessaging.com/thunderbird/whatsnew > > > > Each get passed in a bunch of url arguments, i.e. > > ?locale=[xx]&version=[xx]&platform=[xx] > > Gozer, I assume this also goes for the xml config files that we will be > fetching for the common isp's like gmail, at&t, yahoo, etc? Yes, I'd treat them the same. > Eventually, we'd > like to be able to populate these files from contributors or something like > gerv's database, but it will take time to set up a process for that. I'd like > to get started, however, with some of the basic xml files - do you have any > preference for what directory we put them in? I'm thinking something like > http://live.mozillamessaging.com/emailconfig The idea is to have the client configured to talk to that url, even if under the hood, that ends up redirecting somewhere else. So in this case, for simplicity, we might setup http://emailconfig.mozillamessaging.com/ for where the files actually live, but keep the clients talking to http://live.mozillamessasging.com/emailconfig and blindly redirect for the time being. > - I don't think these files are > particularly Thunderbird-specific (e.g., SM could use them as well). I don't > know if locales are important - I know gmail does things a little differently > in Europe, for example, but I think they use a different host domain name, > which would just be an extra file. We would probably still pass in the full > url, and let redirects handling things on the server-side? Yup, it's better to keep the smarts server-side, instead of in the client. The client passes in as much information as it can, and it lets the server decide how to deal with it.

> I would like to fix the worst of the bugs I fully understand why you'd prefer to changes to be kept small. Unfortunately, the bugs I see here are so fundamental and so severe and so many that I can't keep the changes confined, they'll need to be substantial. That's also why I don't see any point right now to get any testing from users for this, as I just have to poke it a tiny bit for it to fall over badly, I find more and more bugs all the time, more than I can document. In fact, it's so bad that I didn't know where to start and got quite frustrated and consequently couldn't continue.

Philor, thx very much for the review comments. I've addressed about half of them in the autoconfig repo, and will work on the other half asap. One thing - perhaps I'm doing something wrong, but on windows, TB isn't very happy with ellipses in .properties files - it complains on load that they're not utf8 characters.

(In reply to comment #93) > perhaps I'm doing something wrong, but on windows, TB isn't very happy with > ellipses in .properties files - it complains on load that they're not utf8 > characters. Your editor is probably not set up to edit in UTF-8... in Windows-1252, an ellipsis is 0x85 but what you want is a U+2026 which is 0xE2 0x80 0xA6 in UTF-8 or ò€¦ in Windows-1252 (ironically, no saving in bytes!) Note that some UTF-8 editors insert a UTF-8 BOM which we don't want.

Thx, Neil - yes, it definitely seems to be an insert time/edit issue, not a save issue per se. I'm using dev studio, but I think I'll just use xcode for this for now.

If you haven't already done rememberPassword, my working-so-it-stops-annoying-me-testing version is function rememberPassword(server, password) { if (server instanceof Components.interfaces.nsIMsgIncomingServer) var passwordURI = server.type + "://" + server.hostName; else if (server instanceof Components.interfaces.nsISmtpServer) var passwordURI = "smtp://" + server.hostname; if (passwordURI) { let lm = Components.classes["@mozilla.org/login-manager;1"] .getService(Components.interfaces.nsILoginManager); let login = Components.classes["@mozilla.org/login-manager/loginInfo;1"] .createInstance(Components.interfaces.nsILoginInfo); login.init(passwordURI, null, passwordURI, server.username, password, "", ""); lm.addLogin(login); } } (smtp's untested, since we don't verifyLogon for outgoing servers, so unless it's coming from XML we won't ever think we need auth, which is a bit of problem)

thx, Phil, I'll put that it into the autoconfig repo, and then into the next patch I generate. Whether things work with xml or probing seems to depend on who touched the code last :-(

> If we're going to go with the current very frustrating UI of just silently > sitting there with no feedback or guidance when we don't like an email address, If I enter a bad e-mail address, I get a big red error message above the e-mail address field, when I leave the field.

> > The fellow who authored this page compared 11 different e-mail-based regexes > and put each of them up against a test suite of 17 hypothetically-valid and 18 > invalid e-mail addresses: > > http://fightingforalostcause.net/misc/2006/compare-email-regex.php > > In the end, he did find a "winner" that triggered a couple false positives > within the test suite but was able to avoid triggering any false negatives. > That regex is listed first on the page and, for convenience, I've also pasted > it below: > > /^[-_a-z0-9\'+*$^&%=~!?{}]++(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*+@(?:(?![-.])[-a-z0-9.]+(?<![-.])\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d++)?$/iD JS doesn't seem to like this regexp, and my regexp foo is woefully inadequate.

> > If we're going to go with the current very frustrating UI of just silently > > sitting there with no feedback or guidance when we don't like an email > > address, > If I enter a bad e-mail address, I get a big red error message above the e-mail > address field, when I leave the field. Yes, I fixed that last night. Problem was merely a missing error msg in the string bundle.

*cough* *cough* let sslType = sslLabel(config.incoming.socketType); switch (sslType) { case 'SSL': case 'TLS': ... case 'No encryption': sslLabel() is the *translated*, user-visible text. The whole point of config.incoming.socketType being an int was that it's a reliably processable enum. I'll fix that.

philor wrote: > If you haven't already done rememberPassword, my > working-so-it-stops-annoying-me-testing version is > function rememberPassword(server, password) > let lm = Components.classes["@mozilla.org/login-manager;1"] Error: Cc['@mozilla.org/login-manager;1'] is undefined

(In reply to comment #102) > (In reply to comment #96) > > If you haven't already done rememberPassword, my > > working-so-it-stops-annoying-me-testing version is > > function rememberPassword(server, password) > > let lm = Components.classes["@mozilla.org/login-manager;1"] > Error: Cc['@mozilla.org/login-manager;1'] is undefined Upgrade your wallet build to a login manager build?

I may need to re-merge with the trunk for that to work - I'll do that now.

+ try { lm.addLogin(login); + } catch (e if e.message.indexOf("This login already exists") != -1) { + // TODO modify }

Oops, I just wanted to see _something_ work. lm.findLogins({}, passwordURI, null, passwordURI), iterate through them looking for one with server.username == login.username and modifyLogin to change the password if it's different, else save, I think is right (and should be fairly copyable from nsLoginManagerPrompter.js).

(In reply to comment #99) > > JS doesn't seem to like this regexp, and my regexp foo is woefully inadequate. The O'Reilly regexp book is pretty good; I can lend you my copy if you want to try and use it along with MDC to debug that...

- Fixed SMTP server selection (full day of work) - Fixed manual hostname editing a bit, but still broken - Implemented custom input fields and "enable URLs" from config XML files. - Stopped verifyConfig() from guessing and downgrading login auth and username form when config comes from config files. - Localized JS logic files (error messages). (The frontend - the vast majority - has been done by bienvenu earlier.) Localization is complete now, if I haven't overlooked anything. Outstanding, high priority: - State changes / transitions (user edits various fields at various points) buggy to completely broken.

Phil, do you have stuff you want to land? I'd like to make an other pass at your review comments but I don't want to duplicate stuff you've already done.

You won't - I've been doing stuff I hadn't yet mentioned, mostly whitespace and style. The only mentioned part I've got is changing the regex to ^[-_a-z0-9\'+*$^&%=~!?{}]+(?:\.[-_a-z0-9\'+*$^&%=~!?{}]+)*@(?:[-a-z0-9.]+\.[a-z]{2,6}|\d{1,3}(?:\.\d{1,3}){3})(?::\d+)?$ (which is the "will allow @-foo-.com but who cares?" version) which I've been pretending I'll test more, but probably won't.

we're not going to hold b2 for this, but I'd really like to push to get it into b2

> I've been doing stuff I hadn't yet mentioned, mostly whitespace and style You're working off the autoconfig branch, though, are you? Because I see no checkins apart from mine in the last days, and I made a lot of changes, too.

Yeah, that's because I haven't pushed anything. What I *meant* to tell bienvenu was that I would just push nits like trailing whitespace and style, rather than do enormous review comments citing 60 lines with this, and 50 lines with that, and 45 lines with the other, but I still haven't quite figured out how to do that to a moving target, other than maybe make a list and just keep grepping, or do nit after-the-fact reviews on every push that touches anything I've already done.

Well, either you state here what you dislike, e.g. "no trailing whitespace" or "replace ' with " ", or you do it yourself, all at once within 2 hours, and commit it immediately. If you do, please do not mix style and whitespace changes with real code changes.

we seem to have lost the progress widgets, at least on windows - I don't see any progress happening - didn't that used to work on Windows? I'll try the mac again...

I see in David Ascher blog (that is commented in Thunderbird Beta1 Preview Release) a new feature (autoconfig), but I test it with no success. So, I submit a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=477138 But Banner says that this feature is not available yet, and *maybe* spreadsheet available in https://wiki.mozilla.org/MailServerList is not used by developers to develop autoconfig. I think that this spreadsheet have a lot of good information about mail servers. Best regards, Renato

Comment on attachment 356817 [details] [diff] [review] more wip marking obsolete - we've moved on a bit.

putting in m3 - Bryan, please push back if you don't think we're going to make that.

Phil, do you think you could have a look at this? Bryan and I have changed the way this works a bit - now, if you need to edit settings because autodetect won't work for your account, you need to stop the autodiscovery process, make your changes, and then press go. I've set up all my accounts on my new machine with autoconfig, so most things have worked at one point or an other, though I may have messed up generating a diff between the autoconfig repo and the trunk. I'm using your e-mail address regexp, I believe...

Pushed trailing whitespace removal and __end removal to the autoconfig repo.

Looks like chrome://messenger/skin/identity.png as (not) seen in the tooltip for the green button for a happy secure server didn't manage to actually land.

do you mean in the autoconfig repo or in my patch? If the latter, I'll go make sure it gets added to my trunk repo so it'll be in the diffs...

The autoconfig repo: looks like the intention was to copy http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/winstripe/browser/identity.png (and the other two themes) and then use just the middle third of it, so it should really be just that chunk, and drop the -moz-image-region.

Pushed three identity.png icons, and the changes to jar.mn and accountCreation.css to go with them, to the autoconfig repo. But, hmm, poor Gnomestripe doesn't seem to have gotten any of the other icons at all, have to do something about that once I remove the spinner.gif one, since we have perfectly usable throbbers already.

Got rid of spinner.gif, and copied over the Qute icons to Gnomestripe so it'll build. Hope you've got a fairly easy way to go from the autoconfig repo to a patch :)

While removing unused strings, I tripped over the customfields and enableURLs stuff. I know BenB's going to hit the roof, but I don't see any sign that that's ready to land. Our rudimentary collection of config files only includes one with one enableURL which isn't commented out, and since there appears to be no way to get past the t-online.de file to get to the t-online.de imap ssl file which uses only one of those two features, we're not only just guessing that it works, we're also expecting localizers to translate some... rather vague strings without any way to see them (and for that matter, expecting ui-r on something that Bryan's not going to even see).

We removed the UI for the enableURLs stuff because it was ui-r, but I didn't remove the strings and code because at some point we would like to have some sort of UI for this. I'm not sure about the t-online.de stuff...

enableURL is required for gmail

enableURL is required for gmail. I'll also use it to notify people that Hotmail can't work (without extra software). customfields are needed for many, many ISPs. Can we get that in, please? The UI wasn't any more broken than the rest of the UI ;-).

Comment on attachment 371248 [details] [diff] [review] patch against trunk with philor's recent changes >--- a/mail/locales/en-US/chrome/messenger/messenger.dtd >+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd >+<!ENTITY openMessageMenu.label "Open"> That unused entity is just *desperate* to make its way back into the tree, isn't it?

ah, sorry, I'll fix that.

(In reply to comment #129) > enableURL is required for gmail. For Gmail *IMAP*. For which https://bugzilla.mozilla.org/buglist.cgi?quicksearch=gmail+imap might be a better URL. > I'll also use it to notify people that Hotmail can't work (without extra > software). You might have, before March 14th. Now, that's no longer the case. > customfields are needed for many, many ISPs. Then we need a config file which uses them, *before* we land strings, so localizers have fighting chance of seeing what they are doing. Though the 'spec' on the wiki doesn't exactly make it sound like we need a complicated extensible system allowing for every possible type of data and up to five different things with a vaguely worded localized generalized description and then non-localized labels so much as we need to optionally allow for either one or two "username" fields, which could then have localized labels.

Some things I don't currently know how to fix, offhand: * The bad cert listener is still rather fragile (or maybe just broken): for email:goats@harborside.com password:goats (they deserve the suspicious-looking traffic, for exposing such broken crap even unintentionally), we seem to think we've discovered a secure POP server, at pop.harborside.com:993, but then we throw up an exception dialog with "imaps://pop.harborside.com" as the location, throw "Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.port]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: getURI :: line 220" data: no]" in the console, spin pretending we're thinking about it until you get bored and hit the Cancel button, do the same thing twice more, and then say it's an insecure server. Then, for real fun, hit the "Back" button under the address/password, then without changing anything hit "Next" and we'll say it's a lovely green secure server. * Failure to log in breaks the dialog. With the goats@/goats, hit "Create account" and the failure to log in puts up a "FAIL" dialog, but also shoves the right side of the main window over a little with an "Username or password invalid" (which I bet is then a huge shove with a language where that has to be longer), and shoves the "Edit" and "Create Account" buttons halfway out of sight, because the "Configuration could not be verified -- is the username or password wrong?" string is too long and not wrapping, which probably means that things are broken for more than just that one string in more verbose locales than English. * Since "Go to advanced settings" is actually "Create this account right now, or die trying" it really shouldn't be there acting like it will work and trying to work from the very second the window comes up, when it actually won't work more often than not.

* Not sure what we need to do to make the tooltips with insecureCleartext.description or insecureSelfsigned.description wrap, but they're currently a single very long line (and while someone's in the neighborhood, MDC and I don't think "bottom" is a valid value for the align attribute).

Reason to want to drop the unused enableURL strings number n: s/Thunderbird/brandShortName/ for entities was simple, but there are two Thunderbird instances in .properties. One is in findoutmore, which as the TODO comment in the code says should move to an entity, the other is in an enableURL string, so we'll have to include a branding <stringbundle> to brand an unused string. So, if enableURL failed ui-r, what makes us think it will at some point pass ui-r, what will the changes look like, and why do we think we'll make them later rather than now?

* If you don't already have an SMTP server configured, the menulist doesn't show anything at all (probably not a huge concern unless/until we decide to use autoconfig for the first-run first-account wizard, since my way of getting there by creating an RSS account to get past first-run isn't wildly common).

Ultimately, autoconfig will be used for the first run account wizard, so we'll have to figure out the smtp server list for that case. It should be a text box at that point, and I believe it works as one now. Re the cert error, there are a couple ways we encounter cert errors - one is during port probing, if we're able to connect via the SSL port, the other is if we can't connect via the SSL port but end up doing STARTTLS during verifyLogon. It sounds like you encountered the first one...the test case I had access to only did TLS so I've never exercised that code (I think davida wrote that...). I don't know what the plan for using enableURL might be - I know Bryan did not like the UI but I'm not sure what the plan for getting a better UI is.

Note to self: don't forget that .larger-button looks like pure evil except for pinstripe which doesn't style it, and that the style on #back_button to make it look disabled when it isn't disabled is... not entirely in line with typical UI guidelines.

http://hg.mozilla.org/users/dascher_mozilla.com/autoconfig/rev/ecac1d23a1b8 - moved one of the two .properties strings with a hardcoded "Thunderbird" out to the .dtd, but I had trouble coming up with a more accurate name than the previous inaccurate findoutmore, so it probably still needs a rename.

Several notes-to-self: * a fair share of the CSS doesn't actually apply to anything * functions with no callers are evil; global functions with no callers are doubly-evil * functions only called by uncalled functions aren't less evil * semicolons at the end of lines may be optional to the interpreter, they aren't optional to the reviewer * wrapping is good, random indents and random start-of-wrapped-line chars aren't * let kDebug = true; * catch (e) { alert(e.message); throw e; } * TODO * var gEnableURLsDialog

FWIW, I had discussed and agreed the general flow for enableURL and customfields (and the other dialog screens) with Bryan in Barcelona. The current UI looks horrible, because it depended on the main dialog flow / logic to be fixed first. I'll take a look at the new code, and try to fix the review comments, in the next days (probably starting Friday).

I recreated the harborside problem - I think at that point in the process we're supposed to be trying to do a temporary override of the cert without bringing up the UI, which is what we do for the smtp server. The reason the accept cert dialog is broken is that it got put up synchronously, which doesn't give the nss code a chance at the event loop to fetch the cert. The verifyLogon code which intentionally puts up the cert dialog has to do it on a timeout from the actual notification of the cert error. Why the pop/imap confusion, I'm not sure. It may be that we discovered the imap port working after the pop3 port, and if we hadn't got the cert exception, we would have updated the UI to indicate the imap server. But since we prefer IMAP over POP, and secure over non-secure, I'm not sure why that would be.

(In reply to comment #134) > * Since "Go to advanced settings" is actually "Create this account right now, > or die trying" it really shouldn't be there acting like it will work and trying > to work from the very second the window comes up, when it actually won't work > more often than not. I've talked with Bryan about the need to fix that, but we haven't reached any UI conclusions. One thought is to change the label to something like Create and edit. We may want to only enable it if the auto probing has succeeded, or the user has stopped auto probing.

For the dunno pile: the hostname fields and the email address field should have the class uri-element, to force them to be LTR even in an RTL locale, but the emptytext for the email address field shouldn't.

The cert override stuff is weird - if I set breakpoints in venkman, I don't get the cert exception dialogs so I have a feeling this is some sort of timing issue...

In my experience, Venkman breakpoints aren't terribly reliable; it may be worth adding dump statements double-check.

The way that using the Back button and then trying again doesn't give the dialogs again, while not proof of a race, would at least be nicely explained by one. (You are starting over from scratch after having gotten the dialogs, right? I usually go for a whole new profile to be sure nobody's caching certs or the lack thereof.)

my issue is that the breakpoints *were* getting hit, and that changed the timing/event loop stuff in such a way that the error did not happen... Phil, I landed some semicolon fixes, and Bryan's going to have a look at the css and remove stuff that's not used. And I'll try to figure out why Billy Goat can't read e-mail.

(In reply to comment #148) > The way that using the Back button and then trying again doesn't give the > dialogs again, while not proof of a race, would at least be nicely explained by > one. (You are starting over from scratch after having gotten the dialogs, > right? I usually go for a whole new profile to be sure nobody's caching certs > or the lack thereof.) I'm going into options and deleting the cert exceptions. We're supposed to be adding temporary cert exceptions in our port probing but the exceptions seem to survive a restart, which is odd.

> The way that using the Back button and then trying again doesn't give the > dialogs again, while not proof of a race, would at least be nicely explained by > one. I think (cert stuff is not my code) that behaviour is intentional - once you confirmed the scary dialogs, you don't want to see them again - at least not for the same hostname (they should show again for a different hostname, but don't IIRC).

Except that I've *never* confirmed them, because we've never fetched anything we would let me confirm: we throw up a dialog, with a missing port according to the console error, never enable the "confirm exception" button, and all I do is hit the "cancel" button three times.

Phil's right - and it's not your code, Ben, I don't think. I believe it's DavidA's or mine, and I see the bug(s) now - we've hardcoded the url to imaps, among other problems. NSS gave us a perfectly good targetsite, but we forgot it along the way, so I just need to pass that along.

Actually, I'm not sure why the cert exception is permanent, but I'll deal with that after I fix the other issues.

> I'm not sure why the cert exception is permanent You mean the "Yes, I know the cert is insecure" red dialog of the account wizard adding a permanent cert exception? That is intentional, though, for all I know: We want to remember that the user manually confirmed that this cert is fine, and not warn him about this one anymore (e.g. when checking mail).

Not actually having read the patch, I was wondering whether the code was sufficiently modular to provide for a "Test manual settings" button in a) the account wizard b) the account manager.

I think it's permanent because we never remove the override - we return before we hit the code that removes the override. I have the cert exception dialog code working better in my tree - it puts in the right site url, and allows the user to confirm the exception. It pops up twice for the goats account, which is a small issue :-) But the larger issue is, do we want the cert exception dialog at all, or do we want to use the UI BenB was referring to, which is way downstream, i.e., the scary dialogs, which at this point are a bit redundant. Re Neil's question, it seems like the nsIMsgIncomingServer's verifyLogon method could give you most of what you want for Test manual settings.

OK, I've fixed up the cert exception dialog so that it has the right url. I'm still working on figuring out why we get two of them, and fixing that. Phil, I still can't logon to that account with that user name and password - should I be able to? If so, what should my settings have been? (You can e-mail me privately if you want...)

Heh, no, you shouldn't: they're actually my first-ever ISP, not something I control to create working world-accessible accounts with. Real login in the mail, go ahead and delete any spam you happen to find there ;)

> the larger issue is, do we want the cert exception dialog at all, > or do we want to use the UI BenB was referring to, which is way downstream, > i.e., the scary dialogs, which at this point are a bit redundant. I think our red dialog is supposed to replace the standard PSM invalid cert dialog. But Bryan and DavidA will know better :).

* msgNewMailAccount() needs to either not openDialog with a window name, or use whatsit, toOpenWindowByType or some such, depending on whether we want a second click of the menuitem to open a second autoconfig window or just find the existing one - right now, a second click of the menuitem does nothing.

(In reply to comment #142) > FWIW, I had discussed and agreed the general flow for enableURL and > customfields (and the other dialog screens) with Bryan in Barcelona. To be clear on this. We agreed that the enableURL could be helpful and that we need to figure out a way to work in custom fields. However the enableURL as it was implemented was a "force the user to click on the URL before they can proceed" interaction which we didn't agree was correct. That kind of system is not only annoying to us as people trying to test and develop but will be to other people who aren't stupid. That system of interaction will not go into the final product. What we can do with the enableURL is offer it as a helpful option when our effort to login to the IMAP server fails. I understand there is no easy way of knowing why the login failed, essentially we have to try to help the person 'debug' what went wrong: password, username, IMAP enabled. We haven't put much effort toward designing this part yet so it definitely fails to be helpful right now. For the custom fields I didn't want to insert them into the current dialog, but add them as an extra dialog to the config dialog. Nothing about the custom fields requires guessing usernames or ID codes, those are all provided by the ISP and therefore don't require space in the interactive configuration area. We can add a button to open the dialog along with a green light / red light for indicating that the values are correct or incorrect. Likely the dialog can open on it's own when custom configuration values are detected to be necessary.

Bryan, what you describe was your stance *before* we discussed it constructively and came to an agreement that we both liked. Part of the problem was that we cannot even reliably detect a failure. In my testing, I had many cases (for different reasons) were we'd just be stalling for 2 minutes and eventually time out, because a) DNS doesn't resolve, b) the POP server simply doesn't answer (it doesn't like the user account / our IP address) etc.. So, while I agree this "show help on error" sounds reasonable in theory, it simply does not work in practice, in my real world experience. I tried many different ISPs and accounts in my testing. It was a horrible experience (despite me knowing the correct settings), and I want to save our users from that. We had discussed that and several problems in the logic flow (and resulting bugs) of the dialog. The agreement, as I remember it, was that we'll have a dialog with several modes/parts/pages (the left side with name, email, password stays the same, and the right part changes). enableURL and customfields were just two of those changing right parts. The other right parts are: nothing (empty), hostname guessing, manual hostname editing, final configuration/hostname display, and (possibly) cert warning. The parts are sequential and the transition happens with explicit Next buttons.

Can anyone persuade me that there's a reason to keep validatePassword? Right now, if you tab into and then out of the password field, we put up a lying error message, saying "Login did not succeed...", turn the emptytext into eight password bullet characters, and leave a blinking caret there, even though it doesn't have focus. While I don't doubt that's all fixable, I don't see blur as being the right time to be complaining about a zero-length password: as the error string says, when the Login did not succeed is plenty soon enough.

Phil, this and other cases - we have far worse situations - are why we should have explicit Next buttons (see my last comment) instead of relying on blur or the user knowing what he has to do next.

You haven't built autoconfig for a while, have you? We have an explicit Next button. We also have emptytext instead of labels, so actually I can't just get rid of it, at the very least I have to have validatePassword turn it back into text so the emptytext shows up again when you go in-n-out.

phil: yeah that should be changed, I think the blur event was carried over from before.

Apparently I've gotten so license-block blind that I didn't even notice that emailWizard.xul is missing one, and needs to have it.

I've landed most of the autoconfig stuff on the trunk, as NPOTB - this patch is what you need to apply to turn on the autoconfig UI on the trunk. Technically, I probably shouldn't have added the jar file changes, or messenger.dtd, but nightly users won't see the changes.

Ack, or the strings, since that now means that any change of string text requires a change of entity name/key.

(In reply to comment #170) > Ack, or the strings, since that now means that any change of string text > requires a change of entity name/key. I did not add the string files to the mail jar files (I only landed the theme jar file changes). I would hope the localizers would ignore property files that weren't part of the product though that's probably too much to hope for :-(

'Fraid so: even as we type, the quickest locales are looking at pages like http://l10n.mozilla.org/buildbot/compare/hg-comm-langpack/18487 and localizing away.

We still need a UI decision on whether a second click on the menuitem should open a second autoconfig window or should find the existing one, but at least this version doesn't break SeaMonkey's accountcentral link, and points us at the HTTPS MoMo config server.

Ugh, I didn't think about the l10n thing when I proposed going ahead with the landing; my apologies. So in the Firefox 3 timeframe, the way this problem was worked around was that Axel encouraged developers to put all their strings in code directories while development was in progress, and move them into the locale directory once they were more or less done and the strings weren't changed. Since Mercurial is better at moves than CVS ever was, we could conceivably try and do that now, post facto. I'm a little unsure whether that cure is worse than the disease, since it could be a frustrating experience for the localizers who have already started. Adding sipaq to the CC for his thoughts here. clarkbw: do you have a very rough feel for how much string churn we're likely to see here before we're mostly done churning?

s/strings weren't changed/strings weren't being changed much/

Not counting things where Bryan doesn't like the wording, there's certinfo.url, depending on how it goes 10 to 16 customfields/enableURLs strings, one of the pair of cleartext strings where in one we say "email" and the other we say "email and password", the two where we have double-hyphens for an em dash, and perhaps ten or fifteen removals where we're localizing what I think are developer-targetted console error messages like "enum value not supported" that I'm inclined to say we shouldn't be making our localizers try to translate.

i haven't done a real cleanup of the strings in there at all. many of them are left overs of the first attempt. I would plan for a good amount of churn I guess. Even for the strings in there that we plan to keep they don't likely have the final wording as that takes a while and really needs everything else together before it can happen.

There used to be a way to put translation notes in there as message to the translator, IIRC in form of comments, e.g. "DO NOT TRANSLATE" (for "Firefox" etc.). You might just put a note "Do not translate before May" or somesuch at the top of each file.

If someone's in a position to read a l10n note in the en-US file, then they haven't started translating and thus are in a position to be saved by a quick move to content. If not, they won't have any reason to be looking back at the en-US file. And from what I've heard about the lack of support for "ENTIRE FILE" l10n notes, it might not even ever be seen. It's also cruel to say "leave your tinderbox red for four weeks, and every time you check how you're doing, dig through the details and ignore these 100+ issues" - it's exactly equivalent to landing a patch that turns the code tinderbox red, and saying "eh, leave it for four weeks, and just read the build log to see if there are any new errors."

A wee bit unfortunate to be dropping an ifdef right back into mailnews/jar.mn minutes after the previous ifdef went away, but this way it's where it belongs when SM's ready to jar it.

* When we give up on guessing (@hotmail.com is handy for that, since it's pop3.live.com), you can edit the hostname like we tell you to, but we have an Edit button rather than a Go button. It should either still be disabled, or the button should change to Go (or disappear, if what we really mean is "we're bored with this, fix it however you want and then Create account"). * When you change the port and security (@hotmail.com is handy for that, since you want 993 and SSL/TLS, but we leave you at 110 and none), we then ignore that and start over at 110 and STARTTLS.

(In reply to comment #174) > Ugh, I didn't think about the l10n thing when I proposed going ahead with the > landing; my apologies. > > So in the Firefox 3 timeframe, the way this problem was worked around was > that Axel encouraged developers to put all their strings in code directories > while development was in progress, and move them into the locale directory > once they were more or less done and the strings weren't changed much. > > Since Mercurial is better at moves than CVS ever was, we could conceivably > try and do that now, post facto. I'm a little unsure whether that cure is > worse than the disease, since it could be a frustrating experience for the > localizers who have already started. Adding sipaq to the CC for his thoughts > here. That would be the best solution. Localizers who live on the bleeding edge know what they are doing, so this would be acceptable for them. Nevertheless it would be polite to inform of them of this decision via a posting in mozilla.dev.l10n Oh, and once you move the strings back from content/ into locales/ please make sure to remove all obsolete leftovers. Because we've tried hard in the last few months to eliminate all obsolete strings.

(In reply to comment #182) > That would be the best solution. I don't agree 100% here, though it's probably the best of the options for this particular scenario. The best real solution is not landing things that are not ready. If you want to land it because you think it might get ready and needs more widespread nightly testing, you might want to just have the strings in a state that warrants widespread testing as well, including comments from localizers if there are problematic strings or string usages in there. IMHO, landing experimental stuff that is known to not be ready enough for that is wrong in the first place, but it seems to have happened already in this case.

Let me just add that Robert is 100% right here in a general sense, that being that things should not be landed in the general nightly builds until they are ready. If they aren't, you should use a try-server build instead. In this particular situation however, where this was already landed (in error), the proposal from dmose' comment 174 is the best course of action.

Strings moved in http://hg.mozilla.org/comm-central/rev/4e12018f2e8c

I tried to keep to mostly whitespace and wrapping and stray commented-out stuff this round, though some other things slipped in.

Turns out I was wrong in comment 145, and emptytext is smarter than me. Even if you set class="uri-element" and thus direction: ltr;, the emptytext still takes the document direction, so you have [ כתובת דוא״ל] but when you start typing you get [me@foo ].

(In reply to comment #168) > Apparently I've gotten so license-block blind that I didn't even notice that > emailWizard.xul is missing one, and needs to have it. Fixed.

(In reply to comment #183) > > I don't agree 100% here, though it's probably the best of the options for this > particular scenario. The best real solution is not landing things that are not > ready. If you want to land it because you think it might get ready and needs > more widespread nightly testing, you might want to just have the strings in a > state that warrants widespread testing as well, including comments from > localizers if there are problematic strings or string usages in there. > IMHO, landing experimental stuff that is known to not be ready enough for that > is wrong in the first place, but it seems to have happened already in this > case. (In reply to comment #184) > Let me just add that Robert is 100% right here in a general sense, that being > that things should not be landed in the general nightly builds until they are > ready. If they aren't, you should use a try-server build instead. There's plenty of history at Mozilla of landing features at various stages of maturity for a variety of reasons, and it's generally a judgment call: there's no one-size-fits-all policy to be had here. As I mentioned in comment 174, the lead localizer for Firefox (Axel) encouraged all front-end feature developers to land strings in content before they were ready to be localized; I see no evidence at all that if we had actually done that in this case that it would have somehow been "wrong". The error here, which I take responsibility for, is not thinking through the l10n consequences of the early landing before suggesting going forward.

* emailWizard.xul has a non-localized |<label class="heading">Account Information</label>| (wonder how many times I've looked at that without seeing it)

Comment on attachment 372354 [details] [diff] [review] Review nits, round 1 - checked in - errorStr = getStringBundle("chrome://messenger/content/accountCreationUtil.properties") - .GetStringFromName("cannot_contact_server.error"); + var stringBundle = getStringBundle("chrome://messenger/content/accountCreationUtil.properties") + errorStr = stringBundle.GetStringFromName("cannot_contact_server.error"); Is this change for formatting/readability reasons? We don't need the temp var stringBundle...but if it's for readability, maybe use let instead of var? while you're here, can you add some parens around the == clause just to be clear? + if (!oX.@type == "smtp") Everything else looks fine, thanks!

It was for wrapability, and to match the usage a dozen lines up, but I can let them both, sure.

- assert( ! this.result, "Call already returned"); + assert(!this.result, "Call already returned"); Can you leave this, please? I put spaces around the ! quite intentionally, because it's very eas to overlook otherwise. (same thing dozens of times) + if (!oX.@type == "smtp") Good catch, bienvenu. That looks like a bug ("!" takes precedence and the expression then makes no sense and is never true), and it should of course just be != . Looks like bad refactoring in a hurry.

Yeah, I realized that you put the spaces there intentionally, but as you know you don't entirely get to pick your coding style while writing Mozilla code. accountcreation has 13 |( ! [a-zA-Z]|, the whole tree has 28 in \.js$, the rest in js engine tests, accountcreation has one |(! [a-zA-Z]| and the whole tree has 53, accountcreation has 37 |(![a-zA-Z]| and the whole tree has mxr's bignumber, "over 1000". If we decide tree-wide to change, or even to change only for new code, fine, but we're not going to make that another "first author of a file gets to choose" thing, much less an "author of a particular line of code gets to choose" thing.

moving to m5 - Phil is making good progress on the reviews. Bryan has a few things he needs to cleanup. Re the spaces, I agree with Phil - reviews and sr's always say for those to be removed.

Comment on attachment 372355 [details] [diff] [review] Make email address and hostnames ltr in rtl locales - checked in http://hg.mozilla.org/comm-central/rev/f525b7b9b3f5

Comment on attachment 372354 [details] [diff] [review] Review nits, round 1 - checked in http://hg.mozilla.org/comm-central/rev/3aa1bf107060

Oh, no wonder it looks like we ignore your changes of port and protocol: onGo calls _startProbingIncoming, which still thinks it's responding to a blur on hostname and merrily sets protocol/port/socketType to undefined. ui-wanted: if I change the hostname, I might have also changed one of the other bits, or I might not have because I didn't know anything other than the right hostname, or I might not have changed them because they happened to be perfect at the time I stopped probing. I can see how to make it work with buttons that say essentially [Start probing from scratch] and [Try this and nothing else], or with the horrid UI (but perfect behavior) of blanking the port and selecting a blank item at the top of the protocol and socketType menus while entering edit mode, but I don't see any way of divining whether something unchanged was unknown, or already right. Maybe in this case the horrid way is the right way, and "left blank means you figure it out" will make sense to people?

If the user explicitly changes the port and/or protocol, can't we lock those down and make _startProbingIncoming not set them to undefined? Then the issue becomes, what if the user got either of them wrong? I suppose if we ultimately fail, we could clear the fact that the user set the port and/or protocol and start guessing again, if something else (e.g., username/hostname) changed.

I had started on changes that I thought could fix this, but it took longer than I expected to put them together. I was making a menu list widget that contained all the valid probed information, with an additional list item for "Custom...". This menu list sat above the existing form entry which was just visibility:hidden so it would take up the right amount of space. [ IMAP mail.example.com - 143 TLS | v ] | POP mail.example.com - 67 TLS | | POP mail.example.com - 43 None | | Custom... | '----------------------------------' When you selected the "Custom..." it showed our current entry form with our "best guess" (top value) pre-filled in. From there everything was manual entry without trying to stick certain values and keep others. With the richlist box I styled the different pieces so I don't think it looked too bad. (|| == graytext) *PROTOCOL* HOST - |PORT| |SECURITY| But I don't think my dev-foo is quite up to making this happen, or at least I failed at getting it to work correctly.

Onchange may be the best we can do, despite not working for two of my use cases: Smartypants doesn't like us poking around in the wrong places, can't believe we got rid of the wizard, stops probing as quickly as possible, but happens to stop us with his chosen set being probed. He has to either change everything, and then change it back, or know that "Go to advanced settings" means "Create it now." Mr. Patient waits through the three or four minutes of probing, realizes we didn't try the right hostname for his POP/110/None account, changes the hostname, and sits through three or four minutes of probing to get back to there with the right hostname. Sadly, my [ ] v | POP | | IMAP | '------' idea, with leaving it on the blank choice to mean "restart probing this" didn't survive getting home and actually looking at an autoconfig build, since I'd forgotten that there aren't any labels for those menus, so unless an a11y review winds up requiring a label, that's out - I'd do that for "Protocol: [ ] v" but not for "[ ]v".

I was also very confused about the mixing of probing and manual editing (besides the fact that it doesn't work right atm). When I edit manually, I don't want any probing. I can see that a user may know the hostname, but not the port and protocol, and/or not SSL and auth capability. So, I think we should have a "manual edit" mode/page, where the hostname is empty by default, and the other values are defaulting to "auto". If the user enters a setting, we use that (and if it fails, we go back to that screen - I do *not* want other values to be used in that case). If the user leaves the default of "auto", we probe that particular setting.