340167 - Exit survey not appearing since Uninstall wizard was added.

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Tracy Walker [:tracy]]

Seen on Bon Echo Nightly from 20060602

-Uninstall the application From control Panel.
-Follow through the new uninstall wizard. 

no exit survey

Comment 1

[(not reading, please use [e-mail] instead)]

robert is going to give me this bug in order to help get me up to speed on the NSIS installer so that I can give useful reviews and help fix bugs.

he's already pointed out the issue and it should be an easy fix [both trunk and branch] once I get the new installer building.

robert:  thanks for the brain dump about NSIS this afternoon and the info about how to build it.

Comment 2

[(not reading, please use [e-mail] instead)]

update:  I'm following robert's instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=326580#c28 and so far, so good.

it was very easy to get nsis, upx and 7zip.

but I needed to rebuild my tree with --enable-static --disable-shared because otherwise, when I do "cd browser/installer; make installer" it will fail because MOZ_PKG_MANIFEST (or MOZ_PKG_MANIFEST_P) is not defined, which is only defined if BUILD_STATIC_LIBS is defined, see http://lxr.mozilla.org/mozilla1.8/source/browser/installer/Makefile.in#60

one my tree is done rebuilding I can test the one line fix for this (which robert already pointed out to me.)

thanks again to robert for helping me get up to speed on the NSIS installer.

Comment 3

[Robert Strong (they/them - no direct email)]

oops! I should have mentioned -enable-static --disable-shared to you... I ran it to that myself. At the very least we should have an error msg vs. error code when this happens

Comment 4

[(not reading, please use [e-mail] instead)]

> oops! I should have mentioned -enable-static --disable-shared to you... I ran
> it to that myself. At the very least we should have an error msg vs. error code
> when this happens

no worries, I should have known better.  I have added an error the Makefile and I have a fix for this bug, and I'll attach it, but the fix needs work.

I followed your advice, and switched from SURVEY_TEXT to SurveyURL and that fixes it for me.

but here's why I think the fix needs work.  I think we really do want SURVEY_TEXT, because I think the URL is always defined, but we only want the survey if the survey text is localized.

so I'm going to try doing a ${If} ${SURVEY_TEXT} (or something like that) instead of !ifdef SurveyURL

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch for the build issue and for this bug, but better patch coming soon...

Comment 6

[Robert Strong (they/them - no direct email)]

We need this to be app and locale specific which will require the addition of a file into browser/locales/en-US/installer so each app can opt into this on a per locale basis since the survey page may not support a specific locale.

Comment 7

[(not reading, please use [e-mail] instead)]

robert, I think I understand your comment.  here is my understanding and what I propose:

currently SurveyURL is always defined (for browser), and is defined here:

/browser/installer/windows/nsis/defines.nsi.in, line 16 -- !define SurveyURL "https://survey.mozilla.com/1/Mozilla%20Firefox/${AppVersion}%20(${AB_CD})/exit.html"

survey SURVEY_TEXT is also always defined for the en-US local for all products, see

/toolkit/locales/en-US/installer/windows/commonLocale.nsh, line 59 -- LangString SURVEY_TEXT ${LANG_ENGLISH} "&Tell us what you thought of ${BrandShortName}"

So i should create a new file, mozilla/browser/locales/en-US/installer/uninstallLocale.nsi and then do

!include uninstallLocale.nsi

in mozilla/toolkit/mozapps/installer/windows/nsis/installer.nsi

in mozilla/browser/locales/en-US/installer/uninstallLocale.nsi, I'd have:

!define DO_UNINSTALL_SURVEY 1

because for browser and en-US, we can do the survey.

for other uninstallLocale.nsi for other locales and other product (like tbird) we may not have a survey, so we would have something like:

# uncomment the line below if the SurveyURL and SURVEY_TEXT are defined
#!define DO_UNINSTALL_SURVEY 1

does this sound correct to you?

Comment 8

[Robert Strong (they/them - no direct email)]

Sorry it took so long to reply... I headed out the door after my last comment.

I think I see the value in doing it this way in that by having the toolkit locale file contain the survey text it will be localized for all apps that choose to use it as will be done with talkback, etc.

The only problem with this is that I have recently been tasked with making the locale files the same as a properties file. I guess we could do something similar with the DO_UNINSTALL_SURVEY though. I'll work that out later though so don't worry about it.

Since NSIS just uses what is needed when it compiles we don't need separate locale files for install and uninstall so the file name should be generic.

Comment 9

