952493 - composeMsgs.properties should used string based identifiers rather than numbers.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Suyash Agarwal (:sshagarwal)]

composeMsgs.properties currently uses numbers for its identifiers rather than strings. This makes it harder for localisers to work with, and means that if we need to tweak strings then it isn't easy to change the string identifier to notify the localisers.

We should work on replacing the numbers with strings.
This bug serves the same purpose as bug #858238 and bug #551919.

Comment 1

[:aceman]

This can be done after bug 802266 for the remaining messages. Some of the numeric message IDs are also used as error codes so that needs to be coped with.

Comment 2

[:aceman]

It is quite possible we could remove the FormatStringFromID function from http://mxr.mozilla.org/comm-central/source/mozilla/intl/strres/src/nsStringBundle.cpp
as this code may be the last user of it:
http://mxr.mozilla.org/comm-central/ident?i=FormatStringFromID

(GetStringFromID is still used in some other places).

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

An attempt.

Comment 4

[:aceman]

Comment on attachment 8432217 [details] [diff] [review]
Patch v1

Review of attachment 8432217 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding the localization notes.

Looks good to me.
It builds but I found one strange thing while testing:
I tried to send a message to an invalid address. It got properly rejected by the SMTP server. But the message was:
"An error occurred while sending mail. The mail server responded: e. Please check the message recipient  and try again."
So that looks like the $1%s and $2%s placeholders were not substituted correctly. I had a recipient address filled in the composer.

With proper TB24, the message looks like this:
"An error occurred while sending mail. The mail server responded:  5.5.2 <sdf@asd>: Recipient address rejected: need fully-qualified address. Please check the message recipient sdf@asd and try again."

I think it would be usefull to somehow mark the server response to not flow it into our text. Maybe quoting the $1%s would help. Or wrapping the message like this:
An error occurred while sending mail. The mail server responded:\n$1%s\n. Please check the message recipient "$2%s" and try again.

::: mailnews/compose/src/nsComposeStrings.h
@@ +71,5 @@
>  #define NS_ERROR_ILLEGAL_LOCALPART                  NS_MSG_GENERATE_FAILURE(12601)
>  
> +static nsresult nameForID(nsresult aCode, char16_t** aRetString)
> +{
> +  char16_t* aString;

Does this need the temporary variable? Or can you assign aRetString directly?

Does the function need to return a nsresult? Can it ever fail?

::: mailnews/compose/src/nsMsgPrompts.cpp
@@ +87,5 @@
>    return nsMsgDisplayMessageByString(aPrompt, msg.get(), windowTitle);
>  }
>  
>  nsresult
>  nsMsgDisplayMessageByName(nsIPrompt *aPrompt, const nsString &aName, const char16_t *windowTitle)

Shouldn't the aName be changed to char16_t as the other arguments taking the string ID?

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3720,5 @@
> +        exitString == MOZ_UTF16("smtpAuthChangeEncryptToPlainNoSsl") ||
> +        exitString == MOZ_UTF16("smtpAuthChangeEncryptToPlainSsl") ||
> +        exitString == MOZ_UTF16("smtpAuthChangePlainToEncrypt") ||
> +        exitString == MOZ_UTF16("startTlsFailed"))
> +      FormatStringWithSMTPHostNameByName(exitString, getter_Copies(eMsg));

