278549 - Can't configure used certificate per mail identity

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, enhancement)

Description

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

Since different identities generally have a different email address, they need a
different certificate. but the UI does not offer a way to configure one.

Comment 1

[Mike Cowperthwaite]

This appears to be the suite version of bug 252250.  Adding dependency, since 
any fix is likely to be core.

Comment 2

[Mike Cowperthwaite]

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

Comment 4

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds a S/MIME Tab to identity management of accounts

I adopted the changes I proposed in 252250 to the seamonkey application.

Comment 5

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds a S/MIME Tab to identity management of accounts

fixes the style issues reported for the patch in bug 252250

Comment 6

[Robert Kaiser]

Comment on attachment 297133 [details] [diff] [review]
Adds a S/MIME Tab to identity management of accounts

I am no super-reviewer, Neil is.

Comment 7

[[e-mail]]

Comment on attachment 297133 [details] [diff] [review]
Adds a S/MIME Tab to identity management of accounts

>     content/messenger/am-smime.js                                    (resources/content/am-smime.js)
>+    content/messenger/am-smimeIdentityEditOverlay.js                 (resources/content/am-smimeIdentityEditOverlay.js)
>+    content/messenger/am-smimeOverlay.js                             (resources/content/am-smimeOverlay.js)
I think you might be able to contain all the JavaScript code in am-smime.js which would simplify the patch tremendously.

>+  if (typeof(gAccount) == "object")
>+    gSmimeIdentity = gIdentity;
>+  else
>+    gSmimeIdentity = window.arguments[0].identity;
When is gAccount not going to be an object? I think you probably need to be more worried about the new identity case.

>+  var dlg = document.getElementsByTagName("dialog")[0];
>+  dlg.setAttribute("ondialogaccept", "return smimeOnAcceptEditor();");
This doesn't look right, but I guess you don't have much choice.

>+function smimeOnAcceptEditor()
>+{
>+  try {
>+    if (onOk()==false)
>+      return false;
>+  }
>+  catch (ex) {}
>+  smimeSave();
>+}
You don't return true here, although you should. Also you don't need to explicitly compare to false; if you can't write your condition positively simply use the ! operator to negate it. Finally while we're here I should mention that Mozilla style is to space out operators.

>+function smimeSave()
>+{
>+  if (!gSmimeIdentity)
>+    gSmimeIdentity = gIdentity;
gIdentity definitely works here. Why bother with gSmimeIdentity?

>+  <!-- looks like insertafter does not lead to the javascript
>+       being able to find the element bundle_smime so
>+       these properties files are referenced in am-smimeOverlay.js instead
>+     <stringbundle id="bundle_smime"
>+                   src="chrome://messenger/locale/am-smime.properties"
>+                   insertafter="bundle_messenger"/>
>+     <stringbundle id="bundle_brand"
>+                   src="chrome://branding/locale/brand.properties"
>+                   insertafter="bundle_messenger"/>
>+  -->
Put these inside a plain <stringbundleset> element, which (since it has no ID) will be overlaid at the top level.

Comment 8

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds a S/MIME Tab to identity management of accounts

All changes done but

>> +function smimeSave()
>> +{
>> +  if (!gSmimeIdentity)
>> +    gSmimeIdentity = gIdentity;
> gIdentity definitely works here. Why bother with gSmimeIdentity?
This code is used in different situations:
 a) if editing the accounts default identity.
    Here gIdentity ist not set but account passed to onPreInit 
    which then sets gSmimeIdentity.
    I don't use gIdentity there in order to make it clear
    that this method only sets identity object for the S/MIME dialog.
 b) if editing an exisiting identity
    In this case gSmimeIdentity has alread been set when opening
    the dialog.
 c) if editing a new identity
    In this case gSmimeIdentity has been unset when
    initializing the dialog but has been set before smimeSave is called.

Comment 9

[1ac7b2edaa08e4edd3334c5dc4b966af]

"    initializing the dialog but has been set before smimeSave is called."
should be
"    initializing the dialog but is set when smimeSave is called."
(gIdentity is set before smimeSave is called.)

