85310 - ToNewCString, AssignWithConversion, NS_ConvertASCIItoUCS2 in libnet

Status: RESOLVED WORKSFORME

(Core :: Networking, defect)

Description

[nhottanscp]

Separated from bug 51355.

Under mozilla/netwerk, there are
59 of ToNewCstring,
43 of AssignWithConversion,
11 of NS_ConvertASCIItoUCS2

Some of them might not be appropriate (i.e. data might not be ASCII only).
Changing them to ToNewUTF8String, NS_ConvertUTF8toUCS2 would not break ASCII 
(UTF-8 is a superset of ASCII), though I am not sure about performance 
difference if changing all of them unconditionally.

Comment 1

[Doug Turner (:dougt)]

.

Comment 2

[Doug Turner (:dougt)]

what is milestone "mozilla1.0" anyway?  Moving to future.

Comment 3

[Doug Turner (:dougt)]

to gagan.  Given time, we should review these call sites.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: remove all AssignWithConversion in necko (checked in)

Comment 5

[Darin Fisher]

Comment on attachment 151727 [details] [diff] [review]
remove all AssignWithConversion in necko (checked in)

>Index: streamconv/test/TestStreamConv.cpp

> nsresult SendData(const char * aData, nsIStreamListener* aListener, nsIRequest* request) {
>-    nsString data;
>-    data.AssignWithConversion(aData);
>     nsCOMPtr<nsIInputStream> dataStream;
>-    nsresult rv = NS_NewStringInputStream(getter_AddRefs(dataStream), data);
>+    nsresult rv = NS_NewCharInputStream(getter_AddRefs(dataStream), aData);
>     if (NS_FAILED(rv)) return rv;
> 
>     return aListener->OnDataAvailable(request, nsnull, dataStream, 0, -1);
> }

note: you're eliminating a string copy here.  |dataStream| will hold
a pointer to aData, so we're really counting on the fact that
OnDataAvailable is supposed to consume all of |dataStream| before
returning.

hmm... we should be passing strlen(aData) for the ODA's count param.

but, whatever... this is just test code.


r+sr=darin

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 151727 [details] [diff] [review]
remove all AssignWithConversion in necko (checked in)

thanks, patch checked in.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: remove all *WithConversion from necko (checked in)

this kills *WithConversion (which only matches AppendWithConversion)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 153719 [details] [diff] [review]
remove all *WithConversion from necko (checked in)

checked in