The following interfaces require complete JavaDoc style documentation: nsICRLInfo nsIX509Cert nsIX509CertDB nsIX509CertValidity nsIASN1Sequence nsIASN1Object nsICertificateDialogs nsIBadCertListener nsISecurityWarningDialogs

This patch: - adds JavaDoc style comments for all elements in all interfaces we are going to freeze - it also fixes bug 169148 (change return type of caCertExists) - I reordered the elements in nsIX509Cert to a more logical grouping, but I did not change anything within that interface Javi, can you please review?

*** Bug 169148 has been marked as a duplicate of this bug. ***

Comment on attachment 99684 [details] [diff] [review] Patch v1 interface nsICRLInfo : nsISupports { + + /** + * The issueing CA's organization. + */ readonly attribute wstring org; + + /** + * The issueing CA's organizational unit. + */ readonly attribute wstring orgUnit; correct mis-spelling issueing->issuing in both comments I assume the following line will disappear from the nsIX509Cert interface: readonly attribute wstring rsaPubModulus; + /** + * Obtain an single comma Should say "Obtain a single comma..."

Comment on attachment 99684 [details] [diff] [review] Patch v1 interface nsICRLInfo : nsISupports { + + /** + * The issueing CA's organization. + */ readonly attribute wstring org; + + /** + * The issueing CA's organizational unit. + */ readonly attribute wstring orgUnit; correct mis-spelling issueing->issuing in both comments I assume the following line will disappear from the nsIX509Cert interface: readonly attribute wstring rsaPubModulus; + /** + * Obtain an single comma Should say "Obtain a single comma..." Should we just get rid of this line following exportPKCS12File? //[array, size_is(count)] in wstring aCertNames); perhaps the comments for getOCSPResponders should say what to QI the elements in the returned nsISupportsArray to. I'm assuming nsIX509Cert, but I could be wrong. Other than that, r=javi

> correct mis-spelling issueing->issuing in both comments done > I assume the following line will disappear from the nsIX509Cert interface: > readonly attribute wstring rsaPubModulus; yeah, see bug 168976 > Should say "Obtain a single comma..." fixed > Should we just get rid of this line following exportPKCS12File? > //[array, size_is(count)] in wstring aCertNames); indeed, removed > perhaps the comments for getOCSPResponders should say what to QI the elements > in the returned nsISupportsArray to. I'm assuming nsIX509Cert, but I could be > wrong. agreed, added the comment @return Array of OCSP responders, entries are QIable to nsIOCSPResponder

This is the real patch v2 file.

Comment on attachment 99701 [details] [diff] [review] Patch v2 Marking r=javi since I addresses his comments.

Comment on attachment 99701 [details] [diff] [review] Patch v2 Marking r=javi since I addressed his comments.

Alec, can you please review?

Comment on attachment 99701 [details] [diff] [review] Patch v2 i'm going to send kaie a list.

Sigh, is my patch really that bad? Timeless sent me a message where he requests to change nearly every comment, and even some semantics and interface names. I feel a bit reluctant to spend another day on that cleanup.

Comment on attachment 99701 [details] [diff] [review] Patch v2 kaie: timeless is extremely pedantic, but we do appreciate his thoroughness. Timeless, can you tone it down a little and post the toned-down comments to the bug? By toned down, I mean try to use some judgement about what issues are really important right now, and what issues are just nitpicks - it is more important to fix the larger issues than the nitpicks. If timeless's issues with the patch are really that long, then there is either a more global problem (i.e. timeless, you don't have to list every instance of a misspelled or misused word) or there is something wrong with the whole patch. I'm willing to be the mediator on this, but lets try to figure out what issues are most important.

Comment on attachment 99701 [details] [diff] [review] Patch v2 This is really a review of the API using the documentation as a guide. A. there are a bunch of apis where you use true to mean cancel B. and then a bunch of apis where false means cancel (and true means continue). [I'd prefer true to mean continue.] * In fact perhaps "don't continue" should be handled as an exception instead of a return. consider: boolean getPKCS12FilePassword(in nsIInterfaceRequestor ctx, out wstring password); boolean setPKCS12FilePassword(in nsIInterfaceRequestor ctx, out wstring password); it's really wstring promptForPassword/choosePassword(in nsIInterfaceRequestor ctx, long type[PKCS12_File=1]) @throws canceled C. functions named |alertWhatever| which are designed to confirm things should be |confirmWhatever|. Similarly functions which find things shouldn't be |getThing| they should be |findThing|. * boolean unknownIssuer/mismatchDomain. without the idl comment you'd have no idea that this is a confirm function, it needs a better name. I think it's an allow/use/accept. * cACertExists should probably be a confirm but at least it shouldn't begin cA D. there are a bunch of human readable fields on the interfaces, do they map to distinct parts of the certificates? If they don't (i.e. they map to the same datcertificate field as the non human readable interface item), then do they need to live on the interface? especially the time fields, for which the consumer can use a normal time to human readable api. E. there are a bunch of rather similar search functions, i'm wondering if it might make sense to have a single function with all of the search fields instead of lots of similar functions, or perhaps only one search field since they're all strings and a search type field. F. some functions have optional parameters first, (G) some have them last. [i'd prefer for them to be last (right before out/retvals)] H. some of the boolean attributes (processObjects, showObjects) are asking for a prefix like canProcessObjects. - But I don't understand how showObjects would be used so I have no idea what sort of thing it is... I. ADD_<whatever>, some of these things aren't really adds... J. org/orgUnit v. organization/organizationalUnit - unless you think someone will be implementing both of these in a single object i think you should be consistent and use the same attribute names. K. don't use 'usages'. L. functions which could be replaced by a simple attribute: readonly attribute boolean ocspOn; void enableOCSP(); => attribute boolean ocspEnabled; void getRawDER(out unsigned long length, [retval, array, size_is(length)] out octet data); -- couldn't this be a readonly attribute? * misc * boolean isSameCert(in nsIX509Cert other) - Why can't this be called equals? * void deleteCertificate(in nsIX509Cert aCert) - this is removeCert[ificate] -- consistency ... * boolean getCertTrust(in nsIX509Cert cert,...) - isCertTrusted/Trustworthy AC boolean alertEnteringSecure(in nsIInterfaceRequestor ctx); AC boolean alertEnteringWeak(in nsIInterfaceRequestor ctx); AC boolean alertLeavingSecure(in nsIInterfaceRequestor ctx); AC boolean alertMixedMode(in nsIInterfaceRequestor ctx); A boolean confirmPostToInsecure(in nsIInterfaceRequestor ctx); A boolean confirmPostToInsecureFromSecure(in nsIInterfaceRequestor ctx); BC boolean unknownIssuer(in nsIInterfaceRequestor socketInfo, in nsIX509Cert cert, out short certAddType); BC boolean mismatchDomain(in nsIInterfaceRequestor socketInfo, in wstring targetURL, in nsIX509Cert cert); B boolean certExpired(...) B boolean downloadCACert(...) C cACertExists(...) D readonly attribute wstring lastUpdateLocale; D readonly attribute wstring nextUpdateLocale; D readonly attribute wstring issuedDate; D readonly attribute wstring issuedDateSortable; D readonly attribute wstring expiresDate; D readonly attribute wstring expiresDateSortable; ECF nsIX509Cert getCertByNickname(in nsISupports aToken, in wstring aNickname); ECG nsIX509Cert getCertByDBKey(in string aDBkey, in nsISupports aToken); C void getCertNicknames(in nsISupports aToken, in unsigned long aType, out unsigned long count, [array, size_is(count)] out wstring certNameList); EC nsIX509Cert getEmailEncryptionCert(in wstring aNickname); EC nsIX509Cert getEmailSigningCert(in wstring aNickname); ECF nsIX509Cert getCertByEmailAddress(in nsISupports aToken, in string aEmailAddress); H attribute boolean processObjects; H attribute boolean showObjects; HL readonly attribute boolean usesOCSP; I const short ADD_TRUSTED_FOR_SESSION = 1; (TRUST_FOR_SESSION) I const short ADD_TRUSTED_PERMANENTLY = 2; (TRUST_PERMANENTLY)

ok, my take on timeless's comments: They are valid in that the issues he's brought up with the API will change the way the APIs are documented if the issues are addressed. I think its better to fix the interface first, then fix the documentation, rather than fix documentation now, change the APIs, and redocument later, but I'll leave that up to Kai. A. and B. - if he's right, yes, we should clean these up to be consistent. C. I also agree, on all points. D. we've discussed these previously with Kai, and we're going to leave them as they are for now. E. I think the API is fine as it is. Unless Kai can come up with an advantage to having one function, lets leave it F. I think kai should make this call. We want consistent, clear APIs but I'm assuming that the existing ordering is fine. since there is no technical way to have "optional" methods, I consider passing in null to be a valid use of the parameter, with a particular meaning. It doesn't mean that the parameter is "optional" H. I'll let kai explain the interface. In general, we like the attribute name to allow the code to be more human readable. I don't know the context in which to compare foo.processObjects vs. foo.canProcessObjects. My guess though, is that timeless is correct that these should have the "can" part. So overall I think whats happened is that we (Myself, Kai, Juan, etc) have looked at the individual methods to see if they seem valid, but haven't looked at the whole set of interfaces as a cohesive unit. Timeless has provided some valid feedback here that we should address regarding consistency, etc.

Comment on attachment 99701 [details] [diff] [review] Patch v2 I disagree. The suggested changes do not decrease dependencies, nor do they increase functionality. Especially I fear the risk of confusion and regressions caused by reverting the meaning of true/false in existing APIs. But this is not the topic of this bug. If you think we should change the current API, please file separate coding style enhancement bugs. This bug is about adding documentation to the interfaces, which is the task that has been assigned to me. I have done that. If some of my explanations are not good enough, that might be because I don't know it better, as I didn't write the underlying code and didn't study it fully yet. The comments are meant to provide help understanding the APIs as they are now.

somehow, my comments for J, K, and L didn't make it above - I think they are also valid points. "usages" isn't even a word.

Stephane allowed me to spend some resources on this. With the new patch I have addressed your comments that I agree with. I have commented on the parts I disagree with. Please review the new patch. What I changed is explained below. > A. there are a bunch of apis where you use true to mean cancel Only my comment was wrong for the two confirm... functions. Changed for the alert... functions. > B. and then a bunch of apis where false means cancel (and true means continue). > [I'd prefer true to mean continue.] Reread your statement, what are you saying? You prefer if it were as it is??? ;-) I assume you still want to use true to mean continue... I changed the get/setpkcs12 funcs. I changed DownloadCACert. For the other functions you listed, true already means continue... > C. functions named |alertWhatever| which are designed to confirm things should > be |confirmWhatever|. Similarly functions which find things shouldn't be > |getThing| they should be |findThing|. renamed > * boolean unknownIssuer/mismatchDomain. without the idl comment you'd have no > idea that this is a confirm function, it needs a better name. I think it's an > allow/use/accept. renamed > * cACertExists should probably be a confirm but at least it shouldn't begin cA renamed to say notifyCACertExists > D. we've discussed these previously with Kai, and we're going to leave them as > they are for now. nothing changed > E. I think the API is fine as it is. Unless Kai can come up with an advantage to > having one function, lets leave it nothing changed > F. I think kai should make this call. nothing changed > H. some of the boolean attributes (processObjects, showObjects) are asking for a > prefix like canProcessObjects. - But I don't understand how showObjects would > be used so I have no idea what sort of thing it is... You didn't seem to have read my interface comments, in it I explained what showObjects means. This interface enables the object to be used directly as a data structure for a visible represenatation implementation. Anyway, renamed to something that is hopefully clearer. processObjects -> isValidContainer showObjects -> isExpanded > I. ADD_<whatever>, some of these things aren't really adds... You are wrong. These constants are used when *trust is added* to the database. Not changed. > J. org/orgUnit v. organization/organizationalUnit - unless you think someone > will be implementing both of these in a single object i think you should be > consistent and use the same attribute names. renamed > K. don't use 'usages'. I'm not a native speaker so I can not tell whether this word is bad or not. Based on a suggestion from timeless on IRC, I renamed "usages" to "uses" and "usage" to "use". > L. functions which could be replaced by a simple attribute: > readonly attribute boolean ocspOn; > void enableOCSP(); > => attribute boolean ocspEnabled; No. You obviously did not read the comments that I added in the patch. The functions do not change the attribute. They are used to override the attribute, as it is explained in the comments... Not changed. > * misc > * boolean isSameCert(in nsIX509Cert other) - Why can't this be called equals? renamed > * void deleteCertificate(in nsIX509Cert aCert) - this is removeCert[ificate] No, not changed. We delete. > * boolean getCertTrust(in nsIX509Cert cert,...) - isCertTrusted/Trustworthy renamed

> Reread your statement, what are you saying? You prefer if it were as it is??? sorry. I should have said "I prefer B over A". Whether OCSP is enabled in preferences. readonly attribute boolean ocspOn; Use this to temporarily disable OCSP checking. Needed if OCSP checks slow down UI rendering too much. A call to this should be followed with a call to enableOCSP soon afterwards. void disableOCSP(); Sets the OCSP options to correspond with the preferences values. void enableOCSP(); Eek you're right On and Enable aren't a pair. I have no idea how i missed disable. So, there are three, and the correct pair is obviously disabled/enable, could that be: attribute boolean skipOscp; //? Also, 'temporarily' usually is kind of a flag like /* XXX hack fix me before mozilla0.9.2, (arbitrary outdated milestone to indicate that the problem was forgotten) */ Does it really belong on the frozen interface? If at some point you fix it so that it isn't needed, then you will have these useless methods hanging off your interface. fwiw, after reading the patch (which i like a lot), i think 'allow' is more appropriate than 'confirm'. Kaie: thanks for the work.

Your comment regarding the skip OCSP mode is good. I agree this mode looks like a hack. I saw that PSM only uses this hack in one particular situation, and therefore I have changed the code to no longer offer to enable a global skip mode, but rather only offer the single function with the ability to skip OCSP using a boolean parameter.

Comment on attachment 105455 [details] [diff] [review] Patch v4 those changes look great :)

