384979 - Remove nsTextFormatter from the mailnews code.

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Prasad Sunkari [:prasad]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070508 Iceweasel/2.0.0.4 (Debian-2.0.0.4-1)
Build Identifier: 

nsTextFormatter is not available in frozen code.  The only current alternative to part of its functionality is nsIStringBundle.  While string bundle are probably the only place where nsTextFormatter is used, the interface, for technical reasons could not have a variable argument function.

It ended up exporting FormatStringFromID and FormatStringFromName, both of which take an array of 10 elements as params for nsTextFormatter::smprintf - All params PRUnichar arrays.

Hence, as in all other parts of the mozilla tree (browser specially) it is a good idea to migrate all localization strings to use %S as format instead of %d and %ld.  The backend code could then convert the numbers to strings before calling FormatStringById/FormatStringByName.

This migration has a potential to break existing localized versions.  Simple strings in properties files like "4009=Received %ld of %ld messages" in localMsgs.properties will have to change to "4009=Received %s of %s messages"

Reproducible: Always

Comment 1

[Axel Hecht [:Pike]]

The other option would be to make nsTextFormatter available. Benjamin?

Comment 2

[Prasad Sunkari [:prasad]]

I might be relevant to note that there are only about 25 strings that use %d and %ld in the format string.

Comment 3

[Prasad Sunkari [:prasad]]

any update on this bug??? Benjamin, any comments?

Comment 4

[Benjamin Smedberg]

No, not really, however you want to fix it is probably ok.

Comment 5

[Prasad Sunkari [:prasad]]

Attachment: Variable Arguments with Variable Types

Currently nsIStringBundle exposes FormatStringFromID and FormatStringFromName, but imposes a restriction of having not more than ten arguments, which is practical.

How about having overloaded functions FormatStringFromID/Name which take an array of structures instead of array of strings as arguments?  What I am getting at is to have something like the do_print function the attached file.

The options I think we currently have are:
1. Convert all the strings to use only %S as the format specifier and convert all other types to integers before calling FormatStringFromID/Name
2. Make nsTextFormatter available through frozen linkage
3. Something like what is in the attached file.

Comments and any other alternatives welcome

Comment 6

[Scott MacGregor]

Prasad, I would vote for option 2: expose nsTextFormatter to frozen linkage.

Comment 7

[Prasad Sunkari [:prasad]]

Scott, as your vote goes, exposing nsTextFormatter to frozen linkage seems to be the cleanest solution.  Will do that.

Comment 8

[Prasad Sunkari [:prasad]]

Attachment: nsTextFormatter in xpcom glue code.

This patch adds creates a copy of nsTextFormatter in XPCOM glue code.  It uses frozen linkage and nsMemory based memory model.

Comment 9

[Benjamin Smedberg]

Comment on attachment 278220 [details] [diff] [review]
nsTextFormatter in xpcom glue code.

mozilla-new/xpcom/glue/nsTextFormatterAPI.cpp

>+#ifdef DEBUG
>+PRBool nsTextFormatterAPI::SelfTest()

1) This function shouldn't be here. It should be in xpcom/tests and should be hooked up to "make check"
2) This patch doesn't remove the xpcom/ds version of the same code. Why not?

But overall this looks correct. Please submit a new patch with these things fixed.

Comment 10

[Prasad Sunkari [:prasad]]

Attachment: Move nsTextFormatter to Glue

This patch moves nsTextFormatter to glue code.  I changed the memory model and few non-frozen uses of CRT and strings.

This should solve the issue of using nsTextFormatter in mailnews and other places.

Comment 11

[Benjamin Smedberg]

Comment on attachment 279891 [details] [diff] [review]
Move nsTextFormatter to Glue

>diff -N -U 8 -p -r -x CVS xpcom/glue/nsTextFormatter.h xpcom-new/glue/nsTextFormatter.h

>+class NS_COM nsTextFormatter {

needs to be NS_COM_GLUE now

Comment 12

[Scott MacGregor]

I've checked in this change with the NS_COM_GLUE change for Prasad.

Comment 13

[Scott MacGregor]

FYI, I had to adjust the Makefile in xpcom\tests to look like:

@$(RUN_TEST_PROGRAM) $(FINAL_TARGET)/TestTextFormatter$(BIN_SUFFIX)

in order to fix the mac builds which were orange. Thanks to benjamin for suggesting the fix.