174471 - [gtk2] need to port over the nsITheme interface code to gtk2

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christopher Blizzard (:blizzard)]

Need to have native theme interfaces for gtk2.  I think it should be as simple
as setting up a gtk2drawing.c that implements the interfaces in gtkdrawing.h. 
If it's as simple as that, we won't have to change any of the C++ code at all.

Comment 1

[Robin Lu]

Attachment: a proposal patch

works for gtk2, although the scrollbar is not so perfect.

Comment 2

[Christopher Blizzard (:blizzard)]

You should probably be aware of bug #174927, which refactors a lot of this code.

Comment 3

[Andrew Johnson]

Attachment: new improved patch

this is a new patch, incorporating patch for bug #174927, along with better
behaviour for scrollbars/scrollbar buttons arrows, and drop-down arrows. since
the patch for #174927 doesn't work with CVS HEAD, I modified it to compile, aka
new instabilities may have been introduced. Probably needs more testing.
Shouldn't break GTK1 support, but haven't tested.

Comment 4

[Andrew Johnson]

Attachment: real new improved patch

somehow uploaded wrong file.. so this should be the real one.

Comment 5

[Stuart Ballard]

This bug's been sitting around for some time with an alleged patch. Is the patch
any good and is it still applicable after all this time to bitrot?

Comment 6

[Christopher Blizzard (:blizzard)]

I think this was made before brian re-organized, but I'm not sure.

Comment 7

[Andrew Johnson]

oh wow. yeah. the first patch was incompatible when I made mine. and I made that
awhile back. right before a lot of changes got made. it would need to be very
likely completely redone, and either way it was semi-incomplete. I suppose I can
regrab cvs and see how hard it would be to redo, but since its been awhile since
I looked at any mozilla code it might make sense for someone else with more know
how on the new code layout to redo it.

Comment 8

[Christopher Blizzard (:blizzard)]

I wouldn't worry about it unless you're really motivated.  Chris Lahey and I are
hacking on this.

Comment 9

[Andrew Johnson]

Attachment: adds gtk2preview.c for gtk2 drawing

this seems to work well with most themes, though combobox arrows centering may
be wrong, and scrollbars sliders need better logic to handle the orientation
properly

Comment 10

[Christopher Blizzard (:blizzard)]

I re-indented the file and checked it in.  We should start with the bugfixing
from here on out.

Comment 11

[Christopher Blizzard (:blizzard)]

Sorry, just to clarify, it isn't actually being built yet.

Comment 12

[Brian Tarricone]

first bug i can see here:  with this code enabled (via the extra parts of
attachment #120790 [details] [diff] [review]), doing anything that opens a message compose box in mail
(compose, reply, reply all) causes mozilla to quit with exit code 11. 
unfortunately i don't have any debug info yet as i accidentally compiled using
my script to create an optimised debugless build.  i'll get to that soon.

Comment 13

[KENZ (Kenji Irie)]

blizzard:
I found you checked in the patch at bonsai, but it looks that one file
gtk2drawing.c only has added.
Original patch contains changes for two files other than gtk2drawing.c:
  gfx/src/gtk/Makefile.in
  gfx/src/gtk/nsNativeThemeGTK.cpp

These changes haven't checked in yet. Is this mistake?
Or do we have to apply these manually for testing the gtk2 native theme feature?

Comment 14

[Stuart Ballard]

What would it take to check this in behind a configure check, kind of like the
original native GTK theme stuff used to be? You had to hunt down the
commented-out "NATIVE_THEME_GTK=1" (or whatever) line in some obscure source
file, but other than that you could build from a stock tarball.

It would make life much easier for testers if that could be done here...

Comment 15

[Christopher Blizzard (:blizzard)]

I left them out because this code isn't stable yet.  I figure motivated testers
can apply the rest.

Comment 16

[Christopher Blizzard (:blizzard)]

Attachment: patch

This patch fixes problems with the bluecurve theme with gtk2 native themes.

Comment 17

[Christopher Blizzard (:blizzard)]

Attachment: patch that doesn't include commented out code

Oops.

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 122039 [details] [diff] [review]
patch that doesn't include commented out code

>Index: gtkdrawing.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/gtkdrawing.h,v
>retrieving revision 1.18
>diff -u -r1.18 gtkdrawing.h
>--- gtkdrawing.h	4 Dec 2002 01:50:38 -0000	1.18
>+++ gtkdrawing.h	29 Apr 2003 20:02:05 -0000
>@@ -57,12 +57,14 @@
> 
> /*** type definitions ***/
> typedef struct {
>-  guint8 active;
>-  guint8 focused;
>-  guint8 inHover;
>-  guint8 disabled;
>-  guint8 isDefault;
>-  guint8 canDefault;
>+  guint8  active;
>+  guint8  focused;
>+  guint8  inHover;
>+  guint8  disabled;
>+  guint8  isDefault;
>+  guint8  canDefault;
>+  gint32  curpos; /* curpos and maxpos are used for scrollbars */
>+  gint32  maxpos;
> } GtkWidgetState;

There's really no reason for this whitespace change.

>Index: nsNativeThemeGTK.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/gtk/nsNativeThemeGTK.cpp,v
>retrieving revision 1.35
>diff -u -r1.35 nsNativeThemeGTK.cpp
>--- nsNativeThemeGTK.cpp	8 Jan 2003 20:25:24 -0000	1.35
>+++ nsNativeThemeGTK.cpp	29 Apr 2003 20:02:05 -0000
>@@ -55,6 +55,8 @@
> 
> #include <gdk/gdkprivate.h>
> 
>+#include <gdk/gdkx.h>
>+

I can't find any reason that we need this.

Other than that, r=bryner.

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 122039 [details] [diff] [review]
patch that doesn't include commented out code

a/sr=sspitzer

but one comment / question:

1)

+  nsCOMPtr<nsIContent> content;
+  aFrame->GetContent(getter_AddRefs(content));

you check if aFrame is null, but what if GetContent() fails?

I'm ignorant, is that possible?

maybe:

rv = aFrame->GetContent(getter_AddRefs(content));
if (NS_FAILED(rv))
 return 0;

or

aFrame->GetContent(getter_AddRefs(content));
if (!content)
  return 0;

Comment 20

[Christopher Blizzard (:blizzard)]

Checked in with changes from bryner and sspitzer included.

Only makefile changes are required from the last patch to build native themes now.

Comment 21

[Christopher Blizzard (:blizzard)]

OK.  This is turned on now.  Any bugs should be new bugs now that we're building
it by default.