169932 - Replace wstring with AString in IDL

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Kai Engert (:KaiE:)]

Alec and Darin asked me to change wstring to AString in PSM's interfaces.

You convinced me, I'm attaching a patch that does it.

However, I'm also using arrays of strings as parameteres, and I get IDL compiler
warnings when I try to do that AString. I assume it is ok to continue using
wstring for those. Please explain if you disagree.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 2

[Kai Engert (:KaiE:)]

Darin, since you suggested the change, could you review?

Comment 3

[Darin Fisher]

Comment on attachment 99995 [details] [diff] [review]
Patch v1

>Index: mozilla/security/manager/pki/src/nsNSSDialogs.cpp

>+  rv = dialogBlock->SetString(1, NS_ConvertUTF8toUCS2(targetURL).get());

are you converting this to unicode inorder to display it to the user?
if so, then you should really be using nsITextToSubURI::unEscapeURIForUI()
which will try to generate an URL string that is most user friendly.

of course, this means that you'll need to know the charset of the URL.
you can probably just pass nsIURI::originCharset.  or if you have the
document from which this URL originated, then it would be good to pass
the document charset.  either way, this function can really help the UI.


>+  nsString commonName;
>   nsString formattedDate;

is commonName ever < 64 chars?	if so, then nsAutoString might be a good
thing to use here.


>   PRUnichar *formattedDatePR = ToNewUnicode(formattedDate);

NS_ConvertASCIItoUCS2 formattedDateUCS(formattedDate);

>   nsString keyString = NS_ConvertASCIItoUCS2(key);
>   nsString titleKeyString = NS_ConvertASCIItoUCS2(titleKey);

you can avoid a string copy by doing this instead:

NS_ConvertASCIItoUCS2 keyString(key);
NS_ConvertASCIItoUCS2 titleKeyString(titleKey);


>+  rv = dialogBlock->SetString(1, NS_ConvertUTF8toUCS2(targetURL).get());

same UI issue/question here.


>+		PRUnichar *pw;
>+    rv = block->GetString(2, &pw);
>+		if (NS_SUCCEEDED(rv)) {
>+			_password = pw;
>+			nsMemory::Free(pw);
>+		}
>   }

nit: indentation is really wacky here.	likewise below:

>+		PRUnichar *pw;
>+    rv = block->GetString(2, &pw);
>+		if (NS_SUCCEEDED(rv)) {
>+			_password = pw;
>+			nsMemory::Free(pw);
>+		}
>   }


>Index: mozilla/security/manager/ssl/public/nsIBadCertListener.idl

>+                         in AUTF8String targetURL,
>                          in nsIX509Cert cert);

just a side question: would it make sense to pass nsIURI here instead?


>Index: mozilla/security/manager/ssl/src/nsCRLManager.cpp

>+      crlData->GetLastFetchURL(updateURL);
>+      pref->SetCharPref(updateUrlPrefStr.get(),updateURL.get());

does it matter if updateURL contains non-ASCII chars?


>Index: mozilla/security/manager/ssl/src/nsCertTree.cpp

>+      nsString nick;
>+      rv = cert->GetNickname(nick);

nsAutoString?

>+      nsAString::const_iterator start, end, end2;
>+      nick.BeginReading(start);
>+      nick.EndReading(end);
>+			end2 = end;

indentation wierdness.
>+		nsString dummyPurposes;
>+    rv = cert->GetPurposes(&verified, dummyPurposes);

nsAutoString? indentation wierdness.

>Index: mozilla/security/manager/ssl/src/nsNSSCertificate.cpp

>+    nsString temp1;

nsAutoString?


>+  nsString dispNameU, dispValU;

nsAutoString?

>Index: mozilla/security/manager/ssl/src/nsNSSComponent.cpp

>+    nsString fingerprint;
>+    rv2 = pCert->GetSha1Fingerprint(fingerprint);
>+    NS_LossyConvertUCS2toASCII fingerprintStr(fingerprint);

nsAutoString instead of nsString above and below?

>+    nsString orgName;
>+    rv2 = pCert->GetOrganization(orgName);


sorry to nit pick so much... afterall, this is a huge patch.  nice work!! :)

r/sr=darin with the changes mentioned above.

Comment 4

[Kai Engert (:KaiE:)]

> +  rv = dialogBlock->SetString(1, NS_ConvertUTF8toUCS2(targetURL).get());
>
> are you converting this to unicode inorder to display it to the user?
> if so, then you should really be using nsITextToSubURI::unEscapeURIForUI()
> which will try to generate an URL string that is most user friendly.
> 
> of course, this means that you'll need to know the charset of the URL.
> you can probably just pass nsIURI::originCharset.  or if you have the
> document from which this URL originated, then it would be good to pass
> the document charset.  either way, this function can really help the UI.


Darin:
- it looks like this contains only the hostname, since we are at the SSL
handshake phase, which does not yet care for the path
- No content has yet been transfered, since we are at the outer SSL layer, the
charset is not yet known
- I have no idea how I could access the document that was shown previously,
since this network layer crypto code is far away from any content shown. And we
don't know whether the content of the previously shown document is applicable
for this new URL, do we?

Anyway, this changed line is a sideeffect of the string changes. Your request to
unescape is something else. If you want this changed, please let's do this in
the separate bug 170312.

Comment 5

[Kai Engert (:KaiE:)]

> is commonName ever < 64 chars?
> if so, then nsAutoString might be a good thing to use here.
changed

>   PRUnichar *formattedDatePR = ToNewUnicode(formattedDate);
I didn't write that line, but anyway, changed.
(Note that I'm always trying to change as little as possible.)

>  nsString keyString = NS_ConvertASCIItoUCS2(key);
>  nsString titleKeyString = NS_ConvertASCIItoUCS2(titleKey);
Those are just context lines, they are not being touched by my changes.
But anyway, changed.

> nit: indentation is really wacky here. likewise below:
Indeed. My editor was accidentially set to use hard tabs. Fixed all places.

> just a side question: would it make sense to pass nsIURI here instead?
I don't want to change it. The supplier does not have a nsIURI, but a string.

> does it matter if updateURL contains non-ASCII chars?
I don't know. Please let's discuss this in bug 170317.

> nsString => nsAutoString
all changed

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 100238 [details] [diff] [review]
Patch v2

carrying forward r=javi

I addressed all of Darin's comments that were addressed to code that is
actually being changed with this patch. I filed separate bugs for suggestions
unrelated to my changes. Marking sr=darin

Comment 8

[Darin Fisher]

kai: agreed.. just wanted to point these out while i was looking seeing them :)

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 100238 [details] [diff] [review]
Patch v2

I realize I dreamed that Javi already reviewed. :) Removing nonexistant r=javi

Comment 10

[Javier Delgadillo]

Comment on attachment 100238 [details] [diff] [review]
Patch v2

r=javi

Comment 11

[Kai Engert (:KaiE:)]

Fixed on trunk.

Comment 12

[Kai Engert (:KaiE:)]

Marking fixed.

Comment 13

[Kai Engert (:KaiE:)]

This introduced bug 170438.

Comment 14

[John Unruh]

Verified per kaie's comment.