100476 - X.ToNewXXX() -> ToNewXXX(X) where XXX in (Unicode, CString, UTF8String)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[jag (Peter Annema)]

Comment 1

[jag (Peter Annema)]

Attachment: X.ToNewXXX -> ToNewXXX(X)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here are comments on the part up to but not including nsAbAutoCompleteSession.cpp:

(As a side note, I really don't like the |ToNewUnicode| from char* or
nsACString& since it isn't clear whether it's converting from ASCII or
UTF8.)

There are places where you're using NS_ConvertUCS2toUTF8 to pass to
printf (to fix debugging code that leaked).  Maybe you should write and
use NS_LossyConvertUCS2toASCII (which would be a very simple
nsCAutoString wrapper)?


Index: embedding/browser/photon/src/WebBrowserContainer.cpp
+   moz->rightClickUrl = strdup( ToNewCString(rightClickUrl) );

redundant and leaky (not that it wasn't before, but...)

Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
Remove these comments:
 // XXX There is no ASCII->Unicode decoder?
 // XXX How to do this without double allocation/copy?

Index: gfx/src/photon/nsFontMetricsPh.cpp
-   PRUnichar* str = ((nsString*) he->key)->ToNewUnicode();
+   PRUnichar* str = ToNewUnicode(*((nsString*) he->key));
No need to over-parenthesize, but NS_STATIC_CAST would be good too (and
avoid the parens issue).

Index: gfx/src/photon/nsRenderingContextPh.cpp
-   char* tail=(char*)strTail.ToNewUnicode();
+   char* tail=(char*)ToNewUnicode(strTail);
Um?  What's going on here?  Make that an NS_REINTERPRET_CAST at the very
least, with big XXX comments.

Index: js/src/xpconnect/src/xpcconvert.cpp
Could you just use the NS_ConvertASCIItoUCS2 inline rather than making a
new variable?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Index: gfx/src/photon/nsFontMetricsPh.cpp
> -   PRUnichar* str = ((nsString*) he->key)->ToNewUnicode();
> +   PRUnichar* str = ToNewUnicode(*((nsString*) he->key));
> No need to over-parenthesize, but NS_STATIC_CAST would be good too (and
> avoid the parens issue).

(But for that it would need to be NS_STATIC_CAST(const nsString*, he->key) .)

Comment 4

[jag (Peter Annema)]

I'll write the lossy converter.

About ToNewUnicode from a char* / nsACString being unclear what encoding it's
converting from, yeah, maybe we should add a second parameter to indicate which
encoding we're converting from? For now, it's assumed to be ASCII, and nothing
has changed there. From your comment it sounds like this is an issue we could
address seperately.

I've adddressed your other comments and fixed the code. I'll explicitely mention
this here:

/* XXXjag this code looks very bogus to me, but I've no idea what they're
 * trying to accomplish here. At my best guess they're trying to do this:
 * char* text = ToNewCString(nsDependentCString(aString, aLength) +
 *                           NS_LITERAL_CSTRING("M."));
 *              text[aLength + 1] = '\0'; // overwrites the '.' above
 */
nsCString strTail("M");
char* tail=(char*)ToNewUnicode(strTail);
PRUint32 tailLength=strlen(tail);
char* text = (char*) nsMemory::Alloc(aLength + tailLength + 2);

PRUint32 i;
for(i=0;i<aLength;i++)
        text[i]=aString[i];
for(i=0;i<tailLength;i++)
        text[aLength+i] = tail[i];
text[aLength+tailLength]='\0';
text[aLength+tailLength+1]='\0';



Now, I know there is some leaky string code in embedding/browser/XXX/src where
XXX isn't one of the mainstream platforms. Unless the fix was obvious (bar the
few I mixed like the one you pointed out above), I decided not to waste too much
time trying to "fix" it, since it would take me a fair while to find out if my
suspiscion is correct, but even a longer time to get the code to compile,
whereas I know my simple string substitution to still compile.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd rather see the ToNewUnicode(const char*) and ToNewUnicode(nsACString&) be
split into UTF8ToNewUnicode and ASCIIToNewUnicode, or something like that...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You had to use the following pattern repeatedly:
  ToNewUnicode(NS_ConvertUTF8toUCS2(name))
which suggests to me we need a |UTF8ToNewUnicode| (not for this checkin,
though, but soon).  I do expect NS_LossyConvertUCS2toASCII for this
checkin though.  It's trivial, since it's just an
AssignWithConversion, and you shouldn't be printing UTF8 to the console.


File a bug against nsAbUtils.cpp for having an if-else that do the same
thing.  Dare I guess |copyElements| is always true?


I expect the nsMsgSendLater.cpp change won't compile.  Please let me
know why it does.


nsEudoraAddress.cpp:

  You added a shadowing variable uniStr without removing the old
  (unneeded) one.


