Once we're happy that the GTK3 port (bug 627699) is sufficiently stable and we're not going back to re-enabling GTK2 we should remove the code that is behind the |MOZ_WIDGET_GTK == 2| checks.

I don't think we'll be ready to remove this soon. I expect Redhat are still using it? And there is good reason for distributions to be using it due to GTK3 not being a stable platform. I consider it a tier 2 port. I don't compile or test my patches against GTK2 but I aim to write code that should work for both GTK 2 and 3.

Yes, the Gtk2 is widely utilized by Enterprise distros like RHEL and SLES so please leave it here.

Red Hat can go along with the gtk3 only Firefox when it comes in next ESR cycle (FF59).

(In reply to Martin Stránský from comment #3) > Red Hat can go along with the gtk3 only Firefox when it comes in next ESR > cycle (FF59). To be clear, Red Hat is shipping 52 ESR, that means starting with 53, we would be safe to remove gtk2 support?

(In reply to Lee Salzman [:lsalzman] from comment #4) > (In reply to Martin Stránský from comment #3) > > Red Hat can go along with the gtk3 only Firefox when it comes in next ESR > > cycle (FF59). > > To be clear, Red Hat is shipping 52 ESR, that means starting with 53, we > would be safe to remove gtk2 support? Yes, we'd definitely need to ship 52 ESR as gtk2, but we can ship 59 ESR as gtk3 only.

Mike, any issues on the Debian/Ubuntu end, or would we be clear there as well?

(In reply to Lee Salzman [:lsalzman] from comment #4) > (In reply to Martin Stránský from comment #3) > > Red Hat can go along with the gtk3 only Firefox when it comes in next ESR > > cycle (FF59). > > To be clear, Red Hat is shipping 52 ESR, that means starting with 53, we > would be safe to remove gtk2 support? And yes, we ship only ESR lines in RHEL so remove that in 53 is fine for us.

And Wolfgang from SUSE?

As far as Debian is concerned, this works.

Or maybe Peter is the right person from Suse.

After discussing with Jeff, we are going to forge ahead with disabling the option to build with GTK2 as a tentative step aimed at generating more feedback, since so far we have failed to generate sufficient negative feedback to derail this. If someone has a problem with the lack of GTK2 build options, this will better allow us to figure that out. Further removal of all the GTK2 cruft can follow once we're sure we wouldn't want to back this out.

Comment on attachment 8823398 [details] [diff] [review] disallow building with gtk2 toolkit Review of attachment 8823398 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/moz.configure @@ +116,5 @@ > # config.guess, which we want to avoid. Even better, we could actually set > # `choices` depending on the target, but that doesn't pan out for the same > # reason. > option('--enable-default-toolkit', nargs=1, > + choices=('cairo-windows', 'cairo-gtk3', Please rewrap the lines. @@ +140,5 @@ > platform_choices = tuple(value) > else: > platform_choices = ('cairo-android',) > else: > + platform_choices = ('cairo-gtk3') You need a comma after 'cairo-gtk3', like the others, otherwise, python doesn't make it a tuple, and it needs to be a tuple for the things below.

v2... Fixed line wrapping and tuple.

(In reply to Martin Stránský from comment #10) > Or maybe Peter is the right person from Suse. SLE can also go with GTK3 for FF 59 ESR (so removal in FF 53 is ok). As for openSUSE, I think it should work out of the box, unless GTK3 newer than 3.16 is required.

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/529160328d96 disallow building with gtk2 toolkit. r=glandium

(In reply to Jonathan Watt [:jwatt] from comment #0) > Once we're happy that the GTK3 port (bug 627699) is sufficiently stable and > we're not going back to re-enabling GTK2 we should remove the code that is > behind the |MOZ_WIDGET_GTK == 2| checks. As bug 627699 is still open, just wondering why this landed?

I guess because no one closed it :) I just did

Even if one wants to keep using Gtk2 downstream bitrot gets in the way: - mozilla-central changeset a5aa102e8f0f crashes: (lldb) bt * thread #1, stop reason = signal SIGSEGV: invalid address (fault address: 0x0) * frame #0: 0x0000000000000000 frame #1: firefox`mozilla::GetBootstrap(char const*) at nsXPCOMGlue.cpp:372 frame #2: firefox`mozilla::GetBootstrap(aXPCOMFile=<unavailable>) at nsXPCOMGlue.cpp:418 frame #3: firefox`InitXPCOMGlue(argv0=<unavailable>) at nsBrowserApp.cpp:246 frame #4: firefox`main(argc=4, argv=0x00007fffffffe1b8, envp=0x00007fffffffe1e0) at nsBrowserApp.cpp:294 frame #5: firefox`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:72 (lldb) f 1 frame #1: firefox`mozilla::GetBootstrap(char const*) at nsXPCOMGlue.cpp:372 369 "XRE_GlibInit"); 370 // Initialize glib enough for G_SLICE to have an effect before it is unset. 371 // unset. -> 372 XRE_GlibInit(); 373 } 374 #endif 375 if (!mHadGSlice) { - mozilla-central changeset 8c45140a2819 broke build: In file included from objdir/widget/gtk/Unified_c_widget_gtk0.c:2: In file included from widget/gtk/gtk2drawing.c:14: widget/gtk/gtkdrawing.h:49:3: error: must use 'struct' tag to refer to type 'MozGtkSize' MozGtkSize& operator+=(const GtkBorder& aBorder) ^ struct widget/gtk/gtkdrawing.h:49:13: error: expected member name or ';' after declaration specifiers MozGtkSize& operator+=(const GtkBorder& aBorder) ^ widget/gtk/gtkdrawing.h:49:3: error: field has incomplete type 'struct MozGtkSize' MozGtkSize& operator+=(const GtkBorder& aBorder) ^ widget/gtk/gtkdrawing.h:45:8: note: definition of 'struct MozGtkSize' is not complete until the closing '}' struct MozGtkSize { ^ widget/gtk/gtkdrawing.h:49:13: error: expected ';' at end of declaration list MozGtkSize& operator+=(const GtkBorder& aBorder) ^ ; widget/gtk/gtkdrawing.h:74:3: error: unknown type name 'bool' bool initialized; ^ widget/gtk/gtkdrawing.h:76:5: error: must use 'struct' tag to refer to type 'MozGtkSize' MozGtkSize scrollbar; ^ struct widget/gtk/gtkdrawing.h:77:5: error: must use 'struct' tag to refer to type 'MozGtkSize' MozGtkSize thumb; ^ struct widget/gtk/gtkdrawing.h:78:5: error: must use 'struct' tag to refer to type 'MozGtkSize' MozGtkSize button; ^ struct In file included from objdir/widget/gtk/Unified_c_widget_gtk0.c:2: widget/gtk/gtk2drawing.c:3167:31: error: unknown type name 'MozGtkScrollbarMetrics' moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics) ^ 9 errors generated.

(In reply to Jan Beich from comment #19) > Even if one wants to keep using Gtk2 downstream bitrot gets in the way: > - mozilla-central changeset 8c45140a2819 broke build: > > In file included from objdir/widget/gtk/Unified_c_widget_gtk0.c:2: > In file included from widget/gtk/gtk2drawing.c:14: > widget/gtk/gtkdrawing.h:49:3: error: must use 'struct' tag to refer to > type 'MozGtkSize' > MozGtkSize& operator+=(const GtkBorder& aBorder) > ^ > struct > widget/gtk/gtkdrawing.h:49:13: error: expected member name or ';' after > declaration specifiers > MozGtkSize& operator+=(const GtkBorder& aBorder) > ^ > widget/gtk/gtkdrawing.h:49:3: error: field has incomplete type 'struct > MozGtkSize' > MozGtkSize& operator+=(const GtkBorder& aBorder) > ^ > widget/gtk/gtkdrawing.h:45:8: note: definition of 'struct MozGtkSize' is > not complete until the closing '}' > struct MozGtkSize { > ^ > widget/gtk/gtkdrawing.h:49:13: error: expected ';' at end of declaration > list > MozGtkSize& operator+=(const GtkBorder& aBorder) > ^ > ; > widget/gtk/gtkdrawing.h:74:3: error: unknown type name 'bool' > bool initialized; > ^ > widget/gtk/gtkdrawing.h:76:5: error: must use 'struct' tag to refer to > type 'MozGtkSize' > MozGtkSize scrollbar; > ^ > struct > widget/gtk/gtkdrawing.h:77:5: error: must use 'struct' tag to refer to > type 'MozGtkSize' > MozGtkSize thumb; > ^ > struct > widget/gtk/gtkdrawing.h:78:5: error: must use 'struct' tag to refer to > type 'MozGtkSize' > MozGtkSize button; > ^ > struct > In file included from objdir/widget/gtk/Unified_c_widget_gtk0.c:2: > widget/gtk/gtk2drawing.c:3167:31: error: unknown type name > 'MozGtkScrollbarMetrics' > moz_gtk_get_scrollbar_metrics(MozGtkScrollbarMetrics *metrics) > ^ > 9 errors generated. The MozGtkSize is used in Gtk3 only, you can simply encapsulate it by: #if (MOZ_WIDGET_GTK == 3) ... #endif and use original MozGtkScrollbarMetrics for Gtk2. I can attach the patch if anyone needs that.

(In reply to Martin Stránský from comment #7) > (In reply to Lee Salzman [:lsalzman] from comment #4) > > (In reply to Martin Stránský from comment #3) > > > Red Hat can go along with the gtk3 only Firefox when it comes in next ESR > > > cycle (FF59). > > > > To be clear, Red Hat is shipping 52 ESR, that means starting with 53, we > > would be safe to remove gtk2 support? > > And yes, we ship only ESR lines in RHEL so remove that in 53 is fine for us. Don't you need it when 52ESR is EOLed and 59 is the ESR?

Trying to keep the GTK2 port going is not recommended because the client-side graphics rendering will give poor performance when natively themed widgets are read back from the server. Perhaps Flatpak may be a way to support GTK3 apps on older systems. I removed the GTK2 code from nsLookAndFeel for bug 1384701 because it was in the way and no longer used AIUI.

We already established the consensus that 52/ESR was the last release we were going to truly support it. Beyond that, we were/are free to just break/remove anything related to GTK2 we wish. I had a giant patch to just remove all the GTK2 code several months back, but in the run up to 57 I have been too busy to finish it.

Any chance you can finish that off now, or do you need someone else to do that?

Too busy right now to finish this. So if someone else can in short order, that would be great. In any case, any GTK2 code remaining in our tree is to be considered dead anyway, and there is no need to fix it when you're working nearby.

I will give it a try. I am removing other codes

I made some progress on that. Just a question, should I remove dom/plugins/test/testplugin/nptest_gtk2.cpp ?

(In reply to Sylvestre Ledru [:sylvestre] from comment #27) > I made some progress on that. > > Just a question, should I remove dom/plugins/test/testplugin/nptest_gtk2.cpp > ? Plugins like Flash can still legitimately be GTK2. It is just our internal widget code that no longer is.

Ok, that means that we should keep the configure detection then.

Sorry Lee if you are not the right reviewer. As you were going to work on the removal, I assumed that you were the right person. I run a try job (before merging all patches), it is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fd114f294a81763471318f9abb6687742051331 (l10n isn't my fault and is tier-2)

Comment on attachment 8938416 [details] Bug 1278282 - Remove gtk2drawing.c & the gtk2 directory https://reviewboard.mozilla.org/r/209112/#review214930 We still need mozgtk to load the GTK2 flash plugin.

> We still need mozgtk to load the GTK2 flash plugin. OK, thanks. Do you know if we have any testing covering that?

Comment on attachment 8938420 [details] Bug 1278282 - Ride along Remove some trailing whitespaces https://reviewboard.mozilla.org/r/209120/#review214942

Comment on attachment 8938421 [details] Bug 1278282 - remove old and commented mozconfig options https://reviewboard.mozilla.org/r/209122/#review214944

Comment on attachment 8938422 [details] Bug 1278282 - Update of the tests to reflect the removal of the gtk2 https://reviewboard.mozilla.org/r/209124/#review214946

Comment on attachment 8938423 [details] Bug 1278282 - Remove some gtk2 options in the configure https://reviewboard.mozilla.org/r/209126/#review214948

Comment on attachment 8938418 [details] Bug 1278282 - Remove the 'MOZ_WIDGET_GTK == 2' defines https://reviewboard.mozilla.org/r/209116/#review214950 I am not sure it is safe to rip out gtk2xtbin here because of the flash plugin situation.

Comment on attachment 8938417 [details] Bug 1278282 - update of the moz.build files to remove gtk2 references https://reviewboard.mozilla.org/r/209114/#review214952

Comment on attachment 8938419 [details] Bug 1278282 - Replace #if (MOZ_WIDGET_GTK == 3) by #ifdef MOZ_WIDGET_GTK https://reviewboard.mozilla.org/r/209118/#review214956

Overall, I am concerned that this series of patches has some stuff in it that could end up breaking GTK2 flash plugin? Can we verify this still works? Also, we have some workarounds for buggy Cairo/XShm (bug 1271100) living in mozgtk, which we would need to find a new home for before mozgtk could be nuked as well, ignoring the flash plugin situation.

(In reply to Sylvestre Ledru [:sylvestre] from comment #41) > > We still need mozgtk to load the GTK2 flash plugin. > > OK, thanks. Do you know if we have any testing covering that? We might have something in dom/plugins/test/testplugin, but how well or if that tests compatibility with flash plugin, and the extent to which mozgtk is required for that, I am not entirely sure. karlt would probably be better there.

To make thing clear, the scope of this bug is to remove everything that would be built with --enable-default-toolkit=cairo-gtk2, but isn't built with --enable-default-toolkit=cairo-gtk3. IOW, nothing should change for --enable-default-toolkit=cairo-gtk3 builds.

Comment on attachment 8938418 [details] Bug 1278282 - Remove the 'MOZ_WIDGET_GTK == 2' defines https://reviewboard.mozilla.org/r/209116/#review214950 The "#if (MOZ_WIDGET_GTK == 2)" code can be stripped even from plugin code. Plugin code is built with MOZ_GTK2_CFLAGS rather than changing MOZ_WIDGET_GTK The nsNPAPIPlugin gtk2xtbin code was for in-process plugins. It was not ported to GTK3 and is not required. Out of process plugins do support this with GTK3, but Flash does not use it. Other plugins are no longer supported and so all gtk2xtbin code can be removed. That is a separate issue from this bug though, and the patches here don't remove it AFAICS.

(In reply to Lee Salzman [:lsalzman] from comment #50) > (In reply to Sylvestre Ledru [:sylvestre] from comment #41) > > > We still need mozgtk to load the GTK2 flash plugin. > > > > OK, thanks. Do you know if we have any testing covering that? > > We might have something in dom/plugins/test/testplugin, but how well or if > that tests compatibility with flash plugin, and the extent to which mozgtk > is required for that, I am not entirely sure. karlt would probably be better > there. Usually bad things happen when GTK2 and GTK3 libs are loaded in the same process. The different versions of libmozgtk.so prevent that. Without different libmozgtk, GTK2 and GTK3 libs should both be loaded with any of the tests using the test plugin, but I guess the test plugin is not using enough of GTK to trigger the problems, which surprises me. https://searchfox.org/mozilla-central/search?q=application%2Fx-test

Comment on attachment 8938417 [details] Bug 1278282 - update of the moz.build files to remove gtk2 references https://reviewboard.mozilla.org/r/209114/#review217284 ::: dom/plugins/ipc/moz.build:134 (Diff revision 2) > CXXFLAGS += CONFIG['TK_CFLAGS'] > -else: > - # Force build against gtk+2 for struct offsets and such. > - CXXFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] > > +CXXFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] This should still be in the else case. ::: dom/plugins/test/testplugin/testplugin.mozbuild:53 (Diff revision 2) > > -if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3'): > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk3': > CXXFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] > - CFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] > OS_LIBS += CONFIG['MOZ_GTK2_LIBS'] > + CFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] it would be better to leave this next to CXXFLAGS like originally. ::: toolkit/moz.configure:149 (Diff revision 2) > > @depends(toolkit) > def toolkit(toolkit): > - if toolkit == 'cairo-gtk2-x11': > - widget_toolkit = 'gtk2' > elif toolkit == 'cairo-gtk3-wayland' : you need to replace elif with if. ::: toolkit/moz.configure:180 (Diff revision 2) > @depends('--without-x', toolkit) > def x11(value, toolkit): > if not value: > die('--without-x is not supported') > > - x11_toolkits = ('gtk2', 'gtk3') > + x11_toolkits = 'gtk3' The tests that follow want this to be a tuple. Either make it a tuple or change the tests. ::: widget/gtkxtbin/moz.build:23 (Diff revision 2) > FINAL_LIBRARY = 'xul' > > DEFINES['_IMPL_GTKXTBIN_API'] = True > > CFLAGS += CONFIG['MOZ_GTK2_CFLAGS'] > + unnecessary change.

Comment on attachment 8938422 [details] Bug 1278282 - Update of the tests to reflect the removal of the gtk2 https://reviewboard.mozilla.org/r/209124/#review217290 ::: dom/plugins/test/mochitest/mochitest.ini (Diff revision 2) > -[test_plugin_scroll_invalidation.html] > -skip-if = toolkit != "gtk2" > -support-files = plugin_scroll_invalidation.html You could remove those files too.

Comment on attachment 8938418 [details] Bug 1278282 - Remove the 'MOZ_WIDGET_GTK == 2' defines https://reviewboard.mozilla.org/r/209116/#review217350 This all looks good to me, thanks very much. I don't think you need to wait for Lee's review also. ::: dom/plugins/base/nsPluginsDirUnix.cpp:91 (Diff revision 2) > > -#if (MOZ_WIDGET_GTK == 2) > Could remove the second blank line here. ::: gfx/thebes/gfxGdkNativeRenderer.h:65 (Diff revision 2) > > + Extra blank line added here. ::: widget/gtk/IMContextWrapper.cpp:154 (Diff revision 2) > NS_GET_A(aColor)); > } > virtual ~GetTextRangeStyleText() {}; > }; > > -const static bool kUseSimpleContextDefault = MOZ_WIDGET_GTK == 2; > + const static bool kUseSimpleContextDefault = false; Indentation. ::: widget/gtk/gtkdrawing.h (Diff revision 2) > - * Retrieves the colormap to use for drawables passed to moz_gtk_widget_paint. > - */ > -GdkColormap* moz_gtk_widget_get_colormap(); > -#endif > - > -/*** Widget drawing ***/ Could keep this comment, but I don't really mind.

Comment on attachment 8938417 [details] Bug 1278282 - update of the moz.build files to remove gtk2 references https://reviewboard.mozilla.org/r/209114/#review217422 ::: toolkit/moz.configure:180 (Diff revision 3) > @depends('--without-x', toolkit) > def x11(value, toolkit): > if not value: > die('--without-x is not supported') > > - x11_toolkits = ('gtk2', 'gtk3') > + x11_toolkits = ('gtk3') >>> a = ('foo') >>> type(a) <type 'str'> >>> a = ('foo',) >>> type(a) <type 'tuple'>

Comment on attachment 8938418 [details] Bug 1278282 - Remove the 'MOZ_WIDGET_GTK == 2' defines https://reviewboard.mozilla.org/r/209116/#review217618

Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0054a15e3c89 update of the moz.build files to remove gtk2 references r=lsalzman https://hg.mozilla.org/integration/autoland/rev/07457a9823d3 Replace #if (MOZ_WIDGET_GTK == 3) by #ifdef MOZ_WIDGET_GTK r=lsalzman https://hg.mozilla.org/integration/autoland/rev/64a1d5998ef0 Ride along Remove some trailing whitespaces r=lsalzman https://hg.mozilla.org/integration/autoland/rev/28340f2e2631 Update of the tests to reflect the removal of the gtk2 r=lsalzman https://hg.mozilla.org/integration/autoland/rev/26fa4e61b9bc Remove the 'MOZ_WIDGET_GTK == 2' defines r=karlt,lsalzman

https://hg.mozilla.org/mozilla-central/rev/0054a15e3c89 https://hg.mozilla.org/mozilla-central/rev/07457a9823d3 https://hg.mozilla.org/mozilla-central/rev/64a1d5998ef0 https://hg.mozilla.org/mozilla-central/rev/28340f2e2631 https://hg.mozilla.org/mozilla-central/rev/26fa4e61b9bc