Overview: As referenced in bug# 14354... The Account Setting's Outgoing SMTP Server panel should have support for multiple ports. UI work needs to be done and possibley the backend.

massive reassign of account manager bugs -> sspitzer please feel free to put me back on the CC if you have any questions/comments

mass re-assign of account manager bugs to racham.

adding john

*** Bug 85403 has been marked as a duplicate of this bug. ***

Changing platform to all, since my linux bug was marked as a dup of this.

*** Bug 86796 has been marked as a duplicate of this bug. ***

This patch enables setting non-default port for SMTP server (in mailnews prefs panel).

I would really like the backend of this patch fixed as it fixes bug 106631. Bug 106631 has also a patch (more or less a hack) which, if applied will 'rot' that one. Perhaps a dependency should be added between both. Denis - you should perhaps break this patch in 2 (backend and UI), so that if the backend is applied, bug 106631 could be solved by modifying user preferences. - I've also had a look to the backend section of your patch (id=58524) and I have some comments plus some proposed clean-ups. [Note: I do not have any kind of knowledge about the code involved] The split and my comments will perhaps help for the whole patch to be reviewed/applied quicker. From attachment id=58524 RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpServer.cpp,v [...] +nsSmtpServer::GetPort(PRInt32 *aPort) +{ + NS_ENSURE_ARG_POINTER(aPort); NS_ENSURE_ARG_POINTER(aPort); should perhaps go after the variable declarations (2 lines under). That's how it is done in all the other nsSmtpServer::GetXXX() methods. It is perhaps a security against build bustages with some compilers. + nsresult rv; + nsCAutoString pref; + nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); missing a check: if (NS_FAILED(rv)) return rv; + getPrefString("port", pref); missing default initialization: *aPort = PORT_SMTP; (add the missing include file as well) + rv = prefs->GetIntPref(pref.get(), aPort); + if (NS_FAILED(rv)) *aPort = 25; replace this if [...] with rv = prefs->GetIntPref(pref.get(), aPort);); if (NS_FAILED(rv)) rv = getDefaultIntPref(prefs, 0, "port", aPort); // note rv is not needed here... cf clean-up section. + return NS_OK; // not needed cf clean-up section. +} + +NS_IMETHODIMP +nsSmtpServer::SetPort(PRInt32 aPort) +{ + nsresult rv; + nsCAutoString pref; + nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); missing ?? if (NS_FAILED(rv)) return rv; + getPrefString("port", pref); + if (aPort) + return prefs->SetIntPref(pref.get(), aPort); + else + prefs->ClearUserPref(pref.get()); /* XXX or set it to default port? */ + return NS_OK; +} I am not sure of this big if (aPort) [...] It looks like copied from the nsSmtpServer::SetHostname(aHostName) method ;) I would just have: return prefs->SetIntPref(pref.get(), aPort); [...] @@ -204,7 +208,7 @@ urlSpec += (const char*)aSmtpHostName; urlSpec += ':'; - urlSpec.AppendInt(SMTP_PORT); + urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT); I am not sure that this line is useful as we already have the default port value set to SMTP_PORT in getPort(). +++++++++++++++++ other clean ups +++++++++++++++++ I've added some other comments and cleaning up. in file mailnews/compose/src/nsSmtpServer.cpp: - in line 154: nsSmtpServer::getDefaultIntPref doesn't need a return value as it always return NS_OK. (appears misleading to have variables affected to that return value, variables never used) Should we replace if (NS_FAILED(rv)) { // last resort *val = defVal; rv = NS_OK; } return rv; by if (NS_FAILED(rv)) { // last resort *val = defVal; } as a consequence, - line 123, method nsSmtpServer::GetTrySSL(...) doesn't need a parameter neither while method nsSmtpServer::GetAuthMethod(...) returns rv Same for getPrefString() which always return NS_OK - line 105, else not needed (because return value from ClearUserPref() not used. - line 205 else not needed. - line 403 else not needed. - line 427: rv for call to SetRedirectorType(...) ignored - line 90, 102, 184, 202, 400, 413 missings NS_FAILED(rv) checks after do_GetService() - All these "if (NS_FAILED(rv)) return rv;" should perhaps be NS_ENSURE_SUCCESS(rv, rv); // like in line 440? - no error checking in method nsSmtpServer::clearPrefEnum() after call to ClearUserPref(...) - methods: nsSmtpServer::GetHostname(), ::GetUsername() lacks NS_ENSURE_ARG_POINTER(trySSL); ++++++++++++++ Hope it helps.

