Last Comment Bug 624440 - Fully fix onComposerFromChanged() after bug 337964
: Fully fix onComposerFromChanged() after bug 337964
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 337964 624432
  Show dependency treegraph
 
Reported: 2011-01-10 10:44 PST by Serge Gautherie (:sgautherie)
Modified: 2012-12-21 20:41 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) Do not abuse boolean vars with string values (1.77 KB, patch)
2011-01-10 10:50 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Review
(Av1a) Do not abuse boolean vars with string values [Checked in: Comment 5] (1.87 KB, patch)
2011-01-10 10:53 PST, Serge Gautherie (:sgautherie)
mkmelin+mozilla: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2011-01-10 10:44:43 PST
I filed (SM port) bug 624432,
but I'm puzzled by the "changed" case(s) of bug 337964 fix.

I'm confused by the vars being named 'g*OptionChanged' (which seems correct to me) versus the comments about 'manually checked' [Couldn't the case be "manually unchecked" too?]
Shouldn't useEncryption/useSigning value be explicitly set in !=/!signMessage cases too?
Shouldn't gEncryptOptionChanged and gSignOptionChanged be reset to false in some case(s)?

If the code is correct as is, maybe just modifying/adding some more comments would clarify it.
Comment 1 Serge Gautherie (:sgautherie) 2011-01-10 10:50:59 PST
Created attachment 502544 [details] [diff] [review]
(Av1) Do not abuse boolean vars with string values
Comment 2 Serge Gautherie (:sgautherie) 2011-01-10 10:53:36 PST
Created attachment 502545 [details] [diff] [review]
(Av1a) Do not abuse boolean vars with string values
[Checked in: Comment 5]
Comment 3 Magnus Melin 2011-01-10 11:36:34 PST
(In reply to comment #0)
> I'm confused by the vars being named 'g*OptionChanged' (which seems correct to
> me) versus the comments about 'manually checked' [Couldn't the case be
> "manually unchecked" too?]

The code comments "checked" is really implying checked/unckeckecked.

> Shouldn't useEncryption/useSigning value be explicitly set in !=/!signMessage
> cases too?

Matter of taste i guess. This way it's the least amount of code.

> Shouldn't gEncryptOptionChanged and gSignOptionChanged be reset to false in
> some case(s)?

I suppose one could, but i don't really they should ever reset. If they changed the user made a conscious choise.
Comment 4 Magnus Melin 2011-01-10 11:37:53 PST
Comment on attachment 502545 [details] [diff] [review]
(Av1a) Do not abuse boolean vars with string values
[Checked in: Comment 5]

Ok with me.
Comment 5 Serge Gautherie (:sgautherie) 2011-01-10 14:20:47 PST
Comment on attachment 502545 [details] [diff] [review]
(Av1a) Do not abuse boolean vars with string values
[Checked in: Comment 5]

http://hg.mozilla.org/comm-central/rev/57a01a6867f8

Note You need to log in before you can comment on or make changes to this bug.