1354845 - Accessibility Installer rollup ports for c-c apps

Status: RESOLVED FIXED

(MailNews Core :: Build Config, enhancement)

Description

[Frank-Rainer Grahl (:frg)]

At least SeaMonkey and Thunderbird need to port a few recent installer fixes for the a11y Accessibility component. Instantbird is also missing them.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite-WIP.patch

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-mail.patch

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite-WIP.patch

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1354845-Accessibility-mail.patch

Patch looked like mine. Only missing is the AccessibleHandler.dll in package-manifest.in.

Comment 5

[Frank-Rainer Grahl (:frg)]

>> Patch looked like mine. Only missing is the AccessibleHandler.dll in package-manifest.in.

Yes was sloppy.

Yours still has these in:

+@RESPATH@/components/UserAgentOverrides.js
+@RESPATH@/components/UserAgentOverrides.manifest

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1354845-Accessibility-mail.patch

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-IM.patch

Who should check this one?

Comment 8

[Richard Marti (:Paenglab)]

Aaron, please could you check the patches if we can do this or if not, how we could do it?

Comment 9

[Richard Marti (:Paenglab)]

Attachment: 1354845-Accessibility-mail.patch

Patch updated with bugs 1355968 and 1357194.

Bug 1355968 - Ensure AccessibleHandler.dll is registered in PostUpdate
Bug 1357194 - Use MOZ_UPDATE_CHANNEL to distinguish between local/nightly and beta/release builds for AccessibleHandler CLSIDs

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1354845-Accessibility-mail.patch

Updated to tip.

Matt, it seems Aaron has no time to look at this patch. Please could you look, if the patch is correct and should work for TB?

Comment 11

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8882855 [details] [diff] [review]
1354845-Accessibility-mail.patch

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

I can't confidently speak to the "should work for TB" part, because I'm unfortunately almost entirely unfamiliar with TB, but I don't know of anything different from FF that you'd run up against. I am confident that the patch is correctly doing the same things that the referenced FF patches do. You've also correctly left out bug 1358276, since it was reverted in bug 1372357, after bug 1354077 part 2 made it unnecessary.

::: mail/installer/windows/nsis/installer.nsi
@@ +327,5 @@
> +    ${LogUninstall} "DLLReg: \AccessibleHandler.dll"
> +    ${LogMsg} "Registered: $INSTDIR\AccessibleHandler.dll"
> +  ${EndIf}
> +
> +  ClearErrors

nit: This second ClearErrors isn't needed.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: 1354845-Accessibility-mail.patch [landed comment #16]

Thank you Matt for the feedback