> - you should perhaps break this patch in 2 (backend and UI), so that if the > backend is applied, bug 106631 could be solved by modifying user preferences. Jerome, UI part is just 4 more lines in JS plus some xul cut-n-paste. It so trivial , so it can be easily checked in together with backend part IMHO > replace this if [...] with > rv = prefs->GetIntPref(pref.get(), aPort);); > if (NS_FAILED(rv)) > rv = getDefaultIntPref(prefs, 0, "port", aPort); Heh, getDefaultIntPref will always return zero ;-) > if (aPort) > return prefs->SetIntPref(pref.get(), aPort); > else > prefs->ClearUserPref(pref.get()); > > I am not sure of this big if (aPort) [...] > It looks like copied from the nsSmtpServer::SetHostname(aHostName) method ;) > I would just have: > > return prefs->SetIntPref(pref.get(), aPort); I disagree. This is how SetPort implemented for other server as well - Imap, Pop etc. This way we not bloating prefs with unneeded values.... >- urlSpec.AppendInt(SMTP_PORT); >+ urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT); > > I am not sure that this line is useful as we already have the default port value > set to SMTP_PORT in getPort(). This is harmless and protects (in some sense) against bad values in prefs :-) - line 105, else not needed (because return value from ClearUserPref() not used. - line 205 else not needed. - line 403 else not needed. > - no error checking in method nsSmtpServer::clearPrefEnum() after call to > ClearUserPref(...) What we would do with them?

> Jerome, UI part is just 4 more lines in JS plus some xul cut-n-paste. > It so trivial , so it can be easily checked in together with backend part IMHO That's ok for me. BTW, cut-and-pasted has always been the reason for half of my bugs :) >> replace this if [...] with >> rv = prefs->GetIntPref(pref.get(), aPort);); >> if (NS_FAILED(rv)) >> rv = getDefaultIntPref(prefs, 0, "port", aPort); > Heh, getDefaultIntPref will always return zero ;-) ;) OK replace it by if (NS_FAILED(rv)) getDefaultIntPref(prefs, 0, "port", aPort); I still think the call to getDefaultIntPref() can help. Otherwise it is inconsistent with the fact tha all other preferences could have a default value but not that one. >> if (aPort) >> return prefs->SetIntPref(pref.get(), aPort); >> else >> prefs->ClearUserPref(pref.get()); >> >> I am not sure of this big if (aPort) [...] > I disagree. [...] This way we not bloating prefs with unneeded values.... You're the boss >>- urlSpec.AppendInt(SMTP_PORT); >>+ urlSpec.AppendInt((aSmtpPort >= 0) ? aSmtpPort : SMTP_PORT); >This is harmless and protects (in some sense) against bad values in prefs :-) agreed >> - no error checking in method nsSmtpServer::clearPrefEnum() after call to >> ClearUserPref(...) > What we would do with them? I do not know. I just raised the issue. What about returning it? So, apart from the missing getDefaultIntPref(), I am happy with your new patch. Thanks a lot! I would really like this bug fixed. There are 9 votes for bug 106631. They could [should] come on that one. Let's try to get somebody review that patch.

>I would really like this bug fixed. There are 9 votes for bug 106631. They could > [should] come on that one. Let's try to get somebody review that patch. How'd you do it? Mailnews team looks totally deaf these days... CC-ing mscott and bievvenu, maybe this will help...

Try tracking down the mailnews people on IRC to request a review. It's very easy to miss a bug that needs review in the mass of bugzilla notifications they probably receive.

we're in a performance lockdown for the next month (and have been for the past month). During this time, we're not working on new features, only performance,footprint, and memory leaks, and recent regressions.

As one can see from all the duplicate bugs, this is a showstopper for many people (including myself). Sending mail via a different ports *did* work in 0.9.4 - I would call that a recent regression. 0.9.9 is too far away.

I don't disagree, but it's not up to me. I'll try to get special dispensation from the powers that be.

Adding keywords for Pastis51. This has worked in 0.9.4 -> regression And changing summary to make it understandable.

*** Bug 106631 has been marked as a duplicate of this bug. ***

Your patch looks good, I enhanced it a bit: - I removed the hard tabs, replaced with spaces. - I changed nsSmtpServer::GetPort slighty, to have a more similar error handling as GetHostname. - There are two listboxes in preferences that list the hostname of a SMTP server, in "Advanced Account Settings", and in "Advanced Outgoing Server (SMTP) Settings". If a nonstandard-port is used, those strings now show the port number, too. I tested sending to default and non-default ports, both work.

Please keep in mind bug 2679 when designing the interface. At some point there will be a difference between "default" and "25".

