Port GTK2 to GTK3 - build config

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

Trunk
mozilla24
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [check linux try build before requesting checkin])

Attachments

(14 attachments, 2 obsolete attachments)

9.58 KB, patch
glandium
: review+
Details | Diff | Splinter Review
9.39 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
4.65 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.09 KB, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1017 bytes, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
3.41 KB, patch
karlt
: review+
Details | Diff | Splinter Review
10.38 KB, patch
ted
: review+
Details | Diff | Splinter Review
5.05 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.53 KB, patch
ted
: review+
Details | Diff | Splinter Review
24.19 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
6.75 KB, patch
karlt
: review+
Details | Diff | Splinter Review
16.96 KB, patch
karlt
: review+
Details | Diff | Splinter Review
6.75 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
16.96 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
+++ 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?
Attachment #755914 - Flags: review?(benjamin)
Attachment #755914 - Flags: review?(benjamin) → review?(mh+mozilla)
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.
Attachment #755914 - Flags: review?(mh+mozilla) → review+
Posted patch v2 (obsolete) — Splinter Review
Thanks! Do you mean something like this one?
Attachment #756100 - Flags: review?(mh+mozilla)
Comment on attachment 756100 [details] [diff] [review]
v2

carry over the r+
Attachment #756100 - Flags: review?(mh+mozilla) → review+
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
Attachment #756100 - Attachment is obsolete: true
Attachment #758462 - Flags: review+
Attachment #758462 - Flags: checkin?
Karl, can you please check the build changes for widget dir?
Attachment #758504 - Flags: review?(karlt)
Attachment #758462 - Flags: checkin? → checkin+
Configure.in changes in js directory
Attachment #758514 - Flags: review?(karlt)
Attachment #758535 - Flags: review?(benjamin)
Attachment #758535 - Flags: review?(benjamin) → review?(karlt)
Ted, can you please check this one? Thanks!
Attachment #758549 - Flags: review?(ted)
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
Attachment #758549 - Flags: review?(ted) → review+
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.
Attachment #758555 - Flags: review?(ted) → review+
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.
Attachment #758556 - Flags: review?(ted) → review+
Attachment #758514 - Flags: review?(karlt) → review+
Attachment #758516 - Flags: review?(karlt) → review+
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.
Attachment #758504 - Flags: review?(karlt) → review+
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.)
Attachment #758535 - Flags: review?(karlt) → review+
Attachment #758514 - Flags: checkin? → checkin+
Attachment #758516 - Flags: checkin? → checkin+
An updated patch for check-in, contains the xpcom, toolkit, accessible, xulrunner dirs.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=26ff9691f20b
Attachment #760321 - Flags: review+
Comment on attachment 760321 [details] [diff] [review]
patch for check-in

Correction - It contains widget, xpcom, toolkit, accessible, xulrunner dirs.
Attachment #760321 - Flags: checkin? → checkin+
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.
Attachment #760372 - Flags: review?(karlt) → review+
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.
Attachment #760409 - Flags: review?(karlt) → review+
Whiteboard: [leave open for remaining patches][check linux try build before requesting checkin] → [check linux try build before requesting checkin]
Attachment #760956 - Flags: checkin? → checkin+
Attachment #761060 - Flags: checkin? → checkin+
Backed out, because it breaks mochitest-4 on Linux, as the try server results show!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b548b65cf0
https://hg.mozilla.org/mozilla-central/rev/cf635d80ad69
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
See Also: → 882036
Reopening - I need to fix the rest patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Target Milestone: mozilla24 → ---
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.
Attachment #761060 - Flags: checkin+
Fixed the mochitest failure. Key handling test has to be disabled on gtk because gtk overrides some key events.
Attachment #761060 - Attachment is obsolete: true
Attachment #761308 - Flags: review+
Attachment #761308 - Flags: checkin?
Attachment #761308 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/acecde2fbfbe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 1034064
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.