It seems this would be faster if you keep it as it was comparing ExitCodes and only at last call 
nameForID(aExitCode, &exitString);
FormatStringWithSMTPHostNameByName(exitString, getter_Copies(eMsg));

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +98,3 @@
>  
> +  const char16_t* errorString;
> +  if (!(aCode ==  MOZ_UTF16("errorIllegalLocalPart") ||

Again, can you leave comparing the numeric code?

Comment 5

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.2

Okay, I have tried to make the changes you suggested.
But I am unable to test them
neither gmail.com nor zoho.com prompted for incorrect
recipient address, in each case I got "mail delivered successfully"
and then "mail delivery failure email back".

Thanks.

Comment 6

[:aceman]

Comment on attachment 8433941 [details] [diff] [review]
Patch v1.2

Review of attachment 8433941 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsComposeStrings.h
@@ +69,5 @@
>  #define NS_ERROR_SMTP_AUTH_NOT_SUPPORTED            NS_MSG_GENERATE_FAILURE(12600)
>  
>  #define NS_ERROR_ILLEGAL_LOCALPART                  NS_MSG_GENERATE_FAILURE(12601)
>  
> +static void nameForID(nsresult aCode, char16_t** aRetString)

Would it be possible (and practical) to directly return the char16_t* instead of void? I think I have seen a function similar to this one in concept, that does this.

::: mailnews/compose/src/nsMsgPrompts.cpp
@@ +87,5 @@
>    return nsMsgDisplayMessageByString(aPrompt, msg.get(), windowTitle);
>  }
>  
>  nsresult
>  nsMsgDisplayMessageByName(nsIPrompt *aPrompt, const nsString &aName, const char16_t *windowTitle)

char16_t* aName was not addressed.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3720,5 @@
> +        exitString == MOZ_UTF16("smtpAuthChangeEncryptToPlainNoSsl") ||
> +        exitString == MOZ_UTF16("smtpAuthChangeEncryptToPlainSsl") ||
> +        exitString == MOZ_UTF16("smtpAuthChangePlainToEncrypt") ||
> +        exitString == MOZ_UTF16("startTlsFailed"))
> +      FormatStringWithSMTPHostNameByName(exitString, getter_Copies(eMsg));

This wasn't changed as I asked for?

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +107,5 @@
> +      aCode == MOZ_UTF16("errorSendingRcptCommand") ||
> +      aCode == MOZ_UTF16("errorSendingDataCommand") ||
> +      aCode == MOZ_UTF16("errorSendingMessage") ||
> +      aCode == MOZ_UTF16("incorrectSmtpGreeting")))
> +    errorString = MOZ_UTF16("communicationsError");

