802266 - Review NS_* definitions in nsComposeStrings.h

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Mark Banner (:standard8)]

nsComposeStrings.h has a lot of defines in it, and I think some can be removed or simplified.

There are two basic categories that I think we can address:

- Unused definitions
-- e.g. NS_MSG_CANT_POST_TO_MULTIPLE_NEWS_HOSTS
--- This isn't used anywhere, however it also needs its associated string (12507) removing from composeMsgs.properties
-- I think there are several in this category.

- String-only definitions
-- e.g. NS_MSG_ASSEMB_DONE_MSG
--- This needs the define removing, the call to GetStringFromID changing to GetStringFromName, and then the number (12508) replacing by an actual string, e.g. msgAssemblyDoneAlert
- I think the ones that get passed to UpdateStatus could also be changed from a number to a name as well.

Once we've addressed those, we can then think about the other values in that file, but we can do them in a follow-up bug.

Comment 1

[:aceman]

For the second part where the string IDs need to be changed, see how it is done in bug 858238. Also check if this does not clash with the changes in that bug (meaning you would both change the same spots). If yes, you will need to wait until bug 858238 is checked in.

Comment 2

[Randy Thomas]

Hi. I would like to take this and have a patch in line for the first part of the bug.

Comment 3

[Randy Thomas]

Attachment: bug_802266_patch_1_removes_unused_definitions.patch

Comment 4

[Randy Thomas]

Attachment: bug_802266_patch_2_removes_superfluous_definitions.patch

Comment 5

[:aceman]

Ok, the patches must be applied in the order as specified (patch 1 and patch 2).

Comment 6

[:aceman]

Comment on attachment 755433 [details] [diff] [review]
bug_802266_patch_1_removes_unused_definitions.patch

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

Please remove r=aceman, I am not a reviewer. I just mentor you to create the patch and then we send it to a real reviewer. But it seems you managed to do the patches alone, so good work there! ;)

Can you also remove the same strings from suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties ?

Comment 7

[:aceman]

Comment on attachment 755435 [details] [diff] [review]
bug_802266_patch_2_removes_superfluous_definitions.patch

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

Can you also change the same strings in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties ?

I have run TB tests on this and they do pass.
The changes are fine but there are some small changes to be done yet.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +165,5 @@
> +genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
> +
> +## @name NS_MSG_UNDISCLOSED_RECIPIENTS
> +## LOCALIZATION NOTE: this string must be using only US_ASCII characters
> +undisclosedRecipients=undisclosed-recipients

Can you remove all the '@name' lines from the strings you change?

@@ +237,5 @@
> +smtpDeliveringMail=Delivering mail…
> +smtpMailSent=Mail sent successfully
> +
> +assemblingMailInformation=Assembling mail information…
> +gatheringAttachment=Attaching %s…

Can you add a '## LOCALIZATION NOTE' describing what the %s is/will become?

@@ +240,5 @@
> +assemblingMailInformation=Assembling mail information…
> +gatheringAttachment=Attaching %s…
> +creatingMailMessage=Creating mail message…
> +
> +copyMessageStart=Copying message to %S folder…

Can you add a '## LOCALIZATION NOTE' describing what the %S is/will become?

@@ +255,5 @@
> +saveDraftErrorTitle=Save Draft Error
> +saveTemplateErrorTitle=Save Template Error
> +
> +failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> +failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?

Can you add a '## LOCALIZATION NOTE' describing what the %.200s is/will become?

@@ +398,5 @@
>  ## LOCALIZATION NOTE(stopShowingUploadingNotification): This string is used in the Filelink
>  ## upload notification bar to allow the user to dismiss the notification permanently.
>  stopShowingUploadingNotification.accesskey=N
>  stopShowingUploadingNotification.label=Never show this again
> +

Why adding this line?

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +1045,2 @@
>      else
> +      bundle->GetStringFromName(NS_LITERAL_STRING("failureOnObjectEmbeddingWhileSending").get(), getter_Copies(msg));

Please wrap long lines to not cross 80 chars.
You can wrap like this (align arguments after bracket):
long_expression_and_function(argument1, argument2
                             argument3, ...)

