Need to implement nsISerializable and nsIClassInfo.

Are all those classinfo stubs required? Or are there shared generic stubs you could use (or should these be non-stubs that return useful results)? /be

> Are all those classinfo stubs required? Yes, because fastload needs the CID. It's really a pain in the behind. > Or are there shared generic stubs you could use There aren't. I almost considered creating some macros for this.... > (or should these be non-stubs that return useful results) They could be if other consumers want to use them, but not needed for fastload.

(In reply to comment #3) > > Are all those classinfo stubs required? > > Yes, because fastload needs the CID. It's really a pain in the behind. Something here is. My view used to be that lack of universal classinfo was the source of pain. But I'm now inclined to think all XPCOM must die. :-| > > Or are there shared generic stubs you could use > > There aren't. I almost considered creating some macros for this.... Cc'ing :bs. /be

Comment on attachment 272504 [details] [diff] [review] Like so Kai, you first -- I try to sr after the r+. /be

Comment on attachment 272504 [details] [diff] [review] Like so Tabs crept into this patch. Yes, macros would be nice. Yes, making serious changes to XPCOM while being source-code-compatible-or-autorewritable would be even better.

Ugh. I'll fix the tabs. Darned missing modelines!

If guess it's not possibly to make the new constructor > nsNSSCertificate::nsNSSCertificate(); protected or private? I'd prefer that if possibly, but I guess you need it public for the serialization to work? > + if (!!InitFromDER(const_cast<char*>(str.get()), len)) { I guess you don't want 2 exclamation marks? in both > +nsNSSCertificate::Write(nsIObjectOutputStream* aStream) > +nsNSSCertificate::Read(nsIObjectInputStream* aStream) Should you check that aStream is non-NULL before you dereference it? > +nsNSSCertificate::Read(nsIObjectInputStream* aStream) What happens if you call this on a instance that got already initialized? I guess it's unlikely, but for completeness I'd prefer a cleanup like: if (mCert) { destroy old cert set mCert/type/permdelete to initial values (should reading or initFromDER fail) } > + // XXXbz and if CERT_DupCertificate fails? Then mCert == nsnull, which should be ok. (I realize that some methods are dereferencing mCert without checking, but that's another bug. At least this will always be a good crash.) > +nsNSSCertificate::GetInterfaces(PRUint32 *count, nsIID * **array) > +nsNSSCertificate::GetHelperForLanguage(PRUint32 language, nsISupports **_retval) > +nsNSSCertificate::GetContractID(char * *aContractID) > +nsNSSCertificate::GetClassDescription(char * *aClassDescription) > +nsNSSCertificate::GetClassID(nsCID * *aClassID) > +nsNSSCertificate::GetImplementationLanguage(PRUint32 *aImplementationLanguage) > +nsNSSCertificate::GetFlags(PRUint32 *aFlags) > +nsNSSCertificate::GetClassIDNoAlloc(nsCID *aClassIDNoAlloc) Should you use NS_ENSURE_ARG on the pointer params? > +nsNSSCertificate::GetFlags(PRUint32 *aFlags) Out of curiousity, is there a special reason why you limit this to the main thread only? I'll review quickly when you attach a new patch.

cc'ing Bob FYI only

> If guess it's not possibly to make the new constructor protected or private? Hmm... I could try to make it private and make the factory constructor a friend function so that it can still call it. I'll do that. > I guess you don't want 2 exclamation marks? Yikes. No, I do not. > Should you check that aStream is non-NULL before you dereference it? That would be a contract violation for this interface. If you prefer, I can null-check it anyway, of course. Please let me know. > What happens if you call this on a instance that got already initialized? I'd be happy to either do the cleanup or make that case throw. I'd prefer throwing, I think. Again, let me know which you prefer. > Should you use NS_ENSURE_ARG on the pointer params? In general I would not for XPCOM out params. But it's your module; if you prefer I can do that. > Out of curiousity, is there a special reason why you limit this to the main > thread only? Um... no. That's a copy/paste from another class that I had with similar classinfo. If this class is ok off the main thread we should change that. Given your question I assume it is. Will attach new patch once you tell me what you want for some of the above items.

You can make the factory function a static class function to avoid nasty friendship requirements: class myClass { static NS_METHOD Create(...); };

(In reply to comment #10) > > Should you check that aStream is non-NULL before you dereference it? > > That would be a contract violation for this interface. If you prefer, I can > null-check it anyway, of course. Please let me know. Shouldn't be necessary. > > Should you use NS_ENSURE_ARG on the pointer params? > > In general I would not for XPCOM out params. But it's your module; if you > prefer I can do that. Should not be necessary. > > Out of curiousity, is there a special reason why you limit this to the main > > thread only? > > Um... no. That's a copy/paste from another class that I had with similar > classinfo. If this class is ok off the main thread we should change that. > Given your question I assume it is. FastLoad is main thread only, FWIW. /be

> FastLoad is main thread only, FWIW. Sure. But nsNSSCertificates are used by things other than fastload.

(In reply to comment #10) > > If guess it's not possibly to make the new constructor protected or private? > > Hmm... I could try to make it private and make the factory constructor a friend > function so that it can still call it. I'll do that. Both your proposal and Benjamin's proposal sound good to me. > > What happens if you call this on a instance that got already initialized? > > I'd be happy to either do the cleanup or make that case throw. I'd prefer > throwing, I think. Again, let me know which you prefer. I like the idea to abort with a failure in ::Read if mCert is already set. > > Out of curiousity, is there a special reason why you limit this to the main > > thread only? > > Um... no. That's a copy/paste from another class that I had with similar > classinfo. If this class is ok off the main thread we should change that. > Given your question I assume it is. Yes, the class is thread safe. As both you and Brendan confirm checking the parameters for NULL isn't necessary, no need to add the checks.

The constructor is still public, because if I make it private, the factory method will still be public. So there's no net win. If you want me to jump through the hoops needed to make it private anyway, do let me know, but I don't see a benefit to it...

Comment on attachment 275549 [details] [diff] [review] Updated to comments (In reply to comment #15) > The constructor is still public, because if I make it private, the factory > method will still be public. So there's no net win. If you want me to jump > through the hoops needed to make it private anyway, do let me know, but I don't > see a benefit to it... I don't want to ask you to do unnecessary extra work, I had hoped it to be simple. My motivation was the initial design of the class, which didn't allow a way to construct an empty instance, but always required to pass in a cert. But given the fact that one is able to pass NULL to the old constructor, which effectively constructs an empty instance, too, and we therefore need to be prepared for that scenario anyway, it seems acceptable to have this additional default constructor public. I'm ok with this patch, r=kengert FWIW, you have an unnecessary whitespace change in nsNSSCertificate::GetRawDER, but I don't mind. (And I wish the NSS interfaces used const more often, so that your const_cast wouldn't be necessary :-/ )

Comment on attachment 275549 [details] [diff] [review] Updated to comments Requesting 1.9 approval. This adds code to serialize/deserialize NSS certificates; something we need to serialize/deserialize principals (for session restore, e.g.). Risk is very low.

Comment on attachment 275549 [details] [diff] [review] Updated to comments It's been a week and a half. Just self-approving, dammit.

> FWIW, you have an unnecessary whitespace change in nsNSSCertificate::GetRawDER, Checked in without that whitespace change.

(In reply to comment #3) > > Or are there shared generic stubs you could use > > There aren't. I almost considered creating some macros for this.... Hm? Why can't you use the normal classinfo helpers? (http://developer.mozilla.org/en/docs/Using_nsIClassInfo#I.27m_writing_C_code.2C_and_I_use_the_generic_factory_and_nsModuleComponentInfo.)

Because it's actually more painful than implementing nsIClassInfo on the object directly? Cristian caught a bug in this patch, by the way: I didn't change the QI impl! Need to fix that. Would be nice to have actual certs to write tests around...

patch in bug 405481 for that issue