Closed
Bug 169932
Opened 22 years ago
Closed 22 years ago
Replace wstring with AString in IDL
Categories
(Core Graveyard :: Security: UI, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: KaiE, Assigned: KaiE)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
javi
:
review+
KaiE
:
superreview+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•22 years ago
|
||
Comment 3•22 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•22 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•22 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 7•22 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•22 years ago
|
||
kai: agreed.. just wanted to point these out while i was looking seeing them :)
Assignee | ||
Comment 9•22 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•22 years ago
|
||
Comment on attachment 100238 [details] [diff] [review] Patch v2 r=javi
Attachment #100238 -
Flags: review+
Assignee | ||
Comment 12•22 years ago
|
||
Marking fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 14•22 years ago
|
||
Verified per kaie's comment.
Status: RESOLVED → VERIFIED
Priority: -- → P3
Version: unspecified → 2.4
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•