Martin Stránský [:stransky] (ni? me) Assignee
	Description • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 1 • 11 years ago

Benjamin, can you check this one please?

	Mike Hommey [:glandium]
	Comment 2 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 3 • 11 years ago

Thanks! Do you mean something like this one?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 4 • 11 years ago

Comment on attachment 756100 [details] [diff] [review]
v2

carry over the r+

	Karl Tomlinson (:karlt)
	Comment 5 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 6 • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 8 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0acd0b60d2

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 10 • 11 years ago

Configure.in changes in js directory

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 13 • 11 years ago

Ted, can you please check this one? Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/3b0acd0b60d2

	(not currently active) Ted Mielczarek
	Comment 17 • 11 years ago

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

	(not currently active) Ted Mielczarek
	Comment 18 • 11 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 19 • 11 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 20 • 11 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 21 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 24 • 11 years ago

An updated patch for check-in, contains the xpcom, toolkit, accessible, xulrunner dirs.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=26ff9691f20b

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 25 • 11 years ago

Comment on attachment 760321 [details] [diff] [review]
patch for check-in

Correction - It contains widget, xpcom, toolkit, accessible, xulrunner dirs.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/623845a50e24

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 27 • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 29 • 11 years ago

Comment on attachment 760409 [details] [diff] [review]
rest of the tree

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

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/623845a50e24

	Karl Tomlinson (:karlt)
	Comment 31 • 11 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 32 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 34 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf635d80ad69

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/149475141471

	(no longer active)
	Comment 37 • 11 years ago

Backed out, because it breaks mochitest-4 on Linux, as the try server results show!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b548b65cf0

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/cf635d80ad69

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 39 • 11 years ago

Reopening - I need to fix the rest patch.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 40 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 41 • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 42 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/acecde2fbfbe

	Ed Morley [:emorley]
	Comment 44 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/acecde2fbfbe