Closed Bug 320504 Opened 16 years ago Closed 16 years ago

Prompt users to submit feedback when uninstalling FF

Categories

(Firefox :: Installer, enhancement)

x86
Windows XP
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox1.5

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

(Keywords: fixed1.8.0.2, fixed1.8.1, Whiteboard: [rft-dl])

Attachments

(7 files, 2 obsolete files)

When users choose to uninstall FF, it would be nice if we prompted them to optionally send us some feedback as to their reasons for uninstalling FF.

This could provide a source of useful data to help us better understand what
we should be most focusing on to improve FF.

This should be something extremely lightweight that tries not to get in the 
way of users too much.  If a user chooses to submit feedback, then we will
just fire up Internet Explorer and point them at a form on Mozilla's website.
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox1.5
Depends on: 307296
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Might consider for a later dot-release if there was a tested patch.
Flags: blocking1.8.0.1? → blocking1.8.0.1-
Blocks: 322884
Attached patch v1 patchSplinter Review
This patch implements a simple MessageBox prompt that notifies the user that the uninstall process is complete and asks them provide feedback to help us make FF/TB better.  If they agree to do so (which is the default dialog action), then we find iexplore.exe and launch it with the following URL:

  https://survey.mozilla.com/1/$productname$/$useragent/exit.html

The variables are taken from uninstall.ini and the "/ua" command line flag.  So, for example, the URL looks like the following when run from FF 1.5:

  https://survey.mozilla.com/1/Mozilla%20Firefox/1.5%20(en-US)/exit.html

This URL is not as clean as the AUS URL, but it is sufficient, and it is easy to generate from the uninstaller.
Attachment #208067 - Flags: review?(benjamin)
A couple comments about the survey URL:

  -  It uses https:// to give people warm fuzzies about privacy and such.
  -  The path begins with "/1/" as a version field for the rest of the URL,
     which is similar to what we adopted for the AUS URL.
  -  The keyword "exit.html" is appended to indicate that we're interested
     in showing the user an exit survey.  Note: we may have other surveys
     in the future (e.g., inactivity survey).
  -  It does not bother to include the OS since we know the request is only
     for Windows users.  If we want more detailed info about the Windows
     version, then we could probably just inspect IE's UA string.
If the uninstall has finished, the setup program will have correctly restored the user's previous default browser settings (right?), so why not just launch the URL directly? If you're really paranoid, you could do it with IE explicitly after the direct launch fails.
> If the uninstall has finished, the setup program will have correctly restored
> the user's previous default browser settings (right?), so why not just launch
> the URL directly? If you're really paranoid, you could do it with IE explicitly
> after the direct launch fails.

One would think so, but I couldn't find the place where the uninstaller restores the default protocol associations, so I decided to manually invoke IE.
Comment on attachment 208067 [details] [diff] [review]
v1 patch

>Index: browser/installer/windows/uninstall.it

>+; Localize me!!
>+MSG_EXIT_SURVEY=@BRAND_PRODUCT_NAME@ has been uninstalled.  Please provide us with your feedback to assist us in our efforts to make @BRAND_PRODUCT_NAME@ better.

I'd like beltzner to review this string and the general concept before checking the uninstall.it changes in. I personally think this whole idea sounds like hanging on too long after a breakup.

>+  GetPrivateProfileString("Messages", "MSG_EXIT_SURVEY", "", buf, sizeof(buf),
>+                          szFileIniUninstall);

result-check this and bail early please if the while message is not present.

>+  /* launch IE, and point it at the survey URL (whitespace in the product name
>+   * or user agent is okay) */

Please add an XXXcomment explaning why we don't launch the default http/https handler.
Attachment #208067 - Flags: review?(benjamin) → review+
OK, I landed just the code changes w/ the checks to disable the dialog and iexplore execution if the MSG_EXIT_SURVEY string is not defined in uninstall.ini.
I'll post a follow-up patch that includes just the uninstall.it changes.
Attachment #208112 - Flags: review?
Attachment #208112 - Flags: review? → review?(beltzner)
Could this use http://feedback.mozilla.org/ (http://hendrix.mozilla.org/) in some way? Unfortunately https://feedback.mozilla.org/ and https://hendrix.mozilla.org/ don't work as expected...

I'm also wondering about default browser settings. Something somewhere must change the default browser, otherwise opening links from external applications after uninstalling wouldn't work?

