Replace wstring with AString in IDL

VERIFIED FIXED

Status

Core Graveyard
Security: UI
P3
normal
VERIFIED FIXED
15 years ago
a year ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

1.0 Branch

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

72.49 KB, patch
Javier Delgadillo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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.
(Assignee)

Updated

15 years ago
Blocks: 168452
(Assignee)

Comment 1

15 years ago
Created attachment 99995 [details] [diff] [review]
Patch v1
(Assignee)

Comment 2

15 years ago
Darin, since you suggested the change, could you review?

Comment 3

15 years ago
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.
Attachment #99995 - Flags: review+
(Assignee)

Comment 4

15 years ago
> +  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.


(Assignee)

Comment 5

15 years ago
> 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
(Assignee)

Comment 6

15 years ago
Created attachment 100238 [details] [diff] [review]
Patch v2
Attachment #99995 - Attachment is obsolete: true
(Assignee)

Comment 7

15 years ago
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
Attachment #100238 - Flags: superreview+
Attachment #100238 - Flags: review+

Comment 8

15 years ago
kai: agreed.. just wanted to point these out while i was looking seeing them :)
(Assignee)

Comment 9

15 years ago
Comment on attachment 100238 [details] [diff] [review]
Patch v2

I realize I dreamed that Javi already reviewed. :) Removing nonexistant r=javi
Attachment #100238 - Flags: review+

Comment 10

15 years ago
Comment on attachment 100238 [details] [diff] [review]
Patch v2

r=javi
Attachment #100238 - Flags: review+
(Assignee)

Comment 11

15 years ago
Fixed on trunk.
(Assignee)

Comment 12

15 years ago
Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

15 years ago
This introduced bug 170438.

Comment 14

15 years ago
Verified per kaie's comment.
Status: RESOLVED → VERIFIED
Priority: -- → P3
Version: unspecified → 2.4

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

9 years ago
Version: psm2.4 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.