1659298 - Get rid of distribution extensions in favor of application global

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, enhancement)

Description

[Dmitry Butskoy (:buc)]

Currently Lightning, Chatzilla and Inspector are provided as distribution extensions.

They placed in the special "distribution/extensions" directory, which is checked by SeaMonkey and addons from it are copied into the user profile.

It is kinda traditional way for SM and Fx bundled addons, but it has its drawbacks:

• An extra copying into the user profile (ie. two or more instances of the same addon in the filesystem).;
• Too hard to restore an addon if the user (occasionally) remove it (either from UI, or directly in filesystem). It should check extensions.installedDistroAddon.* branch in about:config, find and reset a needed one.
• Disabled state of the addon is not preserved when a new version is copied (it always becomes enabled).

A preliminary testing shows that all the issues gone when the addons are placed in the standard "app-global" location (ie. the same way as for themes).
In the case of Linux for example, it is just moving from /usr/lib64/seamonkey/distribution/extensions to app-global's /usr/lib64/seamonkey/extensions.

Since all the addons mentioned is going to be integrated anyway in the future, such a moving looks perfectly acceptable.

Comment 1

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.3-clean-distroaddons.patch

When new version of SeaMonkey starts to provide the mentioned addons in the "app-global" location, the copied ones of the previous version still remain in the user profile (and since it has the highest priority, still take effect).

Fortunately, each distribution addon should have the correspond extensions.installedDistroAddon. preference, which means that the addon is not installed by the user (manually), but just was copied automatically from a system location.

The patch adds a proper check for this case at the application's update time, and removes the old copies of such addons from the user profile.
Additonally, it allows user to have a more recent version anyway, as well as preserves the disabled states.

Regardless of the acceptance of the proposed change upstream, the patch will be beneficial enough for downstream (ie. Linux), since it is too easy to place addons "just in another dir" in the distro packages.

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9170212 [details] [diff] [review]
seamonkey-2.53.3-clean-distroaddons.patch

Not so sure about 2.53 but maybe an option for 2.57

Comment 3

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.3-clean-distroaddons-v2.patch

A bit fixed for respecting the previously disabled addon state.

(Ill-considered optimization of unknown code written in an unknown language is the source of all troubles. :) )

Comment 4

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.5-extensions-moz.patch

Move "distibution/extensions" => "extensions", mozilla part of code.

Just 3-line change of the hardcoded pathes in mozilla/config/rules.mk.

Comment 5

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.5-extensions-comm.patch

Move "distibution/extensions" => "extensions", comm part of code.

The proper changes for calendar/lightning and suite/installer .

Test OK on Linux.

Comment 6

[Ian Neal]

Comment on attachment 9189764 [details] [diff] [review]
seamonkey-2.53.5-extensions-moz.patch

>-	$(RM) -r '$(DIST)/bin/distribution$(DIST_SUBDIR:%=/%)/extensions/$(INSTALL_EXTENSION_ID)'
>-	$(NSINSTALL) -D '$(DIST)/bin/distribution$(DIST_SUBDIR:%=/%)/extensions/$(INSTALL_EXTENSION_ID)'
>-	$(call copy_dir,$(FINAL_TARGET),$(DIST)/bin/distribution$(DIST_SUBDIR:%=/%)/extensions/$(INSTALL_EXTENSION_ID))
>+	$(RM) -r '$(DIST)/bin/$(DIST_SUBDIR)/extensions/$(INSTALL_EXTENSION_ID)'
>+	$(NSINSTALL) -D '$(DIST)/bin/$(DIST_SUBDIR)/extensions/$(INSTALL_EXTENSION_ID)'
>+	$(call copy_dir,$(FINAL_TARGET),$(DIST)/bin/$(DIST_SUBDIR)/extensions/$(INSTALL_EXTENSION_ID))
Shouldn't it be $(DIST)/bin$(DIST_SUBDIR:%=/%) as when DIST_SUBDIR is defined it does not have any leading or trailing /

Other than that seems fine.

Comment 7

[Ian Neal]