Again, I wanted to keep the numeric error codes compared here. I am not sure a string comparison like this even works (I got some compiler warnings in a similar patch I did. So I used .Equals() method to compare the strings.

::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
@@ +1075,5 @@
>  
>      if (NS_SUCCEEDED(aExitCode))
>        return NS_OK;
>  
> +    char16_t* exitString;

Shouldn't this be const? I got warnings about deprecated conversion from the compiler when I used this construct in my patch.

Comment 7

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #6)
Sorry I forgot those changes.
> 
> > +static void nameForID(nsresult aCode, char16_t** aRetString)
> 
> Would it be possible (and practical) to directly return the char16_t*
> instead of void? I think I have seen a function similar to this one in
> concept, that does this.
I thought it would be bad to pass strings. I'll make the change.
> ::: mailnews/compose/src/nsMsgPrompts.cpp
> @@ +87,5 @@
> >  nsresult
> >  nsMsgDisplayMessageByName(nsIPrompt *aPrompt, const nsString &aName, const char16_t *windowTitle)
> 
> char16_t* aName was not addressed.
There are two separate methods |nsMsgDisplayMessageByName| that uses nsString
and |nsMsgDisplayMessageByString| that uses char16_t*
If you want this to be changed to char16_t* a few callers need to be changed
and one method can go away.

> ::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
> > +    char16_t* exitString;
> 
> Shouldn't this be const? I got warnings about deprecated conversion from the
> compiler when I used this construct in my patch.

Okay, will do.

Thanks.

Comment 8

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #7)
> > > +static void nameForID(nsresult aCode, char16_t** aRetString)
> > 
> > Would it be possible (and practical) to directly return the char16_t*
> > instead of void? I think I have seen a function similar to this one in
> > concept, that does this.
> I thought it would be bad to pass strings. I'll make the change.
Yeah, I don't know. In the mean time I'll try to find the usage I have seen doing this. Also there are versions of GetStringByName at http://mxr.mozilla.org/comm-central/source/mailnews/import/src/nsImportStringBundle.h#20 that return char16_t*. Let's ask Neil if that is a good example to follow.

> > ::: mailnews/compose/src/nsMsgPrompts.cpp
> > @@ +87,5 @@
> > >  nsresult
> > >  nsMsgDisplayMessageByName(nsIPrompt *aPrompt, const nsString &aName, const char16_t *windowTitle)
> > 
> > char16_t* aName was not addressed.
> There are two separate methods |nsMsgDisplayMessageByName| that uses nsString
> and |nsMsgDisplayMessageByString| that uses char16_t*
> If you want this to be changed to char16_t* a few callers need to be changed
> and one method can go away.
No, you can't merge them, you can keep both. nsMsgDisplayMessageByName takes a string name and fetches it from the bundle. nsMsgDisplayMessageByString takes the final string (data) and just shows it.
They are different in function. so in that case nsMsgDisplayMessageByName is similar to the other functions you rename from ByID to ByName and should take char16_t as the others do.

Also, is it possible to remove nsMsgGetMessageByID now?

Comment 9

[Suyash Agarwal (:sshagarwal)]

Oh,then there are issues with my patch I think.
I have replaced nsMsgDisplayMessageByID() with nsMsgDisplayMessageByString()
and that should be wrong :(

Comment 10

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #9)
> Oh,then there are issues with my patch I think.
> I have replaced nsMsgDisplayMessageByID() with nsMsgDisplayMessageByString()
> and that should be wrong :(
I just say that nsMsgDisplayMessageByString and nsMsgDisplayMessageByName WERE different. But I see that in current use nsMsgDisplayMessageByString just displays strings retrieved via nsMsgGetMessageByName. Now that nsMsgDisplayMessageByID is unused and can be removed, there is no need for the common part of nsMsgDisplayMessageByID and nsMsgDisplayMessageByName to be split away into nsMsgDisplayMessageByString. So you can merge nsMsgDisplayMessageByString into nsMsgDisplayMessageByName.

Comment 11

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.8

Comment 12

[:aceman]

Comment on attachment 8434706 [details] [diff] [review]
Patch v1.8

Review of attachment 8434706 [details] [diff] [review]:
-----------------------------------------------------------------

Looks better to me now :)

::: mailnews/compose/src/nsMsgPrompts.cpp
@@ +71,5 @@
>    return nsMsgDisplayMessageByString(aPrompt, msg.get(), windowTitle);
>  }
>  
>  nsresult
>  nsMsgDisplayMessageByString(nsIPrompt * aPrompt, const char16_t * msg, const char16_t * windowTitle)

OK, so can nsMsgDisplayMessageByString now be folded into nsMsgDisplayMessageByName? Or is there any other user of it?

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +253,1 @@
>      

Trailing spaces?

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +98,2 @@
>  
> +  char16_t* exitString;

const ?

::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
@@ +1081,2 @@
>      switch (aExitCode)
>      {    

Many trailing spaces in this whole switch.

Comment 13

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #12)
> Looks better to me now :)
Thanks.
> ::: mailnews/compose/src/nsMsgPrompts.cpp
> @@ +71,5 @@
> >    return nsMsgDisplayMessageByString(aPrompt, msg.get(), windowTitle);
> >  }
> >  nsresult
> >  nsMsgDisplayMessageByString(nsIPrompt * aPrompt, const char16_t * msg, const char16_t * 
> OK, so can nsMsgDisplayMessageByString now be folded into
> nsMsgDisplayMessageByName? Or is there any other user of it?
Actually as you said nsMsgDisplayMessageByName() also gets the string from the bundle
whereas ..ByString() simply shows it, so we still need to keep them separate as there
are calls to ..ByString() to just show the message.

> ::: mailnews/compose/src/nsSmtpProtocol.cpp
> @@ +98,2 @@
> >  
> > +  char16_t* exitString;
> 
> const ?
nameForID() takes char16_t* otherwise I'll have to change all of then callers again :)
If you still want all of them to be const, please let me know.

Will fix the trailing spaces.

Thanks.

Comment 14

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #13)
> > OK, so can nsMsgDisplayMessageByString now be folded into
> > nsMsgDisplayMessageByName? Or is there any other user of it?
> Actually as you said nsMsgDisplayMessageByName() also gets the string from
> the bundle
> whereas ..ByString() simply shows it, so we still need to keep them separate
> as there
> are calls to ..ByString() to just show the message.
OK, if there are other callers that construct the message and then just call nsMsgDisplayMessageByString then leave it. I see some in http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSendReport.cpp#372 .

> > ::: mailnews/compose/src/nsSmtpProtocol.cpp
> > @@ +98,2 @@
> > >  
> > > +  char16_t* exitString;
> > 
> > const ?
> nameForID() takes char16_t* otherwise I'll have to change all of then
> callers again :)
> If you still want all of them to be const, please let me know.
No, sorry. I didn't notice this is a different case. You do not assign exitString in this function, but call nameForID. So leave it as is.

Comment 15

[Josiah Bruner [:jsbruner]]

