456860 - Improve msgCompSMIMEOverlay.js and msgReadSMIMEOverlay.js

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[Serge Gautherie (:sgautherie)]

I started from bug 456672 comment 3,
then ended up "rewriting" the whole files.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) once for all

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080921023908 SeaMonkey/2.0a1pre] (home, debug default) (W2Ksp4)

*Fix the assertion caused by each "window".
*(unrelated) "Rewrite" these files.

Comment 2

[[e-mail]]

Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

>-  if (!gMsgCompose)
>-    return;
>-
>-  if (!gMsgCompose.compFields)
>+  if (!(gMsgCompose && gMsgCompose.compFields))
>     return;
Looks vaguely OK but some of these look more complicated this way.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Looks vaguely OK but some of these look more complicated this way.

I could use
|if (!gMsgCompose || !gMsgCompose.compFields)|
if you prefer...

Comment 4

[Mark Banner (:standard8)]

Given that Bienvenu & Kai are both on the modules list for owning/reviewing mailnews/mime and this is a simple rework patch that just ends up changing how the listeners are added/removed, I would have thought that bienvenu would potentially be the better reviewer choice here.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

Per comment 4: I'm fine with either reviewer.

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

This is a very big patch, it would take me more than one hour to review it, and then I'd probably still be unsure that you don't introduce regressions.

Could you please attach a separate patch, that ONLY fixes the bugs you're trying to fix? And please explain what you fix, and why you fix it.

I'd propose to have separate attachments for bug fixing and pure cleanup.

In general I'd like to avoid cleanup if it doesn't serve a purpose. Many of your changes I don't understand why you feel they are necessary. Removing the symbolic names for strings and making them inline is unnecessary, and could even have a negative impact, should a string be used multiple times (now or later).

I'd propose you avoid making big changes without a need. Or please make sure you make it easier for reviewers.

With this patch, I see that you made many many changes, but you don't explain at all in this bug why or what you're changing.

Please try to avoid unnecessary whitespace changes.

Time is precious, and I'd like to avoid spending time on code changes that don't serve a clear purpose.

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

r- for "too much noise"

Comment 8

[David :Bienvenu]

Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

clearing review request based on Kaie's minus.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) fix + reduced cleanup only

Av1, with comment 6 suggestion(s).

Core fix(es):
*Removing the controllers fixes the assertion.
*Removing the (various) listeners is good practice too,
 especially as the (compose) window can be cached and later reused.
*Adding some |= null| init., to be explicitly safe.

Reduced cleanup(s):
I hope these are trivial enough to be self explanatory.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 345669 [details] [diff] [review]
(Av2) fix + reduced cleanup only

(In reply to comment #4)

Iirc, David wrote +/- "Kaie is the one (and only) for /mailnews/extensions/smime/*"...

Kaie: ping ?

Comment 11

[Serge Gautherie (:sgautherie)]

Kaie, (David): looking for r !!

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a) fix + reduced cleanup only [Checkin: See comment 17]

Av2, without the 2 (superfluous in javascript) |gBundle = null|.

Asking dmose too for review, per irc discussion.

Comment 13

[Robert Kaiser]

This bug is open but targeted for seamonkey2.0b1, which has already been released. Please set the target milestone to an appropriate value, "---" if it has no specific target.

Comment 14

[Serge Gautherie (:sgautherie)]

I'm desperately waiting for a review :-<

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for the delay; I hope to get to this tomorrow.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 369529 [details] [diff] [review]
(Av2a) fix + reduced cleanup only
[Checkin: See comment 17]

Looks good; r=dmose.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 369529 [details] [diff] [review]
(Av2a) fix + reduced cleanup only
[Checkin: See comment 17]


http://hg.mozilla.org/comm-central/rev/216c44724858

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 369529 [details] [diff] [review]
(Av2a) fix + reduced cleanup only
[Checkin: See comment 17]


(In reply to comment #17)
> http://hg.mozilla.org/comm-central/rev/216c44724858

Av2a, unbitrotted and with a few nit fixes.

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) msgReadSMIMEOverlay.js cleanup [Checkin: Comment 20]

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 398891 [details] [diff] [review]
(Bv1) msgReadSMIMEOverlay.js cleanup
[Checkin: Comment 20]


http://hg.mozilla.org/comm-central/rev/f403bbb0e4b5

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-SM) msgCompSMIMEOverlay.js cleanup

