40122 - "save these values" should be prechecked if value was already saved

Status: RESOLVED FIXED

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

Description

[Doron Rosenberg (IBM)]

On sites where there is a pop-up that asks for your username and password, there
is the option to "save these values".  If I select it, it saves them, but on the
next time I see the pop-up, it is unchecked. This is somewhat confusing,
especially since it works if left unselected.

Comment 1

[Stephen P. Morse]

That was done by design.  The checkbox always comes up unchecked, regardless of 
whether you checked it or not on the last usage of the box.  Reason: saving 
passwords might comprimise privacy and we want the user to always do an explicit 
opt-in on each one he wants to save.

In fact, in the original implementation I had the checkbox always coming up 
checked.  This required an opt-out and I immediately got criticized for it.  It 
was decided then to make it either opt-in or to remember previous state.  I 
chose opt-in for the reason given above (and also because then I wouldn't have 
to implement any means of remembering the last state).

Comment 2

[Doron Rosenberg (IBM)]

Actually, I meant that if I go to the same site I was before on which I checked
it, it comes up unchecked. This confuses a bit, as you had previously checked
this on the same site, and the info is inserted.

Comment 3

[Stephen P. Morse]

If you go to the same site, and you resubmit the same username and password, you 
won't need to save it again (it's already saved).  And if you are now using a 
different username at that site, it could very well be a temporary thing in 
which case you don't want to save it.  At least you should be made to opt-in if 
you have changed the conditions (username in this case).

Comment 4

[Stephen P. Morse]

Actually I can see your argument.  If you go to a site for which the form comes 
up prefilled (meaning it was previously saved), then it should also be 
pre-checked.  In fact, that would be a reminder to the user that these values 
are already being saved.  And the implementation would be very simple

So I'll remove the invalid indication and leave it as a request for a future 
enhancement.

Comment 5

[Stephen P. Morse]

Updating the summary to reflect current thinking on this enhancement request.

Comment 6

[sairuh (rarely reading bugmail)]

changed to enhancement...

Comment 7

[Daniel Veditz [:dveditz]]

Changing fictional "M30" to reality

Comment 8

[sairuh (rarely reading bugmail)]

adding helpwanted...

Comment 9

[Blake Ross]

--> me

morse, do we currently save this info anywhere?

Comment 10

[Stephen P. Morse]

Of course we already have saved the URL/name/password that was prefilled for the 
form.  So when the form is submitted, we could go through the same logic that we 
went through at prefill time to see if a value is being saved.  That could serve 
as the trigger for determining whether or not to precheck the checkbox.

Comment 11

[sairuh (rarely reading bugmail)]

spam: mass-moving open password manager (single signon) and form manager
(autofill) bugs to Terri for qa contact. unfortunately, i cannot cc myself with
this form, so feel free and add me if you want to keep me in the loop with any
(but, pls not all :) of these... will also go thru 'em meself, a bit later...

Comment 12

[Doron Rosenberg (IBM)]

any chance of this for 0.9?

Comment 13

[Blake Ross]

No time right now.

Comment 14

[Stephen P. Morse]

Let me just clarify what this bug report is saying.

There are two different situations in which the password manager asks the 
user if he wants to save his login.

Case 1: Site puts up a login form, user fills it out, and then presses submit.  
He gets a "do you want to save" dialog with OK/Cancel buttons but no checkbox.

Case 2: Site requests authentication in which case the password manager puts up 
a login form together with a "do you want to save" checkbox.

This bug report is addressing case 2.  If the password manager is able to 
prefill its own login form, it should also precheck the save box reminding the 
user that a value is already saved.  This bug does not address case 1 since 
there is no checkbox in that case.

Attaching real simple patch to fix this problem.

Comment 15

[Stephen P. Morse]

Attachment: Patch to precheck save box when appropriate

Comment 16

[pnunn]

Makes sense to me.
r:pnunn

Comment 17

[Brendan Eich [:brendan]]

Reviewing surrounding code: how about checking whether those ToNewUnicode calls
fail and returning NS_ERROR_OUT_OF_MEMORY if so?

/be

Comment 18

[Stephen P. Morse]

There's no error code returned from ToNewUnicode, so the best that could be done 
would be to check for the result being of 0 length.

Furtherm, ToNewUnicode is called throughout this module so such a change would 
have to be more globally.  In fact it is probably done thoughout the codebase 
without any check for failure.

In any case, this is tangential to the current bug.  If you feel that such 
checks should be made, shouldn't that be in a separate bug report?

Comment 19

[Brendan Eich [:brendan]]

>There's no error code returned from ToNewUnicode, so the best that could be
>done would be to check for the result being of 0 length.

Huh?  ToNewUnicode returns a pointer to PRUnichar, null on out-of-memory.

>Furtherm, ToNewUnicode is called throughout this module so such a change would
>have to be more globally.  In fact it is probably done thoughout the codebase
>without any check for failure.

Could be, but can we fix what we know to be broken?  Worse is better, whereas
making it everyone's problem makes it no one's problem.

>In any case, this is tangential to the current bug.  If you feel that such
>checks should be made, shouldn't that be in a separate bug report?

I'm asked to review code in order to uphold quality, and I'm doing just that,
including looking at context (see http://www.mozilla.org/hacking/reviewers.html
rule/tip #7).  A separate bug does sound like a fine way to track the unchecked
ToNewUnicode problem.  Are you going to file one?

/be

Comment 20

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is a Netscape beta stopper. Adding Vishy to cc 
list                                                                            

Comment 21

[Stephen P. Morse]

Attachment: Ammending patch to check for out-of-memory condition

Comment 22

[Stephen P. Morse]

Attachment: better patch to check for out-of-memory condition

Comment 23

[Brendan Eich [:brendan]]

sr=[e-mail] on
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=22641.

/be

Comment 24

[Stephen P. Morse]

Fix checked in.