27686 - Return values for prompt routines are incorrect

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P3)

Description

[Stephen P. Morse]

From a mail message that I sent to Jud.  This involves the 
PromptUsernameAndPassword, PromptPassword, and Prompt routines in singsign.cpp.

------------

First off, my naming of the boolean argument as "returnValue" was a poor
choice since it can be confused with the nsresult that is the value of
the function itself.  So I'll try to clearly distinguish between the two
in what I'm about to write.

Looks like I've been very loose about what value I was putting into the
boolean argument and, indeed, in many places I return without having put
anything into it.  But I am consistent as to what the nsresult that the
function itself returns -- it returns NS_OK if a password was obtained
and a failure code otherwise.

So let me propose the following semantics.


1. If a password was successfully obtain (either from the single-signon
database or from a dialog with the user), we return NS_OK for the
nsresult value and PR_TRUE for the boolean "returnValue".

2. If the user presses cancel, we return NS_OK for the nsresult value
and PR_FALSE for the boolean "returnValue".

3. If a password is not collected for any other reason, we return the
failure code for the nsresult value.  The boolean "returnValue" is
undefined in that case.


If you agree with these semantics, then the changes are as follows:


1. Put a comment at the head of the three routines
(PromptUsernameAndPassword, PrompPassword, Prompt) describing these
semantics.

2. Initialize the boolean "returnValue" to PR_TRUE at the head of each
of the three routines.

3. Set the boolean "returnValue" to PR_FALSE in the three cases where
the user pressed cancel (as you did in your dif below).

4. Return NS_OK in the three cases where the user pressed cancel.

Comment 1

[Stephen P. Morse]

Fix checked in.  File involved was singsign.cpp.

Comment 2

[sairuh (rarely reading bugmail)]

from an end-user perspective, i don't think this can be verified since it's in
the source code. setting qa contact to steve so that he can verify it (which i
imagine would be easy since he did the checkin. :-)

Comment 3

[Stephen P. Morse]

You are right -- there is no way for you to verify this by doing black-box 
testing because the change is all "under the hood."  So I'll mark it verified.