125465 - Add CString and UTF8String support to nsIVariant

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Nisheeth Ranjan]

See summary.

Comment 1

[Nisheeth Ranjan]

-> 1.0

Comment 2

[Nisheeth Ranjan]

Attachment: First attempt at patch (incomplete)

This patch compiles.  Not ready for review yet.  I'm attaching it so that I can
share it with Jag.  Need to work with him on UTF8String related conversions.

Jband, if you want to give this a cursory look and point out stuff that jumps
out at you, please do.	Thanks!

Comment 3

[Nisheeth Ranjan]

Attachment: V1.0 of complete patch

This is a complete patch created after help from Jag on strings.  Testing is in
progress but the final patch should be very close to this one.	Reviews can
start.

Jag and Jband, will you please r and sr?  Thanks!

Comment 4

[Nisheeth Ranjan]

There were a couple of lines in attachment 71568 [details] [diff] [review] that exceeded 80 chars.  I've
wrapped them properly in my local build.

Comment 5

[Nisheeth Ranjan]

Attachment: v2.0 of patch

This patch fixes line wrapping and adds string conversion test cases to the JS
variant tests.	I've run through the tests and they all pass.

I think this patch is ready for a serious review, super review.

Jag, please r.	Jband, please sr.  Thanks!

Comment 6

[jag (Peter Annema)]

Comment on attachment 71592 [details] [diff] [review]
v2.0 of patch

