+++ This bug was initially created as a clone of Bug #627699 +++ GTK+ 3.0 and GNOME 3 are approaching and we should get Firefox ready for them. Let's add build config changes to allow compilation of the gtk3 code.

Benjamin, can you check this one please?

Comment on attachment 755914 [details] [diff] [review] configure.in patch Review of attachment 755914 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4535,5 @@ > + USE_FC_FREETYPE=1 > + > + TK_CFLAGS='$(MOZ_GTK3_CFLAGS)' > + TK_LIBS='$(MOZ_GTK3_LIBS)' > + AC_DEFINE(MOZ_WIDGET_GTK,3) You meant MOZ_WIDGET_GTK=3 here, instead of a AC_DEFINE. @@ +4659,5 @@ > fi > > if test "$COMPILE_ENVIRONMENT"; then > + if test "$MOZ_ENABLE_GTK3"; then > + PKG_CHECK_MODULES(MOZ_GTK3, gtk+-3.0 >= $GTK3_VERSION gtk+-unix-print-3.0 glib-2.0 gobject-2.0 $GDK_PACKAGES) trailing whitespaces. @@ +4661,5 @@ > if test "$COMPILE_ENVIRONMENT"; then > + if test "$MOZ_ENABLE_GTK3"; then > + PKG_CHECK_MODULES(MOZ_GTK3, gtk+-3.0 >= $GTK3_VERSION gtk+-unix-print-3.0 glib-2.0 gobject-2.0 $GDK_PACKAGES) > + fi > + if test "$MOZ_ENABLE_GTK2" -o "$MOZ_ENABLE_GTK2_PLUGIN"; then MOZ_ENABLE_GTK2_PLUGIN is defined nowhere @@ +9027,5 @@ > AC_DEFINE(MOZ_REFLOW_PERF) > AC_DEFINE(MOZ_REFLOW_PERF_DSP) > fi > > +if test "$MOZ_ENABLE_GTK2" -o "$MOZ_ENABLE_GTK3" -o "$ACCESSIBILITY"; then Considering the number of times you're checking for MOZ_ENABLE_GTK2 or MOZ_ENABLE_GTK3, you might as well add a MOZ_ENABLE_GTK.

Thanks! Do you mean something like this one?

Comment on attachment 756100 [details] [diff] [review] v2 carry over the r+

Comment on attachment 756100 [details] [diff] [review] v2 >-if test "$ACCESSIBILITY" -a "$MOZ_ENABLE_GTK2" ; then >+if test "$MOZ_ENABLE_GTK" -o "$ACCESSIBILITY"; then Changing -a to -o doesn't look right to me. If you keep the variables in the same order, then it is easier to see the changes.

Okay, will fix that. It's going to try anyway - https://tbpl.mozilla.org/?tree=Try&rev=89cd725f00c7

Karl, can you please check the build changes for widget dir?

Configure.in changes in js directory

Ted, can you please check this one? Thanks!

Comment on attachment 758549 [details] [diff] [review] toolkit build fixes Review of attachment 758549 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/remote/moz.build @@ +15,5 @@ > CPP_SOURCES += [ > 'nsXRemoteService.cpp', > ] > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3'): You could probably just change this to "if CONFIG['MOZ_ENABLE_GTK']". ::: toolkit/crashreporter/client/Makefile.in @@ +48,5 @@ > > LOCAL_INCLUDES += -I$(srcdir) -I$(srcdir)/../google-breakpad/src/common/mac/ > endif > > +ifeq (,$(filter-out gtk2 gtk3,$(MOZ_WIDGET_TOOLKIT))) Same here. ::: toolkit/crashreporter/client/moz.build @@ +8,5 @@ > > if CONFIG['OS_TARGET'] != 'Android': > PROGRAM = 'crashreporter' > # The xpcshell test case here verifies that the CA certificate list > +if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3') and CONFIG['MOZ_PLATFORM_MAEMO']: ...and here