(Removed dumbass comment regarding Mac and Linux... d'oh)
> I'm also wondering about default browser settings. Something somewhere must
> change the default browser, otherwise opening links from external applications
> after uninstalling wouldn't work?

That's right.  It's a bug with our uninstaller.  I just verified the bug by uninstalling FF 1.5.  Sure enough, my registry was left with entries pointing to FF 1.5's old location for http:// and https:// links.
Keywords: late-l10n
Can we get the locale encoded in the URL so we can localize the survey as appropriate?
> Can we get the locale encoded in the URL so we can localize the survey as
> appropriate?

it is.  see comment #2.
Thanks - (sorry for missing comment #2) 
CC'ing axel since there is possible l10n impact.

Darin - can we case the code out so the prompt only runs on en-us?  If so we can avoid localization changes (which we promised would not occur in the 1.5.0.x branches)?
Yes, I can easily make this en-US only if that is desired.  Just let me know how you want it to be.
Yes - let's do en-us only to start to avoid localization isseues (realizing we won't get data from outside the us either).   

Beltzner can we get your review on the prompt?
In 1.5 the uninstaller is always in English (it is not localizable), so we really can't make this en-US only (or I don't see a good way to do this) because it is the exact same file for all locales.
(In reply to comment #18)
> In 1.5 the uninstaller is always in English (it is not localizable), so we
> really can't make this en-US only (or I don't see a good way to do this)
> because it is the exact same file for all locales.

bsmedberg: don't we set the "/ua" command line switch w/ the proper locale in localized builds?  it looks like ab-CD.jst is responsible for writing the registry keys for the add/remove programs menu, which affect the "/ua" command line switch.   Or, am I mistaken?
Comment on attachment 208112 [details] [diff] [review]
uninstall survey text

