Keychain password prefs need clarification

RESOLVED FIXED in Camino1.5

Status

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

Tracking

({fixed1.8.1.1})

Trunk
Camino1.5
PowerPC
macOS
fixed1.8.1.1

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

13 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

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

Updated

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

Comment 1

13 years ago
Posted patch removes second pref (obsolete) — Splinter Review
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

13 years ago
Posted file corresponding nib
(Assignee)

Comment 3

13 years ago
Posted patch correct checkbox default (obsolete) — Splinter Review
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

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

13 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.
(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

13 years ago
Posted patch formFill renamed (obsolete) — Splinter Review
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

13 years ago
Posted patch sans typoSplinter Review
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

13 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 13 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.