And if that is not possible then (2 space indent):
long_expression_and_function(argument1, argument2
  argument3, ...)

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3591,5 @@
>      GetNotificationCallbacks(getter_AddRefs(callbacks));
>  
>      // Tell the user we are sending the message!
>      nsString msg;
> +    mComposeBundle->GetStringFromName(NS_LITERAL_STRING("Sending messageâ¦").get(), getter_Copies(msg));

The string ID here, not the real text.

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +214,5 @@
>  NS_IMETHODIMP nsMsgSendReport::DisplayReport(nsIPrompt *prompt, bool showErrorOnly, bool dontShowReportTwice, nsresult *_retval)
>  {
> +  NS_NAMED_LITERAL_STRING(genericFailureExplanationMsg, "genericFailureExplanation");
> +  NS_NAMED_LITERAL_STRING(sendErrorTitleMsg, "sendMessageErrorTitle");
> +

Why are you adding these variables? In the previous files you put the strings directly into GetStringFromName() . And you do the same later on.

@@ +385,2 @@
>      bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
>                              getter_Copies(dialogMessage));

Good, we will have a separate bug for converting the rest of the strings.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +434,5 @@
>  /////////////////////////////////////////////////////////////////////////////////////////////
>  // End of nsIStreamListenerSupport
>  //////////////////////////////////////////////////////////////////////////////////////////////
>  
> +void nsSmtpProtocol::UpdateStatus(const char* aStatusMsg)

'*Msg' may mislead into thinking this should be the text to display (as in UpdateStatusWithString). Try aStatusID or aStatusName.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Moritz, are you working, or will you be getting back to this?

Comment 9

[Randy Thomas]

Yes, definitely. Except a revised patch some time this weekend.

Comment 10

[Suyash Agarwal (:sshagarwal)]

Moritz are you still working on this?

Comment 11

[Johannes Buchner]

(In reply to :aceman from comment #7)
> ::: mailnews/compose/src/nsMsgSendReport.cpp
> @@ +214,5 @@
> >  NS_IMETHODIMP nsMsgSendReport::DisplayReport(nsIPrompt *prompt, bool showErrorOnly, bool dontShowReportTwice, nsresult *_retval)
> >  {
> > +  NS_NAMED_LITERAL_STRING(genericFailureExplanationMsg, "genericFailureExplanation");
> > +  NS_NAMED_LITERAL_STRING(sendErrorTitleMsg, "sendMessageErrorTitle");
> > +
> 
> Why are you adding these variables? In the previous files you put the
> strings directly into GetStringFromName() . And you do the same later on.

I could not find the quoted changes in the patch.

> @@ +385,2 @@
> >      bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> >                              getter_Copies(dialogMessage));
> 
> Good, we will have a separate bug for converting the rest of the strings.
> 
> ::: mailnews/compose/src/nsSmtpProtocol.cpp
> @@ +434,5 @@
> >  /////////////////////////////////////////////////////////////////////////////////////////////
> >  // End of nsIStreamListenerSupport
> >  //////////////////////////////////////////////////////////////////////////////////////////////
> >  
> > +void nsSmtpProtocol::UpdateStatus(const char* aStatusMsg)
> 
> '*Msg' may mislead into thinking this should be the text to display (as in
> UpdateStatusWithString). Try aStatusID or aStatusName.

So keep as was?
> void nsSmtpProtocol::UpdateStatus(int32_t aStatusID)
>    bundle->GetStringFromID(aStatusID, getter_Copies(msg));

Comment 12

[Johannes Buchner]

Attachment: bug802266-1-cleanup-strings

Comment 13

[Johannes Buchner]

Attachment: bug802266-2-by-name

Comment 14

[Johannes Buchner]

Attachment: bug802266-1-cleanup-strings

Comment 15

[Johannes Buchner]

Attachment: bug802266-2-by-name

Nevermind the second part of my question in Comment 11.

Comment 16

[:aceman]

Comment on attachment 8337319 [details] [diff] [review]
bug802266-1-cleanup-strings

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

Thanks, this looks fine to me. Let's forward to real reviewers.

Comment 17

[:aceman]

Comment on attachment 8337320 [details] [diff] [review]
bug802266-2-by-name

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

This also looks fine to me. I see you didn't touch IDs like 12503 that are used as both error numbers and string IDs. I'm OK if that is done later (may be a new bug) after this patch is checked in, as those need more massaging.

I give preliminary f+ (if you fix the nits below) but I haven't yet compiled this so I'll confirm that later.

Also please remove r=aceman from the patch header and nice that you keep the original author mentioned.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +243,5 @@
> +## LOCALIZATION NOTE: argument is file name/URI of attachment
> +gatheringAttachment=Attaching %s…
> +creatingMailMessage=Creating mail message…
> +
> +## LOCALIZATION NOTE: argument is folder name

Please write "argument %S is folder name".

@@ +262,5 @@
> +## LOCALIZATION NOTE: argument is file name/URI of object to be embedded
> +failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> +## LOCALIZATION NOTE: argument is file name/URI of object to be embedded
> +failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> +returnToComposeWindowQuestion=Would you like to return to the compose window?

These same changes need to be done in the suite composeMsgs.properties file too.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +570,5 @@
>  
>    NS_ASSERTION (m_attachment_pending_count == 0, "m_attachment_pending_count != 0");
>  
> +  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssembling").get(),
> +		  getter_Copies(msg));