Comment on attachment 8434706 [details] [diff] [review]
Patch v1.8

Review of attachment 8434706 [details] [diff] [review]:
-----------------------------------------------------------------

I have quite a bit of feedback about the wording here. Sorry. :)

Note: Any '*' are for emphasis only, do not actually add them in the file.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +9,5 @@
>  ## %S will be replaced with the name of file that could not be opened
>  unableToOpenFile=Unable to open the file %S.
>  unableToOpenTmpFile=Unable to open the temporary file %S. Check your 'Temporary Directory' setting.
>  
> +unableToSaveTemplate=Unable to save your message as template.

as *a* template.

@@ +14,2 @@
>  
> +unableToSaveDraft=Unable to save your message as draft.

as *a* draft.

@@ +16,2 @@
>  
> +couldntOpenFccFolder=Couldn't open Sent Mail folder. Please verify that your Mail preferences are correct.

open *the* Sent Mail folder.

@@ +18,2 @@
>  
> +noSender=No sender was specified. Please fill in your email address in the Mail & Newsgroups account settings.

Could we make this: "No sender was specified. Please add your email address in the..." That's a little less awkward.

@@ +24,2 @@
>  
> +## LOCALIZATION NOTE (errorSendingFromCommand): argument %s is SMTP server response

is *the* SMTP server response.

@@ +27,2 @@
>  
> +## LOCALIZATION NOTE (errorSendingDataCommand): argument %s is SMTP server response

Ditto.

@@ +30,2 @@
>  
> +## LOCALIZATION NOTE (errorSendingMessage): argument %s is SMTP server response

Ditto.

@@ +30,3 @@
>  
> +## LOCALIZATION NOTE (errorSendingMessage): argument %s is SMTP server response
> +errorSendingMessage=An error occurred while sending mail. The mail server responded:  %s. Please check the message and try again.

Check the message what? I think we should indicate what they should look at if possible.

@@ +33,2 @@
>  
> +postFailed=The message could not be posted because connecting to the news server failed. The server may be unavailable or is refusing connections. Please verify that your news server settings are correct and try again, or else contact your network administrator.

No need for the "else".

@@ +35,2 @@
>  
> +errorQueuedDeliveryFailed=An error occurred delivering unsent messages.

occurred *while* delivering *the* unsent messages.

@@ +37,2 @@
>  
> +sendFailed=Sending of message failed.

of *the* message

@@ +39,2 @@
>  
> +## LOCALIZATION NOTE (smtpServerError): argument %s is SMTP server response

is the

@@ +42,2 @@
>  
> +unableToSendLater=Unable to save your message in order to send it later.

Maybe: "Sorry, we were unable to save your message for sending later."

@@ +44,2 @@
>  
> +## LOCALIZATION NOTE (communicationsError): argument %d is error code

is the

@@ +61,2 @@
>  
> +sendFailedButNntpOk=Your message has been posted to the newsgroup but has not been sent to other recipient.

to the other

@@ +65,4 @@
>  
>  followupToSenderMessage=The author of this message has requested that responses be sent only to the author. If you also want to reply to the newsgroup, add a new row to the addressing area, choose Newsgroup from the recipients list, and enter the name of the newsgroup.
>  
> +## LOCALIZATION NOTE (errorAttachingFile): argument %S is file name/URI of object to be attached

is the

@@ +65,5 @@
>  
>  followupToSenderMessage=The author of this message has requested that responses be sent only to the author. If you also want to reply to the newsgroup, add a new row to the addressing area, choose Newsgroup from the recipients list, and enter the name of the newsgroup.
>  
> +## LOCALIZATION NOTE (errorAttachingFile): argument %S is file name/URI of object to be attached
> +errorAttachingFile=There was an error attaching %S. Please check if you have access to the file.

Please check that you have access...

@@ +70,2 @@
>  
> +## LOCALIZATION NOTE (incorrectSmtpGreeting): argument %s is SMTP server greeting

is the

@@ +73,2 @@
>  
> +## LOCALIZATION NOTE (errorSendingRcptCommand): argument %1$S is SMTP server response, argument %2$S is intended message recipient.

is the

@@ +76,2 @@
>  
> +## LOCALIZATION NOTE (startTlsFailed): argument %s is SMTP server

is the

