1) I have many certs from AOL (some expired, some active). They should show up as a single item with a twisty to expand the list per the UI mockup. 2) make sure the buttons are consistent in name and function between the Cert Manager and the other parts of the Mozilla browser.

Add these columns (some per the mockup some per private email): -Purpose (should probably be called "Usage") -Email address (where to get it? "E" field in the CN? SubjustAltName?) -Expiration date I really like the ability to pick which columns to display, and the ability to click on a title to sort.

Re-assigning to mcgreer.

For M2, I implemented the certificate manager using outliner, via a JavaScript object. This is the quick way to get it going, but not the most flexible (and somehow it was causing a crash). To implement the features desired for PSM2, it is neccessary to implement an nsIOutlinerView. I'm currently working on this, and I believe it will fix all of the bugs listed above.

Target -> 2.0

patch forthcoming addresses following issues: * gets rid of "phantom certs" (blank lines in outliner) * implements nsIOutlinerView instead of using JS * removes "others" tab * adds sections to cert listing (name/token/verified/purposes/issued/expires) * error handling for PKCS#12, throwing appropriate dialog windows

r=javi since this is the first in a series of patches towards full functionality.

Things to be addressed in next patch: Using nsIPrompt to alert user after PKCS12 operation Using nsIDateTimeFormat to format the time retrieved from certs for validity dates.

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

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

another patch revision forthcoming. since I had time to work on it, I got outliner to work with nesting by organization. The additions to the patch implement that. Now it looks very much like the mockup (certs are sorted/divided correctly now, by token, then organization, then common name).

when will the code that you added to the comment block in function LoadCerts() be activated? could you prehaps include a ~//XXX waiting for this feature to be implemented see bux yyy~ or something? + <box orient="vertical" flex="1"> have you considered <vbox flex="1"> ... </vbox> ? "software makers" sounds icky + nsAutoString typestr = NS_ConvertASCIItoUCS2("VerifyCAVerifier"); are you sure that shouldn't be NS_LITERAL_STRING?

all of the commented-out code is for support of S/MIME certificates. Since Mozilla does not support S/MIME, it is commented out for now. Much of it was copy-and-paste, so I left it in to make it easier to implement S/MIME support in the future. I made the change to NS_LITERAL_STRING in the next patch.

r=javi on rev 5 of the patch.

Scott, Please super-review the latest patch. Thanks.

What's with the others_tab code that's commented out all over the place? Is that part of the email code that's commented out that's mentioned in the bug? Why is this commented out? +// var windowReference = document.getElementById("certmanager"); +// if (windowReference != null) { +// windowReference.focus(); +// } else { + window.open('chrome://pippki/content/certManager.xul', "", + 'chrome,width=500,height=400,resizable=1'); +// } Do you need to call FreeCertArray() from the destructor? I would suspect that as mCertArray was released that it would unref all of it's members and delete itself. Looking at nsSupportsArray.cpp this seems to be the case. In all of the cmp functions you can probably use an nsXPIDLString() like this: nsXPIDLString aTok; a->GetTokenName(getter_Copies(aTok)); You might be able to avoid the nsAutoString in that case but I'm not sure. This is just a suggestion. I'm just thinking about avoiding the explicit free() calls. + mOutlinerArray = (outlinerArrayEl *)nsMemory::Alloc( + sizeof(outlinerArrayEl) * mNumOrgs); this kind of bugs me but it appears that it's the only place that it's allocated so I guess it's not too bad. +NS_IMETHODIMP +nsCertOutliner::GetCert(const PRUint32 aIndex, nsIX509Cert **_cert) +{ + NS_ENSURE_ARG(_cert); + nsCOMPtr<nsIX509Cert> cert = GetCertAtIndex(aIndex); + if (cert) { + *_cert = cert; + NS_ADDREF(*_cert); + } + return NS_OK; +} + Can't you just do: NS_ENSURE_ARG(_cert) *_cert = GetCertAtIndex(aIndex); return NS_OK; since the getter apparently addrefs? Lots of string copying which may be all required in nsCertOutliner::GetCellText including some: + nsMemory::Free(tmp); + nsMemory::Free(tmps); blah. Other than that the code looks fine to me. These are mostly just nits so take them or leave them. sr=blizzard

> What's with the others_tab code that's commented out all over the place? Is > that part of the email code that's commented out that's mentioned in the bug? Yes. It was in for testing, but we decided not to confuse people by making them believe there was functionality that isn't there. Since it should work, I just commented it out. > Why is this commented out? > +// var windowReference = document.getElementById("certmanager"); > +// if (windowReference != null) { > +// windowReference.focus(); > +// } else { > + window.open('chrome://pippki/content/certManager.xul', "", > + 'chrome,width=500,height=400,resizable=1'); > +// } That was an attempt to make only one cert manager window open. I don't believe it will work until the prefs window isn't modal, which is another bug. > Do you need to call FreeCertArray() from the destructor? I would suspect that > as mCertArray was released that it would unref all of it's members and delete > itself. Looking at nsSupportsArray.cpp this seems to be the case. I didn't. Changed. > In all of the cmp functions you can probably use an nsXPIDLString() like this: changed. > Can't you just do: > NS_ENSURE_ARG(_cert) > *_cert = GetCertAtIndex(aIndex); > return NS_OK; changed. > Lots of string copying which may be all required in nsCertOutliner::GetCellText > including some: > + nsMemory::Free(tmp); > + nsMemory::Free(tmps); I left it in for now, but will look at ways of fixing that.

ignoring modality, danm, what's the correct way to prevent multiple instances? window.open('chrome://pippki/content/certManager.xul', /*shouldn't this be a non empty string naming the window? I can't remember*/ "", 'chrome,width=500,height=400,resizable=1');

Marking as fixed, since the final checkin put cert manager is a state where it closely resembles the mock-up. Individual bugs should be tracked seperately.

Verified. I'll open individual bugs against the UI after this.