The Thunderbird equivalent to Bug 336469. Use the new Vista APIs in the installer for registering Thunderbird's protocols.

Initial installer changes for vista

I know some of this code is going to change when Bug 354005 lands, but I'd like to start getting my vista integration patches out of my tree. Will continue to adjust them down the line. Robert, I was a bit fuzzy on this part which I got from browser: + ${IsVista} $R9 + ${If} $R9 == "true" + SetShellVarContext all ; Set SHCTX to HKLM + ${GetExistingInstallPath} "Software\Mozilla" $R9 + ${If} $R9 != "false" + StrCpy $INSTDIR "$R9" + ${CheckDiskSpace} $R9 + ${If} $R9 == "true" + ${CanWriteToInstallDir} $R9 + ${If} $R9 == "true" + Abort + ${EndIf} + ${EndIf} + ${EndIf} + ${EndIf}

FYI: I've been testing this patch on Vista using VMWare so it has been tested on both win XP and Vista. Also, I'm writing out the vista keys on all versions of Windows, so if you upgrade to vista, the keys will already be there.

Comment on attachment 241874 [details] [diff] [review] updated fix I'm going back to quoting the %1 in the argument strings for the various protocol handlers like I was doing previously.

Comment on attachment 241888 [details] [diff] [review] updated patch using quotes around %1 this patch is obsolete, I'm working on a better way to do this after talking to Rob and Seth.

This patch is now functional for the basic cases on Vista. 1) Using Thunderbird to make itself the default mail or news app works. 2) Using the Vista Defaults UI for being the default app now works. 3) Using the Vista Defaults UI for setting the app to handle specific protocols and document types. 4) PostUpdateWin.js test (thanks Seth) is working. I see the request to allow helper.exe to run get called the first time I run after an update. What's not working: 1) I don't know what to do for feeds yet. 2) MAPI only seems to work if Thunderbird is already running.

Toolkit changes I needed to make to get Thunderbird to play nice with Vista. 1) in common.nsh, the AddHandlerValue macro was only setting DI and SOC values if _ISDDE was true. Thunderbird wants to set DI and SOC for values without picking up DDE. Instead of adding another argument to AddHandlerValue to separate DDE from DI and SOC, set DI if _VALICON is valid and set SOC if _VALOPEN is valid. 2) makensis.mk. uinst.exe is now helper.exe except for SUNBIRD. 3) nsPostUpdateWin.js. The XUL pre-processor doesn't run on this file. We only want to perform checkRegistry if the app is Firefox so check that using nsIXULAppInfo. 4) nsUpdateService.js.in --> PostUpdateWin now works for Thunderbird and Firefox.

Comment on attachment 255408 [details] [diff] [review] trunk toolkit changes >Index: mozapps/installer/windows/nsis/common.nsh ... >@@ -1671,10 +1671,14 @@ > StrCmp $R3 "" 0 +3 ; Only add EditFlags if a value doesn't exist > DeleteRegValue SHCTX "$R4" "EditFlags" > WriteRegDWord SHCTX "$R4" "EditFlags" 0x00000002 >+ >+ StrCmp "$R6" "" +2 0 >+ WriteRegStr SHCTX "$R4\DefaultIcon" "" "$R6" >+ >+ StrCmp "$R5" "" +2 0 >+ WriteRegStr SHCTX "$R4\shell\open\command" "" "$R5" > > StrCmp "$R9" "true" 0 +13 >- WriteRegStr SHCTX "$R4\DefaultIcon" "" "$R6" >- WriteRegStr SHCTX "$R4\shell\open\command" "" "$R5" StrCmp "$R9" "true" 0 +13 will need to change to StrCmp "$R9" "true" 0 +11 >Index: mozapps/update/src/updater/updater.cpp >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v >retrieving revision 1.27 >diff -u -r1.27 updater.cpp >--- mozapps/update/src/updater/updater.cpp 30 Jan 2007 05:14:49 -0000 1.27 >+++ mozapps/update/src/updater/updater.cpp 17 Feb 2007 01:18:04 -0000 >@@ -1336,6 +1336,8 @@ > } > > rv = list.Prepare(); >+ if (rv == UNEXPECTED_ERROR) >+ return OK; > if (rv) > return rv; Did you mean to include this change? If so, I'd prefer if it were in another bug. Without that change r=me

Rob, I didn't meant to include the change to updater.cpp :) New patch addressing Rob's review nits. Carrying forward his r, asking seth for the sr.

I'm far enough along now where I'm going to want to engage nightly trunk testers to help iron out any remaining issues. So far the basics are working for setting thunderbird as the default mail, and separately as the default news client. I'm also able to individually register thunderbird as the default handler for the various protocols and .eml documents. Rob, would it be easier to review this if I listed all of the keys the installer writes to the registry? Comments: 1) This patch uses long paths and the long path is quoted when included with command line arguments. 2) We register as two applications: Thunderbird and Thunderbird (News) 3) Feed urls aren't being handled yet (vista only). 4) Mapi is only working when thunderbird is already running (vista only)

A couple of quickies This doesn't take care of news defaults when an install of Thunderbird is already the default news client... possibly others as well. Looks like ShortPathNameToExe is no longer used.

Rob, this is the same toolkit patch you've already reviewed + your implementation of GetPathFromString.

Comment on attachment 255532 [details] [diff] [review] trunk installer changes After reviewing this code with Rob, I have a new patch coming up.

Rob, this updated trunk installer patch addresses the following issues we identified earlier today: 1) In FixClassKeys use the new GetFullPathName macro to help determine if thunderbird.exe is in the path for Software\Classes\mailto and if it is, call AddHandlerValues to reset the path with that of our current install location. Repeat for news, snews, nntp. 2) In FixClassKeys, look for the bogus DI and SOC keys for Software\classes\.eml and if the values contain thunderbird.exe, delete them. 3) Use a short path for the mapi dll path.

Comment on attachment 255855 [details] [diff] [review] updated trunk toolkit patch >Index: mozapps/installer/windows/nsis/common.nsh >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v >retrieving revision 1.13 >diff -u -w -r1.13 common.nsh >--- mozapps/installer/windows/nsis/common.nsh 13 Feb 2007 23:51:38 -0000 1.13 >+++ mozapps/installer/windows/nsis/common.nsh 21 Feb 2007 02:33:52 -0000 >... > /** >+* Returns the path found within a passed in string. The path is quoted or not The path s/is/can be/ quoted or not with that r=me

Comment on attachment 255866 [details] [diff] [review] updated patch Scott, could you also attach a patch without -w ? Thanks

When Rob and I were debugging the MAPI issue yesterday, we got it working when we used a short path for the DLLPath key but since then I haven't been able to repeat that success. So I'm still investigating that part.

Rob, here's a diff without -w. I also added back the long path for the Mapi DLL since the short path didn't seem to fix anything.

Comment on attachment 256103 [details] [diff] [review] patch without -w, using long path for DLLPath doh! this patch also has -w

Comment on attachment 256103 [details] [diff] [review] patch without -w, using long path for DLLPath Everything up through installer.nsi >Index: mail/installer/windows/nsis/installer.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v >retrieving revision 1.8 >diff -u -w -r1.8 installer.nsi >--- mail/installer/windows/nsis/installer.nsi 5 Feb 2007 15:52:43 -0000 1.8 >+++ mail/installer/windows/nsis/installer.nsi 18 Feb 2007 05:19:55 -0000 Var ShortPathNameToExe can be removed. >@@ -101,10 +107,9 @@ > !insertmacro WriteRegDWORD2 > !insertmacro CanWriteToInstallDir > !insertmacro CheckDiskSpace >- >-!include overrides.nsh >-!insertmacro LocateNoDetails >-!insertmacro TextCompareNoDetails >+!insertmacro AddHandlerValues >+!insertmacro DisplayCopyErrMsg >+!include shared.nsh nit: could you add a newline between !insertmacro DisplayCopyErrMsg and !include shared.nsh? It makes it a tad easier to read. > > Name "${BrandFullName}" > OutFile "setup.exe" immediately below this could you change the following? -InstallDir "$PROGRAMFILES\${BrandFullName}" +InstallDir "$PROGRAMFILES\${BrandFullName}\" This will fix bug 370701 for Thunderbird which is a good thing. >@@ -201,9 +207,30 @@ >... >+ ; During an install Vista checks if a new entry is added under the uninstall >+ ; registry key (e.g. ARP). When the same version of the app is installed on >+ ; top of an existing install the key is deleted / added and the Program >+ ; Compatibility Assistant doesn't see this as a new entry and displays an >+ ; error to the user. See Bug 354000. >+ StrCpy $0 "Software\Microsoft\Windows\CurrentVersion\Uninstall\${BrandFullNameInternal} (${AppVersion})" >+ DeleteRegKey HKLM "$0" nit: trailing spaces >@@ -400,150 +427,35 @@ >... >+ ; XXXrstrong - this should be set in shared.nsh along with "Create Quick >+ ; Launch Shortcut" and Create Desktop Shortcut. > StrCpy $0 "Software\Mozilla\${BrandFullNameInternal}\${AppVersion} (${AB_CD})\Uninstall" >- ${WriteRegStr2} $TmpVal "$0" "Uninstall Log Folder" "$INSTDIR\uninstall" 0 >- ${WriteRegStr2} $TmpVal "$0" "Description" "${BrandFullNameInternal} (${AppVersion})" 0 >+ ${WriteRegDWORD2} $TmpVal "$0" "Create Start Menu Shortcut" $AddStartMenuSC 0 nit: trailing space >... >- ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >- ; End of protocol registration >- ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >- >- ; Write the uninstall registry keys >- StrCpy $0 "Software\Microsoft\Windows\CurrentVersion\Uninstall\${BrandFullNameInternal} (${AppVersion})" >- StrCpy $1 "$INSTDIR\uninstall\uninst.exe" >+ WriteRegDWORD HKLM "$0" "IconsVisible" 1 >+ ${Else} >+ WriteRegDWORD HKLM "$0" "IconsVisible" 0 >+ ${EndIf} >+ ${EndIf} > Immediately following this for Firefox and Sunbird is something you may want to add to this patch. This will allow typing thunderbird in start -> run to launch the app and the removal has already been added to uninstall. + ; These need special handling on uninstall since they may be overwritten by + ; an install into a different location. + StrCpy $0 "Software\Microsoft\Windows\CurrentVersion\App Paths\${FileMainEXE}" + ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0 + ${WriteRegStr2} $TmpVal "$0" "Path" "$INSTDIR" 0 >@@ -709,12 +621,16 @@ > > Function CopyFile > StrCpy $R3 $R8 "" $R2 >+ retry: >+ ClearErrors nit: trailing spaces > ${If} $R6 == "" > ${Unless} ${FileExists} "$R1$R3\$R7" > ClearErrors > CreateDirectory "$R1$R3\$R7" > ${If} ${Errors} > ${LogMsg} "** ERROR Creating Directory: $R1$R3\$R7 **" >+ ${DisplayCopyErrMsg} "$R7" >+ GoTo retry nit: trailing spaces > ${Else} > ${LogMsg} "Created Directory: $R1$R3\$R7" > ${EndIf} >@@ -725,18 +641,25 @@ > CreateDirectory "$R1$R3" > ${If} ${Errors} > ${LogMsg} "** ERROR Creating Directory: $R1$R3 **" >+ ${DisplayCopyErrMsg} "$R3" >+ GoTo retry nit: trailing spaces > ${Else} > ${LogMsg} "Created Directory: $R1$R3" > ${EndIf} > ${EndUnless} > ${If} ${FileExists} "$R1$R3\$R7" > Delete "$R1$R3\$R7" >+ ${If} ${Errors} >+ ${DisplayCopyErrMsg} "$R7" >+ GoTo retry >+ ${EndIf} nit: trailing spaces > ${EndIf} > ClearErrors > CopyFiles /SILENT $R9 "$R1$R3" > ${If} ${Errors} >- ; XXXrstrong - what should we do if there is an error installing a file? > ${LogMsg} "** ERROR Installing File: $R1$R3\$R7 **" >+ ${DisplayCopyErrMsg} "$R7" >+ GoTo retry > ${Else} > ${LogMsg} "Installed File: $R1$R3\$R7" > ${EndIf} Immediately following this the following should change ; If the file is installed into the installation directory remove the ; installation directory's path from the file path when writing to the ; uninstall.log so it will be a relative path. This allows the same - ; uninst.exe to be used with zip builds if we supply an uninstall.log. + ; helper.exe to be used with zip builds if we supply an uninstall.log. >@@ -1049,13 +972,25 @@ > > ReadINIStr $0 $R1 "Install" "CloseAppNoPrompt" > ${If} $0 == "true" >+ ; Try to close the app if the exe is in use. > ClearErrors > ${If} ${FileExists} "$INSTDIR\${FileMainEXE}" > ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ; If unsuccessful try one more time and if it still fails Quit >+ ${If} ${Errors} >+ ClearErrors >+ ${CloseApp} "false" "" >+ ClearErrors >+ ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ${If} ${Errors} >+ Quit >+ ${EndIf} >+ ${EndIf} > ${EndIf} > ${If} ${Errors} > ClearErrors > ${CloseApp} "false" "" >+ ClearErrors > ${DeleteFile} "$INSTDIR\${FileMainEXE}" > ${EndIf} > ${EndIf} Is this intentionally different than Firefox's version as follows? ReadINIStr $0 $R1 "Install" "CloseAppNoPrompt" ${If} $0 == "true" + ; Try to close the app if the exe is in use. ClearErrors ${If} ${FileExists} "$INSTDIR\${FileMainEXE}" ${DeleteFile} "$INSTDIR\${FileMainEXE}" ${EndIf} ${If} ${Errors} ClearErrors ${CloseApp} "false" "" + ClearErrors ${DeleteFile} "$INSTDIR\${FileMainEXE}" + ; If unsuccessful try one more time and if it still fails Quit + ${If} ${Errors} + ClearErrors + ${CloseApp} "false" "" + ClearErrors + ${DeleteFile} "$INSTDIR\${FileMainEXE}" + ${If} ${Errors} + Quit + ${EndIf} + ${EndIf} ${EndIf} ${EndIf}

Comment on attachment 256103 [details] [diff] [review] patch without -w, using long path for DLLPath A couple of more comments >Index: mail/installer/windows/nsis/shared.nsh >=================================================================== >RCS file: mail/installer/windows/nsis/shared.nsh >diff -N mail/installer/windows/nsis/shared.nsh >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ mail/installer/windows/nsis/shared.nsh 18 Feb 2007 05:19:55 -0000 >@@ -0,0 +1,387 @@ >... >+ >+!macro HideShortcuts >+ ${StrFilter} "${BrandFullNameInternal}" "+" "" "" $0 >+ StrCpy $R1 "Software\Clients\Mail\$0\InstallInfo" You can just use ${BrandFullNameInternal} without uppercasing it... StartMenuInternet uses an uppercase app exe name which is why Firefox does this. >... >+!macro ShowShortcuts >+ ${StrFilter} "${BrandFullNameInternal}" "+" "" "" $0 >+ StrCpy $R1 "Software\Clients\Mail\$0\InstallInfo" Same here >Index: mail/installer/windows/nsis/uninstaller.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v >retrieving revision 1.2 >diff -u -w -r1.2 uninstaller.nsi >--- mail/installer/windows/nsis/uninstaller.nsi 30 Jan 2007 05:14:50 -0000 1.2 >+++ mail/installer/windows/nsis/uninstaller.nsi 18 Feb 2007 05:19:56 -0000 >... >@@ -60,7 +63,12 @@ > !include WordFunc.nsh > !include MUI.nsh > >+!insertmacro GetOptions > !insertmacro GetParameters >+!insertmacro StrFilter Since you don't need to uppercase anything I believe this can be removed. I'll finish the review tomorrow.

Rob, I have the worst habit of attaching old diffs to bugs instead of the fresh diffs I've created for you. This is the 2nd time I've done this to you. I'll attach the actual diff with no -u I took yesterday for you. And I'll start working on the review comments you made last night.

a more recent patch without -w, does not included latest review comments

moving the review request forward to this version that has incorporated all of your great feedback so far. I know you are still reviewing the patch. I also removed an unnecessary call to StrFilter in installer.nsi.

Comment on attachment 256179 [details] [diff] [review] updated diff with Robert's latest review comments installer.nsi - just a few nits regarding uneeded spaces. >Index: installer/windows/nsis/installer.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v >retrieving revision 1.8 >diff -u -r1.8 installer.nsi >--- installer/windows/nsis/installer.nsi 5 Feb 2007 15:52:43 -0000 1.8 >+++ installer/windows/nsis/installer.nsi 23 Feb 2007 17:40:50 -0000 >... >@@ -201,10 +207,31 @@ > ${CloseApp} "true" $(WARN_APP_RUNNING_INSTALL) > ; Try to delete it again to prevent launching the app while we are > ; installing. >- ${DeleteFile} "$INSTDIR\${FileMainEXE}" > ClearErrors >+ ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ${If} ${Errors} >+ ClearErrors >+ ; Try closing the app a second time >+ ${CloseApp} "true" $(WARN_APP_RUNNING_INSTALL) >+ retry: >+ ClearErrors >+ ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ${If} ${Errors} >+ ; Fallback to the FileError_NoIgnore error with retry/cancel options >+ ${DisplayCopyErrMsg} "${FileMainEXE}" >+ GoTo retry >+ ${EndIf} >+ ${EndIf} nit: trailing spaces >@@ -285,7 +312,7 @@ > > ${DeleteFile} "$INSTDIR\install_wizard.log" > ${DeleteFile} "$INSTDIR\install_status.log" >- >+ nit: added spaces >@@ -400,150 +427,40 @@ >... >- ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >- ; Add the News registry keys >- ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >- >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}" >- ${WriteRegStr2} $TmpVal "$0" "" "${BrandFullNameInternal}" 0 >- >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\DefaultIcon" >- StrCpy $1 "$\"$INSTDIR\${FileMainEXE}$\",0" >- ${WriteRegStr2} $TmpVal "$0" "" "$1" 0 >- >- ; shell/open/command >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\shell\open\command" >- ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0 >+ ${SetAppKeys} > nit: added spaces >- ${WriteRegStr2} $TmpVal "$0" "" "$1" 0 >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\protocols\nntp\shell\open\command" >- StrCpy $1 "$INSTDIR\${FileMainEXE} -mail $\"%1$\"" >- ${WriteRegStr2} $TmpVal "$0" "" "$1" 0 >+ ; XXXrstrong - this should be set in shared.nsh along with "Create Quick >+ ; Launch Shortcut" and Create Desktop Shortcut. nit: trailing spaces >+ StrCpy $0 "Software\Mozilla\${BrandFullNameInternal}\${AppVersion} (${AB_CD})\Uninstall" >+ ${WriteRegDWORD2} $TmpVal "$0" "Create Start Menu Shortcut" $AddStartMenuSC 0 > nit: additional spaces >- ; protocols/nntp >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\protocols\snews" >- ${WriteRegStr2} $TmpVal "$0" "" "URL:Snews Protocol" 0 >- ${WriteRegStr2} $TmpVal "$0" "URL Protocol" "" 0 >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\protocols\snews\DefaultIcon" >- StrCpy $1 "$\"$INSTDIR\${FileMainEXE}$\",0" >- ${WriteRegStr2} $TmpVal "$0" "" "$1" 0 >- StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}\protocols\snews\shell\open\command" >- StrCpy $1 "$INSTDIR\${FileMainEXE} -mail $\"%1$\"" >- ${WriteRegStr2} $TmpVal "$0" "" "$1" 0 >+ ${FixClassKeys} > nit: additional spaces >... >+ ; These need special handling on uninstall since they may be overwritten by >+ ; an install into a different location. >+ StrCpy $0 "Software\Microsoft\Windows\CurrentVersion\App Paths\${FileMainEXE}" >+ ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0 >+ ${WriteRegStr2} $TmpVal "$0" "Path" "$INSTDIR" 0 >+ nit: additional spaces > !insertmacro MUI_STARTMENU_WRITE_BEGIN Application > > ; Create Start Menu shortcuts >@@ -568,8 +485,8 @@ > ${LogUninstall} "File: $DESKTOP\${BrandFullName}.lnk" > ${EndIf} > >- !insertmacro MUI_STARTMENU_WRITE_END >- >+ !insertmacro MUI_STARTMENU_WRITE_END nit: trailing spaces >+ nit: additional spaces

Comment on attachment 256179 [details] [diff] [review] updated diff with Robert's latest review comments This is mostly nits on the adding of spaces >Index: installer/windows/nsis/shared.nsh >=================================================================== >RCS file: installer/windows/nsis/shared.nsh >diff -N installer/windows/nsis/shared.nsh >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ installer/windows/nsis/shared.nsh 23 Feb 2007 17:40:51 -0000 >@@ -0,0 +1,403 @@ >... >+# Alternatively, the contents of this file may be used under the terms of >+# either the GNU General Public License Version 2 or later (the "GPL"), or >+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+# in which case the provisions of the GPL or the LGPL are applicable insteadw s/insteadw/instead/ >+!macro SetHandlers >+ GetFullPathName $8 "$INSTDIR\${FileMainEXE}" >+ >+ StrCpy $0 "SOFTWARE\Classes" >+ StrCpy $2 "$\"$8$\" -mail $\"%1$\"" >+ StrCpy $3 "$\"$8$\" -compose $\"%1$\"" >+ >+ ; Associate the file handlers with ThunderbirdEML >+ WriteRegStr SHCTX "$0\.eml" "" "ThunderbirdEML" >+ >+ ; An empty string is used for the 5th param because ThunderbirdEML is not a >+ ; protocol handler >+ ${AddHandlerValues} "$0\ThunderbirdEML" "$2" "$8,1" "${AppRegNameMail} Document" "" "" Please add a newline after this to differentiate the file handlers from the protocol handlers nit: trailing spaces >+ ${AddHandlerValues} "$0\Thunderbird.Url.mailto" "$3" "$8,0" "${AppRegNameMail} URL" "true" "" >+ ${AddHandlerValues} "$0\Thunderbird.Url.news" "$2" "$8,0" "${AppRegNameNews} URL" "true" "" >+ >+ ; An empty string is used for the 4th & 5th params because the following >+ ; protocol handlers already have a display name and additional keys required >+ ; for a protocol handler. Display names never need to be set? If any of these aren't already set in Vista it will break the Set Defaults ui provided by Vista. >+ ${AddHandlerValues} "$0\mailto" "$3" "$8,0" "" "" "" >+ ${AddHandlerValues} "$0\news" "$2" "$8,0" "" "" "" >+ ${AddHandlerValues} "$0\nntp" "$2" "$8,0" "" "" "" >+ ${AddHandlerValues} "$0\snews" "$2" "$8,0" "" "" "" >+!macroend >+!define SetHandlers "!insertmacro SetHandlers" >+ >+; XXXrstrong - there are several values that will be overwritten by and >+; overwrite other installs of the same application. >+!macro SetMailClient I first thought this name was to set the app as the default mail client. How about SetClientsMail instead? >+ GetFullPathName $8 "$INSTDIR\${FileMainEXE}" >+ GetFullPathName $7 "$INSTDIR\uninstall\helper.exe" >+ GetFullPathName $6 "$INSTDIR\mozMapi32.dll" nit: trailing spaces >+ >+ StrCpy $0 "Software\Clients\Mail\${BrandFullNameInternal}" >+ ; Remove existing keys so we only have our settings >+ DeleteRegKey HKLM "$0" >+ nit: additional spaces >+ WriteRegStr HKLM "$0" "" "${BrandFullNameInternal}" >+ WriteRegStr HKLM "$0\DefaultIcon" "" "$8,0" >+ WriteRegStr HKLM "$0" "DLLPath" "$6" I believe you should register MapiProxy.dll here and remove the registration from installer.nsi to prevent the registration from pointing to a different installation besides the system wide one. We'll have to come up with a reasonable way to determine what to do on uninstall... perhaps another bug? >+ ; The Reinstall Command is defined at >+ ; http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/programmersguide/shell_adv/registeringapps.asp >+ WriteRegStr HKLM "$0\InstallInfo" "HideIconsCommand" "$\"$7$\" /HideShortcuts" >+ WriteRegStr HKLM "$0\InstallInfo" "ShowIconsCommand" "$\"$7$\" /ShowShortcuts" >+ WriteRegStr HKLM "$0\InstallInfo" "ReinstallCommand" "$\"$7$\" /SetAsDefaultAppGlobal" >+ >+ ; Mail shell/open/command >+ WriteRegStr HKLM "$0\shell\open\command" "" "$\"$8$\" -mail" >+ nit: additional spaces >+ ; options >+ WriteRegStr HKLM "$0\shell\properties" "" "$(CONTEXT_OPTIONS)" >+ WriteRegStr HKLM "$0\shell\properties\command" "" "$\"$8$\" -options" >+ >+ ; safemode >+ WriteRegStr HKLM "$0\shell\safemode" "" "$(CONTEXT_SAFE_MODE)" >+ WriteRegStr HKLM "$0\shell\safemode\command" "" "$\"$8$\" -safe-mode" >+ nit: additional spaces >+ ; Protocols >+ StrCpy $1 "$\"$8$\" -compose $\"%1$\"" >+ ${AddHandlerValues} "$0\Protocols\mailto" "$1" "$8,0" "${AppRegNameMail} URL" "true" "" >+ >+ ; Vista Capabilities registry keys >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationDescription" "$(REG_APP_DESC)" >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationIcon" "$8,0" >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationName" "${AppRegNameMail}" >+ WriteRegStr HKLM "$0\Capabilities\FileAssociations" ".eml" "ThunderbirdEML" >+ WriteRegStr HKLM "$0\Capabilities\StartMenu" "Mail" "${BrandFullNameInternal}" >+ WriteRegStr HKLM "$0\Capabilities\URLAssociations" "mailto" "Thunderbird.Url.mailto" >+ >+ ; Vista Registered Application >+ WriteRegStr HKLM "Software\RegisteredApplications" "${AppRegNameMail}" "$0\Capabilities" >+ nit: additional spaces >+ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >+ ; News >+ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; >+ StrCpy $0 "Software\Clients\News\${BrandFullNameInternal}" >+ ; Remove existing keys so we only have our settings >+ DeleteRegKey HKLM "$0" >+ nit: additional spaces >+ WriteRegStr HKLM "$0" "" "${BrandFullNameInternal}" >+ WriteRegStr HKLM "$0\DefaultIcon" "" "$8,0" >+ WriteRegStr HKLM "$0" "DLLPath" "$6" You might want to have separate InstallInfo for Mail and News so Thunderbird can be set as just the system mail client and not the system news client etc. Perhaps as another bug? >+ ; Mail shell/open/command >+ WriteRegStr HKLM "$0\shell\open\command" "" "$\"$8$\" -mail" >+ >+ ; Vista Capabilities registry keys >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationDescription" "$(REG_APP_DESC)" >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationIcon" "$8,0" >+ WriteRegStr HKLM "$0\Capabilities" "ApplicationName" "${AppRegNameNews}" >+ WriteRegStr HKLM "$0\Capabilities\URLAssociations" "nntp" "Thunderbird.Url.news" >+ WriteRegStr HKLM "$0\Capabilities\URLAssociations" "news" "Thunderbird.Url.news" >+ WriteRegStr HKLM "$0\Capabilities\URLAssociations" "snews" "Thunderbird.Url.news" >+ nit: additional spaces >+ ; Protocols >+ StrCpy $1 "$\"$8$\" -mail $\"%1$\"" >+ ${AddHandlerValues} "$0\Protocols\nntp" "$1" "$8,0" "${AppRegNameNews} URL" "true" "" nit: trailing spaces >+ ${AddHandlerValues} "$0\Protocols\news" "$1" "$8,0" "${AppRegNameNews} URL" "true" "" >+ ${AddHandlerValues} "$0\Protocols\snews" "$1" "$8,0" "${AppRegNameNews} URL" "true" "" >+ >+ ; Vista Registered Application >+ WriteRegStr HKLM "Software\RegisteredApplications" "${AppRegNameNews}" "$0\Capabilities" nit: additional spaces >+!macroend >+!define SetMailClient "!insertmacro SetMailClient" >+ >+!macro FixClassKeys >+ StrCpy $0 "SOFTWARE\Classes" >+ GetFullPathName $8 "$INSTDIR\${FileMainEXE}" >+ nit: additional spaces >+ StrCpy $1 "$\"$8$\" -compose $\"%1$\"" nit: trailing spaces >+ ${AddHandlerValues} "$0\Thunderbird.Url.mailto" "$1" "$8,0" "${AppRegNameMail} URL" "true" "" >+ nit: additional spaces >+ ReadRegStr $2 SHCTX "$0\mailto\shell\open\command" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ ${AddHandlerValues} "$0\mailto" "$1" "$8,0" "" "" "" >+ ${EndUnless} nit: trailing spaces >+ nit: additional spaces >+ StrCpy $1 "$\"$8$\" $\"%1$\"" >+ ${AddHandlerValues} "$0\ThunderbirdEML" "$1" "$8,0" "${AppRegNameMail} Document" "" "" >+ nit: additional spaces >+ StrCpy $1 "$\"$8$\" -mail $\"%1$\"" >+ ${AddHandlerValues} "$0\Thunderbird.Url.news" "$1" "$8,0" "${AppRegNameNews} URL" "true" "" nit: additional spaces >+ nit: additional spaces >+ ReadRegStr $2 SHCTX "$0\news\shell\open\command" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ ${AddHandlerValues} "$0\news" "$1" "$8,0" "" "" "" >+ ${EndUnless} >+ nit: additional spaces >+ ReadRegStr $2 SHCTX "$0\snews\shell\open\command" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ ${AddHandlerValues} "$0\snews" "$1" "$8,0" "" "" "" >+ ${EndUnless} >+ nit: additional spaces >+ ReadRegStr $2 SHCTX "$0\nntp\shell\open\command" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ ${AddHandlerValues} "$0\nntp" "$1" "$8,0" "" "" "" >+ ${EndUnless} >+ nit: additional spaces >+ ; remove DI and SOC from the .eml class if it exists >+ ReadRegStr $2 SHCTX "$0\.eml\shell\open\command" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ DeleteRegKey HKLM "$0\.eml\shell\open\command" >+ ${EndUnless} >+ nit: additional spaces >+ ReadRegStr $2 SHCTX "$0\.eml\DefaultIcon" "" >+ ${GetPathFromString} "$2" $3 >+ GetFullPathName $2 "$3" nit: trailing spaces >+ ClearErrors >+ ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1 >+ ${Unless} ${Errors} >+ DeleteRegKey HKLM "$0\.eml\DefaultIcon" >+ ${EndUnless} nit: trailing spaces >Index: installer/windows/nsis/uninstaller.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v >retrieving revision 1.2 >diff -u -r1.2 uninstaller.nsi >--- installer/windows/nsis/uninstaller.nsi 30 Jan 2007 05:14:50 -0000 1.2 >+++ installer/windows/nsis/uninstaller.nsi 23 Feb 2007 17:40:51 -0000 >... @@ -75,13 +82,21 @@ >... Name "${BrandFullName}" -OutFile "uninst.exe" +OutFile "helper.exe" nit: trailing space >@@ -174,14 +189,16 @@ > ; The Clients\Mail registry key is independent of the default app for the OS > ; settings. The XPInstall base un-installer always removes this key if it is > ; uninstalling the default app and it will always replace the keys when >- ; installing even if there is another install of Firefox that is set as the >+ ; installing even if there is another install of Thunderbird that is set as the > ; default app. Now the key is always updated on install but it is only > ; removed if it refers to this install location. >- ${If} $INSTDIR == $R1 >+ ${If} "$INSTDIR" == "$R1" > ; XXXrstrong - if there is another installation of the same app ideally we > ; would just modify these values. The GetSecondInstallPath macro could be > ; made to provide enough information to do this. > DeleteRegKey HKLM "Software\Clients\Mail\${BrandFullNameInternal}" You should add the following and update the comment to show that this applies to the Clients\News registry key as well. >+ DeleteRegKey HKLM "Software\Clients\News\${BrandFullNameInternal}" >@@ -368,8 +385,14 @@ > ClearErrors > ${un.CloseApp} "true" $(WARN_APP_RUNNING_UNINSTALL) > ; Delete the app exe to prevent launching the app while we are uninstalling. >- ${DeleteFile} "$INSTDIR\${FileMainEXE}" > ClearErrors >+ ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ${If} ${Errors} >+ ClearErrors >+ ${un.CloseApp} "true" $(WARN_APP_RUNNING_UNINSTALL) >+ ClearErrors >+ ${DeleteFile} "$INSTDIR\${FileMainEXE}" >+ ${EndIf} nit: trailing spaces I think this about covers everything but I'll do some testing over the weekend as well.

VS8 loves to insert trailing spaces at the end of lines on me :)

I think I discovered why MAPI wasn't working right on Vista. I found some code I didn't know existed in: http://lxr.mozilla.org/mozilla/source/mailnews/mapi/mapihook/src/Registry.cpp#223 which sets several registry keys when we are made the default mail client. In Vista, we don't have the right privileges to set these keys. Based on that code, I added the following keys to SetClientsMail in shared.nsh: StrCpy $1 "Software\Classes\CLSID\{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}" WriteRegStr HKLM "$1\LocalServer32" "" "$\"$8$\" /MAPIStartup" WriteRegStr HKLM "$1\ProgID" "" "MozillaMapi.1" WriteRegStr HKLM "$1\VersionIndependentProgID" "" "MozillaMAPI" StrCpy $1 "SOFTWARE\Classes" WriteRegStr HKLM "$1\MozillaMapi" "" "Mozilla MAPI" WriteRegStr HKLM "$1\MozillaMapi\CLSID" "" "{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}" WriteRegStr HKLM "$1\MozillaMapi\CurVer" "" "MozillaMapi.1" WriteRegStr HKLM "$1\MozillaMapi.1" "" "Mozilla MAPI" WriteRegStr HKLM "$1\MozillaMapi.1\CLSID" "" "{29F458BE-8866-11D5-A3DD-00B0D0F3BAA7}" And what do you know, MapiProxy.dll started getting loaded and the Mapi calls started working! It looks like we only set these keys when we were made the default app. I think it should be ok to add these keys even if we aren't the default, but need to test around this a bit more.

Comment on attachment 256253 [details] [diff] [review] updated patch based on the latest set of review comments If you are wondering why I am being so nit picky it is because I am using the Firefox files as a baseline for comparison. The only change below that is necessary is the comment update. While there could you fix the nits as well? Thanks and r=me. >Index: installer/windows/nsis/installer.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v >retrieving revision 1.8 >diff -u -r1.8 installer.nsi >--- installer/windows/nsis/installer.nsi 5 Feb 2007 15:52:43 -0000 1.8 >+++ installer/windows/nsis/installer.nsi 24 Feb 2007 05:32:35 -0000 >@@ -400,150 +414,40 @@ >... >+ ; XXXrstrong - this should be set in shared.nsh along with "Create Quick nit: trailing space >Index: installer/windows/nsis/shared.nsh >=================================================================== >RCS file: installer/windows/nsis/shared.nsh >diff -N installer/windows/nsis/shared.nsh >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ installer/windows/nsis/shared.nsh 24 Feb 2007 05:32:35 -0000 >@@ -0,0 +1,418 @@ >... >+ ; MapiProxy.dll can be used by multiple applications but >+ ; is only registered for the last application installed. When the last >+ ; application installed is uninstalled MapiProxy.dll will no longer be >+ ; registered. This comment needs to be updated to something like MapiProxy.dll can exist in multiple installs of the application. Registration occurs as follows with the last action to occur being the one that wins: On install and software update when helper.exe runs with the /PostUpdate argument. On setting the application as the system's default application using Window's "Set program access and defaults". >+ ${LogHeader} "DLL Registration" >+ ClearErrors >+ RegDLL "$INSTDIR\MapiProxy.dll" >+ ${If} ${Errors} >+ ${LogMsg} "** ERROR Registering: $INSTDIR\MapiProxy.dll **" >+ ${Else} >+ ${LogUninstall} "DLLReg: \MapiProxy.dll" >+ ${LogMsg} "Registered: $INSTDIR\MapiProxy.dll" >+ ${EndIf} Uninstall should just check if the current install of MapiProxy.dll is the one that is registered and unregister it if it is. I am working on improving uninstall support to provide something similar for Firefox... I can add this when I finish that up. >Index: installer/windows/nsis/uninstaller.nsi >=================================================================== >RCS file: /cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v >retrieving revision 1.2 >diff -u -r1.2 uninstaller.nsi >--- installer/windows/nsis/uninstaller.nsi 30 Jan 2007 05:14:50 -0000 1.2 >+++ installer/windows/nsis/uninstaller.nsi 24 Feb 2007 05:32:35 -0000 >@@ -170,18 +185,21 @@ > ${GetParentDir} > Pop $R1 > >- ; Only remove the Clients\Mail key if it refers to this install location. >- ; The Clients\Mail registry key is independent of the default app for the OS >- ; settings. The XPInstall base un-installer always removes this key if it is >+ ; Only remove the Clients\Mail and Clients\News key if it refers to this install location. >+ ; The Clients\Mail & Clients\News keys are independent of the default app for the OS nit: greater than 80

Just tried compiling and found !ifndef NO_LOG needs to be added to shared.nsh ; MapiProxy.dll can be used by multiple applications but ; is only registered for the last application installed. When the last ; application installed is uninstalled MapiProxy.dll will no longer be ; registered. !ifndef NO_LOG ${LogHeader} "DLL Registration" !endif ClearErrors RegDLL "$INSTDIR\MapiProxy.dll" !ifndef NO_LOG ${If} ${Errors} ${LogMsg} "** ERROR Registering: $INSTDIR\MapiProxy.dll **" ${Else} ${LogUninstall} "DLLReg: \MapiProxy.dll" ${LogMsg} "Registered: $INSTDIR\MapiProxy.dll" ${EndIf} !endif

updated patch with Rob's latest round of review comments. I'm about to check this into the trunk right now along with the toolkit changes. Thank a lot for your help and the great review Rob!

Fixed on the trunk. I'll start working on a branch port that uses short paths tomorrow. Still need to figure out what to do about feed urls too.

Nit: In uninstaller.nsi: + ; removed if they refers to this install location. refers->refer.

I've found a few articles like this one: http://support.microsoft.com/kb/277015 that suggest that at least some of the keys in Clients\Mail and Clients\News are used on Windows 98. This means I'm going to have to change my branch port to be a bit more agressive about using short paths than I was previously doing. Although interestingly enough that particular article is using long paths for the suggest values for things like: HKEY_LOCAL_MACHINE\Software\Clients\Mail\Outlook Express\shell\open\command Default Value=""C:\PROGRAM FILES\OUTLOOK EXPRESS\MSIMN.EXE\" /mail"

The toolkit changes merged cleanly to the branch and seem to be working fine.

Here's what changed when moving to the branch: * We use a short path for all of the document and protocol handlers we write to Software\Classes in SetHandlers * Short path for DLLPath * InstallInfo keys are long paths * SOC for clients\Mail and Clients\news are short paths * DefaultIcon for Clients\Mail and Clients\News is a long path (need to make sure default icon is not used by win98) * shell commands for options and safe-mode use short paths * Vista Capabilities keys use long paths * FixClassKeys uses a short paths for the protocol and document keys

(In reply to comment #37) > I've found a few articles like this one: > http://support.microsoft.com/kb/277015 > > that suggest that at least some of the keys in Clients\Mail and Clients\News > are used on Windows 98. This means I'm going to have to change my branch port > to be a bit more agressive about using short paths than I was previously doing. > Although interestingly enough that particular article is using long paths for > the suggest values for things like: > > > HKEY_LOCAL_MACHINE\Software\Clients\Mail\Outlook Express\shell\open\command > Default Value=""C:\PROGRAM FILES\OUTLOOK EXPRESS\MSIMN.EXE\" /mail" The concern with Win98 is nothing more than concern vs. actual known problems.

Comment on attachment 256850 [details] [diff] [review] port mail installer changes to the branch we're going to go with long paths on the branch.

Rob, this patch ported cleanly to the branch + a couple white space changes I checked into the trunk after the initial landing. Do you want to put another r stamp on this for the branch?

Comment on attachment 256839 [details] [diff] [review] port toolkit changes to the branch Triage team, we need this patch to land vista support for Thunderbird. It's been on the trunk for almost a week now and rob strong has reviewed the code. Thanks!

Comment on attachment 256839 [details] [diff] [review] port toolkit changes to the branch approved for 1.8 branch, a=dveditz

These patches have now landed on the branch and so far are working well using a tinderbox build for me. The changes will be available to branch testers on the nightly build channel tomorrow! Thanks again Rob.

Tested by installing thunderbird onto an unpriviledged user account in Vista and all defaults were set appropriately. The settings responded to changes from the Vista Default Programs UI as well.

verified 1.8.1.4 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/20070510 Thunderbird/2.0.0.4pre ID:2007051003 and tested the OS Integration. Works fine and the settings in Thunderbird are applied - confirmed also by comment #47 - adding verified keyword