157624 - freeze nsISupportsPrimitives.idl

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Alec Flett]

from bug 157047, I realized that we are freezing interfaces that eventually use
nsISupportsWString (via nsISimpleEnumerator) and other similar interfaces... we
need to freeze them. 

In the process, we really need to change nsISupportsWString and
nsISupportsString to use the new string types, as this structure is more often
used to enumerate strings, and is thus called in a loop.

Comment 1

[Doug Turner (:dougt)]

dbradley, objections?

Comment 2

[Alec Flett]

ok, dmose pointed out some odd stuff, and we came to this conclusion:
right now, the mappings go more or less like:
"String" -> "char*"
"WString" -> "PRunichar*"

I think we should drop the old string primitives, and just use nsA*String&
types, such that
"String" -> "nsAString&"
"CString" -> "nsACString&"

Mostly because the "w" in "wstring" holds meaning in IDL, but aside from these
primitives, the "W" in nsISupportsWString gives us no added value in C++.

Comment 3

[Doug Turner (:dougt)]

dbradley, how does this effect the xpconnect?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh.  Wow.  I did not realize this was still unfrozen...

Whatever changes we make, we should, imo, fix one issue with the current
interface.  Given an nsISupportsW?String, you can get a pointer to the data but
_not_ how much data there is!  This forces people to pass the lengths around in
odd ways...

Comment 5

[timeless]

i'm travelling today and will need a few days to catch up from my weekend vacation. can people give me time to check this idl file before they freeze? I know that I have made at least one change to it or its relatives.

Comment 6

[David Bradley]

Alec, did you mean AString and CString?

As far as the change to nsISupports[W]String that shouldn't be a problem.
Probably have to change the test that uses it, but I don't see anything else in
XPConnect that references it.

As far as freezing nsISupportsPrimitives.idl, does freezing this as a file mean
no new interfaces or that the existing interfaces won't change?

Comment 7

[Alec Flett]

no, I meant what I typed - my argument against putting the "A" in the interface
name is that the interface is meant as a refcounted wrapper for a string object,
in the abstract sense, not a wrapper for the specific C++ type "nsAString" 

dmose and I went around a few times on this already, and more or less agreed to
disagree :)

Comment 8

[timeless]

there's a comment about nsIAllocator. Afaik it needs to be corrected to
nsIMemory. but that's another alecf project :)

Comment 9

[Alec Flett]

ok, I'll take this over - I've got one mammoth patch to do the first thing I
described (rename nsISupportsString->nsISupportsCString and
nsISupportsWString->nsISupportsString) - and no I'm not that fast, I just know
some perl tricks :)

Comment 10

[Alec Flett]

Attachment: change interface names

This patch changes the interface names as I just described, as well as fixing
CIDs, CONTRACTIDs, and concrete class names. Can I get a review?
I'd like to land this first, and then change the API itself.
(and yeah, I'll have a commercial tree patch as well)

Comment 11

[timeless]

Comment on attachment 92914 [details] [diff] [review]
change interface names

I should get a prize for best nit :)
xpctest_primitives.js (both files) had a nicely structured table where the
commas lined up, please fix :)

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 92914 [details] [diff] [review]
change interface names

r=dougt.  what timeless said.

Comment 13

[Alec Flett]

heh. yeah yeah, I fixed up that table real nice-like. :)

Comment 14

[Alec Flett]

Comment on attachment 92914 [details] [diff] [review]
change interface names

got a sr=jag in person

Comment 15

[Alec Flett]

one thing I mentioned in person to jag is that we might also want a
nsISupportsUTF8String at some point. But that's a different bug.

Comment 16

[David Bradley]

So does freezing this mean we'll not be able to add more types?

Comment 17

[Alec Flett]

nah, I would hope not. Either than or we just keep adding
nsISupportsPrimitives2, etc. But I think the primary point of freezing is to
freeze forward - so that new apps that #include "nsISupportsPrimitives.h" don't
break with future versions.

Comment 18

[timeless]

i thought the idea of a frozen interface was that it couldn't change at all
(except for comments, and only for clarification, not for meaning).

As such, i'd suggest freezing all of the interfaces in the file
*except* interface nsISupportsPrimitive which of course doesn't actually work
because everything derives from it glory.

Well can we freeze it with const unsigned short 18..max unsigned short as
unusedSlot18..unusedSlotMax

That way when we need to add another primitive, we are merely renaming a string,
which would of course break jscompat. oh well. forget it. I'll expect to see
nsISupportsPrimitive2 before the year is out.

Comment 19

[Alec Flett]

ok, the first patch has landed. Now to actually make the harder (i.e. not quite
mechanical) change of switching wstring->AString and string->ACString

Comment 20

[Alec Flett]

good news here - I was able to eliminate
setDataWithLength/adoptData/adoptDataWithLength completely, so that the
string-supports objects remain as simple as the other objects. Patch
forthcoming, after I figure out what files I actually changed :)

Comment 21

[Alec Flett]

Attachment: switch to new string classes

ok, this cleans up nsISupportsString/nsISupportsCString and switches us over to
the new string classes. Reviews? Its pretty straightforward. It includes the
removal of nsI

Comment 22

[timeless]

Comment on attachment 94233 [details] [diff] [review]
switch to new string classes

>Index: mozilla/directory/c-sdk/config/Makefile
>-topsrcdir	= ..
>-srcdir		= .
>+topsrcdir	= /cygdrive/c/alecf/string/mozilla/directory/c-sdk
>+srcdir		= /cygdrive/c/alecf/string/mozilla/directory/c-sdk/config
>+VPATH		= /cygdrive/c/alecf/string/mozilla/directory/c-sdk/config
you really don't want to do this. (applies to a bunch of c-sdk Makefiles) blame
dmose?


>Index: mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp,v
>retrieving revision 1.60
>diff -u -r1.60 nsWindowWatcher.cpp
>--- mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp	6 Aug 2002 00:51:57 -0000	1.60
>+++ mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp	6 Aug 2002 20:26:13 -0000
>@@ -1740,11 +1740,11 @@
>       nsCOMPtr<nsISupportsCString> p(do_QueryInterface(argPrimitive));
>       NS_ENSURE_TRUE(p, NS_ERROR_UNEXPECTED);
> 
>-      char *data;
>+      nsCAutoString data;
> 
>-      p->GetData(&data);
>+      p->GetData(data);
> 
>-      JSString *str = ::JS_NewString(cx, data, strlen(data));
>+      JSString *str = ::JS_NewString(cx, ToNewCString(data), data.Length());
>       NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY);

1391 /*
1392  * Strings.
1393  *
1394  * NB: JS_NewString takes ownership of bytes on success, avoiding a copy;
but
1395  * on error (signified by null return), it leaves bytes owned by the
caller.
1396  * So the caller must free bytes in the error case, if it has no use for
them.
1397  * In contrast, all the JS_New*StringCopy* functions do not take ownership
of
1398  * the character memory passed to them -- they copy it.
1399  */
1400 extern JS_PUBLIC_API(JSString *)
1401 JS_NewString(JSContext *cx, char *bytes, size_t length);

you leak ToNewCString on failure :-)

note that this happens at leasta  few times.


>+NS_IMETHODIMP
>+nsPrefLocalizedString::GetData(PRUnichar** _retval)
>+{
>+  nsAutoString data;
...
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsPrefLocalizedString::SetData(const PRUnichar *aData)
>+{
>+    return SetData(nsDependentString(aData));
> }
trivial nit, be consistent about 2/4 space indentation =)

Index: mozilla/widget/src/xpwidgets/nsPrimitiveHelpers.cpp
>@@ -108,13 +112,19 @@
> 
>   if ( strcmp(aFlavor,kTextMime) == 0 ) {
>     nsCOMPtr<nsISupportsCString> plainText ( do_QueryInterface(aPrimitive) );
>-    if ( plainText )
>-      plainText->GetData ( NS_REINTERPRET_CAST(char**, aDataBuff) );
>+    if ( plainText ) {
>+      nsCAutoString data;
>+      plainText->GetData ( data );
>+      *aDataBuff = NS_REINTERPRET_CAST(char*, ToNewCString(data));
>+    }
>   }
the reinterpret cast appears useless here ^v
>   else {
>     nsCOMPtr<nsISupportsString> doubleByteText ( do_QueryInterface(aPrimitive) );
>-    if ( doubleByteText )
>-      doubleByteText->GetData ( NS_REINTERPRET_CAST(PRUnichar**, aDataBuff) );
>+    if ( doubleByteText ) {
>+      nsAutoString data;
>+      doubleByteText->GetData ( data );
>+      *aDataBuff = NS_REINTERPRET_CAST(PRUnichar*, ToNewCString(data));
>+    }
>   }
> 
> }

