Closed Bug 1659298 Opened 11 months ago Closed 5 months ago

Get rid of distribution extensions in favor of application global

Categories

(SeaMonkey :: Build Config, enhancement)

enhancement

Tracking

(seamonkey2.53+ fixed, seamonkey2.57esr? affected)

RESOLVED FIXED
seamonkey 2.84
Tracking Status
seamonkey2.53 + fixed
seamonkey2.57esr ? affected

People

(Reporter: buc, Assigned: buc)

Details

(Whiteboard: SM2.53.7)

Attachments

(4 files, 11 obsolete files)

5.65 KB, patch
frg
: review+
Details | Diff | Splinter Review
7.05 KB, patch
frg
: review+
Details | Diff | Splinter Review
38.94 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-release+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review
10.53 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-release+
iann_bugzilla
: approval-comm-esr60+
Details | Diff | Splinter Review

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.

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.

Flags: needinfo?(frgrahl)
Summary: Get rid of distributed extensions in favor of application global → Get rid of distribution extensions in favor of application global
Status: UNCONFIRMED → NEW
Type: defect → enhancement
Ever confirmed: true
Flags: needinfo?(frgrahl)
Version: SeaMonkey 2.53 Branch → Trunk

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

Attachment #9170212 - Flags: feedback?(iann_bugzilla)
Component: General → Build Config

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. :) )

Attachment #9170212 - Attachment is obsolete: true
Attachment #9170212 - Flags: feedback?(iann_bugzilla)
Attachment #9170212 - Flags: feedback?(iann_bugzilla)

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

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

Flags: needinfo?(iann_bugzilla)

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

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

Test OK on Linux.

Attachment #9189764 - Flags: feedback?(iann_bugzilla)
Attachment #9189765 - Flags: feedback?(iann_bugzilla)
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.
Flags: needinfo?(iann_bugzilla)
Attachment #9189764 - Flags: feedback?(iann_bugzilla) → feedback+
Assignee: nobody → dmitry
Status: NEW → ASSIGNED
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?
Flags: needinfo?(dmitry)

(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)" )

(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... :)

Flags: needinfo?(dmitry)
Attachment #9170212 - Flags: feedback?(iann_bugzilla)
Attachment #9175017 - Flags: feedback?(iann_bugzilla)
Flags: needinfo?(iann_bugzilla)

