Closed Bug 455310 Opened 12 years ago Closed 11 years ago

Unfork Account Manager Security Preferences (am-smime.*)

Categories

(MailNews Core :: Security, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: standard8, Assigned: mkmelin)

References

Details

Attachments

(2 files, 2 obsolete files)

Back in 2003, the am-smime.* files were forked. The differences between these files currently appear to be:

- Thunderbird has "View Certificates" and "Security Devices" buttons (there wasn't a bug referenced for these).
- SeaMonkey has a fix for bug 272149 (misleading question when selecting multiple certificates for signing and encrypting mail)
- SeaMonkey has a fix for bug 278549 (Can't configure used certificate per mail identity), the Thunderbird equivalent is bug 252250.

Thunderbird is especially lacking bug 278549. Although the identities system isn't nice, it would be good to pick up the fix for it.

Neil has said that adding the buttons to SeaMonkey are unlikely to be an issue.

I can see this being a reasonably easy two/three stage patch:

1) Port the buttons to the mailnews/ version of the files
2) Adjust TB's build system to:
a) pick up the mailnews/ version of the files, remove the old ones (note, there may be some accesskey/l10n property differences)
b) pick up the new files/strings from bug 278549
Flags: wanted-thunderbird3+
I thought about if we want these buttons on a per identity basis or not, and I think it's as useful as it is in the main page... And there they are fairly handy, but at least in thunderbird they can be from the main preferences also so there's a bit of overlay.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #349830 - Flags: superreview?(neil)
Attachment #349830 - Flags: review?(neil)
Both Thunderbird and SeaMonkey have a toOpenWindowByType() function that would be useful here. Unfortunately mailCore.js (or tasksOverlay.js for SM) isn't referenced by am-smime*.xul
(In reply to comment #1)
> handy, but at least in thunderbird they can be from the main preferences also
> so there's a bit of overlay.
I assume you mean overlap, and they're in SeaMonkey preferences too ;-)
Attachment #349830 - Flags: superreview?(neil)
Attachment #349830 - Flags: superreview+
Attachment #349830 - Flags: review?(neil)
Attachment #349830 - Flags: review+
Comment on attachment 349830 [details] [diff] [review]
proposed fix, part 1 (add the cert/device buttons to seamonkey)

>+    window.open("chrome://pippki/content/certManager.xul", "",
>+                "chrome,centerscreen,resizable=yes,dialog=no");
I have a slight preference for openDialog here.

>+    window.open("chrome://pippki/content/device_manager.xul", "devmgr",
>+                "chrome,centerscreen,resizable=yes,dialog=no");
Please drop the "devmgr".

>+    <groupbox id="smimeCertificateManager">
>+      <caption label="&certificates.label;"/>
>+      <hbox align="center">
If you use <groupbox orient="horizontal"> you don't need the box. Or are you perhaps thinking of allowing for some extra blurb in future? If you want to keep the box then you probably don't need the align either.

>+                label="&manageCerts.label;" accesskey="&manageCerts.accesskey;"
I notice that mail currently has these in lower case, I take it you're going to change them there too?

>+<!ENTITY manageCerts.label      "View Certificates">
>+<!ENTITY manageCerts.accesskey  "V">
_Manage Certificates…

>+<!ENTITY manageDevices.label    "Security Devices">
Manage Security Devices…
(sadly S is already taken as an access key, never mind)
(In reply to comment #3)
> > handy, but at least in thunderbird they can be from the main preferences also
> > so there's a bit of overlay.
> I assume you mean overlap, and they're in SeaMonkey preferences too ;-)

Ah yes :) Thanks for the review.
Keywords: helpwanted
Carrying forward r+sr=neil
Attachment #349830 - Attachment is obsolete: true
Attachment #350215 - Flags: superreview+
Attachment #350215 - Flags: review+
Attached patch proposed fix (thunderbird part) (obsolete) — Splinter Review
Make thunderbird use the core files. For the dtd, i just copied over what sm had.
(This is to be applied on top of the previous patch.)
Attachment #350223 - Flags: review?(philringnalda)
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
I think we're better off keeping our labels for the View Certificates and Security Devices buttons, which are the same phrases we use for the same buttons in the prefs dialog.

Did you plow through checking all the accesskeys, or do I need to?
I had the "View" version first - comment 4 - but seamonkey uses "Manage". I don't care much either way but it's more of a manager dialog... 

I think the accesskeys are ok, had to check those a few times to find free suitable ones. (The good ones were taken.)
Comment on attachment 350223 [details] [diff] [review]
proposed fix (thunderbird part)

I don't much care either way what SeaMonkey uses (though at some point we'll probably wind up breaking them by forgetting that they have longer buttons there), but I'd like to have Tb's buttons consistent with the ones that millions of Tb2 and Fx3 and Fx2 users have seen in prefs. r=me with "View" and no "Manage" in the mail/ version, thanks.
Attachment #350223 - Flags: review?(philringnalda) → review+
Comment on attachment 350215 [details] [diff] [review]
[checked in] proposed fix (review comments addressed)

changeset:   1362:813aeb3c009d
http://hg.mozilla.org/comm-central/rev/813aeb3c009d
Attachment #350215 - Attachment description: proposed fix (review comments addressed) → [checked in] proposed fix (review comments addressed)
Hmm, should I leave the … or not for mail/ ?
Not. It would only be correct if the buttons brought up a dialog that asked which certificates you wanted to view, that then opened the dialog (despite our occasional mistaken use of ellipses to mean "something's going to open" it actually means "more input will be required before what the label says will actually happen").
->FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.