Comment on attachment 758555 [details] [diff] [review] accessible build fixes Review of attachment 758555 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/atk/Makefile.in @@ +24,5 @@ > +endif > +ifdef MOZ_ENABLE_GTK3 > +CFLAGS += $(MOZ_GTK3_CFLAGS) > +CXXFLAGS += $(MOZ_GTK3_CFLAGS) > +endif Would it make more sense to just define a MOZ_GTK_CFLAGS in configure that uses whichever is appropriate? ::: accessible/src/base/Makefile.in @@ +34,5 @@ > +ifdef MOZ_ENABLE_GTK > +CXXFLAGS += $(MOZ_CAIRO_CFLAGS) > +endif > + > +ifeq (,$(filter-out gtk2 gtk3,$(MOZ_WIDGET_TOOLKIT))) Might as well just collapse this with the ifdef MOZ_ENABLE_GTK above, right? ::: accessible/src/generic/Makefile.in @@ +27,5 @@ > -I$(srcdir)/../../../layout/generic \ > -I$(srcdir)/../../../layout/xul/base/src \ > $(NULL) > > +ifeq (,$(filter-out gtk2 gtk3,$(MOZ_WIDGET_TOOLKIT))) Seems like it'd be more readable to replace all of these with ifdef MOZ_ENABLE_GTK.

Comment on attachment 758556 [details] [diff] [review] xulrunner build fixes Review of attachment 758556 [details] [diff] [review]: ----------------------------------------------------------------- ::: xulrunner/app/Makefile.in @@ +106,1 @@ > libs:: As I said on the other patch, ifdef MOZ_WIDGET_GTK would be more readable.

Comment on attachment 758504 [details] [diff] [review] build config patch - widget directory >+ifdef MOZ_ENABLE_GTK2 >+MODULE_NAME = nsWidgetGtk2Module > LIBRARY_NAME = widget_gtk2 >+else >+MODULE_NAME = nsWidgetGtk3Module >+LIBRARY_NAME = widget_gtk3 >+endif >+ > EXPORT_LIBRARY = 1 > IS_COMPONENT = 1 >-MODULE_NAME = nsWidgetGtk2Module This is OK, but I wonder whether it would be simpler to have one or both of LIBRARY_NAME = widget_gtk MODULE_NAME = nsWidgetGTKModule >+ifdef MOZ_ENABLE_GTK2 >+CFLAGS += $(MOZ_GTK2_CFLAGS) >+CXXFLAGS += $(MOZ_GTK2_CFLAGS) >+else >+CFLAGS += $(MOZ_GTK3_CFLAGS) >+CXXFLAGS += $(MOZ_GTK3_CFLAGS) >+endif Can $(TK_CFLAGS) be used instead, please? Then we can have a single line for each of CFLAGS and CXXFLAGS. >+ifneq (,$(filter gtk3,$(MOZ_WIDGET_TOOLKIT))) >+# gtk3 shares includes with gtk2 > LOCAL_INCLUDES += \ >- -I$(srcdir)/../$(MOZ_WIDGET_TOOLKIT) \ >+ -I$(srcdir)/../gtk2 \ >+ $(NULL) >+else >+LOCAL_INCLUDES += \ >+ -I$(srcdir)/../$(MOZ_WIDGET_TOOLKIT) \ >+ $(NULL) >+endif I've been wanting to move widget/gtk2 to widget/gtk, so one thing to consider might be MOZ_WIDGET_TOOLKIT=gtk for both gtk2 and gtk3, but I don't know whether or not that is simpler overall.

