1027009 - Gtk3 build fails with --enable-warnings-as-errors

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mike Hommey [:glandium]]

/builds/slave/try-l64-0000000000000000000000/build/dom/plugins/base/nsPluginNativeWindowGtk.cpp:344:23: error: ignoring return value of 'gint gdk_error_trap_pop()', declared with attribute warn_unused_result [-Werror=unused-result]

Comment 1

[Mike Hommey [:glandium]]

Note this may not be the only error. That's just the one my try build stopped on. I'm building with --enable-warnings-as-error for now, so I won't find more.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

I'm sure there are other warnings in widget/gtk.

Comment 3

[Mike Hommey [:glandium]]

Those are the warnings I got in widget/gtk:

In file included from /builds/slave/try-l64-0000000000000000000000/build/obj-firefox/widget/gtk/Unified_c_widget_gtk0.c:2:0:
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c: In function 'ensure_toggle_menu_button_widget':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:168:9: warning: implicit declaration of function 'gtk_menu_button_new' [-Wimplicit-function-declaration]
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:168:33: warning: assignment makes pointer from integer without a cast [enabled by default]
In file included from /builds/slave/try-l64-0000000000000000000000/build/obj-firefox/widget/gtk/Unified_c_widget_gtk0.c:2:0:
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c: In function 'moz_gtk_get_focus_outline_size':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:752:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c: In function 'moz_gtk_get_tab_thickness':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:2093:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
In file included from /builds/slave/try-l64-0000000000000000000000/build/obj-firefox/widget/gtk/Unified_c_widget_gtk0.c:2:0:
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c: In function 'moz_gtk_menu_separator_paint':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:2425:5: warning: passing argument 1 of 'gtk_container_get_border_width' from incompatible pointer type [enabled by default]
In file included from /builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtkbin.h:35:0,
                 from /builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtkwindow.h:37,
                 from /builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtkdialog.h:35,
                 from /builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtkaboutdialog.h:32,
                 from /builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtk.h:33,
                 from ../../dist/system_wrappers/gtk/gtk.h:3,
                 from /builds/slave/try-l64-0000000000000000000000/build/widget/gtk/gtk3drawing.c:11,
                 from /builds/slave/try-l64-0000000000000000000000/build/obj-firefox/widget/gtk/Unified_c_widget_gtk0.c:2:
/builds/slave/try-l64-0000000000000000000000/build/gtk/usr/local/include/gtk-3.0/gtk/gtkcontainer.h:114:7: note: expected 'struct GtkContainer *' but argument is of type 'struct GtkWidget *'
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsGtkIMModule.cpp: In member function 'void nsGtkIMModule::PrepareToDestroyContext(GtkIMContext*)':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsGtkIMModule.cpp:254:24: warning: unused variable 'multicontext' [-Wunused-variable]
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp: In member function 'gfxASurface* nsWindow::GetThebesSurface(cairo_t*)':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp:6085:8: warning: suggest explicit braces to avoid ambiguous 'else' [-Wparentheses]
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp: In member function 'void nsWindow::LoseNonXEmbedPluginFocus()':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp:4615:29: warning: ignoring return value of 'gint gdk_error_trap_pop()', declared with attribute warn_unused_result [-Wunused-result]
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp: In member function 'void nsWindow::SetNonXEmbedPluginFocus()':
/builds/slave/try-l64-0000000000000000000000/build/widget/gtk/nsWindow.cpp:4574:25: warning: ignoring return value of 'gint gdk_error_trap_pop()', declared with attribute warn_unused_result [-Wunused-result]

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Fred, is this something you can look at?  Mike, do you need help with this or other blockers to bug 1016641?

Comment 5

[Mike Hommey [:glandium]]

I'd appreciate if someone could look at this one. I have the others figured. I'm getting close to the first Mozilla-automation built Gtk+3 Firefox.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

I'm going to help here too.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

btw. the gdk_error_trap_pop() should be replaced by gdk_error_trap_pop_ignored()

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: some wip

a wip. gdk_error_trap_pop_ignored() is unfortunately gtk3 only so we would need a gtk2 wrapper for it.

Comment 9