Applied the patch to the 0.9.7 source. It works :) I decided to start over from scratch and deleted my ~/.mozilla. Then I discovered a quirk... If you use different localhost ports to (ssh) tunnel your outgoing emails then you will not be able to select them in the "account settings dialog"/advanced tab. Only one localhost entry is visible. The others appeared after I edited the servernames to mailfoo1 and mailfoo2. I aliased the names in my hosts file and it works. But that's a *bad* workaround ;)

*** Bug 117229 has been marked as a duplicate of this bug. ***

*** Bug 117905 has been marked as a duplicate of this bug. ***

*** Bug 118369 has been marked as a duplicate of this bug. ***

This bug shows up almost every day at npm.mail-news and I've seen many people ask why it doesn't work in #mozilla, I think it would be a good idea to get the patch in soonish... Here are a two posts from npm.mail-news, only from this week, from people suffering the regression: <news://news.mozilla.org:119/3C3C7A8A.6DD962F4@sun.com> <news://news.mozilla.org:119/3C3B2C33.7030809@informatik.uni-bonn.de> To reply to comment 14: This is not a feature, it is a regression that many people are suffering from - regressions are allowed in according to sspitzer's initial email. Can this patch get r/sr now?

John, re your comment 20, I think my patch is not in conflict with a need to have a different default port in the future, or do you? The current code simply assumes, if there is no specific port configured, we use 25. In the future, if other default ports are needed, we simply must make sure that port numbers are explicitly saved, and it will work with the code suggested here.

What is in conflict is the assumption that there is a single default port. With bug 2679, there would not be a single default port, the default behavior would be to first try port 587 then try port 25. That default behavior would be different than what one would get by explicitly configuring either port 587 or port 25.

John, thanks for clarifying. Message submission seems to be described in rfc 2476. It seems to me, what is requested is an "additional initial test action, with fallback if necessary". As we need to fallback in case of trouble, and the fallback port is the port requested in this bug 52330, it might make sense to add an additional preference to the smtp settings. We could have a checkbox that says "try to use message submission protocol first, using port [587] <- configurable". If you think it makes sense, that option could then be enabled by default as soon as we have it. I vote for storing that preference separate from what we otherwise need for standard smtp. And I think in case we want to offer overriding the port number to be used for the message submission port could be configurable and separately stored. I'm trying to find out, whether we need to care for bug 2679 now, or whether we can delay it until we are ready to support message submission. John, do you agree that my suggestion is viable, or do you request the current patch to this patch needs to be changed now?

We don't have to implement bug 2679 now, but we should avoid designing the interfaces in such a way as to make implementing it unnecessarily difficult or cumbersome. An additional preference would be unnecessarily complex and confusing. The only time you would want port scanning is for the default case, when the user doesn't know enough or hasn't bothered to configure the SMTP port. The better interface would be "if the port field is blank, do port scanning". So I would much prefer if the patch did not automatically convert between blank and 25 in the preferences interface. The SMTP protocol code should simply interpret a blank preference value as 25.

Here is new patch. Now UI doesn't replaces empty port with port 25. Also I replaces a bunch of tabs with spaces.

*** Bug 120769 has been marked as a duplicate of this bug. ***

*** Bug 121670 has been marked as a duplicate of this bug. ***

*** Bug 122814 has been marked as a duplicate of this bug. ***

*** Bug 123498 has been marked as a duplicate of this bug. ***

*** Bug 123566 has been marked as a duplicate of this bug. ***

*** Bug 125012 has been marked as a duplicate of this bug. ***

This must be heading for most-duped status. Can someone clarify what is holding this bug up?

*** Bug 125269 has been marked as a duplicate of this bug. ***

I really like your product, please continue to support this in the future! I must use SSH tunneling for sending company mail from home. I am using Netscape 6.21 which works great, our administrator helped me to configure it. Is it a mistake that this bug says nsbeta1- (I looked in keyword help), and target milestone says 0.9.9, but keyword says 0.9.7? Thank you

Updating keyword to mozilla0.9.9, to match target milestone.

*** Bug 125598 has been marked as a duplicate of this bug. ***

Just added one thing: if no port specified and hostname contains ':', split hostname string into host and port portions...

[Spam] This bug has had a patch for more than one month. Still no reviews. It is a known regression with more than 20 votes (and probably more if you add the ones from the 20 duplicates). It is part of the top 30 non-fixed most reported bugs and part of the 7 non-fixed most reported bugs targeted for 0.9.9. Could someone please have a look at the patch? Otherwise it will never make it for 1.0.0 Cc Asa.

This will have to wait some more.

Can you give a reason why this has to wait some more?

The people who are the best reviewers for this have other bugs that they need to work on first.

