75123 - Make Cert Manager UI look like the mockup

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Bob Lord]

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.

Comment 1

[Bob Lord]

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.

Comment 2

[David P. Drinan]

Re-assigning to mcgreer.

Comment 3

[Ian 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.

Comment 4

[Bob Lord]

Target -> 2.0

Comment 5

[Ian McGreer]

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

Comment 6

[Ian McGreer]

Attachment: the patch

Comment 7

[Javier Delgadillo]

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

Comment 8

[Javier Delgadillo]

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.

Comment 9

[Ian McGreer]

Attachment: patch which fixes javi's concerns

Comment 10

[Ian McGreer]

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

Comment 11

[Ian McGreer]

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

Comment 12

[Ian McGreer]

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).

Comment 13

[Ian McGreer]

Attachment: said patch

Comment 14

[timeless]

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?

Comment 15

[Ian McGreer]

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.

Comment 16

[Ian McGreer]

Attachment: rev 3

Comment 17

[Ian McGreer]

Attachment: ignore rev 3, from wrong dir. rev 4

Comment 18

[Ian McGreer]

Attachment: rev 5, based on javi's comments

Comment 19

[Javier Delgadillo]

r=javi on rev 5 of the patch.

Comment 20

[David P. Drinan]

Scott,

Please super-review the latest patch.

Thanks.

Comment 21

[Christopher Blizzard (: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?

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

Comment 22

[Ian McGreer]

> 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.

Comment 23

[timeless]

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');

Comment 24

[Ian McGreer]

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.

Comment 25

[John Unruh]

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