Keychain password prefs need clarification

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Preferences
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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).
(Assignee)

Updated

11 years ago
OS: Mac OS X 10.3 → Mac OS X 10.4
Hardware: PC → Macintosh
(Assignee)

Updated

11 years ago
Target Milestone: --- → Camino1.1
(Assignee)

Comment 1

11 years ago
Created attachment 244578 [details] [diff] [review]
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!
Attachment #244578 - Flags: review?
(Assignee)

Comment 2

11 years ago
Created attachment 244579 [details]
corresponding nib
(Assignee)

Comment 3

11 years ago
Created attachment 244581 [details] [diff] [review]
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.
Attachment #244578 - Attachment is obsolete: true
Attachment #244581 - Flags: review?
Attachment #244578 - Flags: review?

Updated

11 years ago
Attachment #244581 - Flags: review? → review?(stridey)

Comment 4

11 years ago
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
Attachment #244581 - Flags: review?(stridey) → review+
(Assignee)

Comment 5

11 years ago
(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

11 years ago
(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.
(Assignee)

Comment 7

11 years ago
Created attachment 244685 [details] [diff] [review]
formFill renamed
Attachment #244581 - Attachment is obsolete: true
Attachment #244685 - Flags: superreview?(mikepinkerton)
+    [KeychainService instance]->mFormPassowrdFillIsEnabled = [[PreferenceManager sharedInstance] getBooleanPref:gUseKeychainPref withSuccess:&success];


typo? mFormPassowrdFillIsEnabled?
(Assignee)

Comment 9

11 years ago
Created attachment 244802 [details] [diff] [review]
sans typo

Doh
Attachment #244685 - Attachment is obsolete: true
Attachment #244802 - Flags: superreview?(mikepinkerton)
Attachment #244685 - Flags: superreview?(mikepinkerton)
Comment on attachment 244802 [details] [diff] [review]
sans typo

sr=pink
Attachment #244802 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 11

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.