mimemoz2.cpp

  |resultString| should be |const char*|.

  You need a second |else /* !stringBundle */| to assign
    tempString = nsCRT::strdup(resultString).


nsIconChannel.cpp:

-      *((char **)getter_Copies(filePath)) = formattedFileExt.ToNewCString(); //
yuck...shove it into our xpidl string...
+      filePath.Adopt(ToNewCString(formattedFileExt)); // yuck...shove it into
our xpidl string...

You can remove the comment now. :-)


nsAccount.cpp:

  + char* lvalue = ToNewCString(v);
  +
  + PL_strcpy(IspLocation[LocCount], lvalue);
  +

Can just use v.get() and remove the |delete[]| which is an FMM anyway.


nsNSSCertificate.cpp:
+  //nsXPIDLCString tmpstrl tmpstr.Adopt(CERT_Hexify(&mCert->serialNumber, 1));

should be ';' instead of 'l'.


==== I skipped the changes in nsString2.cpp -- need to come back to
these. ====

(And really, can we get rid of NS_ConvertUTF8toUCS2(char)?)



photon/nsMenu.cpp:

+  char *st r =ToNewCString(mLabel);

really?


nsPersistentProperties.cpp:

-  cout << "will add " << aKey.ToNewCString() << "=" << aNewValue.ToNewCString()
<< endl;
+  cout << "will add " << ToNewCString(aKey) << "=" << ToNewCString(aNewValue)
<< endl;

These should use NS_LossyConvertUCS2toASCII (with the XXX comment
removed).


nsAppRunner.cpp:

  These changes are in the wrong tree!

nsGlobalHistory.cpp:

-        value = valueStr.ToNewUnicode();
-        len = nsCRT::strlen((PRUnichar*)value);
+        value = ToNewUnicode(valueStr);
+        len = valueStr.Length() * sizeof(PRUnichar);

Skip the |* sizeof(PRUnichar)|.


I've done up to but not including nsUrlBarHistory.cpp, and skipped
nsString2.cpp, as noted above.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The foo.Adopt(ToNewXXX(Bar)); pattern is bad because it defeats sharing.
Does nsXPIDL[C]String::Assign work?  Can we make it work?


nsInstall.cpp:

  -            JSString* propValJSStr = JS_NewUCStringCopyZ(cx, (jschar*)
                 valCStr);
  +            JSString* propValJSStr = JS_NewUCStringCopyZ(cx,
                 NS_STATIC_CAST(const jschar*, pVal.get()));

  some platforms will require an NS_REINTERPRET_CAST

  Also, use a .get() on keyCStr in the JS_SetProperty call.


Now I just have to look at nsString2.cpp.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsString2.cpp:

the iterators (ns{Reading,Writing}Iterator) ought to have a fragment_type so
that you don't have to use nsReadableFragment<char> explicitly.

  +        nsReadableFragment<char> frag(start.fragment());

Maybe

  const nsReadableFragment<char>& frag = start.fragment();

would be better?



How about an NS_ASSERTION(count == mLength, "calculator calculated
incorrect length"); at the end of nsConvertUTF8toUCS2::Init?


I think we need to look at the error-handling of NS_ConvertUTF8toUCS2
carefully since we can expect it to be called on input from the network
(I would think, anyway, although perhaps not).

+                else
+                  {
+                    ucs4 -= 0x00010000;
+                    *mBuffer++ = 0xD800 | (0x000003FF & (ucs4 >> 10));
+                    *mBuffer++ = 0xDC00 | (0x000003FF & ucs4);
+                  }

Is this UCS2 or UTF-16 or what?

Comment 9

[jag (Peter Annema)]

>You had to use the following pattern repeatedly:
>  ToNewUnicode(NS_ConvertUTF8toUCS2(name))
>which suggests to me we need a |UTF8ToNewUnicode| (not for this checkin,
>though, but soon).  I do expect NS_LossyConvertUCS2toASCII for this
>checkin though.  It's trivial, since it's just an
>AssignWithConversion, and you shouldn't be printing UTF8 to the console.

NS_LossyConvertUCS2toASCII added.

>File a bug against nsAbUtils.cpp for having an if-else that do the same
>thing.  Dare I guess |copyElements| is always true?

No, it's not always true. Filed, bug 100743.

>I expect the nsMsgSendLater.cpp change won't compile.  Please let me
>know why it does.

Because |field| is a nsMsgCompFields, which has ::SetFrom(const char*, ...)

>nsEudoraAddress.cpp:
>
>  You added a shadowing variable uniStr without removing the old
>  (unneeded) one.

Fixed.

>mimemoz2.cpp
>
>  |resultString| should be |const char*|.

Fixed

>  You need a second |else /* !stringBundle */| to assign
>    tempString = nsCRT::strdup(resultString).

Fixed, by moving that out of the |if (stringBundle)| and changing it to

if (!tempString)
  tempString = nsCRT::strdup(resultString);

>nsIconChannel.cpp:
>
>-      *((char **)getter_Copies(filePath)) = formattedFileExt.ToNewCString(); //
>yuck...shove it into our xpidl string...
>+      filePath.Adopt(ToNewCString(formattedFileExt)); // yuck...shove it into
>our xpidl string...
>
>You can remove the comment now. :-)