>Index: mozilla/mailnews/import/src/nsImportAddressBooks.cpp
>@@ -543,32 +543,16 @@
> 
> void nsImportGenericAddressBooks::SetLogs( nsString& success, nsString& error, nsISupportsString *pSuccess, nsISupportsString *pError)
> {
>-	nsString	str;
>-	PRUnichar *	pStr = nsnull;
>+	nsAutoString str = nsnull;
> 	if (pSuccess) {
>-		pSuccess->GetData( &pStr);
>-		if (pStr) {
>-			str = pStr;
>-			nsCRT::free( pStr);
>-			pStr = nsnull;
>-			str.Append( success);
>-			pSuccess->SetData( str.get());
>-		}
>-		else {
>-			pSuccess->SetData( success.get());			
>-		}
>+		pSuccess->GetData(str);
>+        str.Append(success);
>+        pSuccess->SetData( success);
i know that it was there before, but the space after the paren bothers me :-(
^v
> 	}
> 	if (pError) {
>-		pError->GetData( &pStr);
>-		if (pStr) {
>-			str = pStr;
>-			nsCRT::free( pStr);
>-			str.Append( error);
>-			pError->SetData( str.get());
>-		}
>-		else {
>-			pError->SetData( error.get());			
>-		}
>+		pError->GetData(str);
>+        str.Append(error);
>+        pError->SetData( error);
> 	}	
> }
> 

this file suffers from tabs (hrm, actually, so did the previous one...):
>Index: mozilla/mailnews/import/src/nsImportMail.cpp

Comment 23

[Boris Zbarsky [:bzbarsky]]

so... the comment in mozilla/xpcom/ds/nsSupportsPrimitives.h makes it sound like
|setDataWithLength|, |adoptData|, |adoptDataWithLength| are exposed, whereas in
reality they are not...  setDataWithLength is no loss, but the adopt* functions
could be useful as [noscript] functions in the idl so that C++ callers could use
them,,,

Comment 24

[Alec Flett]

Attachment: switch to new string classes v1.01

ok, it was worth updating for that odd leak-on-failure, so I did due diligence
on the rest of the review - except the tabs. I'm just not going to fix tabs
this time around. (and I left out those wacky Makefile changes)

as for bz's comment regarding adopt: while I agree that the adopt* functions
"could be useful" I think we've shyed away from Adopt() style mechanisms,
because we don't know how the string was allocated (and thus the proper way to
free it as well) - so I certainly wouldn't keep it in a frozen interface.

And since:
1) nobody uses the mechanism now
2) these objects aren't holding very large buffers

I think its not worth providing the Adopt() "just in case" - it will be more
headaches and there has been no obvious demand for it in the 2-3 years that
this interface has existed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

nsWindowWatcher.cpp:

+      char *dataBytes = ToNewCString(data);
+      JSString *str = ::JS_NewString(cx, dataBytes, data.Length());

Shouldn't you return NS_ERROR_OUT_OF_MEMORY if dataBytes is null?

Same in the wide char version of this code.

I got about a third of the way through and that's all I found, so far.  ;)

Comment 26

[Alec Flett]

ok, let's all agree that I just added that in my tree.. good catch!

Comment 27

[Alec Flett]

Attachment: switch to new string classes v1.1

I noticed there were a number of calls to ToString() where GetData() could now
be used to save us an allocation - so this patch includes that conversion.

again, reviews anyone?

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 94331 [details] [diff] [review]
switch to new string classes v1.1

r=dougt

Comment 29

[jag (Peter Annema)]

Comment on attachment 94331 [details] [diff] [review]
switch to new string classes v1.1