>Index: xpcom/ds/nsVariant.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/ds/nsVariant.cpp,v
>retrieving revision 1.15
>diff -u -r1.15 nsVariant.cpp
>--- xpcom/ds/nsVariant.cpp	20 Feb 2002 06:38:33 -0000	1.15
>+++ xpcom/ds/nsVariant.cpp	27 Feb 2002 01:10:07 -0000
>@@ -782,26 +827,39 @@
> }
> 
> /* static */ nsresult 
>-nsVariant::ConvertToAString(const nsDiscriminatedUnion& data, nsAWritableString & _retval)
>-{
>+nsVariant::ConvertToAString(const nsDiscriminatedUnion& data, 
>+                            nsAWritableString & _retval)
>+{    
>     nsCAutoString tempCString;
>     nsresult rv;
>     
>     switch(data.mType)
>     {
>     case nsIDataType::VTYPE_ASTRING:
>+    case nsIDataType::VTYPE_DOMSTRING:
>         _retval.Assign(*data.u.mAStringValue);
>         return NS_OK;
>+    case nsIDataType::VTYPE_CSTRING:
>+        CopyASCIItoUCS2(*data.u.mCStringValue, _retval);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_UTF8STRING:
>+    {        
>+        // XXX This is an extra copy that should be avoided
>+        // once Jag lands support for UTF8String and associated
>+        // conversion methods.
>+        NS_ConvertUTF8toUCS2 tempUCString(*data.u.mUTF8StringValue);
>+        _retval.Assign(tempUCString);

Generally such one-use temp conversions are inlined like:

	_retval.Assign(NS_ConvertUTF8toUCS2(*data.u.mUTF8StringValue));

>+        return NS_OK;
>+    }
>     case nsIDataType::VTYPE_CHAR_STR:
>-        tempCString.Assign(data.u.str.mStringValue);
>-        CopyASCIItoUCS2(tempCString, _retval);
>+        CopyASCIItoUCS2(nsDependentCString(data.u.str.mStringValue), _retval);

Nice safe on a copy there.

>         return NS_OK;
>     case nsIDataType::VTYPE_WCHAR_STR:
>         _retval.Assign(data.u.wstr.mWStringValue);
>         return NS_OK;
>-    case nsIDataType::VTYPE_STRING_SIZE_IS:
>-        tempCString.Assign(data.u.str.mStringValue, data.u.str.mStringLength);
>-        CopyASCIItoUCS2(tempCString, _retval);
>+    case nsIDataType::VTYPE_STRING_SIZE_IS:        

You're adding spaces to the end of the above line. Remove these and others you
added in several places. If jband agrees, just do a s/\s*$// on this file
before checking in.

>+        CopyASCIItoUCS2(nsDependentCString(data.u.str.mStringValue, 
>+                        data.u.str.mStringLength), _retval);

This is how such code is generally indented:

	CopyASCIItoUCS2(nsDependentCString(data.u.str.mStringValue, 
					   data.u.str.mStringLength),
			_retval);

>         return NS_OK;
>     case nsIDataType::VTYPE_WSTRING_SIZE_IS:
>         _retval.Assign(data.u.wstr.mWStringValue, data.u.wstr.mWStringLength);
>@@ -819,11 +877,133 @@
> }
> 
> /* static */ nsresult 
>+nsVariant::ConvertToACString(const nsDiscriminatedUnion& data, 
>+                             nsACString & _retval)
>+{           
>+    nsresult rv;
>+
>+    switch(data.mType)
>+    {
>+    case nsIDataType::VTYPE_ASTRING:
>+    case nsIDataType::VTYPE_DOMSTRING:
>+        CopyUCS2toASCII(*data.u.mAStringValue, _retval);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_CSTRING:
>+        _retval.Assign(*data.u.mCStringValue);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_UTF8STRING:
>+    {        
>+        // XXX This is an extra copy that should be avoided
>+        // once Jag lands support for UTF8String and associated
>+        // conversion methods.
>+        NS_ConvertUTF8toUCS2 tempUCString(*data.u.mUTF8StringValue);
>+        CopyUCS2toASCII(tempUCString, _retval);

	CopyUCS2toASCII(NS_ConvertUTF8toUCS2(*data.u.mUTF8StringValue),
			_retval);

>+        return NS_OK;
>+    }
>+    case nsIDataType::VTYPE_CHAR_STR:
>+        _retval.Assign(*data.u.str.mStringValue);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_WCHAR_STR:
>+        CopyUCS2toASCII(nsDependentString(data.u.wstr.mWStringValue), 
>+                        _retval);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_STRING_SIZE_IS:        
>+        _retval.Assign(data.u.str.mStringValue, data.u.str.mStringLength);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_WSTRING_SIZE_IS:
>+        CopyUCS2toASCII(nsDependentString(data.u.wstr.mWStringValue, 
>+                        data.u.wstr.mWStringLength), _retval);
>+        return NS_OK;
>+    case nsIDataType::VTYPE_WCHAR:
>+    {
>+        // XXX Extra copies.  Jag will fix when he lands UTF8String
>+        nsAutoString tempString(data.u.mWCharValue);
>+        CopyUCS2toASCII(tempString, _retval);

You could do this to avoid the copy:

	  const PRUnichar* str = & data.u.mWCharValue;
	  CopyUCS2toASCII(Substring(str, str), _retval);

>@@ -841,34 +1021,62 @@
>     switch(data.mType)
>     {
>     case nsIDataType::VTYPE_ASTRING:
>+    case nsIDataType::VTYPE_DOMSTRING:
>         *size = data.u.mAStringValue->Length();
>         *str = ToNewCString(*data.u.mAStringValue);
>         return *str ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>-    case nsIDataType::VTYPE_CHAR_STR:
>-        tempCString.Assign(data.u.str.mStringValue);
>-        *size = tempCString.Length();
>-        *str = ToNewCString(tempCString);
>+    case nsIDataType::VTYPE_CSTRING:
>+        *size = data.u.mCStringValue->Length();
>+        *str = ToNewCString(*data.u.mCStringValue);
>         return *str ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>-    case nsIDataType::VTYPE_WCHAR_STR:
>-        tempString.Assign(data.u.wstr.mWStringValue);
>+    case nsIDataType::VTYPE_UTF8STRING:  
>+    {
>+        // XXX This is doing 1 extra copy.  Need to fix this
>+        // when Jag lands UTF8String
>+        // we want:
>+        // *size = *data.mUTF8StringValue->Length();
>+        // *str = ToNewCString(*data.mUTF8StringValue);
>+        // But this will have to do for now.
>+        NS_ConvertUTF8toUCS2 tempString(*data.u.mUTF8StringValue);
>         *size = tempString.Length();
>         *str = ToNewCString(tempString);
>         return *str ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    case nsIDataType::VTYPE_CHAR_STR:
>+    {
>+        nsDependentCString cString (data.u.str.mStringValue);

Extra space.

...

Yeah, string usage looks fine, all nicely document with XXX and jag :-)

Address the above mentioned nits and show me the new patch so I can put my r=
on it.

Comment 7

[Nisheeth Ranjan]

Attachment: V 3.0 of patch that addresses jag's concerns

Jag, I made the changes you suggested.	The end of line whitespace removal has
resulted in a much larger looking patch.  But, rest assured that it is the same
patch as earlier with your changes added.

Please r=!  Thanks!  :-)

Comment 8

[jag (Peter Annema)]

Comment on attachment 71823 [details] [diff] [review]
V 3.0 of patch that addresses jag's concerns

r=jag

Comment 9

[John Bandhauer]

Comment on attachment 71823 [details] [diff] [review]
V 3.0 of patch that addresses jag's concerns

sr=jband. I'm OK with this stuff. We are little sloppy about treating UTF8 as
ASCII internally here. We also ought to eventually be detecting cases where
wide and UTF8 strings do not convert to ASCII without data loss. We do such
checking for numerical conversions, but not yet for strings.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 71823 [details] [diff] [review]
V 3.0 of patch that addresses jag's concerns

a=asa (on behalf of drivers) for checkin to 0.9.9 and the trunk.

Comment 11

[Nisheeth Ranjan]

I am in the middle of a Mac build.  Once I've verified that this patch builds on
the Mac, I'll check it in.  Don't wanna redden the Mac build like I did last
week!    :-)

Comment 12

[Nisheeth Ranjan]

OK, this will have to wait till Monday.  The Mac build is failing and Mac
experts are in short supply at this time.  I'll catch one on Monday and verify
my patch on the Mac.

If anyone out there can be a Mac buddy for me and verify that this patch builds
on a Mac before Monday, I'll checkin on the weekend.

Adding Mac guys to the cc list...

Comment 13

[Nisheeth Ranjan]

Also adding Peter van der Beken.  Peter, if at all possible, would you see if
the above patch builds on the Mac?  There's lots of type conversions happening
and I'm sure the picky Mac compiler is gonna catch stuff that the Windows
compiler didn't.  Thanks!

Comment 14

[Nisheeth Ranjan]

Thanks a lot to Dan Matejka, who pulled the patch and built it on the Mac. 
Surprisingly, there were no problems.  I could have checked this puppy in last week!

Checked into the trunk.  Am building the 0.9.9 branch and will check into it
later tonight.  Keeping bug open until then.

Comment 15

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 16

[Nisheeth Ranjan]

Moving the target milestone back to Mozilla 1.0.  The fix has already been
checked in on the trunk and is about to be checked in into the branch.

Comment 17

[Nisheeth Ranjan]

Checked the fix into the 0.9.9 branch.

Please also note that the forward declarations of nsACString and nsAString in
nsrootidl.idl got checked in as a part of this patch.  The changes to
nsrootidl.idl are described in attachment 71980 [details] [diff] [review] on bug 84186.

Marking fixed.

Comment 18

[Phil Schwartau]

Verified the check-in on the trunk and 0.9.9 branch