Well... people who are regulars on the project should really be a little more welcoming than that to people who are trying to help out... Marking this as blocking "bugs with patches that need review before they rot". Will try to get to it this Friday.

BTW, fix is quite simple, it won't take too much time to review...

Given the fact that the mail team does not have much time to help on this, let's try to make the patch perfect, to minimize the risk of further delays because of unaddressed issues. If I were to review it, I would have the following comments: - the patch has hard tabulators in it. That is not allowed. Please replace with spaces. Also make sure that both indendation and bracket placement matches the style of the files we modify. - the patch should allow migration from those version of Mozilla which worked with the :port notation. I.e., if you upgrade from 0.9.4, and you currently have a host:port configuration, the code should detect that, and migrate the preferen automatically. I'm not sure whether we are allowed to write the updated prefs to disk immediately. Ideally, the migrated prefs should be used in memory and displayed correctly in the pref screens, but the new pref should now be written to disk until the user ok's the pref dialog containing the migrated setting. - implementation of nsSmtpServer::GetPort should more closely match the implementation of GetHostname. I suggest that instead of *aPort = 0; rv = prefs->GetIntPref(pref.get(), aPort); return NS_OK; we should write: rv = prefs->GetIntPref(pref.get(), aPort); if (NS_FAILED(rv)) *aPort = 0; return NS_OK; - Implementation of GetDisplayName(): We should explicitly set port to zero, if we detect failure of GetIntPref(), because in that cases the value of given out parameters may be undefined. I suggest that instead of PRInt32 port = 0; rv = prefs->GetIntPref(ppref.get(), &port); if (port<0) port = nsISmtpUrl::SMTP_PORT; we should write: PRInt32 port = 0; rv = prefs->GetIntPref(ppref.get(), &port); if (NS_FAILED(rv)) port = 0; As soon as we have that updated patch, we should test it. We could create test builds, and people subscribed on this patch could help testing. This will allow us to detect early any issues not addressed by the patch. If you can please help to created an updated patch. I will work at the weekend and try to help, too. I can also create test builds for Linux and Windows.

Also removed few unneeded (const char*) casts

please do 'man diff', if you don't know what -w means!

Denis, does your latest patch address the "the patch should allow migration from those version of Mozilla which worked with the :port notation" comment? (It looks like it makes the other changes kaie suggested..

Yep. And it will migrate to new version (using port pref) after user visited prefs panel and pressed OK button

OK. Reading through that code it seems quite sensible to me... hwaara, would you be able to revie this?

Comment on attachment 70913 [details] [diff] [review] diff -w version of the above >@@ -85,6 +80,8 @@ > attribute nsIAuthPrompt authPrompt; > attribute nsIInterfaceRequestor notificationCallbacks; > attribute nsISmtpServer smtpServer; >+ >+ const PRInt32 SMTP_PORT = 25; > }; I suggest you rename this constant to DEFAULT_SMTP_PORT to avoid confusion in the future. >+// if GetPort returns 0, it means default port Instead of making all the callers check for 0 and use the default port, can't you just make this getter smart enough to return 25 if necessary? Everything else looks great. r=hwaara with the minor changes above.

> Instead of making all the callers check for 0 and use the default port, can't > you just make this getter smart enough to return 25 if necessary? It was so in early versions of the patch :-) I don't recall now why I changed it, but ok, I'll change it back

Did you do it, because of John's request to support the case, where the default port could be something else but 25? Instead of hard coding 25 everywhere, by storing zero we remember the state whether a port has not been set yet. In that case, a global option changing the default port, would automatically be used.