Javi, can you please review?

Comment on attachment 105455 [details] [diff] [review] Patch v4 r=javi

Alec, can you please review?

Comment on attachment 105455 [details] [diff] [review] Patch v4 this generally looks ok, the one thing I'm now confused on (and maybe its just been a while) is the "issuedDateSortable". I remember that most of the string-based dates were only for display, but if this is a sorting one, why can't this be a PRTime type? those are totally sortable, and independent of timezone, etc. (for instance, what timezone is issuedDateSortable - is it GMT or the current TZ?) in any case, that's more an issue for another pass at these interfaces, I suppose. Thanks for this cleanup. This API is much clearer already. sr=alecf if you put a comment next to the issuedDateSortable saying: 1) the timezone of the string 2) that it should maybe be PRTime?

I would like to add a final statement about the term "usage". I fully disagree with your request to change it from "usage" to "use". I was believing you, because I'm not a native english speaker. However, I meanwhile see that he crypto library uses the term "usage" all over the place in its sourcecode. The X.509 RFC documents speak of "key usage" in the same contexts. I asked our technical writer about this phrase, and it seems OK to him, too. With the patch in this bug, I'm making a change that I really dislike. I'm going away from the term usage, that is used everywhere else. For people new to the code, that might make it harder for them to understand that what we call "use" in PSM is actually the same thing that is described as "usage" in NSS and in the RFCs. I feel tempted to change it back and I wish I had researched this better before I worked on this new patch. However, I don't want to spend yet more time on this nit, and to not lose more time discussing this small nit, I will not change the patch again with regards to "usage". I will attach another patch soon that addresses Alec's comments.