@@ +76,3 @@
>  
> +## LOCALIZATION NOTE (startTlsFailed): argument %s is SMTP server
> +startTlsFailed=An error occurred sending mail: Unable to establish a secure link with SMTP server %S using STARTTLS since it doesn't advertise that feature. Switch off STARTTLS for that server or contact your service provider.

occurred *while* sending mail

@@ +79,2 @@
>  
> +## LOCALIZATION NOTE (smtpPasswordUndefined): argument %S is SMTP account

is the

@@ +79,3 @@
>  
> +## LOCALIZATION NOTE (smtpPasswordUndefined): argument %S is SMTP account
> +smtpPasswordUndefined=An error occurred sending mail: Could not get password for %S. The message was not sent.

occurred *while* sending mail

@@ +82,2 @@
>  
> +## LOCALIZATION NOTE (smtpTempSizeExceeded): argument %s is SMTP server response

is the

@@ +85,2 @@
>  
> +## LOCALIZATION NOTE (smtpPermSizeExceeded1): argument %d is SMTP server size limit

is the

@@ +88,2 @@
>  
> +## LOCALIZATION NOTE (smtpPermSizeExceeded2): argument %s is SMTP server response

is the

@@ +91,2 @@
>  
> +## LOCALIZATION NOTE (smtpSendFailedUnknownServer): argument %S is SMTP server

is the

@@ +94,2 @@
>  
> +## LOCALIZATION NOTE (smtpSendRefused): argument %S is SMTP server

is the

@@ +97,2 @@
>  
> +## LOCALIZATION NOTE (smtpSendInterrupted): argument %S is SMTP server

is the

@@ +100,2 @@
>  
> +## LOCALIZATION NOTE (smtpSendTimeout): argument %S is SMTP server

is the

@@ +103,2 @@
>  
> +## LOCALIZATION NOTE (smtpSendFailedUnknownReason): argument %S is SMTP server

is the

@@ +106,2 @@
>  
> +# LOCALIZATION NOTE (smtpAuthChangeEncryptToPlainNoSsl): $S is server hostname