(In reply to Matt Howell [:mhowell] from comment #11)
> Comment on attachment 8882855 [details] [diff] [review]
> 1354845-Accessibility-mail.patch
> 
> Review of attachment 8882855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/installer/windows/nsis/installer.nsi
> @@ +327,5 @@
> > +    ${LogUninstall} "DLLReg: \AccessibleHandler.dll"
> > +    ${LogMsg} "Registered: $INSTDIR\AccessibleHandler.dll"
> > +  ${EndIf}
> > +
> > +  ClearErrors
> 
> nit: This second ClearErrors isn't needed.

Fixed.

Jörg, with Matt's feedback it should be easier for you to review this patch. I also made a try run and tested the installer. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c099115b33ab107bff8f9dafadb361bd523ab1c8

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8856168 [details] [diff] [review]
1354845-Accessibility-IM.patch

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

::: im/installer/windows/nsis/installer.nsi
@@ +232,5 @@
> +    ${LogUninstall} "DLLReg: \AccessibleHandler.dll"
> +    ${LogMsg} "Registered: $INSTDIR\AccessibleHandler.dll"
> +  ${EndIf}
> +
> +  ClearErrors

We took this second 'ClearErrors' out on suggestion of Matt in comment #11. Same goes for the suite patch.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8882920 [details] [diff] [review]
1354845-Accessibility-mail.patch [landed comment #16]

I'm the total installer expert (NOT!).
Anyway, this looks straight forward.

Comment 15

[Richard Marti (:Paenglab)]

I set c-n for the mail patch. But leave this bug open for the other patches.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8882920 [details] [diff] [review]
1354845-Accessibility-mail.patch [landed comment #16]

https://hg.mozilla.org/comm-central/rev/873ddce7e8e6f9b9272789154752207cb6556652

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite.patch

Final patch for suite.

enable-official-branding needs to be set for release builds to match Fx and TB. It is only used in the installer for this right now.

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-IM.patch

Instantbird was missing a piece in shared.nsh.

Jorg do you want to review it? Untested but I believe it should work. 

This should all make a mess if 2 products with the same class id are installed but the mess shouldn't be worse as before this bug :)

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-IM.patch

Overlooked the clearerrors comment.

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite.patch

same here: two ClearErrors removed.

Comment 21

[Edmund Wong (:ewong)]

Comment on attachment 8885883 [details] [diff] [review]
1354845-Accessibility-suite.patch

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

aside for the minor nits,  it lgtm.

::: suite/config/mozconfigs/win32/release-l10n
@@ +4,5 @@
>  ac_add_options --with-l10n-base=../../l10n
>  ac_add_options --enable-application=suite
>  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
> +ac_add_options --enable-official-branding
> +

you can remove that extra line in 8.

::: suite/installer/windows/nsis/shared.nsh
@@ +768,5 @@
>    StrCpy $0 "SOFTWARE\Classes"
>    ; Remove support for launching gopher urls from the shell during install or
>    ; update if the DefaultIcon is from seamonkey.exe.
>    ${RegCleanAppHandler} "gopher"
>    

while you're at it, can you remove this extra whitespace?

::: suite/installer/windows/nsis/uninstaller.nsi
@@ +355,5 @@
> +  ReadRegStr $R1 HKCR "CLSID\{0D68D6D0-D93D-4D08-A30D-F00DD1F45B24}\InProcServer32" ""
> +  ${If} "$INSTDIR\AccessibleMarshal.dll" == "$R1"
> +    ${UnregisterDLL} "$INSTDIR\AccessibleMarshal.dll"
> +  ${EndIf}
> + 

extra space nit.

Comment 22

[Ian Neal]

Comment on attachment 8885883 [details] [diff] [review]
1354845-Accessibility-suite.patch

r=me

Comment 23

[Jorg K (CEST = GMT+2)]

Comparing the changes for TB and IM, I'm a bit confused.

TB:
+!if "@MOZ_UPDATE_CHANNEL@" == "default"
+#ifdef DEBUG
+!define AccessibleHandlerCLSID "{398FFD8D-5382-48F7-9E3B-19012762D39A}"
+#else
+!define AccessibleHandlerCLSID "{CE573FAF-7815-4FC2-A031-B092268ACE9E}"
+#endif
+!else if "@MOZ_UPDATE_CHANNEL@" == "nightly"
+!define AccessibleHandlerCLSID "{4629216B-8753-41BF-9527-5BFF51401671}"
+!else if "@MOZ_UPDATE_CHANNEL@" == "beta"
+!define AccessibleHandlerCLSID "{21E9F98D-A6C9-4CB5-B288-AE2FD2A96C58}"
+!else if "@MOZ_UPDATE_CHANNEL@" == "release"
+!define AccessibleHandlerCLSID "{1BAA303D-B4B9-45E5-9CCB-E3FCA3E274B6}"
+!else
+!define AccessibleHandlerCLSID "{4A195748-DCA2-45FB-9295-0A139E76A9E7}"
+!endif
+

IM and SM:
+#ifndef MOZ_OFFICIAL_BRANDING
+#ifdef DEBUG
+!define AccessibleHandlerCLSID "{398FFD8D-5382-48F7-9E3B-19012762D39A}"
+#else
+!define AccessibleHandlerCLSID "{CE573FAF-7815-4FC2-A031-B092268ACE9E}"
+#endif
+#elifdef NIGHTLY_BUILD
+!define AccessibleHandlerCLSID "{4629216B-8753-41BF-9527-5BFF51401671}"
+#elifdef RELEASE_OR_BETA
+!define AccessibleHandlerCLSID "{1BAA303D-B4B9-45E5-9CCB-E3FCA3E274B6}"
+#else
+!define AccessibleHandlerCLSID "{4A195748-DCA2-45FB-9295-0A139E76A9E7}"
+#endif
+

So what's the difference between |!if "@MOZ_UPDATE_CHANNEL@" == "default"| and |#ifndef MOZ_OFFICIAL_BRANDING| and why don't IM and SM put beta and release into a single CLSID?

Comment 24

[Frank-Rainer Grahl (:frg)]

Let me check. Has been awhile that I did the original patches.

Comment 25

[Frank-Rainer Grahl (:frg)]

> So what's the difference between |!if "@MOZ_UPDATE_CHANNEL@" == "default"| and |#ifndef 
> MOZ_OFFICIAL_BRANDING| and why don't IM and SM put beta and release into a single CLSID?

Sorry for the late reply. I based the changes on bug 1354208. Since then this has been changed in bug 1357194. I need to redo the patches. Thanks for chatching it.

Comment 26

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-IM-V2.patch

Same block as TB/SM/Fx now

Comment 27

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite-V2.patch

r+ from IanN retained. Config changes removed as per bug 1381211 comment 2.

Block with the ifdefs aligned with Fx and TB as per Jorgs comment.

Comment 28

[Frank-Rainer Grahl (:frg)]

Attachment: 1354845-Accessibility-suite-whitespace.patch

trailing blanks removed in suite installer as additional nitfix.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/da7456b68187
Add IA2Marshal and AccessibleHandler to SeaMonkey. r=IanN
https://hg.mozilla.org/comm-central/rev/33bf974fa8f8
Remove trailing blanks from SeaMonkey installer file. rs=whitespace-only

Comment 30

[Frank-Rainer Grahl (:frg)]

Done for SeaMonkey (hopefully).

Only IM left.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a5dcb54170ce
Add IA2Marshal and AccessibleHandler to Instantbird. r=jorgk DONTBUILD