> why can't this be a PRTime type We talked about this before, but not yet for the certificate, only for the CRL interface. The problem is that we need to work with the string based representation of this attribute from within JS. When I looked at it, I did not find a simple way to do the string formatting from PRTime to a string from within JS. For the CRL interface, I solved the problem by offering both, the time as PRTime and as a formatted string. I just looked what can be done. The sortable representation is only needed from C++. I can do the following: - remove the string_sortable attributes from the interface - add PRTime attributes, and make the consumer C++ code to do the string formatting (the general cert UI column code still needs the date formatted as a string) - document the other attribute is a locale formatted string in local time

Patch grew by 10%. I removed the time attributes from nsIX509Cert completely, since it contains a validity attribute already, which is the place where all time information should be queried from. I added the ability to obtain a day-only string from the validity and changed the consumers accordingly and as described in the previous comment.

Comment on attachment 106025 [details] [diff] [review] Patch v5 thanks for cleaning up the sortable date issue.. as for usage, obviously that aspect of the API is up to you, but at least realize what you're saying: that you prefer that the API use the term "usage" but you refuse to change it. I don't know what to make of it since the use of "Usage" here is not gramatically correct, but if Usage has become a well understood term in the world of security, it seems like it would be worth the effort to get the API right. Otherwise, you'll have to spend just as much work fixing the documentation in the IDL to indicate that Uses == Usages. Up to you. sr=alecf on this patch, but you can also change Uses to Usages and the sr= will still apply.

Thanks for the reviews. This is the same patch as v5, but again using the term usage.

Checked in to trunk, marking fixed.

Verified per kaie's comment.