"global option changing the default.."? The point is, either you use your customized port (in which case it isn't default anymore) or you use the default, which will be 25 - because it's most common one.

See John G. Myers comments #27 http://bugzilla.mozilla.org/show_bug.cgi?id=52330#c27 and #29 http://bugzilla.mozilla.org/show_bug.cgi?id=52330#c29

John requested in comment 27 and comment 29, to NOT store port number 25 in preferences. > > Instead of making all the callers check for 0 and use the default port, can't > > you just make this getter smart enough to return 25 if necessary? > > It was so in early versions of the patch :-) > I don't recall now why I changed it, but ok, I'll change it back Therefore we should not do it. In order to support bug 2679, we should leave it as the patch currently is, i.e. check for !port in the code, which means, try the default port. Håkan, do you agree? > I suggest you rename this constant to DEFAULT_SMTP_PORT to avoid confusion in > the future. Ok, good idea. > > Denis, does your latest patch address the "the patch should allow migration from > > those version of Mozilla which worked with the :port notation" comment? (It > > looks like it makes the other changes kaie suggested.. > Yep. > And it will migrate to new version (using port pref) after user > visited prefs panel and pressed OK button Your code tries to do automatic conversion in one place only: When the user ok's mail preferences, and the hostname is still in the old notation, and no port has been specified yet. In that case, a correct conversion is done. However, sending mail does not work until you have done the above. Thinking more about it, we have two options how the application should behave. Option 1) Suppose we do automatic migration on program start. This will mean: - Sending mail will work immediately, no user action required. - We automatically modify the profile, which might not be what the user wants, because on going back to older version of Mozilla, sending will not work. I don't know whether me must support this, but if we don't, we will break the environment of those people who just want to try out a newer Mozilla, but decide to continue to use the older version (with the same profile). - The patch needs to be enhanced more, because we need to find the right place to add the automatic migration. Option 2) Suppose we do NOT migrate automatically on program start. This will mean: - Sending mail will NOT work immediately when switching to the newer version of Mozilla. - We do not modify the profile, therefore temporarily trying out a newer version of Mozilla will not change the profile that works with the older version of Mozilla. - A user has to know that it is required to make the change. However, those users who found the hack in the past to use to :port notation, will likely open the preferences dialog to see what's configured, and notice the new field for the port value, and fix it manually. If we assume that, that automatic conversion on saving is not even necessary. I'd now suggest: Let's go with Option 2. Be safe (no automatic migration) and simple (no more code needs to be developed). Suppose only power users used the alternate port in the past, so we only risk to surpise power users. In addition, that alternative port was not really supported in the past, so no need for migration. On the other hand, that automatic conversion on save can't hurt, so let's leave it in. Please comment if you disagree. If you agree, I'll make test builds available.

Great to see that this is being worked on. With regard to the comments about conversion from 'host:port' notations: I agree that both solutions may cause problems for some users. So, at the risk of being naive: why not be flexible and allow that system to work too? If someone has host:port in the profile, leave it that way, and interpret it correctly. Then no-one will have upgrade problems.

> However, sending mail does not work until you have done the above. What makes you say that? Did you ever tried it? If no, please do it now. Sending mail works even with old profile with 'server:port' hostname w/o visiting preferences page. This is what "if(!PL_strchr(aSmtpHostName':'))" for (in nsSmtpService.cpp).

> > However, sending mail does not work until you have done the above. > > What makes you say that? Did you ever tried it? If no, please do it now. > Sending mail works even with old profile with 'server:port' hostname w/o > visiting preferences page. > This is what "if(!PL_strchr(aSmtpHostName':'))" for (in nsSmtpService.cpp). Oops, sorry. I missed that change. I just tested, and indeed, it works. You added the workaround that was originally suggested in bug 106631 - but which had been rejected at that time. I agree the code makes sense, because it makes it work without having to worry about the migration issues I mentioned. But now that the other requests from the mail team have been implemented, I'd argue this change helps migrating users, and should be used. I finally think the last patch is very good. Given the fact the patch makes it work with both preference notations, the only thing that is left is the question: Should pressing enter in preferences automatically split host:port or not? If we not do the automatic split, we ensure that older versions of Mozilla will still work with the profile. The user has to manually make that change if he wants to, but is not required to.

we'll need jglick to approve the UI changes and update the spec. http://www.mozilla.org/mailnews/specs/accounts/#AccountSettings

as far as UI goes, this patch also affects the "edit" advanced SMTP server dialog as well, right?

Yes, because "edit" uses same smtp server overlay.

The only UI change is adding a "Port" text field to the "Outgoing Server Settings" panel and the Advanced "SMTP Server" dialog (used to add a new, or edit an existing, SMTP server)? And the text field will be pre-populated with an appropriate default? http://www.mozilla.org/mailnews/specs/accounts/images/AcctServerSmtp1.gif http://www.mozilla.org/mailnews/specs/accounts/images/AcctSmtpAdvDiag1.gif If that is the case, UI change is fine with me and I will update the spec.

> The only UI change is adding a "Port" text field to the "Outgoing Server > Settings" panel and the Advanced "SMTP Server" dialog (used to add a new, or > edit an existing, SMTP server)? Yes, only the dialogs you listed have been changed. > And the text field will be pre-populated with > an appropriate default? Good point, not yet, let's fix that. Right now, the port number field is empty when the default port is configured. I suggest: When these dialogs are constructed, and no port setting is detected, we fill the edit field with the default port number. On closing the dialog, we check whether the port field contains the default port number. If it does, we set the preference internally back to the default setting (which is zero), and store that. (Sideeffect: I removed the automatic host:port split on save. I think we do not need it, because the smtp procotol code works anyway.) I'll attach a new patch. That patch will implement my suggestion and prefills the port number fields. It also contains the suggested global replacement SMTP_PORT => DEFAULT_SMTP_PORT.

