346345 - Should have a category for converting between Gecko and Cocoa strings

Status: NEW

(Core :: Widget: Cocoa, defect, P3)

Description

[Henri Sivonen (:hsivonen)]

Currently, embedding/browser/cocoa/src/NSString+Utils.h is available to Camino, but is not in the right place for use in the Cocoa widget code. 

For the Cocoa widget code, it would be nice to have an NSString category that allowed NSString to be initialized with various Gecko string flavors and that allowed NSString to be converted to various Gecko string flavors.

Comment 1

[Mark Mentovai]

I agree.

Comment 2

[Henri Sivonen (:hsivonen)]

To self.

Comment 3

[Henri Sivonen (:hsivonen)]

Where should the conversion category be placed in the tree? Should the methods that are not strictly about string conversion be left in embedding/browser/cocoa/src/NSString+Utils.h?

Comment 4

[Benjamin Smedberg]

I suggest xpcom/glue/nsMacString.{h,mm} other utility classes/functions can be moved if mento thinks appropriate.

Comment 5

[Håkan Waara]

See also bug 334670, which is about the Core Foundation type (which is the C struct that the objc-only NSString type uses as its core).

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Unpolished header

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Unpolished implementation

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Diff for existing files

(Will upload new files separately. cvs diff -uN isn't working for me.)

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: mozilla/xpcom/string/public/NSString+Moz.h

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: mozilla/xpcom/string/src/NSString+Moz.mm

hwaara, are these changes adequate for your purposes? Would you like to take the bug to add whatever changes you need?

Comment 11

[Håkan Waara]

Comment on attachment 239022 [details]
mozilla/xpcom/string/public/NSString+Moz.h

I don't think I need any other changes, so I'd be happy to help out reviewing this, if you plan to drive the patch.

>+ (id)ellipsisString;

This is not needed; IIRC 10.3 offers ways to do this. The reason that there was a category method, was that Camino used to support 10.1/10.2

>+ (id)stringWithPRUnichars:(const PRUnichar*)inString;
>+ (id)stringWith_nsAString:(const nsAString&)inString;
>+ (id)stringWithUTF8_nsACString:(const nsACString&)inString;

I think this should be named simply stringWith_nsACString to match what the rest of XPCOM string classes are named.

>+ (id)stringWith_nsISupportsString:(nsISupportsString*)inString;

I think nsISupportsString is deprecated, and thus should not be endorsed by new APIs. That goes for all the nsISupports helper methods.

>- (NSString *)stringByRemovingCharactersInSet:(NSCharacterSet*)characterSet;
>- (NSString *)stringByReplacingCharactersInSet:(NSCharacterSet*)characterSet withString:(NSString*)string;
>- (NSString *)stringByTruncatingTo:(unsigned int)maxCharacters at:(ETruncationType)truncationType;

Are these strictly needed? There should be ways to do this with the existing NSString APIs, and they're not really Mozilla-specific.

>// allocate a new unicode buffer with the contents of the current string. Caller
>// is responsible for freeing the buffer.
>- (PRUnichar*)createNewUnicodeBuffer;

Also not sure what this has to do with NSString at all.

Comment 12

[Håkan Waara]

Comment on attachment 239020 [details] [diff] [review]
Diff for existing files

>Index: mozilla/configure.in
>===================================================================
>RCS file: /cvsroot/mozilla/configure.in,v
>retrieving revision 1.1704
>diff -u -r1.1704 configure.in
>--- mozilla/configure.in	31 Jul 2006 18:11:36 -0000	1.1704
>+++ mozilla/configure.in	18 Sep 2006 14:26:30 -0000
>@@ -4537,7 +4537,7 @@
> 	AC_DEFINE(MOZ_WIDGET_PHOTON)
>     ;;
> mac|cocoa)
>-    TK_LIBS='-framework Carbon'
>+    TK_LIBS='-framework Carbon -framework Cocoa'

I bet linking to -framework Foundation is enough.

Comment 13

[Håkan Waara]

Comment on attachment 239025 [details]
mozilla/xpcom/string/src/NSString+Moz.mm

The nsA[C]String utilities can be simplified.

Have a look at http://landfill.mozilla.org/mxr-test/mozilla/source/camino/src/extensions/NSString+Utils.mm

(Note that all files in embedding/cocoa are >3 years out out of sync with their camino counterparts, so that code should really not be copied.)

Comment 14

[Henri Sivonen (:hsivonen)]

The use case I had for this code was not checked in. Should this be WONTFIXed?

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Someone was asking about (wanting) string conversion stuff the other day, so presumably there's still some need for either this bug or Håkan's bug 334670....

Comment 16

[Colin Barrett [:cbarrett]]

This and bug 334670 are probably dupes of each other. CFString and NSString are toll-free bridged. You can just cast the NSString to a CFString, or vice versa.