316710 - Land some generic crypto backend code, mostly cleanup, some small enhancements

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

Bob Relyea has produced several patches.

I have reviewed them, made suggestions to change them and applied my requested changes myself.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: sorry - wrong file attached

Original patch as created by Bob Relyea

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 203261 [details] [diff] [review]
sorry - wrong file attached

I'm fine with your changes in general, but I'm requesting some changes, as we have discussed by email.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Bob, I have worked on the changes that I had suggested.
Here is the updated patch.

Comment 5

[Kai Engert (:KaiE:)]

I would like to explain the code that we are adding.

- provide a new interface nsIX509Cert2 to eliminate the need to cast to non-IDL pointer, make use of it in nsCMS and other places.

- provide a new interface nsIX509CertList to deal with a list of certificates

- clean up the certificate cache implemenentation, use the new cert list

- enable the cert cache implementation to cache any given list of certificates

- in nsCertTree, a data structure used by certificate manager, improve speed by caching all strings intended for display

- to speed up, in nsNSSCertificate, only calculate the logical type of the assicoated NSS certificate once. Remove the not required ability to explicitly set the logical type.

- add backend code that allows a cert manager user interface to display a cert type information column

- for display purposes, when a cert has no isser org, fall back to common name


The changes between Bob's patch v1 and my patch v2 are:

- make nsNSSCertList not adopt the given list by default, to be consistent with our default behaviour "copy on construction". However, the possibility to use the optimized adopt technique is still available.

- do not introduce nsISupportsString and nsTreeString, but make use of existing implementations

- removed unnecessary nsCertTree::mNumColumns

Comment 6

[Kai Engert (:KaiE:)]

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

Comment 7

[Kai Engert (:KaiE:)]

Patch checked in.

Bob, should we have this on 1.8 branch for Firefox 2.0 ?

Comment 8

[Robert Relyea]

What is Camino based off of, the trunk or 1.8?

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 203263 [details] [diff] [review]
Patch v2

This patch implements several requests from Camino developers.

To answer Bob's question: Camino 1.x will be based on the 1.8 branch.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 203263 [details] [diff] [review]
Patch v2

looks like nsIX509CertDB2 needs a new IID...

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 203263 [details] [diff] [review]
Patch v2

Biesi, thanks a lot for pointing this out. I just learned, that although we just add a method to nsIX509CertDB2, this does count as an API change and is not an appropriate change for 1.8 branch.

As the camino people want this functionality on the 1.8 branch, we might consider reverting the change and introducing yet another DB interface?

Comment 12

[Kai Engert (:KaiE:)]

Simon, if you really need or wants this on the 1.8 branch, please speak up.

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #12)
> Simon, if you really need or wants this on the 1.8 branch, please speak up.

I don't think we actually need any of this stuff on the 1.8 branch.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Additional patch to change uuid of nsIX509CertDB2

Bob, could you please review this one line patch?

Comment 15

[Robert Relyea]

Comment on attachment 208225 [details] [diff] [review]
Additional patch to change uuid of nsIX509CertDB2

r+ relyea

Comment 16

[Kai Engert (:KaiE:)]

new IID checked in

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 203263 [details] [diff] [review]
Patch v2

not for 1.8 branch

Comment 18

[Kai Engert (:KaiE:)]

this caused regression 352867