I'll apply it to the TB file too...

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 413126 [details] [diff] [review]
(Cv1-SM) msgCompSMIMEOverlay.js cleanup

I'm fairly sure lets at global scope at the top level are wrong in any mozilla application. Anyway, passing to Karsten/Neil for the reviews as I've got a big backlog at the moment.

Comment 23

[Karsten Düsterloh]

Comment on attachment 413126 [details] [diff] [review]
(Cv1-SM) msgCompSMIMEOverlay.js cleanup

(In reply to comment #22)
> I'm fairly sure lets at global scope at the top level are wrong 

Not as such. Just Neil doesn't seem to like 'em. ;-)
Personally, with let in town, I consider var deprecated.
I'll let(!) Neil decide this.

>+let gNextSecurityButtonCommand;
>+let gBundle;
>+let gBrandBundle;
>+let gSMFields;
>+let gEncryptedURIService;

Please initialize global variables to a sensible default, if they're not initialized on the global scope - and even if they are, that init should happen here, if possible.

>@@ -62,60 +58,58 @@ function onComposerClose()
>+  // Set up the intial security state.
>+  // 0 == never, 1 == if possible, 2 == always Encrypt.
>+  gSMFields.requireEncryptMessage =
>+    gCurrentIdentity.getIntAttribute("encryptionpolicy") == 2;

Define/use suitable global constants for (all) these numerical values. (You might comment out unused ones.)

>   if (gSMFields.signMessage) // make sure we have a cert name...
>   {
...
>   }
>   else
>-  {
>     setNoSignatureUI();
>-  }

Don't. If one if branch has {}, the other should have as well.