is the. (Make that change everywhere below, I'm sick of typing that) :)

@@ +106,3 @@
>  
> +# LOCALIZATION NOTE (smtpAuthChangeEncryptToPlainNoSsl): $S is server hostname
> +smtpAuthChangeEncryptToPlainNoSsl=The SMTP server %S does not seem to support encrypted passwords. If you just set up the account, please try changing to 'Password, transmitted insecurely' as the 'Authentication method' in the 'Account Settings | Server settings'. If it used to work and now suddenly fails, this is a common scenario how someone could steal your password.

Oh, this needs some re-wording. Maybe: The SMTP server $S does not seem to support encrypted passwords. If you just set up the account, try changing the 'Authentication method' in 'Account Settings | Server settings' to: 'Password, transmitted insecurely'. If this used to work, but now doesn't, consider contacting your server administrator as lacking encrypted passwords make you susceptible to getting your password stolen.

@@ +109,3 @@
>  
> +# LOCALIZATION NOTE (smtpAuthChangeEncryptToPlainSsl): $S is server hostname
> +smtpAuthChangeEncryptToPlainSsl=The SMTP server %S does not seem to support encrypted passwords. If you just set up this account, please try changing to 'Normal password' as the 'Authentication method' in the 'Account Settings | Server settings'. If it used to work and now suddenly fails, please contact your email administrator or provider.

Similar idea as above.

@@ +112,3 @@
>  
> +# LOCALIZATION NOTE (smtpAuthChangePlainToEncrypt): $S is server hostname
> +smtpAuthChangePlainToEncrypt=The SMTP server %S does not allow plaintext passwords. Please try changing to 'Encrypted password' as the 'Authentication method' in the 'Account Settings | Server settings'.

Ditto,

@@ +115,3 @@
>  
> +# LOCALIZATION NOTE (smtpAuthFailure): $S is server hostname
> +smtpAuthFailure=Unable to authenticate to SMTP server %S. Please check the password, and verify the 'Authentication method' in 'Account Settings | Outgoing server (SMTP)'.

Remove the comma after "password".

@@ +121,3 @@
>  
> +# LOCALIZATION NOTE (smtpAuthMechNotSupported): $S is server hostname
> +smtpAuthMechNotSupported=The SMTP server %S does not support the selected authentication method. Please change the 'Authentication method' in the 'Account Settings | Outgoing Server (SMTP)'.

Remove the "the" that appears after the "in".

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +9,5 @@
>  ## %S will be replaced with the name of file that could not be opened
>  unableToOpenFile=Unable to open the file %S.
>  unableToOpenTmpFile=Unable to open the temporary file %S. Check your 'Temporary Directory' setting.
>  
> +unableToSaveTemplate=Unable to save your message as template.

Make the same changes here you made in the other composeMsgs.properties file.

Comment 16

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.9

Made the suggested and related changes.
Thanks.

Comment 17

[Josiah Bruner [:jsbruner]]

Suyash, please go back through my comments and double check you got them all. Just a few lines into your patch and I noticed several of my comments were not addressed.

(And I don't want to have to type them out again of course :) )

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.1

Sorry, I figured out I missed a few changes last time. I have attempted to cover all of them this time.

The code didn't suggest any possible area the user can look into for "errorSendingMessage", so I have left it as is.

Also, I am suspecting an issue with "sendLargeMessageWarning" temporarily renamed to "largeMessageSendWarning". So, we can keep it this way for a while. It'll be renamed back to "sendLargeMessageWarning" once we confirm that the issue exists and fix it.

Thanks.

Comment 19

[Josiah Bruner [:jsbruner]]

Comment on attachment 8449983 [details] [diff] [review]
Patch v2.1

Review of attachment 8449983 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 20

[[e-mail]]

Hmm, have we had to deal with messages for error IDs before?

Comment 21

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #20)
> Hmm, have we had to deal with messages for error IDs before?

Not at least in 551919 and 858238.

Comment 22

[Magnus Melin [:mkmelin]]

Neil: review ping? Would be good to get this in so bug 783526 could get some traction again.

Comment 23

[Magnus Melin [:mkmelin]]

Neil: review ping? Would be good to get this in so bug 783526 could get some traction again.

Comment 24

[:aceman]

Comment on attachment 8449983 [details] [diff] [review]
Patch v2.1

Review of attachment 8449983 [details] [diff] [review]:
-----------------------------------------------------------------

Please refresh the patch, it does not apply cleanly today.
I have tested this by compiling it. I get a ton of warnings like:
mailnews/compose/src/nsComposeStrings.h:81:19: warning: deprecated conversion from string constant to 'char16_t*' [-Wwrite-strings]
       *aRetString = MOZ_UTF16("unableToOpenTmpFile");
Can those be fixed?

I also made a tool to check which strings are referenced in the code. There are no new strings that are NOT referenced after this patch so it seems you converted the names correctly without any typo. Congrats :)

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +106,2 @@
>  
> +# LOCALIZATION NOTE (smtpAuthChangeEncryptToPlainNoSsl): $S is the server hostname

There is an %S instead of $S in the relevant string. This also happens for several strings later on. Please check it (fix the Localization note).

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3392,5 @@
>    if ((mMessageWarningSize > 0) && (fileSize > mMessageWarningSize) && (mGUINotificationEnabled))
>    {
>      bool abortTheSend = false;
>      nsString msg;
> +    mComposeBundle->GetStringFromName(MOZ_UTF16("largeMessageSendWarning"), getter_Copies(msg));

For some reason, this string already is "largeMessageSendWarning" these days.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +253,1 @@
>      

Some trailing whitespace.

::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
@@ +1104,2 @@
>        break;
>      }    

Trailing spaces in this switch.

Comment 25

[Suyash Agarwal (:sshagarwal)]

Attachment: Path v2.7

Updated the patch to apply cleanly on the current tree.
I couldn't find a replacement for MOZ_UTF16().

Thanks.

Comment 26

[[e-mail]]

(In reply to aceman from comment #24)
> I have tested this by compiling it. I get a ton of warnings like:
> mailnews/compose/src/nsComposeStrings.h:81:19: warning: deprecated
> conversion from string constant to 'char16_t*' [-Wwrite-strings]
>        *aRetString = MOZ_UTF16("unableToOpenTmpFile");
> Can those be fixed?
Code needs to look like this:
> static char16_t* nameForID(nsresult aCode)
> {
>   switch(aCode)
>   {
>     case NS_MSG_UNABLE_TO_OPEN_FILE:
>       return MOZ_UTF16("unableToOpenFile");
etc.

Comment 27

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.8

Made the change suggested in comment 26.

Comment 28

[:aceman]

(In reply to [e-mail] from comment #26)
> Code needs to look like this:
> > static char16_t* nameForID(nsresult aCode)
> > {
> >   switch(aCode)
> >   {
> >     case NS_MSG_UNABLE_TO_OPEN_FILE:
> >       return MOZ_UTF16("unableToOpenFile");
> etc.
Didn't help:
mailnews/compose/src/nsComposeStrings.h:78:14: warning: deprecated conversion from string constant to 'char16_t*' [-Wwrite-strings]
       return MOZ_UTF16("unableToOpenFile");

Comment 29

[[e-mail]]

(In reply to aceman from comment #28)
> (In reply to comment #26)
> > Code needs to look like this:
> > > static char16_t* nameForID(nsresult aCode)
> > etc.
> Didn't help:
Sorry, I meant static const char16_t*

Comment 30

[Suyash Agarwal (:sshagarwal)]

Do we really need to make this change? It then requires changing a lot many things :)

Comment 31

[[e-mail]]

(In reply to Suyash Agarwal from comment #30)
> Do we really need to make this change?

If you mean returning a const char16_t*, then yes, you need to make that change.

Comment 32

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.9

Made the suggested changes.

Thanks.

Comment 33

[:aceman]

Comment on attachment 8539390 [details] [diff] [review]
Patch v2.9

Review of attachment 8539390 [details] [diff] [review]:
-----------------------------------------------------------------

Looks almost done to me now. I have to check the compile warnings yet.

::: mailnews/compose/src/nsComposeStrings.h
@@ +69,5 @@
>  #define NS_ERROR_SMTP_AUTH_NOT_SUPPORTED            NS_MSG_GENERATE_FAILURE(12600)
>  
>  #define NS_ERROR_ILLEGAL_LOCALPART                  NS_MSG_GENERATE_FAILURE(12601)
>  
> +static const char16_t* nameForID(nsresult aCode)

Maybe the function name would be more clear as "errorStringNameForErrorCode".

@@ +170,5 @@
> +      return MOZ_UTF16("illegalLocalPart");
> +    default:
> +      return MOZ_UTF16("smtpSendFailedUnknownReason");
> +  }
> +}

Hopefully the compilers are smart enough to see this function returns something in all cases and does not complain about "function may return no value".

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +374,5 @@
>    }
>    else
>    {
>      const char16_t* title;
> +    const char16_t* message;

"messageName", not the message content itself, that is in "dialogMessage". See preStrName previously in the patch.

@@ +407,1 @@
>                              getter_Copies(dialogMessage));

Is this second line indented properly?

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +249,1 @@
>  smtpEnterPasswordPrompt=Enter your password for %S:

One more remaining $S.

Comment 34

[:aceman]

The patch is bitrotted again :(
Anyway, the compile warnings I didn't like are gone.
Now there is a new one:
/var/SSD/TB-hg/mailnews/compose/src/nsComposeStrings.h: At global scope:
/var/SSD/TB-hg/mailnews/compose/src/nsComposeStrings.h:73:24: warning: 'const char16_t* nameForID(nsresult)' defined but not used [-Wunused-function]
 static const char16_t* nameForID(nsresult aCode)
                        ^
May it be something with the function being static?

Comment 35

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Sorry for taking so long. Updated the patch.
Not sure of that warning though.

Thanks.

Comment 36

[:aceman]

What about the other nits from comment 33?

Comment 37

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8560918 [details] [diff] [review]
Patch v3

Oops, sorry. Will fix them in a few hours.

Comment 38

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.1

Made the suggested changes.

Thanks.

Comment 39

[:aceman]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Review of attachment 8561032 [details] [diff] [review]:
-----------------------------------------------------------------

So I haven't compiled it, just looked whether the nits were done and it looks good now. Thanks. Let's get it into trunk before string freeze :)

Comment 40

[:aceman]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Seems this needs some code reviews yet. Strings were already checked by Josiah.
This is high priority due to TB38 string freeze being near. Thanks.

Comment 41

[Magnus Melin [:mkmelin]]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Review of attachment 8561032 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! 
Don't want to overload this bug but there's a bunch of text here that are pretty bad/misleading etc. Nothing new though.

Especially 
1) "check your Mail preferences", "Mail & Newsgroups account settings" :
 - this is a relic from suite. we don't have Mail and Browser specific settings, so it's pretty unclear Mail means Thunderbird
 - it's not "preferences", these things are (mostly) under account setting
2) "contact your network administrator"
Seriously. People contact tech support if it's not working and they can't figure things out. There's no need to give such messages.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +112,5 @@
> +      exitString = errorStringNameForErrorCode(aCode);
> +      bundle->GetStringFromName(exitString, getter_Copies(eMsg));
> +      msg = nsTextFormatter::vsmprintf(eMsg.get(), args);
> +      break;
> +    default:

maybe there should be a warning printing the code if we end up here?

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Aj forgot the largeMessageSendWarning change which is wrong, so please revert that part.
That's not a number but a  KB/MB formatted string.

Comment 43

[Kent James (:rkent)]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

I've looked this over as well, and it seems fine.

If we don't get a review from neil for the suite portion in the next 24 hours, let's split that out into a separate patch and land the remaining pieces, as this really need to be in Thunderbird 38.

Comment 44

[:aceman]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Maybe Ian can check the SM strings.

Comment 45

[[e-mail]]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

>+          preStrName = MOZ_UTF16("sendFailed");;
Nit: double semicolon

>+noRecipients=No recipients were specified. Please enter a recipient or newsgroup in the addressing area.
> 
>+errorWritingFile=Error writing temporary file.
> 
>+## LOCALIZATION NOTE (errorSendingFromCommand): argument %s is the SMTP server response
>+errorSendingFromCommand=An error occurred while sending mail. The mail server responded:  %s.  Please verify that your email address is correct in your Mail preferences and try again.
The resulting file looks a little odd because of all the double-spacing. Would you mind removing all the blank lines except just before a comment? For example, you would remove the blank line before errorWritingFile but not after it.

>+startTlsFailed=An error occurred whlie sending mail: Unable to establish a secure link with SMTP server %S using STARTTLS since it doesn't advertise that feature. Switch off STARTTLS for this server or contact your service provider.
Typo: "whlie"

>+smtpAuthChangeEncryptToPlainNoSsl=The SMTP server %S does not seem to support encrypted passwords. If you just set up the account, please try changing the 'Authentication method' in 'Account Settings | Server settings' to 'Password, transmitted insecurely'. If it used to work but now doesn't, consider contacting your server administrator as lacking encrypted passwords make you susceptible to getting your password stolen.
Grammar: lacking encrypted passwords makes you susceptible

>+# LOCALIZATION NOTE (smtpAuthChangeEncryptToPlainSsl): %S is the server hostname
>+smtpAuthChangeEncryptToPlainSsl=The SMTP server %S does not seem to support encrypted passwords. If you just >+# LOCALIZATION NOTE (smtpAuthChangePlainToEncrypt): %S is the server hostname
>+smtpAuthChangePlainToEncrypt=The SMTP server %S does not allow plaintext passwords. Please try changing the 'Authentication method' in 'Account Settings | Server settings' to 'Encrypted password'.
>+# LOCALIZATION NOTE (smtpAuthFailure): %S is the server hostname
>+smtpAuthFailure=Unable to authenticate to SMTP server %S. Please check the password, and verify the 'Authentication method' in 'Account Settings | Outgoing server (SMTP)'.
Huh, shouldn't these all say Outgoing server (SMTP)?

r=me with these fixed.

Comment 46

[Ian Neal]

Comment on attachment 8561032 [details] [diff] [review]
Patch v3.1

Not needed now

Comment 47

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Made the suggested changes.
Carrying over review from mkmelin, rkent and Neil.

Thanks.

Comment 48

[Kent James (:rkent)]

Checked in https://hg.mozilla.org/comm-central/rev/1def2116d8fc

Comment 49

[Ton [:Tonnes]]

>>  Outgoing server (SMTP) / Outgoing server (SMTP) error / Outgoing server (SMTP) settings (serveral occurrences)

Any particular reason to use "Outgoing server (SMTP)" instead of "Outgoing (SMTP) server" either stand-alone or in compound words? After all, SMTP is not an equivalent of "Outgoing server", so I'd treat this as "Outgoing (or SMTP) server error/settings".

This came up during localization, as our locale does not allow spaces within oompound words, where it's OK to use noun after noun after noun in English. Changing the word order to make it grammatically correct (i.e. to "Error in outgoing (SMTP) server) raised the question about the (SMTP) position for en-US.