Please align getter_Copies under NS_LITERAL_STRING (after bracket). See comment 7.

@@ +920,5 @@
>    }
>  
>    // Tell the user we are creating the message...
> +  mComposeBundle->GetStringFromName(
> +    NS_LITERAL_STRING("creatingMailMessage").get(), getter_Copies(msg));

You seem to use different alignment in the same file for the same function call (mComposeBundle->GetStringFromName). Could you consolidate it into a single way?

@@ +963,5 @@
>      }
>    }
>  
> +  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssemblingDone").get(),
> +		  getter_Copies(msg));

Please align getter_Copies under NS_LITERAL_STRING (after bracket).

Comment 18

[:aceman]

Thanks for trying to finish this patch!

Comment 19

[Johannes Buchner]

Attachment: bug802266-2-by-name

My motivation to work on this bug is decreasing ... it's not particularly interesting.

Comment 20

[:aceman]

Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

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

Yeah, this is not a very interesting bug :) But it is easy as a starting bug.

The changes in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties should have been the same as to 
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties, not adding different localization notes.

Comment 21

[[e-mail]]

Comment on attachment 8337319 [details] [diff] [review]
bug802266-1-cleanup-strings

rs=me assuming this compiles.

Comment 22

[:aceman]

Applying both patches does compile for me.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

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

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +306,1 @@
>  mailnews.reply_header_authorwrote=%s wrote

there's no $s there ;)
(same thing with the one below)

Comment 24

[[e-mail]]

Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

>+  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssembling").get(),
>+		                            getter_Copies(msg));
Nit: Always align the arguments i.e. the g should be below the N.
Note: Compiler support has improved to the point where we might be able to use MOZ_UTF16("messageAssembling") instead of NS_LITERAL_STRING(...).get() although I posted a request to dev-platform just to be sure. (The advantage is that it's just a string literal so you can do stuff like assign it to a const char16_t* pointer.)

>-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
>+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
>     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
>                             getter_Copies(dialogMessage));
Any reason you changed the title but not the message?

Comment 25

[[e-mail]]

Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

>+    void UpdateStatus(const char * aStatusName);
Looks like MOZ_UTF16 is good to go these days, in which case you can also fix this to be const char16_t *.

Comment 26

[Suyash Agarwal (:sshagarwal)]

There are just a few changes left (I think). Are you going to complete this?

Comment 27

[Johannes Buchner]

Sorry, no. I stowed away my Moz-dev directory and am involved in other things. Feel free to take it.

Comment 28

[[e-mail]]

Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

In that case, setting review- so this doesn't get accidentally checked in.

Comment 29

[Suyash Agarwal (:sshagarwal)]

Attachment: But802266 by name

(In reply to [e-mail] from comment #24)
> >-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
> >+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
> >     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> >                             getter_Copies(dialogMessage));
> Any reason you changed the title but not the message?

preStrId is nsresult, so changing it will further require changing a few other function calls and definitions and return types as well.
E.g. http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCopy.cpp#181

Should I change it too?

Comment 30

[:aceman]

I think this patch is already large enough.
We ultimately want to convert all numeric IDs in composeMsgs.properties to string IDs. However, those that are used as a return value and also as an error message ID need different conversions. So I think those can be done in a followup bug as standard8 suggests in comment 0.
Neil, would you be fine with that?

