Closed Bug 474792 Opened 13 years ago Closed 12 years ago
Need an API to clear all "login saving enabled" entries (for Clear Recent History)
Bug 380852 added support for clearing site-specific settings from the Clear Recent History dialog. Implementation-wise, this includes ContentPrefs and PermissionManager entries. But Marcia reports that one thing that isn't cleared is "never remember passwords for this site", which is unsurprising since it doesn't use the permission manager (cf: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/storage-mozStorage.js#460 ) If there's already a bug to move pwmgr to permission manager, this is probably dupable, but if that's not desirable, then it would at least be nice to have an API for dropping these "don't save passwords for host X" entries so that people could Clear Recent History without clearing their entire password database.
Nominating this to block final release (P3). If we ship with this, people risk having embarassing information preserved that they expected to have deleted. On the other hand, it is a limited case, most site-specific data is correctly cleared by the code in bug 380852, so to the extent that we are saying some things (P1, P2) "block more" than others, I'd argue this one blocks less. Since it changes an API, we might be reluctant to take it post-release (though it's a purely additive change, so there's less risk, but risk still exists).
Priority: -- → P3
A new API isn't necessarily needed here. You can clear the list with: var hosts = pwmgr.getAllDisabledHosts() for each (var host in hosts) pwmgr.setLoginSavingEnabled(host, true); But I wouldn't be opposed to having a clearAllDisabledHosts() that does the same thing. I'm a *little* wary of having CRH clear this; it feels different than the normal site-specific settings because entries were set as a result of an explicit user choice to prevent us from nagging to remember a login (or a deliberate decision not to not save a sensitive bank login). It's less ephemeral than, say, a saved page-zoom setting. [Or maybe not, I could see arguments either way.] OTOH, having separate UI option just for this list would be overkill, and it's probably not easy to explain as a CRH item in 3-4 words. Good is better than Perfect here, I think.
(In reply to comment #2) > I'm a *little* wary of having CRH clear this; it feels different than the > normal site-specific settings because entries were set as a result of an > explicit user choice to prevent us from nagging to remember a login (or a > deliberate decision not to not save a sensitive bank login). It's less > ephemeral than, say, a saved page-zoom setting. [Or maybe not, I could see > arguments either way.] Yeah, I hear that, but we are already clearing nsIPermissionManager entries like "allow popups" which are similarly explicit choices. I feel silly for not seeing getAllDisabledHosts(), that would certainly be the least invasive way to do this.
Doesn't block, but I'd likely approve a patch and we should get it as cleanup.
Priority: P3 → P2
If I'm the only consumer for such an API, why not just use the thing that already exists? What do you think, Dolske?
Assignee: nobody → johnath
Status: NEW → ASSIGNED
Attachment #359351 - Flags: review?(dolske)
Comment on attachment 359351 [details] [diff] [review] Clear using the existing API, rather than creating a new one Yeah, this is fine. No need to block on API work... I'll spin off a clone of this as a pwmgr bug to add such as API, as I still think it would be a good idea.
Attachment #359351 - Flags: review?(dolske) → review+
Landed on trunk for baking. http://hg.mozilla.org/mozilla-central/rev/ce260f307e23
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.