When bug 2679 will be fixed, "server" and "server:25" will mean different things!

> When bug 2679 will be fixed, "server" and "server:25" will mean different > things! *sigh*, this is really complicated :) In my opinion, as long as bug 2679 is not implemented, server IS identical with server:25. Maybe it is not identical as soon as message submission protocol becomes available, but I suggest to fix that later when bug 2679 gets implemented. In my opinion, if bug 2679 gets implemented, the user should have a way to control this in the UI. The application should not just try out port 587, without power users being aware of that fact. So I'd personally prefer a setting (later), that controls whether "port submission on 587" will be tried or not. By the time we have that new setting, we can make the UI more intelligent. It will still be detectable whether a user has ever configured a nonstandard port or not, because the port setting will be zero. By the time bug 2679 gets fixed, the user interface can detect that, and indicate in the UI whatever is necessary. It could, for example ,show: - default (first try port submission, then try SMTP) - only use port submission - only use SMTP _____ | 587 | port for message submission ------- _______ | 25 | port for SMTP ------- As soon as message submission protocol becomes available, power users will recognize that anyway, and manually configure their client appropriately. If users had ever configured an alternate port, the future implementation could default to the setting "only use SMTP". But I really think we don't have to solve that now. The only thing that we are doing is: We remember correctly, whether a user has ever configured an alternate port or not, and that is all what the implementors of bug 2679 will need IMHO. And the implementators of bug 2679 can then decide how Mozilla should behave, depending on what has been configured in the past.

> (Sideeffect: I removed the automatic host:port split on save. > I think we do not need it, because the smtp procotol code works anyway.) This makes new UI useless/redudant ;-)

Test builds that use the previous patch are available at http://www.kuix.de/mozilla/52330/

> > (Sideeffect: I removed the automatic host:port split on save. > > I think we do not need it, because the smtp procotol code works anyway.) > > This makes new UI useless/redudant ;-) I don't think so. The :port notation was never officially supported. While your patch support migration, because of the smtp protocol code behaviour, we are free to remove support for that notation later, but the UI will survive. And the new UI is consistent with what we are doing for other email protocols, that is offering a edit field for port configuration, not forcing a user to user host:port notation.

Please do not fill in the default port. Please see comments #27 and #29.

In response to comment #75, if mozilla fills in the default port, how do you expect it to later detect whether or not the user explictly configured port 25? With bug 2679 implemented, the UI would remain the same. If the port field were left blank, the code would do the default behavior of first trying port 587 then 25. The user could disable checking port 587 by explicitly configuring port 25 and could disable checking port 25 by explicitly configuring port 587. It does not make sense to configure two ports--if the user is going through the trouble of changing the defaults, they can just configure the one port their particular server uses.

Comment on attachment 71473 [details] [diff] [review] New patch with filled port fields (diff -w version) I'll let mscott deal with the substantive issues, if any, but a stylistic comment first: We like the error returns to be on a separate line (makes it easier to set breakpoints) so instead of if (NS_FAILED(rv)) return rv; should be if (NS_FAILED(rv)) return rv; Do we really need to show the port number in the display name?

> In response to comment #75, if mozilla fills in the default port, how do you > expect it to later detect whether or not the user explictly configured port 25? Sorry, I was assuming this does not make a difference. > With bug 2679 implemented, the UI would remain the same. If the port field were > left blank, the code would do the default behavior of first trying port 587 then > 25. The user could disable checking port 587 by explicitly configuring port 25 > and could disable checking port 25 by explicitly configuring port 587. It does > not make sense to configure two ports--if the user is going through the trouble > of changing the defaults, they can just configure the one port their particular > server uses. Thanks for explaining it again. So I think we have two options: Option 1) ========= Leave the port field empty, as it is currently implemented. This behaves different from the other mail preferences, which always show a number in the port field. If everybody likes that approach, it's fine with me, too. However, Jennifer expected that field would be filled, so I'd like to make another suggestion: Option 2) ========= Server ________________________ x use default port O use alternate port: _____ I.e., a radio button, and a field to explicitly enter the port number. If "use default port" is selected, the edit field for the alternate port is disabled. Jennifer, John, what do you think?

As a consequence of the previous request, the patch will have to be updated, to always store the port number, even if it is equal to the default port. If "default port" is selected, the preferences would store port number zero. If "alternate port" is selected the preferences would store the entered port number.