Comment on attachment 9189765 [details] [diff] [review]
seamonkey-2.53.5-extensions-comm.patch

>+++ seamonkey-2.53.5-OK/suite/installer/package-manifest.in	2020-11-25 02:37:47.369652146 +0300
> [chatzilla]
> #ifdef MOZ_IRC
>-@RESPATH@/distribution/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}.xpi
>+@RESPATH@/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}.xpi
> #ifdef LOCALE_BUILD
>-@RESPATH@/distribution/extensions/langpack-@AB_CD@@chatzilla.mozilla.org.xpi
>+@RESPATH@/extensions/langpack-@AB_CD@@chatzilla.mozilla.org.xpi
> #endif
> #endif
So here the chatzilla langpack is changed to be in extensions rather than distribution/extensions, but I cannot see similar changes elsewhere for the langpack.

Is there reasoning behind this?

Comment 8

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #6)

Shouldn't it be $(DIST)/bin$(DIST_SUBDIR:%=/%) as when DIST_SUBDIR is
defined it does not have any leading or trailing /

In the used form of "$(DIST)/bin/$(DIST_SUBDIR) there is already "/" between "bin" and "$(DIST_SUBDIR)".

Probably it was some kind of inheritance of some old code, or the programmers were not very familiar with GNU make (ie. "distribution$(DIST_SUBDIR:%=/%)" is the same as "distribution/$(DIST_SUBDIR)" )

Comment 9

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #7)

Comment on attachment 9189765 [details] [diff] [review]
seamonkey-2.53.5-extensions-comm.patch

+++ seamonkey-2.53.5-OK/suite/installer/package-manifest.in 2020-11-25 02:37:47.369652146 +0300
[chatzilla]
#ifdef MOZ_IRC
-@RESPATH@/distribution/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}.xpi
+@RESPATH@/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}.xpi
#ifdef LOCALE_BUILD
-@RESPATH@/distribution/extensions/langpack-@AB_CD@@chatzilla.mozilla.org.xpi
+@RESPATH@/extensions/langpack-@AB_CD@@chatzilla.mozilla.org.xpi
#endif
#endif
So here the chatzilla langpack is changed to be in extensions rather than
distribution/extensions, but I cannot see similar changes elsewhere for the
langpack.

It seems it is irrelevant for langpack itself (as well as the other extensions) where it is actually placed. So no changes inside the extensions regarding their new final path.

Anyway, any presence of "distribution/extensions" in the code seem to be handled... :)

Comment 10

[Ian Neal]