Removed, but it's still kinda yucky though ;-)

>nsAccount.cpp:
>
>  + char* lvalue = ToNewCString(v);
>  +
>  + PL_strcpy(IspLocation[LocCount], lvalue);
>  +
>
>Can just use v.get() and remove the |delete[]| which is an FMM anyway.

I think so.

>nsNSSCertificate.cpp:
>+  //nsXPIDLCString tmpstrl tmpstr.Adopt(CERT_Hexify(&mCert->serialNumber, 1));
>
>should be ';' instead of 'l'.

Fixed

>==== I skipped the changes in nsString2.cpp -- need to come back to
>these. ====
>
>(And really, can we get rid of NS_ConvertUTF8toUCS2(char)?)

Maybe.

>photon/nsMenu.cpp:
>
>+  char *st r =ToNewCString(mLabel);
>
>really?

You've uncovered my secret plot to break the photon code. I'll fix it *sigh*.

>nsPersistentProperties.cpp:
>
>-  cout << "will add " << aKey.ToNewCString() << "=" << aNewValue.ToNewCString()
><< endl;
>+  cout << "will add " << ToNewCString(aKey) << "=" << ToNewCString(aNewValue)
><< endl;
>
>These should use NS_LossyConvertUCS2toASCII (with the XXX comment
>removed).

Done.

>nsAppRunner.cpp:
>
>  These changes are in the wrong tree!

Yes they are. Thanks for catching that, would've been a regression.

>nsGlobalHistory.cpp:
>
>-        value = valueStr.ToNewUnicode();
>-        len = nsCRT::strlen((PRUnichar*)value);
>+        value = ToNewUnicode(valueStr);
>+        len = valueStr.Length() * sizeof(PRUnichar);
>
>Skip the |* sizeof(PRUnichar)|.

A little lower, very similar code, also dealing with PRUnichar*, does:

1281         PRUnichar* p;
1282         rv = name->GetValue(&p);
1283         if (NS_FAILED(rv)) return rv;
1284 
1285         len = nsCRT::strlen(p) * sizeof(PRUnichar);
1286         value = p;
This len is used in nsGlobalHistory::URLEnumerator, which uses it with mork,
where they want to know the size in bytes.

I'm just fixing a bug while I'm there :-)

Comment 10

[jag (Peter Annema)]

Btw, the NS_ConvertXXXtoYYY part of the patch really belongs to a different bug.
Wanna move it there (and not delay this patch any further)?

>nsString2.cpp:
>
>the iterators (ns{Reading,Writing}Iterator) ought to have a fragment_type so
>that you don't have to use nsReadableFragment<char> explicitly.

Next pass? New bug?

>  +        nsReadableFragment<char> frag(start.fragment());
>  
>Maybe
>
>  const nsReadableFragment<char>& frag = start.fragment();
>
>would be better?

I could use that.

>How about an NS_ASSERTION(count == mLength, "calculator calculated
>incorrect length"); at the end of nsConvertUTF8toUCS2::Init?

Done.

>I think we need to look at the error-handling of NS_ConvertUTF8toUCS2
>carefully since we can expect it to be called on input from the network
>(I would think, anyway, although perhaps not).

I should do that.

>+                else
>+                  {
>+                    ucs4 -= 0x00010000;
>+                    *mBuffer++ = 0xD800 | (0x000003FF & (ucs4 >> 10));
>+                    *mBuffer++ = 0xDC00 | (0x000003FF & ucs4);
>+                  }
>
>Is this UCS2 or UTF-16 or what?

I'll come back to you on this one :-)

Comment 11

[jag (Peter Annema)]

Attachment: update (still including the string/obsolete/* changes for a different patch)

Comment 12

[jag (Peter Annema)]

Hmm, looks like I didn't create another patch after updating string/obsolete/*.
Anyway, that's bug 73292 so I'll put that there.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 50715 [details] [diff] [review]
update (still including the string/obsolete/* changes for a different patch)

nsDocShell.cpp:

   random piece of whitespace introduced
   
nsGfxListControlFrame.cpp:

   You're using NS_ConvertUCS2toUTF8 for a printf - convert to
   NS_LossyConvertUCS2toASCII

nsAbUtils.cpp:

   Could you just do the minimal change to this file and put the other
   changes in the separate bug already filed (I think -- see above).

nsString.cpp:

   You can optimize with a SetCapacity if you want.

r=dbaron, except on the changes that are part of bug 73292 which are
revised there and which I will review there

Comment 14

[jag (Peter Annema)]

Checked in.