>Index: mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp,v
>retrieving revision 1.60
>diff -u -r1.60 nsWindowWatcher.cpp
>--- mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp	6 Aug 2002 00:51:57 -0000	1.60
>+++ mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp	7 Aug 2002 16:04:27 -0000
>@@ -1740,11 +1740,22 @@
>       nsCOMPtr<nsISupportsCString> p(do_QueryInterface(argPrimitive));
>       NS_ENSURE_TRUE(p, NS_ERROR_UNEXPECTED);
> 
>-      char *data;
>+      nsCAutoString data;
> 
>-      p->GetData(&data);
>+      p->GetData(data);
>+
>+      // JS_NewString takes ownership, but we have to free on error,
>+      // so we need to keep the raw pointer around
>+      char *dataBytes = ToNewCString(data);
>+      if (!dataBytes)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      
>+      JSString *str = ::JS_NewString(cx, dataBytes, data.Length());

Why not use JS_NewStringCopyN?

>+      if (!str) {
>+        nsMemory::Free(dataBytes);
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
> 
>-      JSString *str = ::JS_NewString(cx, data, strlen(data));
>       NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY);

This NS_ENSURE_TRUE is redundant with the above check, but if you use CopyN you
can get rid of the above check instead.

>@@ -1755,15 +1766,25 @@
>       nsCOMPtr<nsISupportsString> p(do_QueryInterface(argPrimitive));
>       NS_ENSURE_TRUE(p, NS_ERROR_UNEXPECTED);
> 
>-      PRUnichar *data;
>+      nsAutoString data;
> 
>-      p->GetData(&data);
>+      p->GetData(data);
> 
>+      // JS_NewUCString takes ownership, but we have to free on error,
>+      // so we need to keep the raw pointer around
>+      PRUnichar *dataBytes = ToNewUnicode(data);
>+      if (!dataBytes)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      
>       // cast is probably safe since wchar_t and jschar are expected
>       // to be equivalent; both unsigned 16-bit entities
>-      JSString *str = ::JS_NewUCString(cx, NS_REINTERPRET_CAST(jschar *, data),
>-                          nsCRT::strlen(data));
>-      NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY);
>+      JSString *str = ::JS_NewUCString(cx,
>+                                       NS_REINTERPRET_CAST(jschar *,dataBytes),
>+                                       data.Length());
>+      if (!str) {
>+        nsMemory::Free(dataBytes);
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }

Same applies.