(In reply to Dmitry Butskoy from comment #8)

(In reply to Ian Neal from comment #6)

Shouldn't it be $(DIST)/bin$(DIST_SUBDIR:%=/%) as when DIST_SUBDIR is
defined it does not have any leading or trailing /

In the used form of "$(DIST)/bin/$(DIST_SUBDIR) there is already "/" between "bin" and "$(DIST_SUBDIR)".

Probably it was some kind of inheritance of some old code, or the programmers were not very familiar with GNU make (ie. "distribution$(DIST_SUBDIR:%=/%)" is the same as "distribution/$(DIST_SUBDIR)" )

What happens if DIST_SUBDIR is not set?
Don't you end up with /bin//extensions/ or does that happen with the original too?

Comment 11

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #10)

Probably it was some kind of inheritance of some old code, or the programmers were not very familiar with GNU make (ie. "distribution$(DIST_SUBDIR:%=/%)" is the same as "distribution/$(DIST_SUBDIR)" )

What happens if DIST_SUBDIR is not set?
Don't you end up with /bin//extensions/ or does that happen with the original too?

Well, end up with /bin//extensions. But the usage context is "filename arguments", where an extra "/" in "//" spoils nothing (at least on UNIX).

Surely it is a bit pedantic "to avoid an extra /", but whether the "%" wildcard should match the empty string or not, is not well documented to rely on.

Whether such "//" can lead to issues on Win? If so, then it make sence...

Comment 12

[Ian Neal]

Attachment: Combined mozilla patches

[Triage Comment]
Combined the two mozilla patches into a single hg ready patch

Comment 13

[Ian Neal]

Attachment: 1659298-extensions-comm-2537.patch

[Triage Comment]
This builds on the original comm patch by doing some removals for cZ langpack and fixing a bug introduced to installer.nsi by Bug 629037

Comment 14

[Ian Neal]

Attachment: 1659298-extensions-extra-2537.patch

This patch does the changes needed for debugQA and some extra removals in some suite Makefile.in files.

Comment 15

[Ian Neal]

I'm just wondering on whether we should depend on aAppChanged or check and set a pref (especially for 2.57 but maybe needed for 2.53.x too) or either.
Something like extensions.distroAddons.check

Comment 16

[Dmitry Butskoy (:buc)]

Comment on attachment 9194594 [details] [diff] [review]
1659298-extensions-comm-2537.patch

LGTM

Comment 17

[Dmitry Butskoy (:buc)]

Comment on attachment 9194595 [details] [diff] [review]
1659298-extensions-extra-2537.patch

LGTM

Comment 18

[Dmitry Butskoy (:buc)]

Comment on attachment 9194592 [details] [diff] [review]
Combined mozilla patches

LGTM

Comment 19

[Dmitry Butskoy (:buc)]

(In reply to Ian Neal from comment #15)

I'm just wondering on whether we should depend on aAppChanged or check and set a pref (especially for 2.57

Well, the actual check for and removing of the user instances of the bundled addons is suitable when a new version of the app appears only.
It is irrelevant during the life of the same app version. Thus aAppChanged is exact flag for this.

BTW, what in 2.57 might be differ here in general?

but maybe needed for 2.53.x too) or either.
I hope we'll not forget about 2.53? Just worry about "obsoleted" 2.53 patches without any new ones...

Comment 20

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9194592 [details] [diff] [review]
Combined mozilla patches

I just updated my build. Overall fine but if you create a new profile all add-ons are initially disabled. This needs to be fixed before we can put it into an official build.

It also seems that cZ and DOMi lost their window icon now too under Windows. This is a bit unrelated. Proabaly just need to copy the icons for them as discussed for cZ Linux over irc. Can go into a followup in any case.

Even if only our branches I would like separate parts for calendar and suite. Also see if mail still builds with it. Using it for bisecting errors and parallel testing.

Other than that LGTM f+ for now.

I don't think we want a pref because if someone installs a lower version the pref will not be cleared.

Comment 21

[Dmitry Butskoy (:buc)]

(In reply to Frank-Rainer Grahl (:frg) from comment #20)

Overall fine but if you create a new profile all add-ons are initially disabled.

Appears enabled on a new profile with 2.53 under Linux. Maybe something changed in 2.57 (some default policies etc.)?

BTW, in general it is not clear enough whether it is better to auto-enable or auto-disable additional stuff in an application. At least some Linux distro have a recommendation to cause user to enable needed things explicitly.
IOW, if "initially disabled" is because of some Fx policy, then it seems better to follow it. (We can then change startup page(s) to show "about:addons" at startup times etc.)

Comment 22

[Frank-Rainer Grahl (:frg)]

Appears enabled on a new profile with 2.53 under Linux.
Not in Windows. I didn't test 2.57 but 2.53.7b1pre

Might have something to do with extensions.startupScanScopes or extensions.autoDisableScopes. Under Windows it seems they are marked and disabled as sideloaded now.

BTW, in general it is not clear enough whether it is better to auto-enable or auto-disable additional stuff in an application.
They should be initially enabled and removed or at least permanetly disabled only if the user requests it.

Comment 23

[Ian Neal]

Attachment: 1659298-cz-domi-icons-2537.patch

Do icons for cZ and DOMi in the SeaMonkey branding.

Comment 24

[Dmitry Butskoy (:buc)]

Might have something to do with extensions.startupScanScopes or extensions.autoDisableScopes.

Yes!

At downstream, we have for years:

pref("extensions.autoDisableScopes", 0);

Scope is a bitmask (1,2,4,8). For app-global extensions, the bit 4 must be cleared in extensions.autoDisableScopes .

Something like:

--- comm/suite/app/profile/suite-prefs.js	2020-12-22 19:18:42.000000000 +0300
+++ comm/suite/app/profile/suite-prefs.js-OK	2020-12-24 21:42:00.210465488 +0300
@@ -578,9 +578,9 @@
 
 // Disable add-ons installed into the shared user and shared system areas by
 // default. This does not include the application directory. See the SCOPE
 // constants in AddonManager.jsm for values to use here
-pref("extensions.autoDisableScopes", 15);
+pref("extensions.autoDisableScopes", 11);
 
 // Enable add-ons installed and owned by the application, like the default theme.
 pref("extensions.startupScanScopes", 4);
 

And sentence "This does not include the application directory" seems not true...

Comment 25

[Ian Neal]

Attachment: 1659298-cz-domi-icons-v1_1-2537.patch

I think I have updated the .ico file correctly but needs testing on Windows.

Comment 26

[Ian Neal]

Attachment: 1659298-cz-domi-icons-v1_2-2536.patch

Fix up packagemanifest and removedfiles

Comment 27

[Ian Neal]

Attachment: 1659298-extensions-comm-v1_1-2537.patch

Comment 28

[Ian Neal]

Attachment: 1659298-extensions-cal-2537.patch

Split calendar part out of comm patch.

Comment 29

[Ian Neal]

Comment on attachment 9194680 [details] [diff] [review]
1659298-cz-domi-icons-v1_2-2536.patch

Moved to Bug 1684186

Comment 30

[Ian Neal]

Attachment: 1659298-extensions-moz-v1_1-2537.patch

Now deals with orphaned add-ons in the user profile and has a hidden pref so version number doesn't have to be bumped (mainly for testing / debugging).

Comment 31

[Ian Neal]

Attachment: 1659298-extensions-comm-v1_2-2537.patch

Includes buc's suggested change for extensions.autoDisableScopes

Comment 32

[Ian Neal]

Attachment: 1659298-extensions-cal-2537.patch

Updated calendar fixes for TB from frg

Comment 33

[Dmitry Butskoy (:buc)]

Comment on attachment 9194744 [details] [diff] [review]
1659298-extensions-moz-v1_1-2537.patch

Well, since we drop distibution/extensions forever, the removing of the orphaned addons looks reasonable.

Since the new pref is for testing only, whether it is useful to preserve it in the code after that?

Anyway, LGTM

Comment 34

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9194595 [details] [diff] [review]
1659298-extensions-extra-2537.patch

r/a+ for all branches.

Comment 35

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9194744 [details] [diff] [review]
1659298-extensions-moz-v1_1-2537.patch

r/a+ for SeaMonkey 2.53 and 2.57 release branches only.

Overall this Bug needs coverage in the next release notes. You no longer can remove the former distributed add-ons only disable them. Given that we plan to integrate cZ and Calendar, get rid of DOMi in 2.57 and only distribute debugQA with beta versions this is ok to me but will cause some noise.

Some migrations left me with initial disabled add-ons but I was unable to reproduce this. Might be gone with the latest pref patch too.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/498c52f433a9
Get rid of distribution extensions in favor of application global. r=IanN
https://hg.mozilla.org/comm-central/rev/b60bb1593e9a
Get rid of distribution extensions in favor of application global - extra removals and fix debugQA. r=frg

Comment 37

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/ab88fc51d9768febdaafc2a7ea8690cf1ba0e9c6
Get rid of distribution extensions in favor of application global - calendar part. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/ab3a5d64fe559042ccfb5d517754c002f27ab192
Get rid of distribution extensions in favor of application global. r=IanN a=IanN
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/eee79abbae3ffe7fede702d9644bc686d202ff89
Get rid of distribution extensions in favor of application global - extra removals and fix debugQA. r=frg a=frg

https://gitlab.com/seamonkey-project/seamonkey-2.53-mozilla/-/commit/790b242e19ca13cf1ea2a53322afa46be021a4b7
Get rid of distribution extensions in favor of application global - mozilla patch. r=frg a=frg

Target 2.53.7