Comment 31

[[e-mail]]

Comment on attachment 8389246 [details] [diff] [review]
But802266 by name

>-    int32_t titleID;
>+    nsString title;
const char16_t* perhaps?

(In reply to Suyash Agarwal from comment #29)
> (In reply to comment #24)
> > >-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
> > >+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
> > >     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> > >                             getter_Copies(dialogMessage));
> > Any reason you changed the title but not the message?
> 
> preStrId is nsresult

Ah, sorry, I'd overlooked that.

(In reply to aceman from comment #30)
> Neil, would you be fine with that?

Yes, that's fine.

Comment 32

[Suyash Agarwal (:sshagarwal)]

Attachment: Bug802266 by name v1.2

Comment 33

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

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

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +378,5 @@
>  
>      switch (mDeliveryMode)
>      {
>        case nsIMsgCompDeliverMode::Later:
> +        title = MOZ_UTF16("sendLaterErrorTitle");

While compiling, I got warnings that this is a deprecated conversion. Is there another way by which I can convert const char* to const char16_t* ?

Comment 34

[:aceman]

Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

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

Looks like this is heavily bitrotted. It didn't apply to any of the files.

Comment 35

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #34)
> Comment on attachment 8389648 [details] [diff] [review]
> Bug802266 by name v1.2
> 
> Review of attachment 8389648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like this is heavily bitrotted. It didn't apply to any of the files.

It applies cleanly for me on the updated tree.
I also applied "cleanup strings" patch before working on this patch, so maybe that's why it failed for you. Applying both the patches works.
Please let me know if that fails too.

Thanks.

Comment 36

[:aceman]

Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

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

Yeah, sorry, I missed the first patch :)

The patch looks fine to me, but I wonder about the missing Seamonkey conversion...

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +630,5 @@
>            rv = stringService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(composeStringBundle));
>            if (NS_SUCCEEDED(rv))
>            {
>              nsString undisclosedRecipients;
> +            rv = composeStringBundle->GetStringFromName(MOZ_UTF16("undisclosedRecipients"), 

Trailing space.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3976,2 @@
>    else
> +    mComposeBundle->GetStringFromName(MOZ_UTF16("copyMessageFailed"), getter_Copies(msg));

In the first string you preserved the word "start", in the second you didn't. Shouldn't it be consistent?

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +300,5 @@
>        mAlreadyDisplayReport = true;
>        return NS_OK;
>      }
>  
> +    bundle->GetStringFromName(MOZ_UTF16("sendErrorTitleMsg"), 

Trailing space.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +442,5 @@
>      nsresult rv = bundleService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(bundle));
>      if (NS_FAILED(rv)) return;
>      nsString msg;
> +    bundle->GetStringFromName(aStatusName,
> +                              getter_Copies(msg));

Didn't this fit on a single line?

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +57,3 @@
>  
>  ## @name SMTP_DELIV_MAIL
>  12521=Delivering mail…

So Seamonkey does not get the conversion to string names? This looks wrong.

Comment 37

[Suyash Agarwal (:sshagarwal)]

Attachment: Bug802266 by name v2

Oh sorry, I didn't look at that, I thought the patch was almost complete :P

Comment 38

[:aceman]

Comment on attachment 8391063 [details] [diff] [review]
Bug802266 by name v2

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

Looks better now :)

Comment 39

[[e-mail]]

Comment on attachment 8391063 [details] [diff] [review]
Bug802266 by name v2

>Bug 802266 - Removes definitoins with ids (numbers) in nsComposeStrings.h that don't need it. Instead, the messages are now
>accessed by name which makes them far more convenient to use.
Typo: definitions
Also, note that Mercurial outputs the first line of a commit message in short logs so you should wrap at a more appropriate place (i.e. at the end of a sentence).

>+genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
The code that refers to this string uses the wrong name.

>+messageAssemblingDone=Assembling messageâ¦Done
>+messageAssembling=Assembling messageâ¦
The original #define contained MSG twice which may have been confusing; these should be switched to assemblingMessage[Done].

>+sendMessageErrorTitle=Send Message Error
The code that refers to this string uses the wrong name.

Comment 40

[:aceman]

(In reply to [e-mail] from comment #39)
> >+genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
>
> >+sendMessageErrorTitle=Send Message Error
> The code that refers to this string uses the wrong name.

Unfortunate side effect of using strings instead of defines - it does not catch typos :(

We could use some script to check whether all string names in .properties files are properly referenced in the whole tree. Is there such a tool already? If not, I could prepare one.

Comment 41

[Suyash Agarwal (:sshagarwal)]

Attachment: Bug802266 by name v2.2

Fixed the typos.

Comment 42

[[e-mail]]

Comment on attachment 8391099 [details] [diff] [review]
Bug802266 by name v2.2

I'm going to give this patch r+ because it seems to be correct, but I would appreciate it if you would consider the following points:

>+gatheringAttachment=Attaching %sâ¦
I just noticed this isn't a proper formatted string (using %S and FormatStringFromName), is there any chance you could convert this at the same time, as this would avoid having to rename the string again? (Or, assuming you check the patch in after the uplift, give this a temporary name now, and file a follow-up bug to convert it.)

>+copyMessageStartComplete=Copy complete.
Please would you rename this to copyMessageComplete everywhere. (Sorry for overlooking it last time.)

>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
(These could do with being formatted as well, although I don't know offhand whether %.200S works.)

>+## LOCALIZATION NOTE (undisclosedRecipients): this string must be using only US_ASCII characters
Nit: please fix this to say "this string must use only US_ASCII characters"

Comment 43

[Suyash Agarwal (:sshagarwal)]

Attachment: Bug802266 by name for check-in

Fixed the nits. Carrying over review from Neil.

Comment 44

[[e-mail]]

Comment on attachment 8391753 [details] [diff] [review]
Bug802266 by name for check-in

>+gatheringAttachment=Attaching %sâ¦
You forgot to change the %s to %S.

>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
(Also I still don't know whether %.200S works or not.)

>+      params.Assign("?");
This calls strlen, use either AssignLiteral or '?' instead.

>+      nsAutoString params = NS_ConvertUTF8toUTF16(m_attachments[i]->m_realName);
NS_ConvertUTF8toUTF16 params(m_attachments[i]->m_realName);

Comment 45

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Sorry, made the changes.

(In reply to [e-mail] from comment #44)

> >+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
> >+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> >+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
> >+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> (Also I still don't know whether %.200S works or not.)

I couldn't test this.
So, shall we get this in as is and see if someone experiences any errors when running into this?

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply to comm-central tip. Please rebase.

Comment 47

[Suyash Agarwal (:sshagarwal)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Doesn't apply to comm-central tip. Please rebase.

Can you please re-try with both the patches?
bug802266-1-cleanup-strings followed by
Patch v3

(In reply to Suyash Agarwal (:sshagarwal) from comment #45)
> > >+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> > (Also I still don't know whether %.200S works or not.)
> 
> I couldn't test this.
> So, shall we get this in as is and see if someone experiences any errors
> when running into this?

I think we can check this in as it was this way before too.

Thanks.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/1ab1fff092a5
https://hg.mozilla.org/comm-central/rev/cf87c5c76627

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/comm-central/rev/13a79c6de877

https://tbpl.mozilla.org/php/getParsedLog.php?id=36666940&tree=Thunderbird-Trunk

Comment 50

[:aceman]

(In reply to [e-mail] from comment #25)
> Comment on attachment 8337650 [details] [diff] [review]
> bug802266-2-by-name
> 
> >+    void UpdateStatus(const char * aStatusName);
> Looks like MOZ_UTF16 is good to go these days, in which case you can also
> fix this to be const char16_t *.

Yeah, looks like this change didn't arrive into the last patch version.
But it seems only OS X compiler is picky enough to fail on it so we didn't notice;)

Comment 51

[Suyash Agarwal (:sshagarwal)]

So, since this is an issue with OS X, should we wait for MOZ_UTF16() to work on OSX or should I revert the function definition to const char* instead of char16_t* ?

Comment 52

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.1 fixed OSX build failure cause

Comment 53

[Suyash Agarwal (:sshagarwal)]

build passes on try server.
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8a178dce265e

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/bf99513cdccd
https://hg.mozilla.org/comm-central/rev/30d91b2583f1