As a follow-up to the changes made in bug 398718 BobR requested (comment 47) that the getErrorMessage code be cleaned up. > Also, the block is a very long code block with lots of extra conditionals > embedded in a middle of a switch statement for an alreadly very long function. > It's crying out to be broken up into at least one or two extra functions > (making getting the SAN it's own function would greatly simplify the logic and > get rid of lots of if (useSAN) blocks.

moved here, from bug 398718 comment 57 Bob, here is the requested cleanup you requested. This patch is a nightmare to review, because it contains moved code and change of indenting. Because of that I will attach a separate patch that displays the changes much more easily.

moved here, from bug 398718 comment 58 This patch illustrates what I'm really doing: - moved code to separate 4 functions that each append some text - declare some more local helper variables, where needed - enhanced the code that produces the expiration error message. If we're unable to format the time, we should not abort without a message. We should still display the correct wording, and simply leave the time value empty. Note, in this special version of the patch: - the new functions are below getInvalidCertErrorMessage, so the order of the code is kept, and therefore the diff is minimal - added forward declarations to make it compile - ignores whitespace changes But I really want to check in the other version of the patch, which has correct whitespace and has the new functions before getInvalidCertErrorMessage, so the forward declarations are not necessary.

Comment on attachment 286875 [details] [diff] [review] Patch v1 (see next attachment for easier reviewing) r- I like the separate functions, but my code that raised the issue with me was code structured: PRBool myBool= PR_TRUE; if (myBool) { rv = doSomething(); if (!rv) { myBool = PR_FALSE; } } if (myBool) { rv = doSomethingElse(); if (!rv) { myBool = PR_FALSE; } } It makes the code hard to read when what you really want is func1() { rv = doSomething(); if (!rv) { return rv; } rv = doeSomething2(); if (!rv) { return rv; } . . . return OK; } rv = func1(); if (rv) { finish1(); } else { finish2(); } or equivalent. Not only did you keep the old version with this structure, you added a new version with the same structure. bob

(In reply to comment #3) > > I like the separate functions, but my code that raised the issue with me was > code structured: Ok, I see. Well, my patch is not exactly what you describe. Because I don't really want to return. The end of the function contains an action that depends on the earlier code. So, your request required me to completely restructure the two functions. Patch coming up.

Comment on attachment 286983 [details] [diff] [review] Patch v2 (reviewer friendly, ignores whitespace) Bob, can you please review?

Comment on attachment 286982 [details] [diff] [review] Patch v2 r+ = rrelyea that's much better. Much easier to read and see what is going on. bob

Comment on attachment 286983 [details] [diff] [review] Patch v2 (reviewer friendly, ignores whitespace) r+ the original. removing review request on reviewer friendly..

This patch differs from the reviewed patch in the following minor details: - one string ID got renamed by a different check in - I moved the separate functions further up in the file, so the forward declarations have become unnecessary (as announced I would do, earlier in this bug)

fixed

This patch introduced the use of an uninitialized variable. Var "trueExpired_falseNotYetValid" is defined and immediately used on the folowing line: http://mxr.mozilla.org/firefox/source/security/manager/ssl/src/nsNSSIOLayer.cpp#910

(In reply to comment #12) > This patch introduced the use of an uninitialized variable. Var > "trueExpired_falseNotYetValid" is defined and immediately used on the folowing > line: Thanks. It's a real regression, introduced by the cleanup... The variable is supposed to be used as an in/out parameter, and therefore param 3 of GetDateBoundary must be changed to be a reference. Reopening to get this fixed.

patch v3 checked in, marking fixed again