Closed Bug 456860 Opened 13 years ago Closed 11 years ago

Improve msgCompSMIMEOverlay.js and msgReadSMIMEOverlay.js

Categories

(SeaMonkey :: Security, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

(Keywords: assertion)

Attachments

(5 files, 5 obsolete files)

11.77 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
4.52 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
17.67 KB, patch
mnyromyr
: review+
Details | Diff | Splinter Review
15.78 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
1.94 KB, patch
mnyromyr
: review+
Details | Diff | Splinter Review
I started from bug 456672 comment 3,
then ended up "rewriting" the whole files.
Attached patch (Av1) once for all (obsolete) — Splinter Review
[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.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #340206 - Flags: superreview?(neil)
Attachment #340206 - Flags: review?(kaie)
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.
Attachment #340206 - Flags: superreview?(neil) → superreview+
(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...
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.
Blocks: 438927
Attachment #340206 - Flags: review?(bienvenu)
Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

Per comment 4: I'm fine with either reviewer.
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 on attachment 340206 [details] [diff] [review]
(Av1) once for all

r- for "too much noise"
Attachment #340206 - Flags: review?(kaie) → review-
Attachment #340206 - Flags: review?(bienvenu)
Comment on attachment 340206 [details] [diff] [review]
(Av1) once for all

clearing review request based on Kaie's minus.
Attached patch (Av2) fix + reduced cleanup only (obsolete) — Splinter Review
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.
Attachment #340206 - Attachment is obsolete: true
Attachment #345669 - Flags: review?(kaie)
Attachment #345669 - Flags: review?(bienvenu)
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 ?
Attachment #345669 - Flags: review?(bienvenu)
Kaie, (David): looking for r !!
Av2, without the 2 (superfluous in javascript) |gBundle = null|.

Asking dmose too for review, per irc discussion.
Attachment #345669 - Attachment is obsolete: true
Attachment #369529 - Flags: review?(kaie)
Attachment #345669 - Flags: review?(kaie)
Attachment #369529 - Flags: review?(dmose)
Whiteboard: [needs review kaie, dmose]
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.
I'm desperately waiting for a review :-<
Target Milestone: seamonkey2.0b1 → ---
Sorry for the delay; I hope to get to this tomorrow.
Comment on attachment 369529 [details] [diff] [review]
(Av2a) fix + reduced cleanup only
[Checkin: See comment 17]

Looks good; r=dmose.
Attachment #369529 - Flags: review?(kaie)
Attachment #369529 - Flags: review?(dmose)
Attachment #369529 - Flags: review+
Whiteboard: [needs review kaie, dmose]
Comment on attachment 369529 [details] [diff] [review]
(Av2a) fix + reduced cleanup only
[Checkin: See comment 17]


http://hg.mozilla.org/comm-central/rev/216c44724858
Attachment #369529 - Attachment description: (Av2a) fix + reduced cleanup only → (Av2a) fix + reduced cleanup only [Checkin: Comment 17]
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.
Attachment #369529 - Attachment description: (Av2a) fix + reduced cleanup only [Checkin: Comment 17] → (Av2a) fix + reduced cleanup only [Checkin: See comment 17]
Attachment #398891 - Flags: superreview?(bugzilla)
Attachment #398891 - Flags: review?(bugzilla)
Attachment #398891 - Flags: superreview?(bugzilla)
Attachment #398891 - Flags: superreview+
Attachment #398891 - Flags: review?(bugzilla)
Attachment #398891 - Flags: review+
Comment on attachment 398891 [details] [diff] [review]
(Bv1) msgReadSMIMEOverlay.js cleanup
[Checkin: Comment 20]


http://hg.mozilla.org/comm-central/rev/f403bbb0e4b5
Attachment #398891 - Attachment description: (Bv1) msgReadSMIMEOverlay.js cleanup → (Bv1) msgReadSMIMEOverlay.js cleanup [Checkin: Comment 20]
I'll apply it to the TB file too...
Attachment #413126 - Flags: superreview?(bugzilla)
Attachment #413126 - Flags: review?(bugzilla)
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.
Attachment #413126 - Flags: superreview?(neil)
Attachment #413126 - Flags: superreview?(bugzilla)
Attachment #413126 - Flags: review?(mnyromyr)
Attachment #413126 - Flags: review?(bugzilla)
Attachment #413126 - Flags: review?(mnyromyr) → review-
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.
Attachment #413126 - Attachment is obsolete: true
Attachment #413126 - Flags: superreview?(neil)
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.
Attachment #416311 - Flags: superreview?(neil)
Attachment #416311 - Flags: review?(mnyromyr)
(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 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.
Attachment #416311 - Flags: review?(mnyromyr) → review-
Cv2-SM, with comment 26 suggestion(s).
Attachment #416311 - Attachment is obsolete: true
Attachment #419383 - Flags: superreview?(neil)
Attachment #419383 - Flags: review?(mnyromyr)
Attachment #416311 - Flags: superreview?(neil)
Attachment #419383 - Flags: review?(mnyromyr) → review+
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.)
Attachment #419383 - Flags: superreview?(neil) → superreview+
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).
Attachment #419383 - Attachment description: (Cv3-SM) msgCompSMIMEOverlay.js cleanup → (Cv3-SM) msgCompSMIMEOverlay.js cleanup [Checkin: See comment 29]
Port Cv3-SM to TB, at least the obvious part.
Attachment #419561 - Flags: review?(philringnalda)
Dv1-TB, unbitrotted and with msgHdrViewSMIMEOverlay.xul fix.
Attachment #419561 - Attachment is obsolete: true
Attachment #419856 - Flags: review?
Attachment #419561 - Flags: review?(philringnalda)
Attachment #419856 - Flags: review? → review?(mkmelin+mozilla)
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.
Attachment #419856 - Flags: review?(mkmelin+mozilla) → review+
(In reply to comment #32)
> I'd have inlined ifps too. 

As it's used twice, I'll just let it be (ftb).
Comment on attachment 422212 [details] [diff] [review]
(Ev1-SM) Nits after Thunderbird review
[Checkin: Comment 37]


mnyromyr, ping for trivial review.
Attachment #422212 - Flags: review?(mnyromyr) → review+
Attachment #422212 - Flags: superreview?(neil)
Attachment #422212 - Flags: superreview?(neil) → superreview+
Attachment #419856 - Attachment description: (Dv1a-TB) msgCompSMIMEOverlay.js cleanup → (Dv1a-TB) msgCompSMIMEOverlay.js cleanup [Checkin: Comment 36]
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 on attachment 422212 [details] [diff] [review]
(Ev1-SM) Nits after Thunderbird review
[Checkin: Comment 37]


http://hg.mozilla.org/comm-central/rev/4769df747170
Attachment #422212 - Attachment description: (Ev1-SM) Nits after Thunderbird review → (Ev1-SM) Nits after Thunderbird review [Checkin: Comment 37]
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).
Attachment #419856 - Attachment description: (Dv1a-TB) msgCompSMIMEOverlay.js cleanup [Checkin: Comment 36] → (Dv1a-TB) msgCompSMIMEOverlay.js cleanup [Checkin: See comment 36]
Why is this still open ?
Yeah I'll close this as FIXED. Serge please file followup bugs for any remaining issues.
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.