Last Comment Bug 793634 - Force builds to be compatible with gtk 2.18/glib 2.22
: Force builds to be compatible with gtk 2.18/glib 2.22
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla18
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on: 794285
Blocks: 772563 793638
  Show dependency treegraph
 
Reported: 2012-09-24 03:06 PDT by Mike Hommey [:glandium]
Modified: 2013-03-19 06:43 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Force builds to be compatible with gtk 2.18/glib 2.22 (22.77 KB, patch)
2012-09-24 03:13 PDT, Mike Hommey [:glandium]
karlt: review-
Details | Diff | Splinter Review
Force builds to be compatible with gtk 2.18/glib 2.22 (22.87 KB, patch)
2012-09-25 04:36 PDT, Mike Hommey [:glandium]
karlt: review+
Details | Diff | Splinter Review
Force builds to be compatible with gtk 2.18/glib 2.22. (22.74 KB, patch)
2012-09-25 05:42 PDT, Mike Hommey [:glandium]
mh+mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-09-24 03:06:09 PDT

    
Comment 1 Mike Hommey [:glandium] 2012-09-24 03:13:35 PDT
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.
Comment 2 Karl Tomlinson (back Dec 13 :karlt) 2012-09-24 16:26:50 PDT
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.
Comment 3 Mike Hommey [:glandium] 2012-09-24 23:37:34 PDT
(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.
Comment 4 Mike Hommey [:glandium] 2012-09-25 04:36:29 PDT
Created attachment 664441 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22
Comment 5 Karl Tomlinson (back Dec 13 :karlt) 2012-09-25 05:17:43 PDT
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.
Comment 6 Mike Hommey [:glandium] 2012-09-25 05:25:55 PDT
(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 :(
Comment 7 Karl Tomlinson (back Dec 13 :karlt) 2012-09-25 05:34:51 PDT
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
Comment 8 Mike Hommey [:glandium] 2012-09-25 05:42:54 PDT
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
Comment 9 Mike Hommey [:glandium] 2012-09-25 05:43:26 PDT
Comment on attachment 664465 [details] [diff] [review]
Force builds to be compatible with gtk 2.18/glib 2.22.

Carrying over r+
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-09-25 14:38:51 PDT
https://hg.mozilla.org/mozilla-central/rev/3218db438fdf
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-25 16:08:34 PDT
This broke comm-central. See bug 794285.
Comment 13 Chris AtLee [:catlee] 2012-10-02 11:37:10 PDT
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
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-02 14:34:56 PDT
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 :)
Comment 15 Scoobidiver (away) 2012-10-04 02:35:59 PDT
Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/9be5b2790273

Note You need to log in before you can comment on or make changes to this bug.