Comment on attachment 758535 [details] [diff] [review] xpcom build fixes > ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT))) > CXXFLAGS += $(MOZ_GTK2_CFLAGS) > endif > >+ifneq (,$(filter gtk3,$(MOZ_WIDGET_TOOLKIT))) >+CXXFLAGS += $(MOZ_GTK3_CFLAGS) >+endif >+ Can this be simplified to the following, please: ifdef MOZ_WIDGET_GTK CXXFLAGS += $(TK_CFLAGS) endif in xpcom/base/ and xpcom/components/, assuming that works. (I expect we don't need MOZ_ENABLE_GTK now that we have MOZ_WIDGET_GTK.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/910c2005bea3 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e07d25dbd87

https://hg.mozilla.org/mozilla-central/rev/910c2005bea3 https://hg.mozilla.org/mozilla-central/rev/5e07d25dbd87

An updated patch for check-in, contains the xpcom, toolkit, accessible, xulrunner dirs. Try run: https://tbpl.mozilla.org/?tree=Try&rev=26ff9691f20b

Comment on attachment 760321 [details] [diff] [review] patch for check-in Correction - It contains widget, xpcom, toolkit, accessible, xulrunner dirs.

Try build for this patch: https://tbpl.mozilla.org/?tree=Try&rev=ef94c313fdc1

Comment on attachment 760409 [details] [diff] [review] rest of the tree Try for this build: https://tbpl.mozilla.org/?tree=Try&rev=0fc0e528827b

Comment on attachment 760372 [details] [diff] [review] gfx build fixes >-elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk2': >+elif CONFIG['MOZ_ENABLE_GTK']: I think MOZ_WIDGET_GTK is better, assuming that works, in 3 files.

Comment on attachment 760409 [details] [diff] [review] rest of the tree >-elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gtk2': >+elif CONFIG['MOZ_ENABLE_GTK']: >-ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT))) >+ifdef MOZ_ENABLE_GTK I think MOZ_WIDGET_GTK is better. >--- a/image/decoders/icon/gtk/Makefile.in >+++ b/image/decoders/icon/gtk/Makefile.in >@@ -12,16 +12,18 @@ include $(DEPTH)/config/autoconf.mk > > LIBRARY_NAME = imgicongtk_s > LIBXUL_LIBRARY = 1 > FAIL_ON_WARNINGS = 1 > > ifdef MOZ_ENABLE_GNOMEUI > LOCAL_INCLUDES += $(MOZ_GNOMEUI_CFLAGS) > else >-LOCAL_INCLUDES += $(MOZ_GTK2_CFLAGS) >+ifdef MOZ_ENABLE_GTK >+LOCAL_INCLUDES += $(TK_CFLAGS) >+endif > endif The MOZ_ENABLE_GTK test shouldn't be needed in this file. >-#ifdef HAVE_LIBXSS >+#if defined(HAVE_LIBXSS) && defined(MOZ_WIDGET_GTK2) >- GdkPixbuf* screenshot = gdk_pixbuf_get_from_drawable(NULL, window, NULL, >- 0, 0, 0, 0, >- gdk_screen_width(), >- gdk_screen_height()); >- >+ GdkPixbuf* screenshot = NULL; >+#if defined(MOZ_WIDGET_GTK2) >+ screenshot = gdk_pixbuf_get_from_drawable(NULL, window, NULL, >+ 0, 0, 0, 0, >+ gdk_screen_width(), >+ gdk_screen_height()); >+#endif Please add TODO GTK3 comments.

An updated one. Try run - https://tbpl.mozilla.org/?tree=Try&rev=2efdd30ef96d

Backed out, because it breaks mochitest-4 on Linux, as the try server results show! https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b548b65cf0

Reopening - I need to fix the rest patch.

Comment on attachment 761060 [details] [diff] [review] rest of the tree for check-in It was backed out because of mochitest-4 failure. So resetting the checkin flag.

Fixed the mochitest failure. Key handling test has to be disabled on gtk because gtk overrides some key events.

Comment on attachment 761308 [details] [diff] [review] rest of the tree for check-in, v2 Try for this patch: https://tbpl.mozilla.org/?tree=Try&rev=e07fbae7250c