> function doSecurityButton()
> {
>-  var what = gNextSecurityButtonCommand;
>-  gNextSecurityButtonCommand = "";
>+  let what = gNextSecurityButtonCommand;
>+  gNextSecurityButtonCommand = null;

That's wrong. 'what' is used as a string should be init'ed as such.

>@@ -288,136 +275,123 @@ function setEncryptionUI()
>+  window.openDialog(
>+    "chrome://messenger-smime/content/msgCompSecurityInfo.xul",
>+    "", "chrome,resizable=1,modal=1,dialog=1",

Please stick to the "one line per parameter" pattern.

> function onComposerSendMessage()
> {
>   let missingCount = new Object();
>   let emailAddresses = new Object();
> 
>   try {

While you're touching the world, please move the { o its own line.

>     // Does the current identity override the global preference?
>     if (gCurrentIdentity.overrideGlobalPref)
>       autocompleteDirectory = gCurrentIdentity.directoryServer;
>     else
>     {

Missing {} on the if branch, while you're at it.

>+      let prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                             .getService(Components.interfaces.nsIPrefBranch);

No need to use that pattern anymore, just use Application.prefs (see mailWindowOverlay.js for examples).

>+      window.openDialog("chrome://messenger-smime/content/certFetchingStatus.xul",
>+                        "", "chrome,resizable=1,modal=1,dialog=1",
>+                        autocompleteDirectory, emailAddresses.value);

Please stick to the "one line per parameter" pattern.

>+  // 0 == never, 1 == if possible, 2 == always Encrypt.

See above.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2-SM) msgCompSMIMEOverlay.js cleanup.

Cv1-SM, with comment 23 suggestion(s),
but the following:

(In reply to comment #23)

> >+let gNextSecurityButtonCommand;
> >+let gBundle;
> >+let gBrandBundle;
> >+let gSMFields;
> >+let gEncryptedURIService;
> 
> Please initialize global variables to a sensible default, if they're not
> initialized on the global scope - and even if they are, that init should happen
> here, if possible.

Javascript initializes them to "undefined" ... that should be fine/enough.

> > function doSecurityButton()
> > {
> >-  var what = gNextSecurityButtonCommand;
> >-  gNextSecurityButtonCommand = "";
> >+  let what = gNextSecurityButtonCommand;
> >+  gNextSecurityButtonCommand = null;
> 
> That's wrong. 'what' is used as a string should be init'ed as such.

Correct, but this value is "unused": the purpose of this assignment is only to clear the previous/real value ... 'null' should be fine/better for that purpose.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> >+      let prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >                             .getService(Components.interfaces.nsIPrefBranch);
> 
> No need to use that pattern anymore, just use Application.prefs (see
> mailWindowOverlay.js for examples).

Is there a bug filed on doing this on the whole (suite) tree?

Comment 26

[Karsten Düsterloh]

Comment on attachment 416311 [details] [diff] [review]
(Cv2-SM) msgCompSMIMEOverlay.js cleanup.

> > Please initialize global variables to a sensible default, if they're not
> > initialized on the global scope - and even if they are, that init should happen
> > here, if possible.
> 
> Javascript initializes them to "undefined" ... that should be fine/enough.

I disagree. This is just ... dirty.
Eg. every "undefined" needs to be casted before using it in comparisons etc. pp.

> > >+  let what = gNextSecurityButtonCommand;
> > >+  gNextSecurityButtonCommand = null;
> > 
> > That's wrong. 'what' is used as a string should be init'ed as such.
> 
> Correct, but this value is "unused": the purpose of this assignment is only to
> clear the previous/real value ... 'null' should be fine/better for that
> purpose.

I disagree.

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Cv3-SM) msgCompSMIMEOverlay.js cleanup [Checkin: See comment 29]

Cv2-SM, with comment 26 suggestion(s).

Comment 28

[[e-mail]]

Comment on attachment 419383 [details] [diff] [review]
(Cv3-SM) msgCompSMIMEOverlay.js cleanup
[Checkin: See comment 29]

>+const kEncryptedURIService =
>+        Components.classes["@mozilla.org/messenger-smime/smime-encrypted-uris-service;1"]
>+                  .getService(Components.interfaces.nsIEncryptedSMIMEURIsService);
This should be a var g, not a const k. Also, only 4-space continuation indent.

>-var gNextSecurityButtonCommand = "";
>-var gBundle;
>-var gBrandBundle;
>-var gSMFields;
>+let gNextSecurityButtonCommand = "";
>+let gSMFields = null;

>-  var what = gNextSecurityButtonCommand;
>+  let what = gNextSecurityButtonCommand;

>-var SecurityController =
>+let SecurityController =

>-var SecurityController =
>+let SecurityController =
And there's no reason to change these from var to let, especially the globals. (New/edited locals are OK if the module owner likes them.)

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 419383 [details] [diff] [review]
(Cv3-SM) msgCompSMIMEOverlay.js cleanup
[Checkin: See comment 29]


http://hg.mozilla.org/comm-central/rev/00a5b3b3ce38
Cv3-SM, with comment 28 suggestion(s).

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-TB) msgCompSMIMEOverlay.js cleanup

Port Cv3-SM to TB, at least the obvious part.

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a-TB) msgCompSMIMEOverlay.js cleanup [Checkin: See comment 36]

Dv1-TB, unbitrotted and with msgHdrViewSMIMEOverlay.xul fix.

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 419856 [details] [diff] [review]
(Dv1a-TB) msgCompSMIMEOverlay.js cleanup
[Checkin: See comment 36]

>+// const kEncryptionPolicy_IfPossible = 1;

Please note down that this is a ns4 only thing.

>+
>   var ifps = Components.interfaces.nsIPromptService;
>-
>   let promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>                                 .getService(ifps);
>-  setupBundles();
>+  if (!promptService)
>+    return;

I'd have inlined ifps too. 
The promptService check looks unnecessary to me. If we don't get a promptservice just let things blow up instead.

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> I'd have inlined ifps too. 

As it's used twice, I'll just let it be (ftb).

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-SM) Nits after Thunderbird review [Checkin: Comment 37]

Per comment 32 and 33.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 422212 [details] [diff] [review]
(Ev1-SM) Nits after Thunderbird review
[Checkin: Comment 37]


mnyromyr, ping for trivial review.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 419856 [details] [diff] [review]
(Dv1a-TB) msgCompSMIMEOverlay.js cleanup
[Checkin: See comment 36]


http://hg.mozilla.org/comm-central/rev/07feda0b0043

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 422212 [details] [diff] [review]
(Ev1-SM) Nits after Thunderbird review
[Checkin: Comment 37]


http://hg.mozilla.org/comm-central/rev/4769df747170

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 419856 [details] [diff] [review]
(Dv1a-TB) msgCompSMIMEOverlay.js cleanup
[Checkin: See comment 36]


(In reply to comment #36)
> http://hg.mozilla.org/comm-central/rev/07feda0b0043

Dv1a-TB, with comment 32 and 33 suggestion(s).

Comment 39

[Ludovic Hirlimann [:Usul]]

Why is this still open ?

Comment 40

[Philip Chee]

Yeah I'll close this as FIXED. Serge please file followup bugs for any remaining issues.