Closed Bug 547225 Opened 11 years ago Closed 3 years ago

Split the uninstaller and the default browser/registry integration code into separate executables

Categories

(Toolkit :: NSIS Installer, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: khuey, Unassigned)

References

Details

Attachments

(3 obsolete files)

When we start producing MSI installation packages we don't want to ship the NSIS uninstaller with them.  Currently the NSIS installer also includes all the code to handle registry entries for default browser stuff/shell integration, etc.

Splitting that code apart is the first step to not shipping a deadweight NSIS uninstaller in the MSI package.
Robert, Axel,

Would something like the INI file that the crashreporter uses[0] be acceptable for l10n purposes?  If not, what's the best way to go about localizing a native Win32 executable?

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/crashreporter/crashreporter.ini
Yes for the registry / file system strings that need to be localized though there shouldn't be UI displayed... true?
Correct.  There will be no UI at all, only registry and file system strings will need to be localized.
Attached patch Patch (obsolete) — Splinter Review
Attachment #429473 - Flags: review?(robert.bugzilla)
Awesome turnaround on this. I'm not going to have time to review this for a few days but I do have a couple of changes. The exe name should be something other than helper.exe... we compromised on that name since it was serving as both the uninstaller and the exe to set defaults. Not sure what to go with yet but I'll think of something if you don't come up with something first. Instead of putting the files under browser/installer please put them in browser/components/shell/. If possible it would be great if the exe could be built somewhere in toolkit if it could be made generic enough to support all Windows toolkit apps.
(In reply to comment #5)
> Awesome turnaround on this. I'm not going to have time to review this for a few
> days but I do have a couple of changes. The exe name should be something other
> than helper.exe... we compromised on that name since it was serving as both the
> uninstaller and the exe to set defaults. Not sure what to go with yet but I'll
> think of something if you don't come up with something first. Instead of
> putting the files under browser/installer please put them in
> browser/components/shell/. If possible it would be great if the exe could be
> built somewhere in toolkit if it could be made generic enough to support all
> Windows toolkit apps.

That's fine, I'm not going to have time to push forward on anything else for at least a week because of midterms.

I put some of the basic primitive routines in a common file in toolkit/ (which I guess we'll want to move to toolkit/components/shell?) but a lot of the code is browser specific so I'm not sure how much better we can do.  I more or less translated the NSIS code to C++ so there's probably some refactoring we could do to make a few things more generic (FixShellIconHandler and maybe another function or two).

As for the name, how about "setdefault.exe" or "makedefault.exe"?
Actually, that tryserver build was from a slightly earlier version.  New builds will show up at https://build.mozilla.org/tryserver-builds/me@kylehuey.com-try-f6d9313a4f27/ in a bit.
Attachment #429473 - Attachment is obsolete: true
Attachment #429473 - Flags: review?(robert.bugzilla)
Attached file Patch V1.1 (obsolete) —
Same idea as before, changed the location of the files and the name of the executable to setdefault.exe
Attachment #431017 - Flags: review?(robert.bugzilla)
Attachment #431017 - Attachment is obsolete: true
Attachment #431017 - Flags: review?(robert.bugzilla)
Attached patch Patch V1.3 (obsolete) — Splinter Review
Attachment #432599 - Flags: review?(robert.bugzilla)
Kyle, I have a couple of projects to finish in the next couple of weeks. Would you mind waiting on my reviewing this until next month? Jim Mathies might be able to take a look if you'd like it reviewed sooner or I might be able to get to it sooner if so.
(In reply to comment #11)
> Kyle, I have a couple of projects to finish in the next couple of weeks. Would
> you mind waiting on my reviewing this until next month? Jim Mathies might be
> able to take a look if you'd like it reviewed sooner or I might be able to get
> to it sooner if so.

That's not a problem at all.  This doesn't really block anything on my end, and I'm fine with the review waiting for a while.
Attachment #432599 - Attachment is obsolete: true
Attachment #432599 - Flags: review?(robert.bugzilla)
So I've been thinking about this and I'm not sure this is the best path forward.  I think a better path would be to write a small shim executable that passes the call along to either the NSIS helper/uninstaller or calls into the appropriate MSI APIs (detecting at runtime which is appropriate).  All of the shortcuts and registry entries that we want to create can be done with native MSI facilities and the few other things that we do (such as using AppAssocReg) can be recreated through simple custom actions.  Using the native facilities would make supporting rollback, proper uninstall, etc. trivial.

The downside is that we would have to maintain two separate lists of these shortcuts and registry entries (in the NSIS script and in the WiX XML), but that is IMO preferable to maintaining one copy in C++.
Assignee: khuey → nobody
Closing out old bugs; we can refile something like this when/if we figure out how to handle MSI.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.