Synchronize the remaining installer files with the FX versions

RESOLVED FIXED in Thunderbird 55.0

Status

defect
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 55.0
All
Windows

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Posted 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)

Comment 2

2 years ago
Sorry, extremely busy right now, so this will need to wait a bit.

Comment 3

2 years ago
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?
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8833752 - Flags: review?(jorgk)
(Assignee)

Comment 5

2 years ago
Posted 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 6

2 years ago
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+
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
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 10

2 years ago
Comment on attachment 8838744 [details] [diff] [review]
installerSync.patch v2

Let's do it.
Attachment #8838744 - Flags: feedback?(robert.strong.bugs) → review+
(Assignee)

Comment 11

2 years ago
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 13

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
https://hg.mozilla.org/comm-central/rev/6c8ca70fc623f516dcf414226cc3f5dbe5502dd0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
(Assignee)

Updated

9 months ago
Duplicate of this bug: 873039
You need to log in before you can comment on or make changes to this bug.