Comment 10

[[e-mail]]

> This code is used in different situations:
>  a) if editing the accounts default identity.
Ah, you mean in the main account manager window? In that case, I think it might make sense for the caller to pass in the correct identity. (Note: I haven't read your new patch yet.)

Comment 11

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds an S/MIME tab to identity management of accounts

I changed function smimeSave() into function smimeSave(identity) taking the correct identity as an argument. Additionally, I changed onSave and smimeOnAcceptEditor to pass the correct identity to smimeSave(identity).
In case gSmimeIdentity has not been set on initialization, smimeOnAcceptEditor now cares for taking gIdentity.
Though, smimeOnAcceptEditor cannot skip this check as it might be called for the ondialogaccept event of am-smime.xul since then there wouldn't be any gIdentity object.

Comment 12

[[e-mail]]

Comment on attachment 297324 [details] [diff] [review]
Adds an S/MIME tab to identity management of accounts

Sorry about the delay but this has been a busy week :-(

This patch is much easier to understand than the last patch, but it would have been easier still if you hadn't done so much reformatting. We try to avoid basic stuff like wrapping lines unless we're changing the affected code anyway, but you've also removed a number of braces and changed some comment styles, which is generally a no-no, unless specifically requested by your reviewers.

>-function onInit() 
>+function smimeInitializeFields()
It's also easier to see what's going on if you put the new onInit here, just before the old onInit, now known as smimeInitializeFields. The same goes for onSave and smimeSave.

>   // Disable all locked elements on the panel
>   onLockPreference();
You should wrap this in a null-check, rather than checking in onLockPreference

I'm still confused about gIdentity. When called from am-smime.xul, onPreInit sets gIdentity. When called from am-smimeIdentityEditOverlay.xul, gIdentity doesn't exist for a new identity, which I understand. But when you click OK, smimeOnAcceptEditor calls onOk which creates an identity. So, by the time you call smimeSave, gIdentity always exists.

>+  var dlg = document.getElementsByTagName("dialog")[0];
>+  dlg.setAttribute("ondialogaccept", "return smimeOnAcceptEditor();");
You can use document.documentElement here.

>+  <!-- looks like insertafter does not lead to the javascript
>+       being able to find the element bundle_smime so
>+       these properties files are referenced in am-smime.js instead
>+    <stringbundleset>
>+      <stringbundle id="bundle_smime"
>+                    src="chrome://messenger/locale/am-smime.properties"
>+                    insertafter="bundle_messenger"/>
>+      <stringbundle id="bundle_brand"
>+                    src="chrome://branding/locale/brand.properties"
>+                    insertafter="bundle_messenger"/>
>+    </stringbundleset>
>+  -->
insertafter only works if the parent has an id, but you shouldn't need one.

Comment 13

[1ac7b2edaa08e4edd3334c5dc4b966af]

Thanks for reviewing the patch.

onInit for the account management has been to a different file because it should have interfered with the onInit for the identity management dialog.
I looked up most stuff in enigmail and as you told me a single js file would be better it just worked why ever.

You're complaining about whitespace changes. When I posted the patch for 252250, I was told to change these things. To remove brace, to adopt to the mozilla coding style. Do you really thing I did these changes because I had to much time?!
I cannot understand why the hell you are now complaing about me sticking to the coding style and to recommondations given to the patches?! I don't see the point having basically different files for thunderbird and seamonkey, where the only (js) difference are the certManager functions.

Yes, I didn't do much work on rejoining the three js files into one, that is why onInit and smimieInit.... are in a bad order. 

I guess document.documentElement is really important to change and to confuse the patch.

I don't know about gSmimeIdentity any better than you, I just looked it up in other code where it just works. (e.g. enigmail).

As you might have noticed, insertafter referes to an element with an id, and it was you who proposed stringbundle without an id.

It looks like the account manager window passes the identity to onPreInit, whereas the identity management window has gIdentity set.
In order not to break any existing code, I decided to use gSmimeIdentity.
By the time save is called on creating of a new identity , the identity management window managed to set gIdentity, so it can be copied.

If I ever have some time, maybe I will change the patch but for now I'm fed up

Comment 14

[1ac7b2edaa08e4edd3334c5dc4b966af]

by the way most code of onInit went into smimeInit... so it should be easier to read this way instead of moving onInit above.

Comment 15

[1ac7b2edaa08e4edd3334c5dc4b966af]

Note: If you dislike whitespace changes, have a look at one of the first patches.
If you dislike js functionality of different dialogs to be merged into a single file, have a look at a middle patch.

Comment 16

[Magnus Melin [:mkmelin]]

Please keep it civil. My comments in bug 252250 about whitespace/braces/following conventions were meant regarding code you were touching anyway. Sorry if I wasn't clear on that.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 297324 [details] [diff] [review]
Adds an S/MIME tab to identity management of accounts

Neil already sr- this . (and ssaux - not sure who that is).

Please post a new post addressing his comments, and re-request review from the him.

Comment 18

[1ac7b2edaa08e4edd3334c5dc4b966af]

>This patch is much easier to understand than the last patch, but it would have
>been easier still if you hadn't done so much reformatting. We try to avoid
>basic stuff like wrapping lines unless we're changing the affected code anyway,
>but you've also removed a number of braces and changed some comment styles,
>which is generally a no-no, unless specifically requested by your reviewers.

this was the most annoying thing to revert.

>>-function onInit() 
>>+function smimeInitializeFields()
>It's also easier to see what's going on if you put the new onInit here, just
>before the old onInit, now known as smimeInitializeFields. The same goes for
>onSave and smimeSave.

didn't though that it would really look so much different, i guessed the diff would show up the old function being removed and added under a new name. as this is not the case, done.

>
>>   // Disable all locked elements on the panel
>>   onLockPreference();
>You should wrap this in a null-check, rather than checking in onLockPreference

then the null-check would be done twice: onLockPreference is called by thunderbird itself, so removing the null check from within onLockPreference and putting it into smimeInitializeFields results (i did a test on it) in an error leading to the settings not being saved on creating of a new identity.

all I could offer is to double null check.

>
>I'm still confused about gIdentity. When called from am-smime.xul, onPreInit
>sets gIdentity. When called from am-smimeIdentityEditOverlay.xul, gIdentity
>doesn't exist for a new identity, which I understand. But when you click OK,
>smimeOnAcceptEditor calls onOk which creates an identity. So, by the time you
>call smimeSave, gIdentity always exists.

the idea is just that smimeOnLoadEditor should not set gIdentity.
but using both, gIdentity and gSmimeIdentity to get and set data is to my mind worse that once checking if gSmimeIdentity has not yet been set and to refresh it then.

>
>>+  var dlg = document.getElementsByTagName("dialog")[0];
>>+  dlg.setAttribute("ondialogaccept", "return smimeOnAcceptEditor();");
>You can use document.documentElement here.

I don't know it and did not find a useful example. 
The current code works, though.

>
>>+  <!-- looks like insertafter does not lead to the javascript
>>+       being able to find the element bundle_smime so
>>+       these properties files are referenced in am-smime.js instead
>>+    <stringbundleset>
>>+      <stringbundle id="bundle_smime"
>>+                    src="chrome://messenger/locale/am-smime.properties"
>>+                    insertafter="bundle_messenger"/>
>>+      <stringbundle id="bundle_brand"
>>+                    src="chrome://branding/locale/brand.properties"
>>+                    insertafter="bundle_messenger"/>
>>+    </stringbundleset>
>>+  -->
>insertafter only works if the parent has an id, but you shouldn't need one.

My idea was to use <stringbundle ...> instead of strBundleService.createBundle .
But whatever I tried, all the good and bad examples, they didn't work for me.
On explanation I found was to that insertafter is needed to insert the element directly into the dialog window instead of leaving it within the overlay, so that mozilla would load the stringbundle. Actually, document.getElementById("bundle_smime") remains null.
This is a problem with overlays, not dialogs.

Comment 19

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds an S/MIME Tab to identity management of accounts

(1)

>>>   onLockPreference();
>>You should wrap this in a null-check, rather than checking in onLockPreference
> ... (i did a test on it) in an error ...

looks like it was a typo instead.

Comment 20

[1ac7b2edaa08e4edd3334c5dc4b966af]

Comment on attachment 302882 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

please review it

Comment 21

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds an S/MIME Tab to identity management of accounts

fixes indention for lines within a newly added if-block.

Comment 22

[[e-mail]]

(In reply to comment #18)
>this was the most annoying thing to revert.
But without that I wouldn't have been able to review your patch...

>done.
Thanks.

>the idea is just that smimeOnLoadEditor should not set gIdentity.
That's fine. Just leave it in onPreInit.

>>>+  var dlg = document.getElementsByTagName("dialog")[0];
>>>+  dlg.setAttribute("ondialogaccept", "return smimeOnAcceptEditor();");
>>You can use document.documentElement here.
>I don't know it and did not find a useful example.
document.documentElement.setAttribute("ondialogaccept",
                                      "return smimeOnAcceptEditor();");

>But whatever I tried, all the good and bad examples, they didn't work for me.
I think I noticed the problem now. (Maybe I was too tired last time. Sorry.)
See the rest of the comments I have for the latest patch.

Comment 23

[[e-mail]]

Comment on attachment 303227 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

>+% overlay chrome://messenger/content/am-identity-edit.xul                      chrome://messenger/content/am-smimeOverlay.xul
>+% overlay chrome://messenger/content/am-identity-edit.xul                      chrome://messenger/content/am-smimeIdentityEditOverlay.xul
I only just noticed that this isn't guaranteed to work. Instead am-smimeIdentityEditOverlay.xul should overlay am-smimeOverlay.xul directly, like am-smime.xul does.

>+  if (!gSmimeIdentity) {
>+/* The user is going to create a new identity.
>+ * Set everything to default values.
>+ * Do not take over the values from gAccount.defaultIdentity
>+ * as the new identity probably is going to have a different mail address.
>+ */
>+    gEncryptionCertName.value = "";
>+    gSignCertName.value = "";
Wrong comment style. it should look something like this:
  if (!gIdentity) {
    // The user is going to create a new identity.
    // Set everything to default values.
    // Do not take over the values from gAccount.defaultIdentity
    // as the new identity is going to have a different mail address.
    gEncryptionCertName.value = "";
    gSignCertName.value = "";
(yes, I removed the word probably as well!)

>+    gEncryptionCertName.value = gSmimeIdentity
>+                                 .getUnicharAttribute("encryption_cert_name");
If you are going to wrap these I woudl probably use
gEncryptionCertName.value =
    gSmimeIdentity.getUnicharAttribute("encryption_cert_name");

>+    if (!gEncryptionCertName.value) {
File style is to use
if (!gEncryptionCertName.value)
{
  gEncryptAlways.setAttribute("disabled", true);
  gNeverEncrypt.setAttribute("disabled", true);
}
else {
  enableEncryptionControls(true);
}
Looks like you got it right for signing though.

>+  identity.setUnicharAttribute("encryption_cert_name",
>+                                     gEncryptionCertName.value);
Assuming you still want to wrap this, the g goes under the "

>   if (!otherCertInfo.value.length) {
>-    if (matchingOtherCert) {
>-      userWantsSameCert = askUser(gBundle.getString(msgNeedCertWantSame));
>-    }
>-    else {
>-      if (askUser(gBundle.getString(msgNeedCertWantToSelect))) {
>+    if (matchingOtherCert)
>+      userWantsSameCert = askUser(gBundle
>+                             .GetStringFromName(msgNeedCertWantSame));
>+    else if (askUser(gBundle.GetStringFromName(msgNeedCertWantToSelect)))
>         smimeSelectCert(pref);
>       }
>-    }
>-  }
>-  else {
>-    if (matchingOtherCert) {
>-      userWantsSameCert = askUser(gBundle.getString(msgWantSame));
>-    }
>-  }
>+  else if (matchingOtherCert)
>+      userWantsSameCert = askUser(gBundle.GetStringFromName(msgWantSame));
Again, don't alter the bracing.

>+  <!-- looks like insertafter does not lead to the javascript
>+       being able to find the element bundle_smime so
>+       these properties files are referenced in am-smime.js instead
Oops. Why didn't I notice this before? Just omit the insertafter.
In fact, you could probably put the bundles in am-smimeOverlay.

>+    <vbox id="smimeTabPanel" name="smimeEditingTab" flex="1">
>+      <label id="identityName" value="(unknown ID)"/>
I'm not convinced you need this, but if you do, it needs to be a new entity.
You don't have to use a separate vbox either, you can call this one id="smimeEditing" and the label will automatically appear before the overlay elements.

>+  <script type="application/x-javascript">
>+<![CDATA[
>+    window.addEventListener("load", smimeOnLoadEditor, false);
>+]]>
>+  </script>
Indentation on the <![CDATA[ is wrong, it should be at least as much as the <script>

>+  <label hidden="true" wsm_persist="true" id="identity.encryptionpolicy" />
Nit: no space needed before / - that's pseudo-xhtml, not XML!

I really think we're nearly there on this one. Thanks for keeping up!

Comment 24

[1ac7b2edaa08e4edd3334c5dc4b966af]

> Again, don't alter the bracing.
note i replaced getString with GetStringFromName so these would have been altered anyway.

Comment 25

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds an S/MIME Tab to identity management of accounts

>>+    <vbox id="smimeTabPanel" name="smimeEditingTab" flex="1">
>>+      <label id="identityName" value="(unknown ID)"/>
>I'm not convinced you need this, but if you do, it needs to be a new entity.
>You don't have to use a separate vbox either, you can call this one
>id="smimeEditing" and the label will automatically appear before the overlay
>elements.
I just saw it with enigmail and it did look nice though it is not necessary, so I removed it.

All other changes are done.

Comment 26

[[e-mail]]

Attachment: Patch with my review comments actually addressed...

Comment 27

[[e-mail]]

Attachment: -w version for review

Comment 28

[1ac7b2edaa08e4edd3334c5dc4b966af]

why should this work without modifying jar.mn? How did you solve the stringbundle issues? Note your patch breaks indention.

Comment 29

[1ac7b2edaa08e4edd3334c5dc4b966af]

"Patch with my review comments actually addressed..." sounds like you're claiming I did not add the changes you proposed to my patch.

Though that would be a lie.

Comment 30

[1ac7b2edaa08e4edd3334c5dc4b966af]

Well, I just took a fresh HEAD copy of mozilla suite sources and applied your patch to it.
First, die Security Tab in the Account Manager does not work anymore.
 The error console tells me: 
  "Error: no element found
Source File: chrome://messenger/content/am-smimeOverlay.xul
Line: 1"
  "Warning: Failed to load overlay from chrome://messenger/content/am-smimeOverlay.xul.
Source File: chrome://messenger/content/am-smime.xul
Line: 0"
  "Error: gEncryptionCertName is null
Source File: chrome://messenger/content/am-smime.js
Line: 102"

Second, there is no S/Mime (related) Tab in the identity manager.

It looks to me like while "actually addressing" your review comments, you managed to remove the entire (new and old) functionality.

Comment 31

[1ac7b2edaa08e4edd3334c5dc4b966af]

Comment on attachment 304086 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

As the patch obsoleting this neither adds the required functionality nor keeps the old one, this patch is not obsolete.

I hope I didn't miss anything really bad - remember this is my first code for mozilla - but I indeed addressed all review comments of the reviewer when uploading this.

Comment 32

[1ac7b2edaa08e4edd3334c5dc4b966af]

Comment on attachment 304086 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

While you're looking at this issue anyway could you please also have a look a this patch.
Thanks a lot.

Comment 33

[1ac7b2edaa08e4edd3334c5dc4b966af]

A few words about gIdentity:
When used from the account manager, there is no gIdentity variable given and it therefore needs to be declared.

When used from the identity manager, there is already a gIdentity variable given so gIdentity may need be declared.

This leads me to the conclusion that one cannot declare gIdentity in am-smime.js

Comment 34

[1ac7b2edaa08e4edd3334c5dc4b966af]

sry, "may need be declared" => "may not be declared".

Comment 35

[1ac7b2edaa08e4edd3334c5dc4b966af]

A few words to the manifest modifications:
In order to get the new xul files into the messenger.jar , they need to be added to jar.mn using a line like "content/messenger/....".

In order to get a new smime tab added to the identity manager, am-smimeIdentityEditOverlay.xul needs to be loaded when opening am-identity-edit.xul .
This cannot be achieved by adding an xul-overlay tag to am-identity-edit.xul, because the s/mime extension is not always present. 
Instead, there needs to be an overlay statement in jar.mn regarding am-identity-edit.xul .

Comment 36

[1ac7b2edaa08e4edd3334c5dc4b966af]

A few words about the stringbundle issue:
If I add the stringbundles to an overlay, they are not accessible by the javascript dom functionaly. At least I did not manage to get working neither by using insertafter nor by using stringbundleset.

This problem is also described here:
http://www.babelzilla.org/forum/index.php?s=dfcad41259c6f15762090d68acd54a2a&showtopic=1719&st=0&p=38058&#entry38058

Comment 37

[1ac7b2edaa08e4edd3334c5dc4b966af]

A few words about idention:
As far as I understood about the mozilla coding style, good coding style is used simplify understanding of what the code actually does.

On the other hand, one would never just clean up the code as this would disable patches being applied to HEAD and a special trunk if the file(s) was/were cleaned up in between.

Usually, if one changes a statement e.g. by changing the name of the method or by wrapping it in an if clause, the semantics of the modified or wrapped code has changed. Meaning that a patch written to change this block would probably not achieve the expected goal whether it cleanly applies or not.

Therefore, if one adds e.g. an if/else clause or changes stuff like "getString" into "getStringFromName", there should be no problem fixing indention for these lines.
Whether it is reasonable or not to clean surrounding statements as well needs to be checked individually for each patch, I guess.

Comment 38

[1ac7b2edaa08e4edd3334c5dc4b966af]

Well and all together I this is exactly what I hope to achieve with the attachment  304086 [details] [diff] [review] (my most recent).

Comment 39

[Ian Neal]

(In reply to comment #26)
> Created an attachment (id=305405) [details]
> Patch with my review comments actually addressed...
> 
As pointed out, this patch is missing the jar.mn changes but will review assuming those have been added.

Comment 40

[Ian Neal]

(In reply to comment #33)
> A few words about gIdentity:
> When used from the account manager, there is no gIdentity variable given and it
> therefore needs to be declared.
> 
> When used from the identity manager, there is already a gIdentity variable
> given so gIdentity may need be declared.
> 
> This leads me to the conclusion that one cannot declare gIdentity in
> am-smime.js
> 

I think I see what you are saying - sometimes gIdentity is being declared more than once?

Comment 41

[Ian Neal]

Comment on attachment 305405 [details] [diff] [review]
Patch with my review comments actually addressed...

Missing jar.mn changes

>Index: mailnews/extensions/smime/resources/content/am-smimeIdentityEditOverlay.xul
>===================================================================
>+  <tabs id="identitySettings">
>+    <tab label="&securityTab.label;" id="smimeSecurity"/>
Does it need an id?
>+  </tabs>
>+
>+  <tabpanels id="identityTabsPanels">
>+    <vbox id="smimeEditing" name="smimeEditingContent" flex="1"/>
Nit: The order in this vbox is different to the file it is overlaying (that goes flex, name, id I think).

>Index: mailnews/extensions/smime/resources/content/am-smimeOverlay.xul
>===================================================================
>RCS file: mailnews/extensions/smime/resources/content/am-smimeOverlay.xul
>diff -N mailnews/extensions/smime/resources/content/am-smimeOverlay.xul
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mailnews/extensions/smime/resources/content/am-smimeOverlay.xul	22 Feb 2008 15:24:53 -0000
>@@ -0,0 +1,128 @@
>+<?xml version="1.0"?>
>+
>+<!--
>+
>+ ***** BEGIN LICENSE BLOCK *****
>+ Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+
>+ The contents of this file are subject to the Mozilla Public License Version
>+ 1.1 (the "License"); you may not use this file except in compliance with
>+ the License. You may obtain a copy of the License at
>+ http://www.mozilla.org/MPL/
>+
>+ Software distributed under the License is distributed on an "AS IS" basis,
>+ WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ for the specific language governing rights and limitations under the
>+ License.
>+
>+ The Original Code is mozilla.org Code.
>+
>+ The Initial Developer of the Original Code is
>+ Netscape Communications Corporation.
>+ Portions created by the Initial Developer are Copyright (C) 1998-2001
>+ the Initial Developer. All Rights Reserved.
>+
>+ Contributor(s):
>+   [e-mail]
>+   Scott MacGregor <[e-mail]>
>+
>+ Alternatively, the contents of this file may be used under the terms of
>+ either of the GNU General Public License Version 2 or later (the "GPL"),
>+ or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ in which case the provisions of the GPL or the LGPL are applicable instead
>+ of those above. If you wish to allow use of your version of this file only
>+ under the terms of either the GPL or the LGPL, and not to allow others to
>+ use your version of this file under the terms of the MPL, indicate your
>+ decision by deleting the provisions above and replace them with the notice
>+ and other provisions required by the GPL or the LGPL. If you do not delete
>+ the provisions above, a recipient may use your version of this file under
>+ the terms of any one of the MPL, the GPL or the LGPL.
>+
>+ ***** END LICENSE BLOCK ***** -->
>+
>+<?xml-stylesheet href="chrome://messenger/skin/accountManage.css"
>+                 type="text/css"?>
>+
>+<!DOCTYPE overlay SYSTEM "chrome://messenger/locale/am-smime.dtd">
>+
>+<overlay xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+  <vbox id="smimeEditing">
Shouldn't everything after this be indented more?

r=me with those comments addressed.

Comment 42

[1ac7b2edaa08e4edd3334c5dc4b966af]

>> This leads me to the conclusion that one cannot declare gIdentity in
>> am-smime.js
>I think I see what you are saying - sometimes gIdentity is being declared more
>than once?
Jup, one will either have gIdentity not being declared or have it declared twice where one wants to access to old(er) value.
I'm not sure how mozilla handles this, but in many common languages like java, this is not possible.

Comment 43

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds an S/MIME Tab to identity management of accounts

Fixes indention in am-smimeOverlay.xul	.

Comment 44

[1ac7b2edaa08e4edd3334c5dc4b966af]

Comment on attachment 304086 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

I tried neils patch (+jarmn changes) and though I cannot understand _why_ it works - as earlier tests with my thunderbird patch lead to the stringbundle problems, - it looks like that now for seamonkey having js included into a single file and stringbundles moved to smimeOverlay.xul it just works.

Comment 45

[1ac7b2edaa08e4edd3334c5dc4b966af]

Comment on attachment 306245 [details] [diff] [review]
Adds an S/MIME Tab to identity management of accounts

Same for this too.

Comment 46

[1ac7b2edaa08e4edd3334c5dc4b966af]

Sooo. I played a little with the patch and finally got at:
 if I move (back) the stringbundles from smimeOverlay.xul to
 am-smime.xul and am-smimeIdentityOverlay.xul,
 the following error occurs:
   Error: gBundle is null
   Source File: chrome://messenger/content/am-smime.js
   Line: 298

So, at least this error is not restricted to thunderbird and though nobody told me that it disappeared while applying neils comments (comment 23) . Perhaps he even did not know.

@Neil: would have been great if you had told me this, as I expect you did test your patch might got at it.

Comment 47

[1ac7b2edaa08e4edd3334c5dc4b966af]

The patch neil added does not contain a stringbundleset wrapping stringbundle,
though he himself requested this in comment 7.

This is wanted or is this unwanted? Perhaps, you the developers, could agree with each of yours.

Comment 48

[1ac7b2edaa08e4edd3334c5dc4b966af]

Attachment: Adds a S/MIME Tab to identity management of accounts

This is targetting comment 42 .

>Missing jar.mn changes
added

>>Index: mailnews/extensions/smime/resources/content/am-smimeIdentityEditOverlay.xul
>>===================================================================
>>+  <tabs id="identitySettings">
>>+    <tab label="&securityTab.label;" id="smimeSecurity"/>
>Does it need an id?
am-identity-edit.xul does not require an id so they are not required here.

>>+  <tabpanels id="identityTabsPanels">
>>+    <vbox id="smimeEditing" name="smimeEditingContent" flex="1"/>
>Nit: The order in this vbox is different to the file it is overlaying (that
goes flex, name, id I think).
done.

>>Index: mailnews/extensions/smime/resources/content/am-smimeOverlay.xul
>>+  <vbox id="smimeEditing">
>Shouldn't everything after this be indented more?

done.
I also added a stringbundleset element as requested in comment 7.

>r=me with those comments addressed.
done.

Comment 49

[Ian Neal]

Comment on attachment 306507 [details] [diff] [review]
Adds a S/MIME Tab to identity management of accounts

>Index: mailnews/extensions/smime/resources/content/am-smime.js
>===================================================================
>+function smimeOnAcceptEditor()
>+{
>+  try {
>+    if ( !onOk() )
Any reason for the extra spaces in the brackets (sorry I did not spot that last time round)?

r=me with that addressed.

Comment 50

[[e-mail]]

Comment on attachment 306507 [details] [diff] [review]
Adds a S/MIME Tab to identity management of accounts

Thanks for fixing my patch, it will be really great to get this fixed.

>+  <vbox id="smimeEditing">
>+
>+    <stringbundleset>
>+      <stringbundle id="bundle_smime" src="chrome://messenger/locale/am-smime.properties"/>
>+      <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>+    </stringbundleset>

I still don't remember why I didn't go with my original plan, which was this:

>+  <stringbundleset>
>+    <stringbundle id="bundle_smime" src="chrome://messenger/locale/am-smime.properties"/>
>+    <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>+  </stringbundleset>
>+
>+  <vbox id="smimeEditing">

Comment 51

[Magnus Melin [:mkmelin]]

neil: do you want to do the sr for this?

Comment 52

[[e-mail]]

Comment on attachment 306507 [details] [diff] [review]
Adds a S/MIME Tab to identity management of accounts

Well, I'm not sure whether my sr will count to a patch I've touched - why not play it safe and add your review too ;-)

Comment 53

[Magnus Melin [:mkmelin]]

Attachment: patch for checkin

Yep, looks good to me. Addressed the last review comment, removed some extra whitespace and lined up a couple of attributes. I'll check this patch in.

Comment 54

[Magnus Melin [:mkmelin]]

1ac7b2edaa08e4edd3334c5dc4b966af: thx for stickin in there!

Checking in mailnews/extensions/smime/jar.mn;
/cvsroot/mozilla/mailnews/extensions/smime/jar.mn,v  <--  jar.mn
new revision: 1.17; previous revision: 1.16
done
Checking in mailnews/extensions/smime/resources/content/am-smime.js;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smime.js,v  <--  am-smime.js
new revision: 1.16; previous revision: 1.15
done
Checking in mailnews/extensions/smime/resources/content/am-smime.xul;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smime.xul,v  <--  am-smime.xul
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smimeIdentityEditOverlay.xul,v
done
Checking in mailnews/extensions/smime/resources/content/am-smimeIdentityEditOverlay.xul;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smimeIdentityEditOverlay.xul,v  <--  am-smimeIdentityEditOverlay.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smimeOverlay.xul,v
done
Checking in mailnews/extensions/smime/resources/content/am-smimeOverlay.xul;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smimeOverlay.xul,v  <--  am-smimeOverlay.xul
initial revision: 1.1
done
Checking in suite/locales/en-US/chrome/mailnews/smime/am-smime.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/smime/am-smime.dtd,v  <--  am-smime.dtd
new revision: 1.11; previous revision: 1.10
done

->FIXED