(In reply to comment #6)
> I'd like beltzner to review this string and the general concept before 
> checking the uninstall.it changes in. I personally think this whole idea 
> sounds like hanging on too long after a breakup.

I'll happily revoice the concern that uninstall surveys skew the data in two fairly predictable ways based on the fact that the people who take them have a) elected to take them, which skews slightly to the angriest of users, and b) have bothered to uninstall software on Windows, which to the best of my knowledge isn't that common an action in these days of mega disks and indicates a user with a decent amount of computer knowledge. But that's more about how we interpret the data.

As currently expressed, I agree that the request for feedback does feel a little clingy. The aim should be for this dialog to not be seen as a "nag"; at no point should any user think in their mind "I said go away, Firefox, stop asking me questions". I'd suggest that we make the dialog a simple "OK" button dialog, and put the request for feedback as a checkbox or link inside it.

So:

 ,--------------------------------------------------.
 |                                                  |
 | Firefox 1.5 has been removed from your computer. |
 |                                                  |
 | [ ] Tell us what you thought of Firefox          |
 |                                                  |
 |                   [ OK ]                         |
 '--------------------------------------------------'
 
or

 ,--------------------------------------------------.
 |                                                  |
 | Firefox 1.5 has been removed from your computer. |
 |                                                  |
 | _Let us know what you thought of Firefox_        |
 |                                                  |
 |                    [ OK ]                        |
 '--------------------------------------------------'

This makes the dialog primarily informative (confirming the uninstall task) and allows a feedback route for those inclined to take it.

Even if we don't go this route, I'd suggest the following rephrasing to the string:

MSG_EXIT_SURVEY=@BRAND_PRODUCT_NAME@ has been removed from your computer. Would you like to fill out a short survey to tell us what you thought of @BRAND_PRODUCT_NAME@?

Finally, I think (if I'm reading the code right) you're using an OK/CANCEL box here, and it should be a [YES]/[NO] choice, instead.
Attachment #208112 - Flags: review?(beltzner) → review-
FWIW, I don't think you can do anything more complex that text in a standard message box, so putting a checkbox or link in one would require creating a dialog template and so on, and you'd probably miss some of the standard features of a message box (like XUL ones currently do :( ). Not to say you shouldn't, but it *is* a lot of work to do a proper MessageBox replacement.
also... why not just ShellExecute the URL? that avoids having to find where iexplore is, and they may actually have a different browser installed (afaik we restore the settings to the previous ones, so they might have used e.g. opera or something)
biesi, see comment 4, comment 5 and comment 11.
Mike: Thanks for taking the time to suggest better UI.  I agree with your suggestions.  I also agree with James' point about your UI being more complex to implement.  That doesn't mean that it can't be done, of course ;-)

As for the exit survey being a skewed data source, I agree.  However, we have other skewed data sources as well, such as the broken website reporter (how discoverable is that anyways?), and yet it proves to be useful... right?

I have also considered prompting the user to take a survey if they have not used FF in over a month or so... thought, that sort of prompt is much more likely to raise objections -- I'm sure -- for being obnoxious.  The point being that the exit survey need not be our only means of prompting the user for feedback.
I've been investigating the use of NSIS for the Win32 installer and built a basic installer this weekend. Here is a screenshot of a finish page that implements this.
> screenshot - NSIS uninstall finish page

Rob, that looks great!  Let's get in sync with respect to this uninstaller work.  I am still very interested in getting this into FF 1.5.0.x (hopefully 1.5.0.2) as I think it will help provide some useful data for the 2.0 product cycle.  Is your new uninstaller wizard planned for 2.0?
Yes, the new uninstaller will match the new installer which is targeted at ff2.
Attached patch v2 patchSplinter Review
OK, this implements beltzner's suggested UI.
Attachment #209479 - Flags: review?(benjamin)
Attached image screenshot for v2 patch
(In reply to comment #24)
> I have also considered prompting the user to take a survey if they have not
> used FF in over a month or so... thought, that sort of prompt is much more
> likely to raise objections -- I'm sure -- for being obnoxious.  The point 

I would be one of the first people to raise that point. Really what you want to do here is file a bug on Windows asking them to point out to people that the software they installed several years ago is rusting or something, and should really be removed ;)

But I do agree that some data is better than no data; just wanted to make sure that we're aware of the statistical significance, &c.

(In reply to comment #29)
> Created an attachment (id=209481) [edit]
> screenshot for v2 patch

Ugh, so can I nitpick my own design? :)

 * the checkbox and string should be indented a little bit, I think
 * s/Tell/Fill out a short survey to tell/

with those nits, ui-review=beltzner
> Ugh, so can I nitpick my own design? :)
> 
>  * the checkbox and string should be indented a little bit, I think
>  * s/Tell/Fill out a short survey to tell/

Yeah, I know it's an ugly dialog.  Using raw Win32 UI primitives is quite tedious ;-)

I'll make those suggested changes.  It will result in the checkbox message being longer than the main dialog text.  That means that indenting the checkbox a bit will result in a stair-case look to the two lines of text.  Is that okay, or do you have suggestions for how to present it in a better way?
I think I'd like to keep the shorter text for the checkbox.  Aesthetically, it fits better into the dialog.  Also, it seems clear enough to me that it results in further action, and what does it really add to say that the next thing will specifically be a survey?

I went ahead and made the indentation change locally (4 pixels as that seemed consistent with other dialogs on my system).
Yeah, OK, sold. Keep the shorter text. :)
Attachment #209479 - Flags: review?(benjamin) → review+
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #208067 - Flags: approval1.8.0.2?
Comment on attachment 209479 [details] [diff] [review]
v2 patch

The patch for this bug is broken into two pieces.  We'd need both pieces for a FF 1.5.0.2 release.
Attachment #209479 - Flags: approval1.8.0.2?
Depends on: 325042
If you really want feedback, how about doing what google toolbar does?  (Uninstaller uninstalls the app immediately.  You are then sent to an feedback form on google's site that asks something similar to: "What is the primary reason you uninstalled?", with a few choices, a textbox, and a submit button.  Very simple, guarantees a much higher rate of feedback, and the user can close the browser window if he doesn't want to say anything.
This patch is only needed on the trunk.
Attachment #210235 - Flags: review?(benjamin)
Attachment #210235 - Flags: review?(benjamin) → review+
Attachment #210235 - Attachment description: fix for trunk (use deerpark name) → fix for trunk (use deerpark name) [fixed-on-trunk]
I'm tempted to back this out.

I have no idea how to fix the l10n impact of this.

#if in l10n is evil, I'm tempted to make it verboten. This bug is not fixed until
someone comes up with a way to reliably fix the l10n build process, see bug 325924
Having two different keys sounds like a much more reliable way to go and #ifdef in
the calling site. (Shouldn't there be a better thing than to spread an old code
name all over the place, too?)
((using ah@numerik is completly pointless, filed bug 325997))
Axel: This was done explicitly for en-US only.  We are going to have an entire new installer and uninstaller for FF2, so this patch was just a crutch to help us get additional feedback from the FF 1.5 release.  Benjamin advised me not to try to make any string changes to FF 1.5, so that meant that the only thing I could do is restrict this to en-US.  Also, please note that the current uninstaller that ships with FF 1.5 is not localized.
Actually this is not limited to en-US, it ships in all locales with English strings.
Comment on attachment 210235 [details] [diff] [review]
fix for trunk (use deerpark name) [fixed-on-trunk]

This patch was apparently commited on the trunk, and is burning all l10n boxens.

This is Firefox 3, right? I suggest to remove the #ifdef stuff from the l10n repository, it's dead-to-be code, it's wrong, and currently it just sets a bad example.
This is not ff3 (or even ff2) code, it is targeted at ff1.5.0.2 or 1.5.0.3

#if in l10n is not evil per-se, as long as we use it correctly. The ifdef hacks in place are merely there to make sure that official and unofficial branding both work correctly.
Odd. If this is only for 1.5.0.x, how comes that stuff landed on the trunk?
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=uninstaller.inc&branch=&root=/cvsroot&subdir=mozilla/browser/locales/en-US/installer&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3

And how comes that the tinderboxens pick it up? Right, the files doesn't say
that it must not be localized. Benjamin, you wanted l10n on the trunk, keep it
up.
Please fix the bustages for pl et al who actually added that file, see
http://lxr.mozilla.org/l10n/find?string=uninstaller.inc.

Note, I don't think that #if's have a use case that doesn't break the all
toolchains.
And I certainly won't approve those #ifs in the way they're used here, too.
Product names are trademarks relevant, and the stuff that landed on the trunk
would require QA to verify that string in each locale. I don't see that
happening.
This was landed on trunk because we obviously need a place to test it before landing it on a stable branch. It doesn't matter if the file is localized because the localized version will not be used (see my original comments about this being in English in all builds). It is perfectly fine if you want to make compare-locales ignore the uninstall file for the time being. Having the uninstaller be English-only preserves the status quo on the branch.

Robstrong is working on the new installer architecture for ff2/ff3. That installer will be fully localizable (and will probably be multi-locale).
I have more important things on my toolchain work than fixing abuse of build 
foo and testdrives.

If you make me fix it, I'll back out the #if stuff in l10b and be done with it.
Pike, I appreciate you're busy but there's no reason to threaten me. This is a properly reviewed checkin and is not to be backed out. Removing the (unused) uninstaller.inc from pl and whatever other locales is a better solution that does not break the uninstaller on trunk.
Patch committed to ignore "uninstaller.inc" in compare-locales.pl
Note that I don't consider this fixed.

My current assessement is that I can't keep l10n alive on the trunk. 
I don't see any information on where the l10n part of this stuff is supposed to
go, and thus there is no way for me to help, neither design nor communication 
wise. There aren't comments in the source, I didn't see any bugmail nor posts in
.l10n nor anything.

I will continue to work on reducing the complexity of l10n and the patch that
landed here is a kick in the crotch for that. I'll just forget about this stuff
for now and bother fixing the mess once I have to look at the trunk.
Hey Axel, I really don't have any desire to leave l10n high-and-dry here.  Frankly, I wasn't aware of the l10n processes, and given that I thought I wasn't introducing any regression (based on the fact that the prior uninstaller is not localized), I figured that my change was not something that l10n folks would need to worry about.  I really apologize for causing any problems with the l10n trunk, and I want to work to fix those problems.  Is there a document somewhere that you can point me to that describes the main issues to watch out for when making changes that impact l10n.  For instance, I didn't know that #ifdef's in uninstaller.inc would be considered bad.  I also didn't know that I should be watching a special tinderbox for l10n bustage.  Thanks!
This is the complete patch that would be needed for the 1.8.0 branch to present the exit survey to FF 1.5.0.x users.  It is what I landed on the 1.8 branch, and it contains the fix for bug 307296 as well as a couple minor follow-up fixes attached elsewhere.
Attachment #211301 - Flags: approval1.8.0.2?
fixed1.8.1
Keywords: fixed1.8.1
Comment on attachment 211301 [details] [diff] [review]
branch patch (including fix for bug 307296)

I suggest a- for this, this will turn all our l10n builds non-green.
I'm not fond of disabling our build foo for a release, either.

Darin, there is no document for not-breaking l10n so far, this is likely as other stuff, you find that out by doing it.

Bad thing here is duplicate keys, as any tool but the build process is going to ignore the preprocessor instructions.

I don't see this being ready for 0.2, we should fix trunk and 1.8 branch before landing this for a release.
Axel: What tools are you talking about?  I really don't understand what needs to be done differently here?  Would it help you if I moved uninstaller.inc to a different location in the tree?  Right now, it is not included in ab-CD.xpi, it is not even touched by a make under browser/locales.  Remember that the uninstaller in FF 1.5 is only in english too.  So, please... explain what it is that I need to do to prevent the tinderboxen from freaking out over this change.
I checked in the same compare-localse.pl change on the 1.8 branch w/ r+a=bsmedberg, which is apparently needed to prevent the locale system from bitching about the totally benign uninstaller.inc :-)

I will revise the 1.8.0 branch patch to include that patch.
OK, now this version is ready for 1.5.0.2
Attachment #211301 - Attachment is obsolete: true
Attachment #211307 - Flags: approval1.8.0.2?
Attachment #211301 - Flags: approval1.8.0.2?
Sorry, but the stuff that bsmedberg did is a hack that I only accept as a 
temporary measure.

Moving that file out of en-US is one solution that I can live with, if it's not
supposed to be localized, don't put it into locales/en-US.

"Tools" is anything listed (or not) on http://wiki.mozilla.org/L10n:Tools, 
including hacks like converting back and forth to po files and other stuff.
> Sorry, but the stuff that bsmedberg did is a hack that I only accept as a 
> temporary measure.

Yup, he agrees that it is a hack.  Why is that hack not acceptable for the 1.8.0 branch?  Remember, we are getting a new fancy installer+uninstaller for FF2, so there isn't much to be gained from localizing or making _this_ solution all nice and pretty right now.  We just need something that works for FF 1.5.0.2 that doesn't regress anything and is easy to test/verify.  The current solution seems to fit the bill.


> Moving that file out of en-US is one solution that I can live with, if it's 
> not supposed to be localized, don't put it into locales/en-US.

We decided to put this file into locales/en-US in the off chance that we would need to localize it.  Given the current hackish workaround, does it really buy us anything to move the file around at this point?  Again, we're only talking about a hack for FF 1.5.0.2.
I disagree.

I'm sitting here half a world away and I try to live with the state of the 
branches. And I will note my strong opposition to this for each branch this
spreads on to.

The 1.8.0 branch is l10n frozen, and that means that a bonsai query on the
locales/ directories should show no changes, because that is what localizers do.
Adding files at one place and removing them at the other is brittle, and it
failed before, though it doesn't show on the tinderbox that it fails, it still
does. Just see how many locales still have that file on CVS in the above
lxr query.

I know that I should more clearly state what an non-l10n impact patch for frozen
tree looks like (as in, don't change en-US and fall back, but just hard code it
in content). On the other hand, where would I put that?
OK, I will move uninstaller.inc into browser/installer/windows.  I suggest documenting this information on the WIKI.  Send mail to the newsgroups, mailing-lists, blog about it, etc.  Spread the word to all the module owners and folks who review code.
OK, moving uninstaller.inc is covered by bug 327465.
Depends on: 327465
Attachment #208067 - Flags: approval1.8.0.2?
Attachment #209479 - Flags: approval1.8.0.2?
adding flag to put on radar of FF 1.5.0.2 triage team
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 211307 [details] [diff] [review]
revised branch patch (including fix for bug 307296)

What I really want to approve for the 1.8.0 branch is a patch that combines this and the changes in bug 327465. There's no point in checking in and back out on the stable branch.
Attachment #211307 - Flags: approval1.8.0.2? → approval1.8.0.2-
> What I really want to approve for the 1.8.0 branch is a patch that combines
> this and the changes in bug 327465. There's no point in checking in and back
> out on the stable branch.

Bah, ok... it's not like I was going to submit a broken patch ;-)  I will post final bits shortly that include all of the follow-up patches.
For reference, I'm referring to the patches in bug 327039 and bug 327465.
Depends on: 327039
Attachment #211307 - Attachment is obsolete: true
Attachment #212702 - Flags: approval1.8.0.2?
Comment on attachment 212702 [details] [diff] [review]
revised 1.8.0 branch patch (including all regression fixes)

Thanks! Approved for 1.8.0 branch, a=dveditz
Attachment #212702 - Flags: approval1.8.0.2? → approval1.8.0.2+
fixed1.8.0.2

does this bug really need the late-l10n keyword?  what impact does it actually have on l10n?  IMO, none.
Keywords: fixed1.8.0.2
late-l10n can go
Keywords: late-l10n
Whiteboard: [rft-dl]
Depends on: 329237
You need to log in before you can comment on or make changes to this bug.