> I'll let mscott deal with the substantive issues, if any, but a stylistic > comment first: I'd like to state my commitment that I'll do whatever is required to land this patch for Mozilla 1.0. > We like the error returns to be on a separate line (makes it easier to set > breakpoints) so instead of Sure, I will fix this in the patch. > Do we really need to show the port number in the display name? The port number is only displayed, if it is different from the default port number. It seems one scenario for the alternate port is tunneling. And if you use tunneling, all your host names will be the same. I think it is a common scenario to have two alternate servers for sending mail. So both host names would be displayed as "127.0.0.1" and you are only able to differ them by the port number. I notice that the current implementation uses a display notation of "host:25". As we do not support that notation, should we display "host (port 25)" instead?

Sorry, for the spam, I correct myself: > The port number is only displayed, if it is different from the default port number. In the alternate model, having a checkbox to decide whether to use the default port or an alternate port, this sentence is correctly: "The port number in the display string is only displayed, if it has been explicitly configured". I think this affects only the advanced smtp server dialog, where multiple servers are listed in the list box.

Re comment #82, either option is fine by me. As long as we can later distinguish between "default" and "explicit 25".

jglick, can you please review this UI? It realizes your wish to not have empty port fields.

This patch implements the alternate UI suggestion as shown by the previous screen shots. The code realizes jgmyers's request to be able to distinguish the different user choices. The code layout has been fixed to follow bienvenu's request.

>Option 1) >========= >Leave the port field empty, as it is currently implemented. This is fine with me. Option 2) When you have radio buttons, you should have descriptive test above the radio buttons, so another line would need to be added. When sending messages: x use the default port O use an alternate port: _____ Hence, to keep things simple and take up less space, Option 1 is fine with me.

Marking nsbeta1-. You can still get this into 1.0 with the proper reviews and drivers approval. I'd like to reassign this to kye@gmx.de but bugzilla doesn't appear to be letting me do it. Moving to 1.2. Once it gets reassigned you can put this back in 1.0

Assigning to me, back to milestone 1.0.

