Provide option to remove profiles during uninstall

VERIFIED FIXED in Firefox 3 beta1

Status

()

defect
P1
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
Firefox 3 beta1
x86
Windows Vista
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Posted image screenshot (obsolete) —
PRD Item INST-003a

Mike, I could use your help with the strings on this dialog. I'd also like to display an "Are you really really realllllly sure" messagebox prior to uninstalling when this checkbox is checked and could use your string wizardry there if you are ok with displaying a messagebox when the checkbox is checked.
Attachment #283399 - Flags: ui-review?(beltzner)
FWIW, I still think this is a very bad idea and should be WONTFIX: see http://blogs.msdn.com/oldnewthing/archive/2007/09/17/4948130.aspx for a cogent argument.
I agree for the most part but we are only doing this for the current as stated in that blog entry:
"Now, if you want, you can clean up the per-user data for the current user (after asking for permission), but you definitely should not be messing with the profiles of other users."

That's exactly what this bug is about. The default will be to not remove the profiles and if I get my way the user will have to confirm that they "really, really, realllllly" want to do this before proceeding.
Requesting blocking‑firefox3 since this was added to the PRD as a P1
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: uiwanted
btw: I have a patch ready to go as long as the ui isn't made too complex (e.g. installers typically don't have all of the ui options as apps have) as soon as I get the strings.
Whiteboard: [need ui-review beltzner]
Comment on attachment 283399 [details]
screenshot

"Profiles" is obviously not the thing, since it requires an understanding of the multiple profile thing which we don't officially support :)

How about:

[ ] Remove my &brandShortName personal data

clicking that could cause the following to appear below on a yellow background or as a confirmation dialog: 

Removing Personal Data

This will permanently remove your bookmarks, saved passwords, cookies and customizations. You may wish to keep this information if you plan on installing another version of &brandShortName in the future.

(( Remove my personal data )) ( Keep my personal data )
Attachment #283399 - Flags: ui-review?(beltzner) → ui-review-
Posted image screenshot
This implements your comments inside the wizard.
Attachment #283399 - Attachment is obsolete: true
Attachment #285278 - Flags: ui-review?(beltzner)
Posted image screenshot w/ scrollbar (obsolete) —
To circumvent problems with locales I've decided to go with a scrollbar on this control.
Attachment #285285 - Flags: ui-review?(beltzner)
Attachment #285278 - Flags: ui-review?(beltzner)
Attachment #285292 - Attachment description: patch rev1 → patch rev1 (requires patch from bug 399381 and Bug 399665 to apply cleanly)
Comment on attachment 285292 [details] [diff] [review]
patch rev1 (requires patch from bug 399381 and Bug 399665 to apply cleanly)

Seth brought up an excellent point non relative regarding profiles possibly being directories we don't want to delete. For example My Documents or for that matter any directory that wasn't created by the app since it may contain user data not related to the profile. Firefox doesn't try to prevent using these directories and deleting them would be to say the least bad. For non-relative directories I will probably just delete extremely well-known directories and files.
Attachment #285292 - Flags: review?(sspitzer)
< +      RmDir /r "$R7\bookmarkbackups"
< +      RmDir /r "$R7\Cache"
< +      RmDir /r "$R7\chrome"
< +      RmDir /r "$R7\extensions"
< +      RmDir /r "$R7\minidumps"
< +      Delete "$R7\bookmarks.html"
< +      Delete "$R7\bookmarks.preplaces.html"
< +      Delete "$R7\cert8.db"
< +      Delete "$R7\compatibility.ini"
< +      Delete "$R7\compreg.dat"
< +      Delete "$R7\content-prefs.sqlite"
< +      Delete "$R7\cookies.sqlite"
< +      Delete "$R7\downloads.sqlite"
< +      Delete "$R7\extensions.cache"
< +      Delete "$R7\extensions.ini"
< +      Delete "$R7\extensions.rdf"
< +      Delete "$R7\formhistory.sqlite"
< +      Delete "$R7\key3.db"
< +      Delete "$R7\localstore.rdf"
< +      Delete "$R7\places.sqlite"
< +      Delete "$R7\pluginreg.dat"
< +      Delete "$R7\prefs.js"
< +      Delete "$R7\search.sqlite"
< +      Delete "$R7\secmod.db"
< +      Delete "$R7\signons2.txt"
< +      Delete "$R7\urlclassifier3.sqlite"
< +      Delete "$R7\XPC.mfl"
< +      Delete "$R7\xpti.dat"
< +      Delete "$R7\XUL.mfl"

For grins, I looked in my profile dir and I had these files that are not on your list:

blocklist.xml
formhistory.sqlite
localstore.rdf
mimeTypes.rdf
sessionstore.bak
sessionstore.js
webappsstore.sqlite

Perhaps instead we could just check for existence of some well know / guaranteed to be there files in $R7 (to make sure we're in the right place), and if we find them, then RmDir /r "$R7".

What do you think, robert?
We can't do that since the non relative profile could very well be "My Documents", the user's desktop, or a user created directory that has user documents that they selected in the profile manager so at best we can RmDir /r well known dirs in the profile and list out the well known files... hence my "yeah right" comment about being able to remove the profile dir when done.

I can add those files easily though I know we will almost always be missing some.
Comment on attachment 285390 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 and Bug 399665 to apply cleanly)

r=sspitzer, thanks for clarifying robert.

a few questions / comments:


1) can you elaborate the comment to explain why we are doing RmDir "$R7" (and not RmDir /r "$R7), for the My Documents scenario?

2) Was this prd item (PRD Item INST-003a) a request from a partner, or something we wanted to do for firefox?  

if for partners only, is it possible to not show this checkbox at all (for the standard firefox version of the uninstaller)?

reading that blog post that bsmedberg linked to points out some good reasons why removing user data on uninstall can be a bad idea.
Attachment #285390 - Flags: review?(sspitzer) → review+
RmDir "$R7" is so we don't remove a directory unless it is empty. So, if the user selected "My Documents" for their profile directory it will only be deleted if it is empty after the specific dir / file removal.

It is for Firefox in general and was requested by mconnor.

Agreed regarding the blog post. I think it is also true that it is a bad idea in general to have an app provide the ability to have multiple profiles as we do. As the blog post also points out it is acceptable to remove the current user's personal data for an app and it would be much simpler for us to provide this option if we didn't provide the ability for the user to specify a profile path outside of the one under appdata\mozilla\firefox\profiles. It is also important to note that with offline apps our cache will only increase in size.

I'm going to discuss the non relative profile case with mconnor to figure out what we should do here. To be blunt, I'm not terribly thrilled with doing this from the uninstaller in the first place since it can never have the exact same logic as used by the app and the potential for evil happenings is rather high. On the other hand I do understand why this has been asked for in the past and was added to the PRD.
Non relative we will not delete... patch coming up.
Seth, could you give this a once over? The only change is that we no longer try to remove non relative profiles per mconnor.
Attachment #285390 - Attachment is obsolete: true
Attachment #285411 - Flags: review?(sspitzer)
Comment on attachment 285411 [details] [diff] [review]
patch rev3 (requires patch from bug 399381 and Bug 399665 to apply cleanly)

r=sspitzer, thanks robert.
Attachment #285411 - Flags: review?(sspitzer) → review+
Comment on attachment 285411 [details] [diff] [review]
patch rev3 (requires patch from bug 399381 and Bug 399665 to apply cleanly)

Requesting a1.9 for this P1 PRD item
Attachment #285411 - Flags: approval1.9?
Comment on attachment 285278 [details]
screenshot

as discussed in over-the-shoulder review, the scrollbar makes me sad.

instead, let's move the checkbox so that it's one linespace beneath the textbox, and then leave the remaining space for the warning (which should allow for plenty of l10n leeway)

Also, I think maybe "Remove my history and bookmarks" works better than "Remove my Minefield personal data"; I know it's not 100% accurate, but it's more likely to cue users to the actual choice they're making.
Attachment #285278 - Flags: ui-review-
Posted image screenshot
Attachment #285285 - Attachment is obsolete: true
Attachment #285550 - Flags: ui-review?(beltzner)
Attachment #285285 - Flags: ui-review?(beltzner)
After discussing with Mike we've decided to go with "Remove my Minefield personal data and customizations"
Priority: -- → P1
Whiteboard: [need ui-review beltzner]
Target Milestone: --- → Firefox 3 M9
Comment on attachment 285550 [details]
screenshot

Received a verbal ui-r+ from beltzner with the string change
Attachment #285550 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 285557 [details] [diff] [review]
patch - updated to comments and w/o dependencies on bug 399665

>Index: browser/locales/en-US/installer/custom.properties
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v
>retrieving revision 1.10
>diff -u -8 -r1.10 custom.properties
>--- browser/locales/en-US/installer/custom.properties	21 Sep 2007 22:09:04 -0000	1.10
>+++ browser/locales/en-US/installer/custom.properties	20 Oct 2007 01:06:17 -0000
>@@ -76,16 +76,23 @@
>+UN_CONFIRM_PAGE_TITLE=Uninstall ${BrandFullName}
>+UN_CONFIRM_PAGE_SUBTITLE=Remove ${BrandFullName} from your computer.
>+UN_CONFIRM_UNINSTALLED_FROM=${BrandShortName} will be uninstalled from the following location:
>+UN_CONFIRM_CLICK=Click Uninstall to continue.
>+UN_REMOVE_PROFILES=&Remove my ${BrandShortName} personal data
>+UN_REMOVE_PROFILES_DESC=This will permanently remove your bookmarks, saved passwords, cookies and customizations. You may wish to keep this information if you plan on installing another version of $BrandShortName in the future.
s/$BrandShortName/${BrandShortName}
I will fix this before checkin
s/$BrandShortName/${BrandShortName}
I will fix this before checkin

robert, you know that perl script that is part of this bug?  

Should we run it on en-US as well, to help catch mistakes like that?  (or would that not work?)
It is actually a part of bug 399665 and the fix I mentioned has to do with this bug already being set to blocking Firefox 3.0 since it is a P1 PRD item and bug 399665 not having a1.9 yet since it is a P2 PRD item. I considered it and I think the note in the locale file should be sufficient.
bah... I wasn't clear... it wouldn't have caught this mistake since I was backing out the dependency on bug 399665 as mentioned in the patch description
Checked in to trunk
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.31; previous revision: 1.30
done
Checking in mozilla/browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  custom.properties
new revision: 1.11; previous revision: 1.10
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified fixed using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre on a clean Vista Business VM. The only issue I see, and I am not sure whether it is expected, is an empty Firefox directory persists in the AppsData\Roaming\Mozilla directory.
Status: RESOLVED → VERIFIED
I didn't want to remove the Firefox dir since people sometimes copy profile files to that location
Blocks: 407008
No longer blocks: 407008
You need to log in before you can comment on or make changes to this bug.