(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?

Flags: needinfo?(iann_bugzilla) → needinfo?(dmitry)

(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...

Flags: needinfo?(dmitry) → needinfo?(iann_bugzilla)
Attached patch Combined mozilla patches (obsolete) — Splinter Review

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

Attachment #9175017 - Attachment is obsolete: true
Attachment #9189764 - Attachment is obsolete: true
Attachment #9175017 - Flags: feedback?(iann_bugzilla)
Flags: needinfo?(iann_bugzilla)
Attachment #9194592 - Flags: review+
Attachment #9194592 - Flags: feedback?(dmitry)
Attachment #9194592 - Flags: approval-comm-release+
Attachment #9194592 - Flags: approval-comm-esr60+

[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

Attachment #9189765 - Attachment is obsolete: true
Attachment #9189765 - Flags: feedback?(iann_bugzilla)
Attachment #9194594 - Flags: review+
Attachment #9194594 - Flags: feedback?(dmitry)
Attachment #9194594 - Flags: approval-comm-release+
Attachment #9194594 - Flags: approval-comm-esr60+

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

Attachment #9194595 - Flags: review?(frgrahl)
Attachment #9194595 - Flags: feedback?(dmitry)
Attachment #9194595 - Flags: approval-comm-release?
Attachment #9194595 - Flags: approval-comm-esr60?

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

Flags: needinfo?(frgrahl)

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

LGTM

Attachment #9194594 - Flags: feedback?(dmitry) → feedback+

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

LGTM

Attachment #9194595 - Flags: feedback?(dmitry) → feedback+

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

LGTM

Attachment #9194592 - Flags: feedback?(dmitry) → feedback+

(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...

Flags: needinfo?(iann_bugzilla)

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.

Flags: needinfo?(frgrahl)
Attachment #9194592 - Flags: review-
Attachment #9194592 - Flags: review+
Attachment #9194592 - Flags: approval-comm-release+
Attachment #9194592 - Flags: approval-comm-esr60+

(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.)

Flags: needinfo?(frgrahl)

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.

Flags: needinfo?(frgrahl)
Attached patch 1659298-cz-domi-icons-2537.patch (obsolete) — Splinter Review

Do icons for cZ and DOMi in the SeaMonkey branding.

Attachment #9194671 - Flags: review?(frgrahl)
Attachment #9194671 - Flags: approval-comm-release?
Attachment #9194671 - Flags: approval-comm-esr60?

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...

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

Attachment #9194671 - Attachment is obsolete: true
Attachment #9194671 - Flags: review?(frgrahl)
Attachment #9194671 - Flags: approval-comm-release?
Attachment #9194671 - Flags: approval-comm-esr60?
Attachment #9194675 - Flags: review?(frgrahl)
Attachment #9194675 - Flags: approval-comm-release?
Attachment #9194675 - Flags: approval-comm-esr60?

Fix up packagemanifest and removedfiles

Attachment #9194675 - Attachment is obsolete: true
Attachment #9194675 - Flags: review?(frgrahl)
Attachment #9194675 - Flags: approval-comm-release?
Attachment #9194675 - Flags: approval-comm-esr60?
Attachment #9194680 - Flags: review?(frgrahl)
Attachment #9194680 - Flags: approval-comm-release?
Attachment #9194680 - Flags: approval-comm-esr60?
Attachment #9194594 - Attachment is obsolete: true
Attachment #9194681 - Flags: review+
Attachment #9194681 - Flags: approval-comm-release+
Attachment #9194681 - Flags: approval-comm-esr60+

Split calendar part out of comm patch.

Attachment #9194683 - Flags: review+
Attachment #9194683 - Flags: approval-comm-release+
Attachment #9194683 - Flags: approval-comm-esr60+

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

Moved to Bug 1684186

Attachment #9194680 - Attachment is obsolete: true
Attachment #9194680 - Flags: review?(frgrahl)
Attachment #9194680 - Flags: approval-comm-release?
Attachment #9194680 - Flags: approval-comm-esr60?

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).

Attachment #9194592 - Attachment is obsolete: true
Flags: needinfo?(iann_bugzilla)
Attachment #9194744 - Flags: review?(frgrahl)
Attachment #9194744 - Flags: feedback?(dmitry)
Attachment #9194744 - Flags: approval-comm-release?
Attachment #9194744 - Flags: approval-comm-esr60?

Includes buc's suggested change for extensions.autoDisableScopes

Attachment #9194681 - Attachment is obsolete: true
Attachment #9194745 - Flags: review+
Attachment #9194745 - Flags: feedback?(frgrahl)
Attachment #9194745 - Flags: approval-comm-release+
Attachment #9194745 - Flags: approval-comm-esr60+

Updated calendar fixes for TB from frg

Attachment #9194683 - Attachment is obsolete: true
Attachment #9194746 - Flags: review+
Attachment #9194746 - Flags: approval-comm-release+
Attachment #9194746 - Flags: approval-comm-esr60+

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

Attachment #9194744 - Flags: feedback?(dmitry) → feedback+

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

r/a+ for all branches.

Attachment #9194595 - Flags: review?(frgrahl)
Attachment #9194595 - Flags: review+
Attachment #9194595 - Flags: approval-comm-release?
Attachment #9194595 - Flags: approval-comm-release+
Attachment #9194595 - Flags: approval-comm-esr60?
Attachment #9194595 - Flags: approval-comm-esr60+

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.

Attachment #9194744 - Flags: review?(frgrahl)
Attachment #9194744 - Flags: review+
Attachment #9194744 - Flags: approval-comm-release?
Attachment #9194744 - Flags: approval-comm-release+
Attachment #9194744 - Flags: approval-comm-esr60?
Attachment #9194744 - Flags: approval-comm-esr60+

Pushed by frgrahl@gmx.net:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Whiteboard: SM2.53.7
Target Milestone: --- → seamonkey 2.84

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

Attachment #9194745 - Flags: feedback?(frgrahl) → feedback+
You need to log in before you can comment on or make changes to this bug.