>Index: mozilla/netwerk/base/src/nsIOService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsIOService.cpp,v
>retrieving revision 1.147
>diff -u -r1.147 nsIOService.cpp
>--- mozilla/netwerk/base/src/nsIOService.cpp	6 Aug 2002 00:52:42 -0000	1.147
>+++ mozilla/netwerk/base/src/nsIOService.cpp	7 Aug 2002 16:04:27 -0000
>@@ -517,13 +517,13 @@
>         if (NS_FAILED(rv)) break;
> 
>         // get the entry string
>-        nsXPIDLCString entryString;
>-        rv = entry->GetData(getter_Copies(entryString));
>+        nsCAutoString entryString;
>+        rv = entry->GetData(entryString);
>         if (NS_FAILED(rv)) break;
> 
>-        if (strcmp(entryString, scheme) == 0) {
>+        if (entryString.Equals(scheme) == 0) {

This is not what you want.

Can you get another r= on this patch and e-mail me when you think it's ready
for sr=?

Comment 30

[jag (Peter Annema)]

To clarify, I haven't looked at the rest of this patch.

Comment 31

[Alec Flett]

sorry, I don't know the JS API - I'll try using JS_NewStringCopyN and fix the
other stuff...

Comment 32

[jag (Peter Annema)]

Nothing to be sorry for, alecf, just pointing it out so you can improve your
patch (and hopefully future ones). As for the |.Equals(...) == 0| thing, it's
unfortunate you missed that, dougt, but it's good that it was caught. I'll take
a look at the rest of this patch when I get bored again of being on vacation
;-), in the mean time, I'd appreciate it if you (both) could take another look
at this patch and catch any other oversights.

Comment 33

[Alec Flett]

Attachment: switch to new string classes v1.2

ok, this has been cleaned up - and I found one other typo in the os2 widget
code that I've cleaned up... but I think this patch is now ready... using
JS_NewStringCopyN etc...

Comment 34

[jag (Peter Annema)]

Comment on attachment 95141 [details] [diff] [review]
switch to new string classes v1.2

This is just the part for nsBookmarksService.cpp

Comment 35

[Alec Flett]

Attachment: switch to new string classes v1.2 again

argh! Sorry jag, I don't know how that happened.
Take two, and asking again.. can I get reviews here?

Comment 36

[Darin Fisher]

ok, some questions about the API change...

 interface nsISupportsCString : nsISupportsPrimitive
 {
+    attribute ACString data;
     string toString();
-
-    /**
-     * Note: setting |data| and using |setDataWithLength| make a copy of
-     * the data string argument. Using |adoptData| and |adoptDataWithLength|,
-     * which are not scriptable, subsumes the data string as the internal
-     * buffer.
-     *
-     * Also note that the |length| param should not include space for a null
-     * terminator, nor should it account for the size of a char. It should
-     * only be the number of characters for which there is space in the string.
-     */
-
-    void setDataWithLength(in unsigned long length,
-                           [size_is(length)] in string data);
-
-    [noscript] void adoptData(in charPtr data);
-
-    [noscript] void adoptDataWithLength(in unsigned long length,
-                                        [size_is(length)] in charPtr data);
 };

why don't we want toString() to return ACString?  just too much work to fix all
the callsites?  also, why don't we want to keep the adopt methods?  nsCString
supports Adopt, so wouldn't these be trivial to provide?  same thing goes for
nsISupportsString.

Comment 37

[Alec Flett]

I didn't change toString() because I as I recall, xpconnect doesn't do automatic
conversion to strings in JS without it.

This might be remedied easily by just fixing XPConnect.. but since I've updated
all the c++ callsites to use GetData(), I'd like to handle the toString() in
another patch.

see comment 24 for my "Why no adopt?" answer.. The short answer is, nobody uses
it and it opens up funky ownership/allocator issues that I don't think should
cloud this interface.

Comment 38

[Darin Fisher]

oh, i didn't realize XPCONNECT did something special with toString()...
interesting.  in that case, it might be nice to document the fact that
toString() exists for that purpose.

i'm fine with leaving the Adopt methods off since they aren't used, and we could
always achieve the same thing using sharable strings.  however, i don't really
like the argument that Adopt is bad because of mixed allocator problems...
that's why we have nsMemory::Alloc isn't it?  afterall, we have the identical
problem when some foreign component implements an interface with a |string| or
|wstring| getter.

Comment 39

[Alec Flett]

darin: cool. I agree, the allocator argument is a little weak :)

Hey, can I get a review from you?
dougt is on vacation right now...

Comment 40

[Darin Fisher]

Comment on attachment 95970 [details] [diff] [review]
switch to new string classes v1.2 again

>Index: mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp

>+      JSString *str = ::JS_NewStringCopyN(cx, data.get(), data.Length());

too bad there isn't a way for a nsCString to yield ownership of its buffer :(


>Index: mozilla/js/src/xpconnect/src/xpccomponents.cpp

>+                        JSString* idstr = JS_NewStringCopyZ(cx, name.get());

nit: use JS_NewStringCopyN to eliminate a call to strlen


>Index: mozilla/modules/libpref/src/nsPref.cpp

>+    if (NS_FAILED(rv))
>+      return rv;
>+    
>+    nsAutoString data;
>+    rv = theString->GetData(data);
>+    if (NS_FAILED(rv))
>+      return rv;
>+
>+    *_retval = ToNewUnicode(data);
>+    if (!*_retval)
>+      return NS_ERROR_OUT_OF_MEMORY;

how about using ToString() here to avoid the extra buffer copy?  this
same pattern repeats.


>Index: mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp

>+        str->SetData(nsDependentCString(APPLICATION_GZIP,
>+                                        sizeof(APPLICATION_GZIP) - 1));

how about using NS_LITERAL_CSTRING instead?  this same pattern
repeats.


i stopped here:

>Index: mozilla/xpcom/ds/nsSupportsPrimitives.cpp

Comment 41

[Alec Flett]

darin: ok nits fixed.. I was thinking I wasn't going to be able to do
NS_LITERAL_CSTRING because of the whole macros issue with prepending an "L" onto
the macro name - but looking at your comment I'm realizing its not a wide
string. duh!