[Frederic Plourde (:fred23)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Fred, is this something you can look at?  Mike, do you need help with this
> or other blockers to bug 1016641?

Hey Milan, just saw your comment.
I'm off tomorrow (thu.), but sure, I'll gladly take a look on friday.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: build patch

Build patch v.2, try: https://tbpl.mozilla.org/?tree=Try&rev=69a205b4b4a2

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Fixed try: https://hg.mozilla.org/try/rev/9bedb8617145

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8444358 [details] [diff] [review]
patch

>+#define gdk_error_trap_pop_ignored gdk_error_trap_pop_ignored_
>+#include_next <gdk/gdk.h>
>+#undef gdk_error_trap_pop_ignored
>+
>+static inline void
>+gdk_error_trap_pop_ignored(void)
>+{
>+  int xerror = gdk_error_trap_pop();
>+}

We want to use the new _pop_ignored() function where available (GDK3), so the
function definition here should be conditional on GDK_MAJOR_VERSION.
That makes #define gdk_error_trap_pop_ignored unnecessary.

I would have thought that this would produce a set-but-not-used warning for
xerror.  "(void)gdk_error_trap_pop()" might avoid that.

>-        gHPanedWidget = gtk_hpaned_new();

>-        gVPanedWidget = gtk_vpaned_new();

>-        gVertScrollbarWidget = gtk_vscrollbar_new(NULL);

>-        gHorizScrollbarWidget = gtk_hscrollbar_new(NULL);

>-    gHScaleWidget = gtk_hscale_new(NULL);

>-    gVScaleWidget = gtk_vscale_new(NULL);

>-    gtk_widget_size_request(gComboBoxArrowWidget, &arrow_req);

>-        gdk_cursor_unref(cursor);

Can these symbols be removed from mozgtk.c now?

>+#if GTK_CHECK_VERSION(3,4,0)
>+    case GDK_SCROLL_SMOOTH:
>+        break;
>+#endif

Please leave this part out.

It will be fixed in bug 958868, which just needs a try run and it should be
able to land.

>-        gdk_cursor_unref(cursor);
>+        g_object_unref(cursor);

GdkCursor is not a GObject in GDK2.

>     if (cr) {
>         cairo_surface_t *surf = cairo_get_target(cr);
>         if (cairo_surface_status(surf) != CAIRO_STATUS_SUCCESS) {
>           NS_NOTREACHED("Missing cairo target?");
>@@ -6099,17 +6103,17 @@ nsWindow::GetThebesSurface(cairo_t *cr)
>     } else
> #endif
> #endif // (MOZ_WIDGET_GTK == 3)
>         mThebesSurface = new gfxXlibSurface
>             (GDK_WINDOW_XDISPLAY(mGdkWindow),
>              gdk_x11_window_get_xid(mGdkWindow),
>              visual,
>              size);
>-
>+    }

Can you indent the statements in the block while you are changing this,
please?

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch, v.2

Thanks, this should address it.

>-        gHPanedWidget = gtk_hpaned_new();
>-        gVPanedWidget = gtk_vpaned_new();
>-        gVertScrollbarWidget = gtk_vscrollbar_new(NULL);
>-        gHorizScrollbarWidget = gtk_hscrollbar_new(NULL);
>-    gHScaleWidget = gtk_hscale_new(NULL);
>-    gVScaleWidget = gtk_vscale_new(NULL);
>-    gtk_widget_size_request(gComboBoxArrowWidget, &arrow_req);
>-        gdk_cursor_unref(cursor);
>
> Can these symbols be removed from mozgtk.c now?

Those are still used by gtk2drawing.c but I moved them to gtk2 section in mozgtk.c.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 8445099 [details] [diff] [review]
patch, v.2

> >-        gHPanedWidget = gtk_hpaned_new();
> >-        gVPanedWidget = gtk_vpaned_new();
> >-        gVertScrollbarWidget = gtk_vscrollbar_new(NULL);
> >-        gHorizScrollbarWidget = gtk_hscrollbar_new(NULL);
> >-    gHScaleWidget = gtk_hscale_new(NULL);
> >-    gVScaleWidget = gtk_vscale_new(NULL);
> >-    gtk_widget_size_request(gComboBoxArrowWidget, &arrow_req);
> >-        gdk_cursor_unref(cursor);
> >
> > Can these symbols be removed from mozgtk.c now?
> 
> Those are still used by gtk2drawing.c but I moved them to gtk2 section in
> mozgtk.c.

mozgtk.c is used only when MOZ_WIDGET_GTK == 3, where gtk2drawing is not
built, and so these symbols need not be in the stubs.
The GTK2_SYMBOLS section is just for symbols used only by the plugin
container.

Similarly, gdk_cursor_unref won't be needed when mozgtk.c is used, and so can
also be removed completely from mozgtk.c.

r+ without the additions to GTK2_SYMBOLS.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

Thanks! Try https://tbpl.mozilla.org/?tree=Try&rev=88ceae081199

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3974edb6aaed

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3974edb6aaed