Closed Bug 1430483 Opened 2 years ago Closed 2 years ago

Remove remaining gtk2 and qt build options from Thunderbird and SeaMonkey

Categories

(MailNews Core :: Build Config, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: frg, Assigned: frg)

Details

Attachments

(3 files, 1 obsolete file)

It is no longer possible to build Thunderbird or SeaMonkey with gtk2 or qt. Only gtk3 is supported with *nix and qt was already removed some time ago.

There are still references for both in the build and make files.
Attached patch 1430483-mozwidget-tb.patch (obsolete) — Splinter Review
Attachment #8942524 - Flags: review?(mozilla)
First reviewer of this patch wins :)
Attachment #8942525 - Flags: review?(iann_bugzilla)
Attachment #8942525 - Flags: review?(ewong)
Comment on attachment 8942525 [details] [diff] [review]
1430483-mozwidget-sm.patch

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

r+ w/ nit fixed.

::: suite/installer/package-manifest.in
@@ +141,5 @@
>  #ifdef MOZ_FFVPX
>  @BINPATH@/@DLL_PREFIX@mozavutil@DLL_SUFFIX@
>  @BINPATH@/@DLL_PREFIX@mozavcodec@DLL_SUFFIX@
>  #endif
> +#ifdef MOZ_GTK

as I understand this, this should retain the MOZ_GTK3.
Attachment #8942525 - Flags: review?(ewong) → review+
> as I understand this, this should retain the MOZ_GTK3.

No longer needed. This was previously set in 
suite/installer/Makefile.in and could now be simplified to only set gtk. Previously there was gtk2 and gtk3 which is no longer the case. I didn't find any other cases where this was used in the installer.

https://dxr.mozilla.org/comm-central/search?q=MOZ_GTK3&redirect=false

> ifneq (,$(filter gtk%,$(MOZ_WIDGET_TOOLKIT)))
> DEFINES += -DMOZ_GTK=1
> ifeq ($(MOZ_WIDGET_TOOLKIT),gtk3)
> DEFINES += -DMOZ_GTK3=1

Fx still retains it for now but it is imho a useless differentiation. gtk is gtk3 now. Lets see if Tom agrees for TB.
Comment on attachment 8942524 [details] [diff] [review]
1430483-mozwidget-tb.patch

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

We should verify whether the bellow comment about drag and drop is still true, or if it is just something that we have carried forward long past when the original issue is fixed upstream.

Other than that, this looks good.

::: mailnews/mailnews.js
@@ +857,5 @@
>  // Work around bug 482811 by disabling slow script warning for chrome scripts on Mac
>  pref("dom.max_chrome_script_run_time", 0);
>  #endif
>  
> +// gtk3 (*nix) lacks transparent/translucent drag support (bug 376238), so we

Is this actually true? It looks like firefox enables this unconditionally now, so I think the bug this is working around has actually been fixed, so we can remove this setting (both here and in `mail/app/profile/all-thunderbird.js`.
Attachment #8942524 - Flags: review?(mozilla) → feedback+
The option is still in the source and might be needed for non compositing windows managers:

https://en.wikipedia.org/wiki/Compositing_window_manager

I also missed that it is duplicated in all-thunderbird.js:

https://dxr.mozilla.org/comm-central/search?q=nglayout.enable_drag_images&redirect=false

I would rather only deal with the comments here and open a followup bug in case we remove the options and it turns out that it needs to stay in and be backed out. Or I split the patch in 2 parts.
rebased patch
Attachment #8942524 - Attachment is obsolete: true
Attachment #8943200 - Flags: review?(mozilla)
Additional patch removing the option from the pref files. This probably needs to be tested first.
Attachment #8943201 - Flags: feedback?(mozilla)
Attachment #8943201 - Flags: feedback?(jorgk)
Comment on attachment 8943201 [details] [diff] [review]
1430483-mozwidget-tb-part2.patch

The comment refers to bug 376238 which was fixed in two changesets:
https://hg.mozilla.org/mozilla-central/rev/4369a4cf2b0a
https://hg.mozilla.org/mozilla-central/rev/a59b394a47aa

So as far as I can see, dragging images on Linux works in some way, so I think we can lose the code as proposed here. The pref is still at that value today:
https://dxr.mozilla.org/mozilla-central/rev/04c0a07b8de21300856ec89b7d118d4be9b86250/modules/libpref/init/all.js#1134
Attachment #8943201 - Flags: feedback?(jorgk) → review+
Comment on attachment 8942525 [details] [diff] [review]
1430483-mozwidget-sm.patch

As per addtional irc comments ewong is ok with removing MOZ_GTK3 and only using MOZ_GTK in the installer package. Patch needs a minor rebase for new MOZ_WAYLAND there which I will do when checking it in.
Attachment #8942525 - Flags: review?(iann_bugzilla)
Comment on attachment 8943201 [details] [diff] [review]
1430483-mozwidget-tb-part2.patch

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

This should land before the previous patch, but otherwise it looks good.
Attachment #8943201 - Flags: feedback?(mozilla) → review+
Attachment #8943200 - Flags: review?(mozilla) → review+
Thanks. Jorg, do you want to land all three?
Flags: needinfo?(jorgk)
Keywords: checkin-needed
Yes, right now.
Flags: needinfo?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b866cf8e5eb4
Remove no longer valid gtk2 build options from SeaMonkey. r=ewong
https://hg.mozilla.org/comm-central/rev/d0d84ec8115f
Remove no longer valid gtk2 and qt build options from Thunderbird. r=tomprince
https://hg.mozilla.org/comm-central/rev/fdc55627daf8
Remove no longer needed gtk pref nglayout.enable_drag_images from Thunderbird. r=tomprince,jorgk
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.