do you mind finishing the review so that I don't have to keep attaching patches,
asking people to review them AGAIN, and so forth? As you can see I've been a few
rounds with this patch already :)

Comment 42

[Darin Fisher]

no problem, i only stopped halfway through because i ran out of time ;-)
...hoping to resume sometime today.

Comment 43

[Darin Fisher]

Comment on attachment 95970 [details] [diff] [review]
switch to new string classes v1.2 again

>Index: mozilla/xpcom/ds/nsSupportsPrimitives.cpp

>+NS_IMETHODIMP nsSupportsCStringImpl::GetData(nsACString& aData)
>+    if (mData)
>+        aData = nsDependentCString(mData, mLength);
>     else
>+        aData.Truncate(0);
>+    return NS_OK;

nit: why not make mData be a nsCString?  it seems like that would
greatly simplify the implementation of this class.  you could do
away with a lot of the code... especially in the SetData case.
there doesn't seem to be any advantage to storing a raw pointer and
length.


>Index: mozilla/mailnews/import/src/nsImportAddressBooks.cpp

>+	nsAutoString str;
> 	if (pSuccess) {
>+		pSuccess->GetData(str);
>+        str.Append(success);
>+        pSuccess->SetData(success);
> 	}
> 	if (pError) {
>+		pError->GetData(str);
>+        str.Append(error);
>+        pError->SetData(error);
> 	}	

nit: fix indentation


>Index: mozilla/mailnews/import/src/nsImportMail.cpp

>+	nsAutoString str;
> 	if (pSuccess) {
>+		pSuccess->GetData(str);
>+        str.Append(success);
>+        pSuccess->SetData(str);
> 	}
> 	if (pError) {
>+		pError->GetData(str);
>+        str.Append(error);
>+        pError->SetData(str);
> 	}	

nit: fix indentation

otherwise, the patch looks good.

Comment 44

[Alec Flett]

argh. the indentation in that file is because the import stuff uses tabs. I'll
try to re-indent the lines around those that I changed.

Yeah, I guess I was debating what made sense there.. I think that this was
originally made when nsCString was a 20+ byte structure, so maybe people were
optimizing for space, I don't know. These days its just 12 bytes (4 more bytes
than  mLength and mData combined) so I'll just switch us over.

Comment 45

[Alec Flett]

ah, no I see the original purpose - it was to support AdoptData(), because this
pre-dated Adopt() in strings. in any case, I'm dropping it so this is going to
get signifigantly simpler.

Comment 46

[Alec Flett]

Attachment: switch to new string classes v1.21

ok, addressing all of darin's comments....
as for indenting in nsImportMail.cpp - I correctly indented every line I
touched, but the whole file is a confusion of tabs and spaces - using either
wouldn't make the patch look right. At least it has a correct emacs header so
that the tabs are correctly expanded to 4 spaces.

Can I get final reviews? thanks folks.

Comment 47

[Darin Fisher]

Comment on attachment 96347 [details] [diff] [review]
switch to new string classes v1.21

r/sr=darin

Comment 48

[Doug Turner (:dougt)]

Comment on attachment 95141 [details] [diff] [review]
switch to new string classes v1.2

r=dougt

Comment 49

[Alec Flett]

Comment on attachment 96347 [details] [diff] [review]
switch to new string classes v1.21

re-applying the has-review flag

Comment 50

[Boris Zbarsky [:bzbarsky]]

If we're using nsCString for mData, we had better explicitly comment that .get()
should never be called on it...

Even as it is, I'm not completely sure that nsCString always does the right
thing if the data has embedded nulls in it (and nsISupportsCString should
certainly support embedded nulls).

Comment 51

[Alec Flett]

yeah, nsCString supports embedded nulls fine, as far as I know. This was a big
thing that gessner used to brag about when the string classes first arrived.

in any case, nsSupportsCString does not do anything with the data itself, it
just copies it to/from the internal string object.

Comment 52

[Alec Flett]

ok, interfaces have been marked "under_review" so that we can document them
further and see if there's anything else we need.

Comment 53

[Alec Flett]

looks like doug got the last of the @status FROZEN in another bug. Marking fixed.