323471 - Remove nsTString::ToCString

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brian Ryner (not reading)]

This method generally has better alternatives available, and we should remove it instead of propagating it to nsTSubstring for bug 323470.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

In order to cover some of the use cases for ToCString, I added versions of Lossy[Convert|Append]UTF16toASCII that write into a raw char buffer.  The caller must pass in the length of the buffer, and the method fails if the buffer turns out not to be large enough.

Comment 2

[Darin Fisher]

Comment on attachment 208522 [details] [diff] [review]
patch

>Index: widget/src/mac/nsMenuX.cpp

>+    short inKey = NS_LossyConvertUTF16toASCII(keyEquivalent).get()[0];

You could also use .First()


>Index: xpcom/string/public/nsReadableUtils.h

>+// These methods copy or append into the raw buffer |aDest|.
>+// |aDestSize| should be the size of the buffer.

You should mention that aDest will be null terminated upon return, and that
aDestSize must be > 0 to include room for the terminating null byte.


>+// Returns PR_TRUE on successful copy, or PR_FALSE if the buffer is too small.
>+NS_COM PRBool LossyCopyUTF16toASCII( const nsAString& aSource, char* aDest, PRUint32 aDestSize );
>+// aDest must be null-terminated when calling LossyAppendUTF16toASCII
>+NS_COM PRBool LossyAppendUTF16toASCII( const nsAString& aSource, char* aDest, PRUint32 aDestSize, PRUint32 aDestLength = PRUint32(-1) );

It just occured to me that nsTFixedString could perhaps be used instead...

  char *writableBuf;
  PRUint32 writableBufSize;

  nsFixedCString str(writableBuf, writableBufSize);
  LossyCopyUTF16toASCII(utf16str, str);

  if (str.get() != writableBuf) {
    // copy required more than writableBufSize bytes ...
  }

But the new function signatures are easier to use for consumers.  However,
it would be nice if there were UTF16 -> UTF8 versions (and the rest of 
the permutations for consistency).


>Index: xpcom/string/src/nsReadableUtils.cpp

>+      // right now, this won't work on multi-fragment destinations
>+    LossyConvertEncoding<PRUnichar, char> converter(aDest + aDestLength);

I'm confused by this comment.  The destination (aDest) is never going to be
multi-fragment, right?


Since this is a string API addition, I'd like dbaron to approve it as well.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> But the new function signatures are easier to use for consumers.  However,
> it would be nice if there were UTF16 -> UTF8 versions (and the rest of 
> the permutations for consistency).

Yikes. This is turning into a lot of work for history import.

> >Index: xpcom/string/src/nsReadableUtils.cpp
> 
> >+      // right now, this won't work on multi-fragment destinations
> >+    LossyConvertEncoding<PRUnichar, char> converter(aDest + aDestLength);
> 
> I'm confused by this comment.  The destination (aDest) is never going to be
> multi-fragment, right?

The comment was copy and pasted, I don't know what it means either.

Comment 4

[Brian Ryner (not reading)]

(In reply to comment #2)
> It just occured to me that nsTFixedString could perhaps be used instead...

I would actually rather do this for the places where we need this functionality, than implement all of the permutations in nsReadableUtils that would go largely unused.

I'll post a version of the patch that does that.

Comment 5

[Brian Ryner (not reading)]

Attachment: like this

Comment 6

[Darin Fisher]

Comment on attachment 208610 [details] [diff] [review]
like this

looks great!  r+sr=darin

Comment 7

[Brian Ryner (not reading)]

checked in.

Comment 8

[David G King]

This checking seems to have caused the following on my Win32 cygwin MinGW system

e:/mozilla/source/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp:123
0: error: call of overloaded `NS_LossyConvertUTF16toASCII(Value&)' is ambiguous
../../../../dist/include/string/nsString.h:108: note: candidates are: NS_LossyCo
nvertUTF16toASCII::NS_LossyConvertUTF16toASCII(char)
../../../../dist/include/string/nsString.h:91: note:                 NS_LossyCon
vertUTF16toASCII::NS_LossyConvertUTF16toASCII(const PRUnichar*)
make[7]: *** [nsXULTemplateBuilder.o] Error 1

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> This checking seems to have caused the following on my Win32 cygwin MinGW
> system

And on the Firefox balsa tinderbox:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1138141200.29582.gz

Comment 10

[Chris Lawson (gone)]

(In reply to comment #9)
> (In reply to comment #8)
> > This checking seems to have caused the following on my Win32 cygwin MinGW
> > system
> 
> And on the Firefox balsa tinderbox:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1138141200.29582.gz

And when trying to build Camino on Mac OS X.

cl

Comment 11

[Darin Fisher]

I checked in a fix for the balsa bustage.  I think the problem there was that Value is not a nsString, and unfortunately the code was inside a PR_LOGGING block that perhaps isn't compiled in many configurations ;-)

Marking FIXED again.