Thanks for the confirmation on the easier UI. (I'm obsoleting the screenshot images that we are not be using.) This patch has all the code layout changes from the previous patch, but uses the simpler UI, where the port field is empty in the default case. Please review the code in this patch.

Comment on attachment 72835 [details] [diff] [review] (same as previous, diff -w -u10 for easier reviewing) Looks good to me. I think it would be best if bienvenu or mscott did official review though, since they know more about this code than me. I suggest you email them a review request, and CC reviewers@mozilla.org

Comment on attachment 72835 [details] [diff] [review] (same as previous, diff -w -u10 for easier reviewing) r=bienvenu, I'd try mscott for an sr.

Version 1.103+ of nsSmtpService.cpp "breaks" the patch (can't just run patch anymore). This makes the patch unusable on mozilla 0.9.9. I'd be really happy if I got a new patch and someone *please* integrate this patch into the CVS. Thanks! // Johan

> Version 1.103+ of nsSmtpService.cpp "breaks" the patch (can't just run patch > anymore). This makes the patch unusable on mozilla 0.9.9. Just a minor conflict in the context, I'm attaching a new patch that applies cleanly. > I'd be really happy if > I got a new patch and someone *please* integrate this patch into the CVS. I already posted twice to reviewers@mozilla.org, but no reaction yet.

Brendan, can you please superreview?

> I already posted twice to reviewers@mozilla.org, but no reaction yet. You have to add the relevant people to the To addresses, see <http://www.mozilla.org/hacking/reviewers.html>.

> You have to add the relevant people to the To addresses, see > <http://www.mozilla.org/hacking/reviewers.html>. I did, it was addressed to mscott as bienvenu suggested, but I guess he did not have time.

Comment on attachment 73961 [details] [diff] [review] (Resolved small conflict to make it apply to current tree) with r=bienvenu, I'm happy to consider stamping sr=brendan@mozilla.org. Note to anyone who cares: mailnews/compose/public/*.idl files do not agree on whether to use long or PRInt32, e.g. I think the latter are better, at a glance you don't have to shift frame from C/C++ (where long is not a portable type in general) to IDL (where it has a specific XP size). But why not use PRUint32 and avoid the ugly cast in nsSmtpProtocol.cpp? IP ports are unsigned, so I think the IDL should use an unsigned integral type. Another thing: if LoadUrl fails, won't we leak due to the QI-result non-COMPtr? + NS_ADDREF(smtpProtocol); + rv = smtpProtocol->LoadUrl(aUrl, aConsumer); // protocol will get destroyed when url is completed... + smtpProtocol->QueryInterface(NS_GET_IID(nsIRequest), (void **) aRequest); + NS_RELEASE(smtpProtocol); I think you want to QI iff NS_SUCCEEDED(rv). Fix these issues if you agree, and sr=brendan@mozilla.org. /be

> mailnews/compose/public/*.idl files do not agree on > whether to use long or PRInt32, e.g. I think the latter are better, at a > glance you don't have to shift frame from C/C++ (where long is not a portable > type in general) to IDL (where it has a specific XP size). In our patch, we introduced a variable with type "long". I changed that to PRInt32. (I can't speak for the other types in the existing mail code that we are not touching with this patch.) > But why not use PRUint32 and avoid the ugly cast in nsSmtpProtocol.cpp? > IP ports are unsigned, so I think the IDL should use an unsigned integral type. When I first read this sentence, I agreed this were a good idea. I started to change all occurrences in our patch to unsigned. However, I finally did not do it, because: - the preferences code stores a signed int - I also would have to adjust nsSmtpService::GetDefaultPort, or the "ugly cast" would happen there. - However, method GetDefaultPort is widespread across the source tree, and everybody used a signed int, so I think we should be consistent. Therefore, in order to avoid the ugly cast, my next idea was to change the type of m_port. In the end I saw that this variable is completely irrelevant. For safety reasons I even scanned the whole sources below mailnews/, and that variable is only accessed inside nsSmtpProtocol::Initialize() and only values are assigned to that variable, but it's contents are never used! I removed this variable from the header, and the useless lines in nsSmtpProtocol.cpp, including the line with the ugly cast. Everything still compiles and works fine. Note that this makes our patch even simpler, because those lines which manipulated m_port where introduced by our patch, and because of that I think there is no need for the mail team to review again. > Another thing: if LoadUrl fails, won't we leak due to the QI-result non-COMPtr? > > + NS_ADDREF(smtpProtocol); > + rv = smtpProtocol->LoadUrl(aUrl, aConsumer); // protocol will get > destroyed when url is completed... > + smtpProtocol->QueryInterface(NS_GET_IID(nsIRequest), (void **) > aRequest); > + NS_RELEASE(smtpProtocol); > I think you want to QI iff NS_SUCCEEDED(rv). The line containing LoadUrl is not changed by this patch. It is not contained in the diff -w patch. We just fixed some whitespace in that area to make it match the general style of the file. Therefore I would like to transfer your comment to a new bug. > Fix these issues if you agree, and sr=brendan@mozilla.org. Thanks a lot!

Bienvenu, to help you agreeing with the new patch, here is what changed since you reviwed it: You saw: + attribute long port; Now it's: + attribute PRInt32 port; You saw: - m_port = SMTP_PORT; + m_port = 0; Now it's: - m_port = SMTP_PORT; Code that assigned values to m_port removed from the patch. New change: Index: mozilla/mailnews/compose/src/nsSmtpProtocol.h =================================================================== RCS file: /cvsroot/mozilla/mailnews/compose/src/nsSmtpProtocol.h,v retrieving revision 1.38 diff -u -d -r1.38 nsSmtpProtocol.h --- mozilla/mailnews/compose/src/nsSmtpProtocol.h 28 Sep 2001 20:06:57 -0000 1.38 +++ mozilla/mailnews/compose/src/nsSmtpProtocol.h 17 Mar 2002 04:36:48 -0000 @@ -161,7 +161,6 @@ PRInt32 m_previousResponseCode; PRInt32 m_continuationResponse; nsCString m_responseText; /* text returned from Smtp server */ - PRUint32 m_port; char *m_addressCopy; char *m_addresses; (I could have omitted this change, but because that variable is never used, I removed it.)

Because I changed the type of port in nsISmtpServer from long to PRInt32, I just feared that I had forgotten to also change this type in the implementation methods GetPort and SetPort. But I noticed that these methods were already using PRInt32, so we improved correctness by changing the interface type. BTW: I filed bug 131522 for the LoadUrl issue Brendan mentioned.

Comment on attachment 74586 [details] [diff] [review] Patch after working on Brendan's suggestions I read the -w -u10 patch, thanks for attaching that. I'd like bienvenu to stamp his r= here, I won't presume to do it for him. /be

Comment on attachment 74586 [details] [diff] [review] Patch after working on Brendan's suggestions r=bienvenu, thx, kye.

Comment on attachment 74586 [details] [diff] [review] Patch after working on Brendan's suggestions a=dbaron for trunk checkin

Patch has been checked in, resolving as FIXED ! Thanks a lot to everbody who helped.

*** Bug 133806 has been marked as a duplicate of this bug. ***

*** Bug 134502 has been marked as a duplicate of this bug. ***

Trunk build 2002-04-01: WinMe, Linux RH 7.1, Mac 10.1.3 Verified Fixed.