Force builds to be compatible with gtk 2.18/glib 2.22

RESOLVED FIXED in Firefox 17

Status

()

Core
Widget: Gtk
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla18
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 663990 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22

This does several things:
- Bump our required gtk version at build time to 2.18. I don't think it's worth supporting older versions at this point (see https://bugzilla.mozilla.org/show_bug.cgi?id=772563#c13 )
- Get rid of gtk2compat.h and replace it with ad-hoc header wrappers. This allows not to care about including gtk2compat.h where needed (and avoid left-overs).
- Contrary to gtk2compat.h, the header wrappers lead to builds compatible with gtk 2.18/glib 2.22, whatever version of gtk/glib development files are used.
Attachment #663990 - Flags: review?(karlt)
Blocks: 793638
Comment on attachment 663990 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22

Not having to explicitly include gtk2compat.h is nice.

gdk_x11_window_get_drawable_impl is in GTK+ 2.18:
http://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkx.h?id=2.18.0
I guess it is a nice optimization to skip the function call.
Was there another reason for including this function?

I'm not happy substituting g_ptr_array_free for g_ptr_array_unref as they mean
different things.  The code in UPowerClient.cpp wasn't reviewed and should
have just used g_ptr_array_free (bug 712442 comment 1), but anyway both
symbols are in GLib 2.22, which we now require.

For g_new, g_new0, g_renew in gmem.h, can you include the
(gsize)num <= G_MAXSIZE / sizeof(type) overflow checks, please?
There shouldn't be any need to cast sizeof() to gsize.
If it makes things easier, then perhaps instead define static inline functions
g_malloc_n, g_malloc0_n, g_realloc_n to perform the overflow check.
Attachment #663990 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #2)
> Comment on attachment 663990 [details] [diff] [review]
> Force builds to be compatible with gtk 2.18/glib 2.22
> 
> Not having to explicitly include gtk2compat.h is nice.
> 
> gdk_x11_window_get_drawable_impl is in GTK+ 2.18:
> http://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkx.h?id=2.18.0
> I guess it is a nice optimization to skip the function call.
> Was there another reason for including this function?

Looks like leftovers from when I tried to make things binary compatible with older versions.

> I'm not happy substituting g_ptr_array_free for g_ptr_array_unref as they
> mean
> different things.  The code in UPowerClient.cpp wasn't reviewed and should
> have just used g_ptr_array_free (bug 712442 comment 1), but anyway both
> symbols are in GLib 2.22, which we now require.

Got there like gdk_x11_window_get_drawable_impl, I guess.
Created attachment 664441 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22
Attachment #664441 - Flags: review?(karlt)
Attachment #663990 - Attachment is obsolete: true
Comment on attachment 664441 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22

Is mozilla/Assertions.h used?
I wonder why you avoided G_GSIZE_FORMAT?
I'm guessing the (gsize)(num) casts are unnecessary, and we'd actually be
better off with compiler warnings/errors should anything odd be passed for
that argument.
Attachment #664441 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #5)
> Comment on attachment 664441 [details] [diff] [review]
> Force builds to be compatible with gtk 2.18/glib 2.22
> 
> Is mozilla/Assertions.h used?

Gah, a leftover from when i thought about using MOZ_CRASH instead of g_error.

> I wonder why you avoided G_GSIZE_FORMAT?

Because it's not exported in glib headers :(
Here it's hidden away at /usr/lib64/glib-2.0/include/glibconfig.h but that should be pulled in by gmem.h.
http://developer.gnome.org/glib/stable/glib-Basic-Types.html#G-GSIZE-FORMAT:CAPS
Created attachment 664465 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22.

Here is a refreshed patch with review comments addressed
Attachment #664441 - Attachment is obsolete: true
Comment on attachment 664465 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22.

Carrying over r+
Attachment #664465 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3218db438fdf
https://hg.mozilla.org/mozilla-central/rev/3218db438fdf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 794285
This broke comm-central. See bug 794285.
Comment on attachment 664465 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 772446
User impact if declined: ESR17 on ancient build platform
Testing completed (on m-c, etc.): already landed on m-c
Risk to taking this patch (and alternatives if risky): changes runtime requirements for linux users
String or UUID changes made by this patch: none
Attachment #664465 - Flags: approval-mozilla-aurora?
Comment on attachment 664465 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22.

yes to not having ESR17 on ancient build platform :)
Attachment #664465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

5 years ago
Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/9be5b2790273
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.