[(not reading, please use [e-mail] instead)]

robert, what do you think of this?

I move the define for SURVEY_TEXT from http://lxr.mozilla.org/mozilla1.8/source/toolkit/locales/en-US/installer/windows/commonLocale.nsh#59 to a new file, something like mozilla/toolkit/locales/en-US/installer/windows/commonLocale.inc
(and switch the proper way of defining the string since .inc and .nsh files appear to do things different.)

but instead of SURVEY_TEXT, I'd define SURVEY_TEXT_SHARED or something, which would be localized once for all apps, since it is in toolkit.

then, in mozilla/browser/locales/en-US/installer/installer.inc (which already exists), before browser en-US, since we can do the survey, I'd do:

#define SURVEY_TEXT SURVEY_TEXT_SHARED

Then leave all the existing code alone ("!ifdef SURVEY_TEXT" / #!ifndef SURVEY_TEXT"), and it would work as expected.  in this case, it only shows up for browser / en-US

sorry if I am making this more complicated than it needs to be, and for requiring so much hand holding on my first NSIS bug.

Comment 10

[(not reading, please use [e-mail] instead)]

just talked to robert about this.

he pointed out that mozilla/browser/locales/en-US/installer/installer.inc is used by the old xpinstaller, and not by the new NSIS installer.

so the solution is to create a new file (like mozilla/browser/locales/en-US/installer/installerLocale.nsi) in which I'll have:

# enable this define if your product and locale has a uninstall survey
# see SURVEY_TEXT and SurveyURL
!define DO_SURVEY 1

the I'll rename the conditional references to SURVEY_TEXT to be DO_SURVEY

patch coming up...

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: uninstall survey checkbox screenshot, patching coming next...

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: installer patch, still thinking about the change to toolkit/mozapps/installer/windows/install_sub.pl though...

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: better patch

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 224494 [details] [diff] [review]
better patch

seeking a= for the branch.  I'll land this on the trunk, too.

Comment 15

[(not reading, please use [e-mail] instead)]

fixed checked into both trunk and branch.

thanks to robert for getting me started on NSIS and for all the help on this bug.

Comment 16

[Mike Schroepfer]

ccing Axel to make sure l10n issues are covered

Comment 17

[Axel Hecht [:Pike]]

The file format is new, undocumented and not announced, and the build changes
don't work for anything but en-US. Bug 340173 shows how to pull files from
the l10n repository, and how to avoid adding new formats to l10n.

Comment 18

[Tracy Walker [:tracy]]

verified with Bon Echo build from 0612. The exit survey check box is in the final screen of the uninstall wizard.

Comment 19

[Hiro]

In Mac OS X, it cannot make a package with --enable-shared(default option).
If there is not a problem, you should be able to make a package with --enable-shared in Mac.

error message:
error you need a "--enable-static --disable-shared" build to create an installer

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> In Mac OS X, it cannot make a package
The Mac OS X installer doesn't have an uninstaller so there is no exit survey option when uninstalling. This is a Win32 only bug.

Comment 21

[Hiro]

(In reply to comment #20)
> (In reply to comment #19)
> > In Mac OS X, it cannot make a package
> The Mac OS X installer doesn't have an uninstaller so there is no exit survey
> option when uninstalling. This is a Win32 only bug.
> 
But when I made a DMG package according to the following pages, an error of comment#19 is displayed and cannot make a DMG package.
(Even "--enable-shared" build was able to make a DMG package so far.)
http://developer.mozilla.org/en/docs/Build_and_Install

  Step:
  cd top of mozilla source directory
  make -C browser/installer

Comment 22

[Hiro]

"--enable-shared" is invalidated unconditionally in the following lines.
why?
(The problem had nothing in "--enable-shared" in Mac so far.)

http://lxr.mozilla.org/mozilla/source/browser/installer/Makefile.in#69

 61 ifdef BUILD_STATIC_LIBS
 62 ifeq (WINNT,$(OS_ARCH))
 63 MOZ_PKG_MANIFEST_P = $(srcdir)/windows/packages-static
 64 else
 65 ifneq (,$(filter-out OS2 Darwin,$(OS_ARCH)))
 66 MOZ_PKG_MANIFEST_P = $(srcdir)/unix/packages-static
 67 endif
 68 endif
 69 else
 70 $(error you need a "--enable-static --disable-shared" build to create an installer)
 71 endif
 72 

Comment 23

[Robert Strong (they/them - no direct email)]

http://www.mozilla.com/support/