355033 - Keychain password prefs need clarification

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Stuart Morgan]

There are a couple of issues with the password prefs:
- The preference "Auto-fill passwords in web forms" isn't accurately named, as it also controls prefill of any gecko-originated password prompts (such as the ones you get for HTTP-auth).
- The dual-prefs seem odd in general. I don't see a real use-case for wanting to store passwords but not fill them (I thought we'd discussed this before, but I can't find anything).

I think we should just have one check box that is something like:
_ Auto-fill passwords in web forms from the Keychain
and that it should only control web forms--the password prompts from Gecko will toss up a sheet regardless, so there's not really any added cost to always showing the checkbox (which we do anyway, causing confusion like bug 318146).

Comment 1

[Stuart Morgan]

Attachment: removes second pref

Removes the second pref and makes http auth dialogs independent of the pref (and defaulting to not store, since the checkbox could be easy to overlook and we want to be more secure by default). Negative code *and* negative prefs!

Comment 2

[Stuart Morgan]

Attachment: corresponding nib

Comment 3

[Stuart Morgan]

Attachment: correct checkbox default

Except that behavior would be dumb, because people would end up deleting their saved passwords all the time by accident.  What we really want is to default to off if they don't already have a password stored, but on if they do.

Comment 4

[froodian (Ian Leue)]

Comment on attachment 244581 [details] [diff] [review]
correct checkbox default

I think the text in the UI should closer match what you proposed in comment 0–we're not just saving the passwords, we're filling them too, and if we're filling them, obviously they must be saved somewhere.

Also, I don't like the use of the formFill terminology–that's what we call filling from the Address Book.  What about passwordFill or something similar?

Other than that, looks good.  r=me with those changes

Comment 5

[Stuart Morgan]

(In reply to comment #4)
> (From update of attachment 244581 [details] [diff] [review] [edit])
> I think the text in the UI should closer match what you proposed in comment
> 0–we're not just saving the passwords, we're filling them too, and if we're
> filling them, obviously they must be saved somewhere.

And if we are saving them, we are obviously doing something with them, so it goes both ways.  I don't think we should use the comment 0 language because I didn't like it when I put it in; it raises the question "what is it filling them with?"  The current text makes it clearer what's actually going on, which is that it's remembering things that the user puts in.

> Also, I don't like the use of the formFill terminology–that's what we call
> filling from the Address Book.  What about passwordFill or something similar?

passwordFill doesn't distinguish from the http auth stuff though, which is why I changed it.  I can switch it to formPasswordFill.

Comment 6

[froodian (Ian Leue)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 244581 [details] [diff] [review] [edit] [edit])
> > I think the text in the UI should closer match what you proposed in comment
> > 0–we're not just saving the passwords, we're filling them too, and if we're
> > filling them, obviously they must be saved somewhere.
> 
> And if we are saving them, we are obviously doing something with them, so it
> goes both ways.  I don't think we should use the comment 0 language because I
> didn't like it when I put it in; it raises the question "what is it filling
> them with?"  The current text makes it clearer what's actually going on, which
> is that it's remembering things that the user puts in.

Sure, that makes sense.  If people get confused, we can change it later.

> passwordFill doesn't distinguish from the http auth stuff though, which is why
> I changed it.  I can switch it to formPasswordFill.
> 

Sounds lovely.

Comment 7

[Stuart Morgan]

Attachment: formFill renamed

Comment 8

[Mike Pinkerton (not reading bugmail)]

+    [KeychainService instance]->mFormPassowrdFillIsEnabled = [[PreferenceManager sharedInstance] getBooleanPref:gUseKeychainPref withSuccess:&success];


typo? mFormPassowrdFillIsEnabled?

Comment 9

[Stuart Morgan]

Attachment: sans typo

Doh

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 244802 [details] [diff] [review]
sans typo

sr=pink

Comment 11

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH