Closed Bug 1336804 Opened 4 years ago Closed 4 years ago

Synchronize the remaining installer files with the FX versions

Categories

(Thunderbird :: Installer, defect)

All
Windows
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 2 obsolete files)

Like bug 1335857 for the uninstaller.nsi and the maintenanceservice_installer.nsi we need to synchronize installer.nsi, shared.nsh and updater_append.ini to be more up-to date and make it easier to port changes.
Attached patch installerSync.patch (obsolete) — Splinter Review
There are still a lot of differences because TB registers different things than FX.

I checked also the Registry to be sure it still does the same things as before. As I can say, it does.

Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ca5dfe6c2c8e70a3273476fb2bdf320ef1ff42d4
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8833752 - Flags: review?(jorgk)
Sorry, extremely busy right now, so this will need to wait a bit.
Comment on attachment 8833752 [details] [diff] [review]
installerSync.patch

Review of attachment 8833752 [details] [diff] [review]:
-----------------------------------------------------------------

I've looked this over now. updater_append.ini is trivial, in installer.nsi I don't understand why the Kaspersky stuff is left there when it's removed from shared.nsh.

installer.nsi has too many new sections (green) that make me uncomfortable in signing this off. Also, I think you copied some FF processing that we don't need for TB.

shared.nsh also has too many changes to be comfortable. I'm already ignoring changes where ${RegKey} is used instead of HKLM.

What can we do? If there's no better reviewer, perhaps you can comment a bit yourself on the green additions and where they come from. And please review the hunks you added which only appear to apply to FF.

::: mail/installer/windows/nsis/installer.nsi
@@ +257,1 @@
>    ; Delete two files installed by Kaspersky Anti-Spam extension that are only

This Kaspersky stuff is left untouched.

@@ +427,5 @@
>      ${Else}
>        WriteRegDWORD HKLM "$0" "IconsVisible" 0
>      ${EndIf}
> +  ${ElseIf} ${AtLeastWin8}
> +    ; Set the Start Menu Internet and Registered App HKCU registry keys.

This talks about the Start Menu Internet entry. But that's for FF and not TB.

@@ +1012,5 @@
> +    ${If} $TmpVal == "HKLM"
> +      SetShellVarContext all ; Set SHCTX to all users
> +    ${EndIf}
> +    ; If Firefox isn't the http handler for this user show the option to set
> +    ; Firefox as the default browser.

Where does this come from? And why are we checking that FF is the default handler? People might use Chrome and Thunderbird. They might not even have FF installed.

@@ -1050,5 @@
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Left   "0"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Right  "-1"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Top    "124"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Bottom "145"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" State  "1"

What's happening here?

::: mail/installer/windows/nsis/shared.nsh
@@ -20,5 @@
>  
> -  ; Upgrade the copies of the MAPI DLL's
> -  ${UpgradeMapiDLLs}
> -
> -  ; Delete two files installed by Kaspersky Anti-Spam extension that are only

And this Kaspersky stuff is removed.

@@ +187,5 @@
> +      ${If} "$0" == "$INSTDIR\${FileMainEXE}"
> +        Delete "$SMPROGRAMS\${BrandFullName}.lnk"
> +      ${EndIf}
> +    ${EndIf}
> +  ${EndIf}

Where does the block above come from?

@@ +238,5 @@
> +      ${EndUnless}
> +    ${EndIf}
> +  ${EndUnless}
> +
> +  SetShellVarContext all  ; Set $SMPROGRAMS to All Users

Where does the block above come from?

@@ +486,5 @@
> +  ${EndUnless}
> +!macroend
> +!define FixShellIconHandler "!insertmacro FixShellIconHandler"
> +
> +; Add Software\Mozilla\ registry entries (uses SHCTX).

Where does the block above come from? And why does it talk about FF?
Comment on attachment 8833752 [details] [diff] [review]
installerSync.patch

Review of attachment 8833752 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/installer/windows/nsis/installer.nsi
@@ +257,1 @@
>    ; Delete two files installed by Kaspersky Anti-Spam extension that are only

The Kaspersky stuff in shared.nsh was a duplicate of this one -> removed it from other files as it doesn't need to be in a shared file.

@@ +427,5 @@
>      ${Else}
>        WriteRegDWORD HKLM "$0" "IconsVisible" 0
>      ${EndIf}
> +  ${ElseIf} ${AtLeastWin8}
> +    ; Set the Start Menu Internet and Registered App HKCU registry keys.

It's only the comment which is wrong. I fix this in the next patch.

@@ +430,5 @@
> +  ${ElseIf} ${AtLeastWin8}
> +    ; Set the Start Menu Internet and Registered App HKCU registry keys.
> +    ; If we're upgrading an existing install, replacing the old registry entries
> +    ; (without a path hash) would cause the default browser to be reset.
> +    ReadRegStr $0 HKCU "Software\Clients\Mail\${ClientsRegName}\DefaultIcon" ""

You can see here, it's the path for Mail.

@@ +764,5 @@
> +    EnableWindow $0 0
> +    GetDlgItem $0 $HWNDPARENT 2 ; Cancel button
> +    EnableWindow $0 0
> +    GetDlgItem $0 $HWNDPARENT 3 ; Back button
> +    EnableWindow $0 0

This and the following green and blue parts are to synchronize the Installer dialog pages.

@@ +1012,5 @@
> +    ${If} $TmpVal == "HKLM"
> +      SetShellVarContext all ; Set SHCTX to all users
> +    ${EndIf}
> +    ; If Firefox isn't the http handler for this user show the option to set
> +    ; Firefox as the default browser.

Again a oversight by me to change the comment. The Installer works with variables. So it uses TB stuff and not FX stuff.

I fix the comment in the next patch.

@@ -1050,5 @@
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Left   "0"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Right  "-1"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Top    "124"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Bottom "145"
> -    WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" State  "1"

This moved to line 998 ff.

::: mail/installer/windows/nsis/shared.nsh
@@ +187,5 @@
> +      ${If} "$0" == "$INSTDIR\${FileMainEXE}"
> +        Delete "$SMPROGRAMS\${BrandFullName}.lnk"
> +      ${EndIf}
> +    ${EndIf}
> +  ${EndIf}

This is to register the program in the start menu.

@@ +238,5 @@
> +      ${EndUnless}
> +    ${EndIf}
> +  ${EndUnless}
> +
> +  SetShellVarContext all  ; Set $SMPROGRAMS to All Users

Register the program to sdesktop.

@@ +486,5 @@
> +  ${EndUnless}
> +!macroend
> +!define FixShellIconHandler "!insertmacro FixShellIconHandler"
> +
> +; Add Software\Mozilla\ registry entries (uses SHCTX).

This is unneeded. I remove this.
Attachment #8833752 - Flags: review?(jorgk)
Attached patch installerSync.patch v2 (obsolete) — Splinter Review
This one should be better. I was probably too enthusiastic to synchronize installer.nsi. The prevoius version had no default mailer checkbox.

Try of this version: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ee593f916be31a021fbc7e520e4045dea2926540
Attachment #8833752 - Attachment is obsolete: true
Attachment #8838744 - Flags: review?(jorgk)
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Review of attachment 8838744 [details] [diff] [review]:
-----------------------------------------------------------------

I've looked at this for a while and compared "textual building blocks" ("Textbausteine"). I couldn't see anything wrong apart from the two nits below. However, I'm not really authorised to review this and I don't really feel comfortable. Besides, this has not seen any x64 testing the try C-C only works for x86. Also, this installer covers so many cases, that just running it once and then uninstalling isn't sufficient test coverage really. So what can we do?

::: mail/installer/windows/nsis/installer.nsi
@@ +408,4 @@
>    ${If} $TmpVal == "HKLM"
>      ; Set the Start Menu Internet and Registered App HKLM registry keys.
> +    ; If we're upgrading an existing install, replacing the old registry entries
> +    ; (without a path hash) would cause the default browser to be reset.

Default browser?

@@ +429,5 @@
>      ${EndIf}
> +  ${ElseIf} ${AtLeastWin8}
> +    ; Set the Start Menu Mail and Registered App HKCU registry keys.
> +    ; If we're upgrading an existing install, replacing the old registry entries
> +    ; (without a path hash) would cause the default browser to be reset.

Default browser?
Attachment #8838744 - Flags: review?(jorgk) → feedback+
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Robert, we have the problem nobody of us TB developers knows really the installer language. Like this it's really hard to determin if my changes are correct or not.

If you have some spare time, please could you look at this patch if my changes are correct and I have no error incorporated? Our aim is to synchronize the TB files as much as possible to have it easier to follow future changes in FX.
Attachment #8838744 - Flags: feedback+ → feedback?(robert.strong.bugs)
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Brian, Jim, maybe you have some time to look at my patch if my changes are correct and I have no error incorporated? Our aim is to synchronize the TB files as much as possible to have it easier to follow future changes in FX.
Attachment #8838744 - Flags: feedback?(netzen)
Attachment #8838744 - Flags: feedback?(jmathies)
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Review of attachment 8838744 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok to me, although I've done very little work in tb's installer code. (bbondy no longer works for mozilla.)
Attachment #8838744 - Flags: feedback?(netzen)
Attachment #8838744 - Flags: feedback?(jmathies)
Attachment #8838744 - Flags: feedback+
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Let's do it.
Attachment #8838744 - Flags: feedback?(robert.strong.bugs) → review+
Same patch but updated with bug 1340568 (only the installer.nsi part https://hg.mozilla.org/mozilla-central/rev/3395a077e804#l1.1 ).

There should also all comment fixed to reflect mail ans news and no more internet.
Attachment #8838744 - Attachment is obsolete: true
Attachment #8846064 - Flags: review?(jorgk)
Comment on attachment 8846064 [details] [diff] [review]
installerSync.patch v3

OK, I did a couple of cross-checks re. AddStartMenuSC/AddTaskbarSC. Looks like this is now treated the same in browser and mail installers.

So with the f+ we have, this should be OK. I never claimed to understand it, we'll just have to keep an eye open when (un)installing in the next few months to come.
Attachment #8846064 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/6c8ca70fc623f516dcf414226cc3f5dbe5502dd0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Duplicate of this bug: 873039
You need to log in before you can comment on or make changes to this bug.