If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bump GTK+ version and cleanup work arounds for old versions

RESOLVED FIXED in mozilla35

Status

()

Core
Widget: Gtk
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Timothy Arceri, Assigned: Timothy Arceri)

Tracking

Trunk
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

3 years ago
Now that Seamonkey is finally building with CentOS 6 (bug #795354) we can bump the gtk version and to some code tidy ups.

I'll be adding some patches here over the next couple of days.
(Assignee)

Comment 1

3 years ago
I'm proposing we require a minimum version on 2.18 as this allows the removal of all special case code for different GTK 2 versions.

2.18 has been around for 5 years so I dont think this is unreasonable.

This will have an impact on releases for RHEL 5 (and clones) which ship 2.10 but other than that SEL 11 has 2.18 and SEL 10 is already below the current minimum at only 2.8.
RHEL 5 has just a touch over 2 years left of normal security support but a patch to undo this change should be fairly easy to maintain for those distros.
(Assignee)

Comment 2

3 years ago
Created attachment 8491353 [details] [diff] [review]
Bump GTK required version
Attachment #8491353 - Flags: review?(karlt)
(Assignee)

Comment 3

3 years ago
Created attachment 8491355 [details] [diff] [review]
GTK print includes clean up
Attachment #8491355 - Flags: review?(karlt)
(Assignee)

Comment 4

3 years ago
Created attachment 8491356 [details] [diff] [review]
GTK_CHECK_VERSION macro cleanups
Attachment #8491356 - Flags: review?(karlt)
(Assignee)

Comment 5

3 years ago
Created attachment 8491358 [details] [diff] [review]
gtk_check_vesion function cleanup
Attachment #8491358 - Flags: review?(karlt)
(Assignee)

Comment 6

3 years ago
Created attachment 8491360 [details] [diff] [review]
GTK_CHECK_VERSION-cleanup.diff
Attachment #8491356 - Attachment is obsolete: true
Attachment #8491356 - Flags: review?(karlt)
Attachment #8491360 - Flags: review?(karlt)
Duplicate of this bug: 474599
Attachment #8491355 - Flags: review?(karlt) → review+
Attachment #8491358 - Flags: review?(karlt) → review+
Comment on attachment 8491353 [details] [diff] [review]
Bump GTK required version

Yes, Mozilla builds against GTK+ 2.18 IIRC, so older versions are not really supported, and this warns of that.

But please format the comment consistently with other commits, with the bug number on the first line and then a blank line, in this and other patches i.e.

b=1068964 Bump GTK required version to 2.18.0 r=karlt

Now that Seamonkey is building with CentOS 6
we can bump the gtk version ready for some code tidy ups.
Attachment #8491353 - Flags: review?(karlt) → review+
Comment on attachment 8491360 [details] [diff] [review]
GTK_CHECK_VERSION-cleanup.diff

Martin, what do you think about this?

Would you like to maintain the bits in gtk/compat for versions older than 2.18.
I think we can leave these in gtk/compat, where it is out of the way, if they are helpful to you.

I think the time has come, though, to cease doing workarounds for old GTK in the core Gecko code.
Attachment #8491360 - Flags: feedback?(stransky)
(Assignee)

Comment 10

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #8)
> Comment on attachment 8491353 [details] [diff] [review]
> Bump GTK required version
> 
> Yes, Mozilla builds against GTK+ 2.18 IIRC, so older versions are not really
> supported, and this warns of that.
> 

If its built using CentOS 6 then it would be 2.20 but it seems kind of pointless to bump it to anything higher than 2.18

> But please format the comment consistently with other commits, with the bug
> number on the first line and then a blank line, in this and other patches
> i.e.
> 
> b=1068964 Bump GTK required version to 2.18.0 r=karlt
> 
> Now that Seamonkey is building with CentOS 6
> we can bump the gtk version ready for some code tidy ups.

No problem. Sorry about that, the format seems to have changed since I last contributed a patch.
(Assignee)

Comment 11

3 years ago
Created attachment 8491387 [details] [diff] [review]
Bump GTK required version
Attachment #8491353 - Attachment is obsolete: true
Attachment #8491387 - Flags: review+
(Assignee)

Comment 12

3 years ago
Created attachment 8491389 [details] [diff] [review]
gtk_check_vesion function cleanup
Attachment #8491358 - Attachment is obsolete: true
Attachment #8491389 - Flags: review+
(In reply to Timothy Arceri from comment #10)
> If its built using CentOS 6 then it would be 2.20 but it seems kind of
> pointless to bump it to anything higher than 2.18

FWIW:
CentOS 6.5 may have 2.20 but earlier CentOS 6.x have 2.18.y.
Packages on Mozilla's build machines are not usually updated, and I don't know exactly which version of CentOS is being used.
(Assignee)

Comment 14

3 years ago
Created attachment 8491392 [details] [diff] [review]
GTK print includes clean up
Attachment #8491355 - Attachment is obsolete: true
Attachment #8491392 - Flags: review+
(Assignee)

Comment 15

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #13)
> (In reply to Timothy Arceri from comment #10)
> > If its built using CentOS 6 then it would be 2.20 but it seems kind of
> > pointless to bump it to anything higher than 2.18
> 
> FWIW:
> CentOS 6.5 may have 2.20 but earlier CentOS 6.x have 2.18.y.
> Packages on Mozilla's build machines are not usually updated, and I don't
> know exactly which version of CentOS is being used.

oh, no worries. Works out well then. Thanks for the quick reviews, I've updated the comments and set the patch to review to + I'm not sure if thats right or if I should have requested you do it.

Comment 16

3 years ago
Comment on attachment 8491360 [details] [diff] [review]
GTK_CHECK_VERSION-cleanup.diff

Well, this one can be easily reverted so remove it if you please. There are bigger problems on RHEL5, like the old python/gcc and so.
Attachment #8491360 - Flags: feedback?(stransky) → feedback+
(Assignee)

Comment 17

3 years ago
Created attachment 8492544 [details] [diff] [review]
GTK_CHECK_VERSION-cleanup.diff
Attachment #8491360 - Attachment is obsolete: true
Attachment #8491360 - Flags: review?(karlt)
Attachment #8492544 - Flags: review?(karlt)
Attachment #8492544 - Flags: review?(karlt) → review+
(In reply to Timothy Arceri from comment #15)
> I've
> updated the comments and set the patch to review to + I'm not sure if thats
> right or if I should have requested you do it.

You don't need to re-request review after an r+ with minor changes.

There isn't a single convention for marking such updated patches.
I usually don't add a self r+, and leave it to the bug history to indicate whether the patch is ready.  Others do what you have done.

I've pushed these patches to the "Try" server to check there's no unexpected problems.

https://tbpl.mozilla.org/?tree=Try&rev=942a5aa62f2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dff6dffd137f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a782a973a3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c819a499e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd07d9db1c5
Had to back out the gtk_check_version patch due to

nsGtkIMModule.cpp:251:19: error: unused variable 'container' [-Werror=unused-variable]

https://tbpl.mozilla.org/php/getParsedLog.php?id=48558950&tree=Mozilla-Inbound
Assignee: nobody → t_arceri
Keywords: leave-open
Attachment #8492544 - Flags: checkin+
Attachment #8491392 - Flags: checkin+
Attachment #8491387 - Flags: checkin+
(Assignee)

Comment 21

3 years ago
Created attachment 8492925 [details] [diff] [review]
gtk_check_vesion_function-cleanup_v2.diff

Whoops. V2 removes the unused variable.
Attachment #8491389 - Attachment is obsolete: true
Attachment #8492925 - Flags: review?(karlt)
https://hg.mozilla.org/mozilla-central/rev/dff6dffd137f
https://hg.mozilla.org/mozilla-central/rev/72c819a499e0
https://hg.mozilla.org/mozilla-central/rev/ecd07d9db1c5
Comment on attachment 8492925 [details] [diff] [review]
gtk_check_vesion_function-cleanup_v2.diff

Thanks.  I sometimes get caught out too with the compiler only detecting unused variables when optimizing.
Attachment #8492925 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4957f174a3
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7d4957f174a3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.