Closed Bug 82886 Opened 23 years ago Closed 23 years ago

[CertManager] Title selection popup has extra checkbox

Categories

(Core Graveyard :: Security: UI, defect, P2)

1.0 Branch
x86
Other
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.1

People

(Reporter: lord, Assigned: inactive-mailbox)

References

Details

Attachments

(2 files)

In the Certificate Manager, you can select which titles are displayed. At the end of the list there is a blank space with a check box next to it. This box does not seem to control anything. This extra checkbox should be removed.
-> Future
Target Milestone: --- → Future
Keywords: nsenterprise
t->2.1 kai
Assignee: ddrinan → kai.engert
Priority: -- → P2
Target Milestone: Future → 2.1
Target Milestone: 2.1 → Future
removing nsenterprise keyword from PSM bugs with target milestone of future.
Keywords: nsenterprise
Attached patch patch (deleted) — Splinter Review
<outliner> seems to create the the column picker out of the specified <outlinercol>'s so the column picker doesn't need to be declared in each .xul file
There is code in the C++ files, that fills the cell with an internal certificate key. My assumption is, this was used solely for debugging purposes. However, the original programmer assumed that using the collapsed="true" attribute hides the column completly. As we know now, this is not true for the outliner's column picker. It still shows up there. The solution is to remove the column from XUL completely. However, as this might be a very helpful debugging feature for future programmers working on this code, I'm suggesting to deactivate the code by commenting it out (and not removing it). For completeness, I'm commenting out the C++ code that provides the cell contents as well. Ian, can you please review the next patch?
Status: NEW → ASSIGNED
Keywords: patch, review
Attached patch Updated patch (deleted) — Splinter Review
Actually, the db key field is there from an early implementation of the cert outliner view. IIRC, I first thought storing an array of all the certs displayed would cost too much memory. Thus I only stored the db key (via a hidden field in the outliner view), and used that to look up the cert as needed. For reasons I don't recall at the moment, I changed my mind, and decided to keep an array of certs, thus removing the need for the db key field (though I apparently forgot to actually remove the code ;). For this bug, commenting out the db key code is fine. However, you might consider the larger design issues. When PSM/mozilla supports S/MIME, a user could potentially have 100's of certs. Thus keeping them all in memory would be bad. Rather, only the parts needed for the outliner display (nickname or email, verified, validity, etc.) should be kept in memory, and when the user tries to view the rest of the cert, a lookup is done using the db key. Actually, even in that case, the db key should be stored along with the aforementioned fields. It should not need to be part of the view at all. To be more clear, I imagine something like this: struct outlinerCertDisplay { PRUnichar *displayName; /* nickname or email */ PRUnichar *validity; ... char *dbkey; } Those fields are accessed by nsCertOutliner::GetCellText(), with the exception of dbkey. It is only used for nsCertOutliner::GetCert(). Thus I think you should completely remove all references to the db key column in the XUL files. The underlying implementation should not have to depend on that hidden field.
Thanks for explaining. I looked through the sources and couldn't find any other occurrences of the column we want to remove. I agree with your ID of the minimalistic in memory structure, which would reduce memory usage and increase performance for larger lists. I also see the advantage of having the db ID of a cert available. In the future, we could have this temporary items stored in an array where array index equals (expanded) outliner entry index, so we have access to the key for the selected row without storing it in a cell. And we could put this logic into nsIOutlinerView::getRowProperties. Would you like to add r= for now?
yes, r=mcgreer
sr=blizzard
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
Checked in patch for Kai.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 94207 has been marked as a duplicate of this bug. ***
I see not yon checkbox namethed... Marking VERIFIED FIXED on: -MacOS91 2001-08-15-08-trunk (commercial) -MacOS_X 2001-08-15-05-trunk (commercial) -LinRH62 2001-08-15-08-trunk (commercial) -Win98SE 2001-08-15-06-trunk (commercial)
Status: RESOLVED → VERIFIED
Changing target to 2.1
Target Milestone: Future → 2.1
Product: PSM → Core
Version: psm2.0 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: