GTK+ 3.0 and GNOME 3 are approaching and we should get Firefox ready for them. Attaching WIP patch which I've been working on with Martin Stransky. Patch also contain changes from bug #611953 (which are going to be removed in final version). So far with this patch we are able to compile mozilla-central with GTK3, but there is still much work to do (fix exposing issues and couple of crashes).

updated version, against trunk, builds and runs. A gtk widget implementation is not finished yet, background is not rendered properly, sometimes crashes.

fixed background rendering and style borders

This looks like great work. We need to think about how to support gtk2 for a while as well as gtk3. widget/src/gtk2 could be duplicated to widget/src/gtk3. Some of the rest of the code still works with gtk2 I guess.

Most of the changes are in widget/src/gtk2/gtk2drawing.c and widget/src/gtk2/nsWindow.cpp, other files may need some clean up only.

We focused on wrongly drawn widgets: - buttons/checkboxs/ratio buttons - scrollbars - treeview - tabs - and few crashes We still have issues with menu and tooltips and random crashes which Martin is investigating. Also fonts are pink in specific desktop configuration.

Another WIP: - Fixed wrong colors delived from GtkStyle - Tabs better drawn - Scrollbars looks fine now - menu looks much better - still some issues remains Have a look and test with your GTK3 and favourite theme. Feedback welcomed.

Wrong patch, reuploading.

I get this build error: g++ -o nsWindow.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/hussam/packages/firefox/config/gcc_hidden.h -DNATIVE_THEME_SUPPORT -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD -DOS_LINUX=1 -DOS_POSIX=1 -DCAIRO_GFX -I/home/hussam/packages/firefox/ipc/chromium/src -I/home/hussam/packages/firefox/ipc/glue -I../../../ipc/ipdl/_ipdlheaders -I/home/hussam/packages/firefox/widget/src/gtk2 -I. -I../../../dist/include -I../../../dist/include/nsprpub -I/usr/include/nspr -I/usr/include/nss -I/home/hussam/packages/firefox/widget/src/gtk2/../xpwidgets -I/home/hussam/packages/firefox/widget/src/gtk2/../shared -I/home/hussam/packages/firefox/other-licenses/atk-1.0 -I/home/hussam/packages/firefox/widget/src/gtk2/../shared/x11 -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fomit-frame-pointer -finline-limit=50 -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -DGSEAL_ENABLE -pthread -I/usr/include/gtk-3.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng14 -I/usr/include/gtk-3.0/unix-print -I/usr/include/startup-notification-1.0 -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsWindow.pp /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp In file included from /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:49:0: /home/hussam/packages/firefox/widget/src/gtk2/../xpwidgets/nsBaseWidget.h:123:24: warning: ‘virtual gfxASurface* nsBaseWidget::GetThebesSurface()’ was hidden /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.h:339:18: warning: by ‘gfxASurface* nsWindow::GetThebesSurface(GtkWidget*, cairo_t*)’ /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp: In function ‘void SetUserTimeAndStartupIDForActivatedWindow(GtkWidget*)’: /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:5: error: ‘GdkDrawable’ was not declared in this scope /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:18: error: ‘drawable’ was not declared in this scope /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1377:72: error: ‘GDK_DRAWABLE’ was not declared in this scope /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1385:62: error: ‘gdk_x11_drawable_get_xdisplay’ was not declared in this scope /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:1406:76: error: ‘gdk_x11_drawable_get_xid’ was not declared in this scope /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp: In member function ‘void nsWindow::LoseNonXEmbedPluginFocus()’: /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:5097:29: warning: ignoring return value of ‘gint gdk_error_trap_pop()’, declared with attribute warn_unused_result /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp: In member function ‘void nsWindow::SetNonXEmbedPluginFocus()’: /home/hussam/packages/firefox/widget/src/gtk2/nsWindow.cpp:5056:25: warning: ignoring return value of ‘gint gdk_error_trap_pop()’, declared with attribute warn_unused_result make[3]: *** [nsWindow.o] Error 1 make[3]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu/widget/src/gtk2' make[2]: *** [libs] Error 2 make[2]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu/widget/src' make[1]: *** [libs] Error 2 make[1]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu/widget' make: *** [default] Error 2 gtk+ 3.0.2 (but gtk+ 2.24 is also installed although it shouldn't matter as they are installable in parallel) P.S. I'm just a user.

It may be caused by some problem in your config files. I'm just finishing a patch which separates gtk3 code to widget/src/gtk3 and allows to build ff with both widgets so it addresses such issues.

Martin separated gtk3 from gtk2 so we now have original widgets/src/gtk2 and new widgets/src/gtk3. He's currently working on plugins (crash when we build with gtk3 and use gtk2 plugin). Finished some widgets: - scrollbars - tabs - fixed drag&drop but it still flicker when moving. WIP: custom cursors, gtk3 miss gdkpixmap Apply patch and build with --enable-default-toolkit=cairo-gtk3.

Is the patch diffed against the RC tarball or latest HG? because I couldn't get it to patch against mozilla-central.

It would be easier to read the patch if it were split into one which did the duplication and another which made the modifications to the new gtk3 dir.

Using "hg copy" would make the duplication easier to follow (and may make splitting the modifications unnecessary).

(In reply to comment #12) > Is the patch diffed against the RC tarball or latest HG? because I couldn't get > it to patch against mozilla-central. It is diffed against changeset 62828:b1ef0685b2e0 (mozilla-central from Feb 18). We don't know how to make changes to our local hg repository and stay synced with mozilla-central in the same time. So we unbitrot patch from time to time.

(In reply to comment #14) > Using "hg copy" would make the duplication easier to follow (and may make > splitting the modifications unnecessary). Hm, I'll check the "hg copy", it looks like good idea.

(In reply to comment #15) > We don't know how to make changes to our local hg repository and stay > synced with mozilla-central in the same time. Getting the equivalent of git commit --amend is not quite trivial with mercurial. Most people use the mq extension for this. It sounded a bit much effort to me, but, once I actually tried it, it wasn't so hard to learn. https://developer.mozilla.org/en/Mercurial_Queues This does come in very handy for separating projects into manageable-sized patches, particularly when each patch in the series is still under development. The gtk2 plugin work at least should be separate from the base gtk3 patch (and perhaps could even be split into a number of smaller patches). http://weblogs.mozillazine.org/roc/archives/2010/01/more_on_patch_d.html

I've been doing my best to read the patch since I take particular interest in the native theming portion, but from what I can gather, doesn't GTK3's use of Cairo for drawing widgets mean we can bypass XlibNativeRenderer (and all it's ugly slowness) completely? I may have missed this, but I don't see you setting the enable-system-cairo configure flag by default when GTK3 is built, which can cause nastiness.

Sure, the code should be optimized and refactorised. Our goal is to create a minimal working patch and then improve it.

(In reply to comment #18) > I may have missed this, but I don't see you setting the enable-system-cairo > configure flag by default when GTK3 is built, which can cause nastiness. Yeah, we missed that. We use enable-system-cairo option of course, it should be probably part of configure.in patch.

Used hg copy for creating gtk3 directory. Custom cursor fixed (RMB on toolbar/Customize) I'm going to check the way to sync with mozilla-central.

At some point we will probably have to stop supporting system cairo to get decent performance ... there may be big wins from punching through cairo's public API. We can make GTK3 require system cairo, for now, but later we'll probably need to create a way for them to coexist (and bridge them for theme drawing).

Just reading more of this patch out of interest... +++ b/embedding/browser/gtk/src/EmbedWindow.cpp - ownerAsWidget->allocation.width, - ownerAsWidget->allocation.height); + gtk_widget_get_allocated_width(ownerAsWidget), + gtk_widget_get_allocated_width(ownerAsWidget)); You mean allocated_height? There's a lot of code duplication going on between GTK2 and GTK3. Do our JS and CSS preprocessors not support the && or || operators? It'd be interesting to see if we can support Ubuntu's proposed overlay scrollbars, but that's for a later time... Obviously we need system Cairo because the Cairo version on the system could be anything, but roc, I can't imagine how a bridge would work without cancelling out any performance gains we'd get from using the private API.

Bridging would be relatively easy: figure out what the moz-cairo context is actually drawing to; construct a system-cairo context to draw to the same target; set its state to match the state of the moz-cairo context.

(In reply to comment #19) > Sure, the code should be optimized and refactorised. Our goal is to create a > minimal working patch and then improve it. If the "minimal working" code is based on existing gtk2 code, then it makes sense to even land this code first and do the optimization/refactoring separately.

(In reply to comment #23) > There's a lot of code duplication going on between GTK2 and GTK3. Do our JS and > CSS preprocessors not support the && or || operators? Unfortunately https://developer.mozilla.org/en/Build/Text_Preprocessor does not claim the && or || are supported, I'd love to use them.

(In reply to comment #26) > (In reply to comment #23) > > There's a lot of code duplication going on between GTK2 and GTK3. Do our JS and > > CSS preprocessors not support the && or || operators? > > Unfortunately https://developer.mozilla.org/en/Build/Text_Preprocessor does not > claim the && or || are supported, I'd love to use them. Have you tried? I'm somewhat sure I've seen/used them before...

As for the gtk2 plug-ins support inside gtk3 browser - I managed to build libxul for gtk3 and gtk2 simultaneously (Jan is going to attach the patch) and I expect we want to have two plugin-containers and select it when plug-in is loaded. I expect we're not going to support gtk2 plug-ins inside gtk3 browser without OOP, right?

We just disabled building without ipc (bug 638755), so you should expect it to be there, yes.

Some plugins are still run in-process only (like Adobe reader) regardless of the ipc state.

(In reply to comment #28) > I expect we're not going to support gtk2 plug-ins inside gtk3 browser without > OOP, right? The only issue AFAIK would be a gtk2 java plugin. Java plugins are "in-process by default on all platforms because of bugs relating to window.java and various other things." (bug 640908 comment 5) (I don't expect Xt plugins are using gtk2, but, I am trying to find time to review your patches to move Xt plugins OOP.)

Attaching rebased patch: - Martin made gtk2 plugins in gtk3 code possible! - Fixed custom mouse cursors

jhorak, any updates on the patch or its status?

(In reply to comment #33) > jhorak, any updates on the patch or its status? Are you looking for something particular?

(In reply to comment #34) > (In reply to comment #33) > > jhorak, any updates on the patch or its status? > > Are you looking for something particular? Not necessarily, I just have a keen interest on this bug. Although I suppose I would like to know if you're making optimizations to native theme rendering since that was a big performance drag for us in GTK2.

(In reply to comment #23) > It'd be interesting to see if we can support Ubuntu's proposed overlay > scrollbars, but that's for a later time... > If you're interested in supporting those, I'd be more than happy to help out :)

> > Are you looking for something particular? > > Not necessarily, I just have a keen interest on this bug. Although I suppose I > would like to know if you're making optimizations to native theme rendering > since that was a big performance drag for us in GTK2. Not yet. We're working on basic functionality (gtk rendering/plugins) and going to split the patch to separated pieces and ask for review for them. But feel free to attach/send me any patch and we'll merge it with the recent patch set.

We're trying to divide changes to more patches. We start with gtk3 directory which has no effect to main tree until makefile/configure changes are pushed. It's still a big patch anyway. Please have a look and let us know how to proceed.

Comment on attachment 525346 [details] [diff] [review] gtk3 directory patch v1 I've had a look at gtk3drawing and nsNativeThemeGTK, thanks. I'll write some general comments here and attach specifics. hg copy definitely makes reviewing easier (=faster), thank you. Even something that compiles is a significant milestone, so in patches we're not necessarily looking for complete solutions but monotonically improving steps to get us to the goal. Patches involving logical steps are easier to review (and track in history) than simply dividing changes across files. In the beginning, patches to port specific files are fine, but try to pick patches to avoid breaking APIs for callers changed in other patches. What will help us track what remains to be done is a single way of marking items that are unfinished. Most such items are marked to TODO here but some are not. If a drawing section needs significant updating and you'd prefer to leave that for a later patch (which is fine), then I'd prefer an "#if TODO" block than experimental changes that don't really work. There should be something stating that the cairo_t passed to the moz_gtk_widget_paint needs to be a system-cairo cairo_t. I assume there is no support for tiling themes in GTK3 - at least I can't imagine how they would work. It looks like the GTK3 style system is designed so that it is no longer necessary to have a GtkWidget to draw the appropriate styling. If we were writing gtk3drawing.c from scratch now, we'd probably use GtkStyleContext and GtkWidgetPath. Following the GTK2 port and continuing to use widgets is a sensible step where this works. For child widgets that are no longer accessible through GTK3, it may be easier and cleaner to use GtkStyleContext/GtkWidgetPath than searching through private descendants of a GtkTreeView widget, for example. As the new style system doesn't pass GtkWidgets to the theme engines, many of the operations that were performed on the widgets now have no effect on drawing, so there's no point updating them to new methods. This includes the following methods and properties: gtk_widget_grab_default gtk_widget_set_relief gtk_widget_grab_focus gtk_toggle_button_set_active, gtk_widget_size_allocate, gtk_adjustment_set_page_size, gtk_tree_view_column_set_sort_indicator, gtk_widget_size_allocate, gtk_check_menu_item_set_active, "has-default", "has-focus" I think most of these can be removed, though some may need replacing with a TODO indicating what remains to be done. gtk_widget_override_background_color is an exception because it modifies the widget's StyleContext, but I'm not sure whether still is necessary. The clip_rect argument should be unnecessary now, as that is already on the cairo_t. ThemeRenderer::DrawWithGDK could use some tidying up to remove that (and I don't follow why it is important to remove the offsets). In nsNativeThemeGTK.cpp, there is no longer any need to specify the visual to renderer.Draw() (pass NULL, instead) as themes now draw to a cairo_t with any target. That makes moz_gtk_widget_get_colormap() unnecessary.

Thanks for the update. We already have a patch which completely works (only bug there I know about is broken rendering of menu separators when firefox is launched via. ssh). I'm going to port it to latest trunk and address your comments there.

I'm concerned that copying the whole gtk2 directory is not going to lead to the easiest way to maintain gtk2 and gtk3 ports. Of 56 files copied, only 25 files include changes, and few of these have significant changes. Some of the minor changes would be fine even for our GTK2 builds (against GTK+-2.10). The majority of the other changes seem to be just changing from accessing structs directly to using accessor methods. This suggests that the best approach is to copy only the files with major changes but keeping it in the same directory, just changing the makefile. gtk3drawing.c deserves a copy for sure. I don't know whether there's any other candidates. Then there's a number of situations where the same code would work fine for GTK 2 and 3. gdk_x11_display_get_xdisplay(gdk_display_get_default()) or DefaultXDisplay() in X11Util.h, for example can replace GDK_DISPLAY() and are fine in both ports. (Sometimes there's even a more appropriate function such as gtk_clipboard_get_display().) gtk_window_get_group() is suitable for both ports. nsBidiKeyboard.cpp can use PR_FindFunctionSymbolAndLibrary instead of choosing the library by soname. (If there's a reason why "We don't want it" for GTK3 then it should be stated.) For nsGTKToolkit, GetSharedGC is not used so can be removed (or a stub if necessary). For all the accessor methods, this can be simplified by modifying existing files to use the new accessor functions, and adding a header file that is included to define accessor functions when not available. For example, one entry would be: #if !GTK_CHECK_VERSION(2, 14, 0) static inline GdkWindow* gtk_widget_get_window(GtkWidget *widget) { return widget->window; } #endif

Hi, for the GTK+2 version, why not simply depend on the latest GTK+ version, ie 2.24? So you have all the accessor and you avoid the use of a lot of ifdefs Also, why support the GTK+2 version at all? why not switch to GTK+3 completely?

Because it still needs to run on systems which don't support GTK3. For example, Ubuntu 10.04 LTS is supported until April 2013, and people do still use that.

(In reply to comment #42) > This suggests that the best approach is to copy only the files with major > changes but keeping it in the same directory, just changing the makefile. > gtk3drawing.c deserves a copy for sure. I don't know whether there's any > other candidates. Okay, I'm fine with that. The same approach is used in modules/plugin/base/src and so.

Is there anyway in the nightlies to play with / test the GTK3 version? Or is more involved? Excited to see work done on this.

Unless someone is willing to publish binary, I imagine that GTK3 Firefox is still very much a from-scratch process. On a somewhat related note, I'm curious to know whether GTK3 adoption will use pure GTK3 or a custom implementation similar to the GTK2 used by Firefox currently.

A complete patch for latest trunk, It has a new directory structure in widget (widget/gtk2 and widget/gtk2/gtk3). I'm going to attach separated pieces for review and address the Karl's comments there.

Doesn't apply to latest mozilla-central (I'm assuming that's trunk) Hunk #10 FAILED at 166. Hunk #11 FAILED at 200. Hunk #12 FAILED at 212. Hunk #13 FAILED at 237. Hunk #14 succeeded at 308 with fuzz 2 (offset 17 lines). Hunk #15 FAILED at 302. Hunk #16 succeeded at 338 with fuzz 2 (offset 30 lines). 5 out of 16 hunks FAILED -- saving rejects to file dom/plugins/base/nsPluginNativeWindowGtk2.cpp.rej

Fixed the comments, except the gTreeHeaderCellWidget and gTreeHeaderSortArrowWidget. Let's proceed and fix it later, I don't know how to fix it for now.

patch to widget/src/gtk2/mozcontainer.c

A Makefile patch. Old (gtk2) library is built as widget/src/gtk2/libwidget_gtk2, new (gtk3) is built to widget/src/gtk2/gtk3/libwidget_gtk3.

Comment on attachment 540030 [details] [diff] [review] mozcontainer.c patch These are mostly changes involving accessor methods that could be made to the existing mozcontainer.c (and used in both GTK+ 2 and 3 builds) with the help of gtk2compat.h. There are only 2 or 3 statements that need to be different, and these could be handled through preprocessor conditionals. >+ gtk_widget_set_allocation(widget, allocation); > > tmp_list = container->children; >@@ -336,10 +339,11 @@ moz_container_size_allocate (GtkWidget > } > >- if (GTK_WIDGET_REALIZED (widget)) { >- gdk_window_move_resize(widget->window, >- widget->allocation.x, >- widget->allocation.y, >- widget->allocation.width, >- widget->allocation.height); >+ gtk_widget_get_allocation(widget, &tmp_allocation); >+ if (gtk_widget_get_realized(widget)) { >+ gdk_window_move_resize(gtk_widget_get_window(widget), >+ tmp_allocation.x, >+ tmp_allocation.y, >+ tmp_allocation.width, >+ tmp_allocation.height); It should be fine to use the "allocation" variable here instead of using get_allocation to fetch the value just set. (I don't imagine calling size_allocate on children will change the parent's allocation.)

Comments on most of attachment 540032 [details] [diff] [review]. The situation with the "draw" event is more complex and I need to think about that.

DirectFB is no longer supported by GTK 3 and apparently even in 2 the backend hasn't worked since 2.18. It's probably time we stripped the DirectFB code, but at least here don't make any effort to maintain support. http://lists-archives.org/gtk-devel/11896-dropping-directfb-backend-from-gtk-3.html

I can think of 2 possible approaches with the cairo_t passed in the "draw" signal. 1. We pay attention to everything in the cairo_t. 2. We assume that, because we have gtk_widget_set_double_buffered(widget, FALSE), we can just paint directly to the X Window for the GdkWindow using only the clip region on the cairo_t. Comments on the drawing part of attachment 540032 [details] [diff] [review]. Currently in this patch, approach 2 is used if UseShm() returns true or if the layer manager is an OpenGL layer manager. With basic layers, the approach here is taking the target surface from the cairo_t in GetThebesSurface and then creating a gfxContext (with its new cairo_t) around the target surface. This is pretty much approach 1. It is not transferring the matrix from one cairo_t to the other, but I think that actually works, even for client-side-windows where the GdkWindow is at an offset from the X Window, because GTK/GDK uses separate cairo_surface_t's for the client side windows sharing the same X Window (gdk_window_create_cairo_surface). The offsets are therefore on the surface rather than the matrix. I think this is all OK. I'm just writing it down, because it took me time to get it clear in my head.

Does this means that if Firefox is ported to GTK+ 3, Firefox will run on Wayland? Please support Wayland. Thanks.

(In reply to comment #61) > Does this means that if Firefox is ported to GTK+ 3, Firefox will run on > Wayland? These two things are unrelated.

(In reply to comment #62) > (In reply to comment #61) > > Does this means that if Firefox is ported to GTK+ 3, Firefox will run on > > Wayland? > > These two things are unrelated. Sure, I just want to know if Firefox will work on Wayland once it's ported to GTK+ 3.

(In reply to comment #63) > (In reply to comment #62) > > (In reply to comment #61) > > > Does this means that if Firefox is ported to GTK+ 3, Firefox will run on > > > Wayland? > > > > These two things are unrelated. > > Sure, I just want to know if Firefox will work on Wayland once it's ported > to GTK+ 3. I ask this because GTK+ 3 has a Wayland back-end. Thanks for the help.

Gecko's GTK/GDK X11 port uses some features of the GDK X11 backend, so porting to GTK3 is only part of work required for a GTK/Wayland port.

Just out of curiosity, has any progress on this been made in the last month?

I'm porting it to latest trunk right now.

Comment on attachment 540034 [details] [diff] [review] gtk2 makefile patch I don't really grasp what's happening with the gtk2/gtk3 subdirectory here, but I suspect it has something to do with supporting GTK2 plugins in the GTK3 port. We should get a build config peer[1] to review any such changes as some stage but, at this stage, I'd prefer not to think about plugins. I can review less structural changes to the build system. [1] https://wiki.mozilla.org/Modules/Core#Build_Config

(In reply to Karl Tomlinson (:karlt) from comment #68) > Comment on attachment 540034 [details] [diff] [review] > gtk2 makefile patch > > I don't really grasp what's happening with the gtk2/gtk3 subdirectory here, > but I suspect it has something to do with supporting GTK2 plugins in the > GTK3 port. This patch allows to build the widget gtk2 module along with the gtk3 one. Each module needs an extra directory and many files are shared, so the easiest way is to build gtk3 as a subdir of gtk2. It means that old gtk2 is built as /widget/src/gtk2/widget_gtk2.a and new gtk3 is built as /widget/src/gtk2/gtk3/widget_gtk3.a.

I assume the gtk2 module is only for plugin support. There may be a few possible solutions to support gtk2 plugins. e.g. one option might be to use a completely separate build to make a gtk2 libxul. But I think the path here should be to get small changesets landed, so the whole series doesn't need to be remerged with trunk while other changesets are reviewed/modified. Leaving out gtk2 plugins is a good way to reduce the complexity in what gets done first.

No, the gtk2 module is the recent one which is used by cairo-gtk2 target. The new one (gtk3) is the /widget/src/gtk2/gtk3.

To be clear, it's built as: cairo-gtk2 target => /widget/src/gtk2 => widget_gtk2.a (there's no change there) cairo-gtk3 target => /widget/src/gtk2/gtk3 => widget_gtk3.a (the new one for gtk3)

OK, thanks. I mis-assumed, then. But then, only one of them needs to be made in a single build, right? i.e. the build is either cairo-gtk2 or cairo-gtk3, so the object files for each build can appear in the same directory, much like how winnt/mac/pango/freetype/gtk/qt builds exist in the gfx/thebes directory.

(In reply to Karl Tomlinson (:karlt) from comment #73) > OK, thanks. I mis-assumed, then. > > But then, only one of them needs to be made in a single build, right? > i.e. the build is either cairo-gtk2 or cairo-gtk3, so the object files for > each build can appear in the same directory, much like how > winnt/mac/pango/freetype/gtk/qt builds exist in the gfx/thebes directory. Yes, that's correct. But I think it's better to split the dirs. If you'd like to have a version without plugin support for now I'll prepare a patch with minimal build system changes in widget/gtk2, all build changes can be moved to widget/gtk2/gtk3 dir.

If gtk2 plugin support is the only reason why it might be better to split the directories, then, yes, let's not do directory splitting before gtk2 plugins are added.

(In reply to Karl Tomlinson (:karlt) from comment #75) > If gtk2 plugin support is the only reason why it might be better to split > the directories, then, yes, let's not do directory splitting before gtk2 > plugins are added. No, I'd like to do the split because it's just easier.

There's an updated patch for nsWindow and gtk2compat.h files. I hope I addressed all your comments, except: >+#if defined(MOZ_WIDGET_GTK3) && !defined(MOZ_PLUGIN_GTK2) >+#include <gtk/gtkx.h> >+#endif > #ifdef MOZ_X11 > #include <gdk/gdkx.h> > Something is not right here with including gtkx.h twice. They are two different headers, gdk/gdkx.h and gtk/gtkx.h. >+static PRInt32 >+GetBitmapStride(PRInt32 width) >+{ >+#ifdef MOZ_X11 >+ return (width+7)/8; >+#else >+ return cairo_format_stride_for_width(CAIRO_FORMAT_A1, width); >+#endif >+} I keep here the logic from ApplyTransparencyBitmap() - MOZ_X11 and !MOZ_X11 && GTK2 uses (width+7)/8 and GTK3 calls cairo_format_stride_for_width(). But is it possible to build GTK2/GTK3 without MOZ_X11? >@@ -3984,7 +4131,9 @@ nsWindow::Create(nsIWidget *aPare > // WM_TAKE_FOCUS, focus is requested on the parent window. > gtk_widget_realize(mShell); >- gdk_window_add_filter(mShell->window, >+#if defined(MOZ_WIDGET_GTK2) || defined(MOZ_PLUGIN_GTK2) >+ gdk_window_add_filter(gtk_widget_get_window(mShell), > popup_take_focus_filter, NULL); > #endif >+#endif > Any reason why this should be disabled with GTK3 for now? I disabled it in GTK3 because it's aimed to support plugins on GTK2. Enabled now, we can support it in GTK3 too. >@@ -6398,5 +6688,7 @@ void > nsWindow::DispatchRestoreEventAccessible(void) > { >+#if defined(MOZ_WIDGET_GTK2) || defined(MOZ_PLUGIN_GTK2) > DispatchEventToRootAccessible(nsIAccessibleEvent::EVENT_WINDOW_RESTORE); >+#endif > } > > I assume these are all TODO GTK3? We disabled them because Gnome Shell does not handle these events nicely. But let's enable it, there are other options than GS and they support them.

sorry, there were some missing nits, fixed now.

(In reply to Martin Stránský from comment #77) > But is it possible to build GTK2/GTK3 without MOZ_X11? That only ever worked with DirectFB, but now there's no reason to keep that working, if it still does even. (comment 59)

what patches do I need to try this against latest trunk?

Comment on attachment 557153 [details] [diff] [review] nsWindow.cpp - v3 This is looking very good. I'll attach some review comments. Most things should be easy to touch up. The one exception is dealing with unref'ed GdkWindows during draw_window_of_widget(). If that ends up getting complicated, then just leave a TODO. This is a great body of work and I'm keen to get it into mozilla-central.

I hope this addresses the comments. Changes to nsShmImage.h/nsShmImage.cpp are included. As for the draw_window_of_widget(), I marked it as TODO. btw. Is it a problem actually? If this or any other window is destroyed, get_window_for_gdk_window() or gdk_window_get_children() just returns NULL, right? Or are you concerned with already obtained list of windows from children list? With the GTK3/Xt plugins I meant "we don't care about Xt plugins" here :)

Comment on attachment 561682 [details] [diff] [review] nsWindow.cpp, v4 I think this can land for mozilla-central now (but please check it passes try server). (In reply to Martin Stránský from comment #83) > If this or any other window is destroyed, > get_window_for_gdk_window() or gdk_window_get_children() just returns NULL, > right? Yes, provided their GdkWindow parameter still exists. > Or are you concerned with already obtained list of windows from > children list? Yes, and gdk_window_get_children() is called after dispatching the event. Even events dispatched for painting can flush layout, causing JS notifications, and so anything can happen.

Comment on attachment 561701 [details] [diff] [review] nsWindow.cpp, v4, commit friendly version, r=karlt https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=59eea1f86cc9

added missing gtk2compat.h file

Comment on attachment 561718 [details] [diff] [review] nsWindow.cpp, v5, commit friendly version, r=karlt https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=489e44b5530a

Comment on attachment 561718 [details] [diff] [review] nsWindow.cpp, v5, commit friendly version, r=karlt Try run failed with: https://tbpl.mozilla.org/php/getParsedLog.php?id=6500092&tree=Try#error0 { In file included from ../../../../widget/src/gtk2/nsWindow.cpp:89:0: ../../../../widget/src/gtk2/gtk2compat.h: In function 'gboolean gdk_window_is_destroyed(GdkWindow*)': ../../../../widget/src/gtk2/gtk2compat.h:115:94: error: 'mGdkWindow' was not declared in this scope ../../../../widget/src/gtk2/gtk2compat.h: In function 'gint gdk_visual_get_depth(GdkVisual*)': ../../../../widget/src/gtk2/gtk2compat.h:123:10: error: 'depth' was not declared in this scope ../../../../widget/src/gtk2/nsWindow.cpp: In member function 'virtual gfxASurface* nsWindow::GetThebesSurface()': ../../../../widget/src/gtk2/nsWindow.cpp:6705:44: error: 'gdk_window_get_width' was not declared in this scope ../../../../widget/src/gtk2/nsWindow.cpp:6706:46: error: 'gdk_window_get_height' was not declared in this scope ../../../../widget/src/gtk2/gtk2compat.h: In function 'gint gdk_visual_get_depth(GdkVisual*)': ../../../../widget/src/gtk2/gtk2compat.h:124:1: error: control reaches end of non-void function ../../../../widget/src/gtk2/gtk2compat.h: In function 'gboolean gdk_window_is_destroyed(GdkWindow*)': ../../../../widget/src/gtk2/gtk2compat.h:116:1: error: control reaches end of non-void function make[7]: *** [nsWindow.o] Error 1 }

I hope I address the build issues. Unfortunately I don't have GTK2 box handy right now, will check it later.

Comment on attachment 561753 [details] [diff] [review] nsWindow.cpp, v6 Looks more promising (run not yet finished, but at least compiles): https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=341e2eb61f26 :-)

(In reply to Martin Stránský from comment #90) > Created attachment 561753 [details] [diff] [review] > nsWindow.cpp, v6 > > I hope I address the build issues. Unfortunately I don't have GTK2 box handy > right now, will check it later. I finished a gtk2 build with mozilla-central and only this patch. what other patches do I need to make a gtk3 build?

Comment on attachment 561753 [details] [diff] [review] nsWindow.cpp, v6 Passed try. Happy for me to land now?

(In reply to Ed Morley [:edmorley] from comment #93) > Comment on attachment 561753 [details] [diff] [review] > nsWindow.cpp, v6 > > Passed try. Happy for me to land now? Yes, Please. (In reply to Hussam Al-Tayeb from comment #92) > I finished a gtk2 build with mozilla-central and only this patch. what other > patches do I need to make a gtk3 build? You need a complete patch, this one is just a small step. The easiest way is to grab the complete one attached here (attachment #538243 [details] [diff] [review]), download mozilla-central from 2011-06-09, patch it and build.

Comment on attachment 561753 [details] [diff] [review] nsWindow.cpp, v6 https://hg.mozilla.org/integration/mozilla-inbound/rev/875cb4f20eac

This build fix is needed when MOZ_ENABLE_STARTUP_NOTIFICATION is enabled.

Comment on attachment 561999 [details] [diff] [review] build fix - startup notification Make it static inline, like the others. No need for the GDK_DRAWABLE() cast as GdkWindow is the same type as GdkDrawable. I'd prefer to drop the GDK_IS_WINDOW check too. I don't think that adds any value.

Fixed. I prefer to keep GDK_DRAWABLE() here, it's the original GTK 2.24 implementation.

Comment on attachment 562004 [details] [diff] [review] build fix - startup notification, v2 GDK_DRAWABLE() adds code for an extra function call per caller. The C type cast is pointless because the C types are the same. The GType check-instance is pointless because all GDK_TYPE_WINDOW objects are of type GDK_TYPE_DRAWABLE, and gdk_drawable_get_screen will check GDK_IS_DRAWABLE again anyway.

Okay, removed.

Comment on attachment 562011 [details] [diff] [review] build fix - startup notification, v3 Thanks. (In reply to Karl Tomlinson (:karlt) from comment #99) > GDK_DRAWABLE() adds code for an extra function call per caller. Actually two function calls: gdk_drawable_get_type and g_type_check_instance.

Comment on attachment 561753 [details] [diff] [review] nsWindow.cpp, v6 nsWindow.cpp part: https://hg.mozilla.org/mozilla-central/rev/875cb4f20eac

small intermission from a lurker: (In reply to Karl Tomlinson (:karlt) from comment #101) > > GDK_DRAWABLE() adds code for an extra function call per caller. > > Actually two function calls: gdk_drawable_get_type and g_type_check_instance. Only if you don't compile with -DG_DISABLE_CAST_CHECKS which people definitely should do for releases. In that case those casts are just casts. FWIW, using GNOME coding style, I'd make every cast use the macro both because of style ad because I know there's no performance difference, but huge debuggability improvements (once you turn it on).

Comment on attachment 562011 [details] [diff] [review] build fix - startup notification, v3 https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=003fcee02889 https://hg.mozilla.org/integration/mozilla-inbound/rev/2137c5bd3c36

Comment on attachment 562011 [details] [diff] [review] build fix - startup notification, v3 startup notification part: https://hg.mozilla.org/mozilla-central/rev/2137c5bd3c36

(In reply to Benjamin Otte from comment #103) > Only if you don't compile with -DG_DISABLE_CAST_CHECKS which people > definitely should do for releases. In that case those casts are just casts. We compile with C flags from pkgconfig, which do not seem to include this. I notice GTK+ does compile with -DG_DISABLE_CAST_CHECKS by default. I don't see any documentation, but it looks like this could only help. Seems a little strange to set an undocumented symbol that would change G_TYPE_CHECK_INSTANCE_CAST to not do what it says it does, but all g_type_check_instance_cast would do for us is print a warning (unless G_DEBUG is in the environment). It doesn't return null if the check fails.

It should address the remarks, my comments are at top of the patch.

Comment on attachment 563817 [details] [diff] [review] mozcontainer.c patch, v2 >+#include <gdk/gdkx.h> Surrounding the gdk_x11_* function definitions in gtk2compat.h with something like "#ifdef GDK_WINDOW_XDISPLAY" would make this include unnecessary. >- widget->style = gtk_style_attach (widget->style, widget->window); >+ gtk_widget_style_attach(widget); gtk_widget_style_attach is not needed in GTK3, so it may make sense to remove gtk_widget_style_attach from gtk2compat.h and make the gtk_style_attach line #ifdef MOZ_WIDGET_GTK2. >- >- GTK_WIDGET_SET_FLAGS (widget, GTK_MAPPED); >+ >- >- GTK_WIDGET_SET_FLAGS(widget, GTK_REALIZED); >+ Unnecessary extra white space added in the blank lines. >+ gtk_widget_set_window(widget, gdk_window_new (gtk_widget_get_parent_window (widget), >+ &attributes, attributes_mask)); Can you align &attributes with gtk_widget_get_parent_window, please?

Fixed comments, ready to commit.

Comment on attachment 563434 [details] [diff] [review] gtk drawing patch v.2 + comments >>> if (interior_focus) { >>>+ GtkBorder border; >>>+ gtk_style_context_get_border(style, 0, &border); >>Don't we want to pass the state flags here? > > What do you mean? gtk_style_context_get_border() "Gets the border for a given state [...]" and uses the "state" parameter rather than the state on the StyleContext. However, this code is not passing anything for the GtkStateFlags state parameter. Most, but not all, gtk_style_context_get_border calls in GTK widgets pass the state flags. >> gtk_tree_view_column_create_button uses GTK_SHADOW_IN. > > gtk_tree_view_column_create_button()? I don't see it anywhere. http://git.gnome.org/browse/gtk+/tree/gtk/gtktreeviewcolumn.c?id=3.2.0#n876 >> In moz_gtk_tab_paint: >>+ gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL); >> I assume _NORMAL is the default, so this is unnecessary. > > I don't see GTK_STATE_FLAG_NORMAL in moz_gtk_tab_paint(), it's only in: > > moz_gtk_progressbar_paint > moz_gtk_progress_chunk_paint > moz_gtk_tabpanels_paint > moz_gtk_check_menu_item_paint I must have pasted something in the wrong place there. I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL) calls are unnecessary. The pattern used in GTK is to create the style context with default state and only ever set the state with a save/restore during draw. Removing these would save a little code, but I don't feel strongly about it, if you prefer to leave them in. >>>+ gtk_render_frame_gap(style, cr, >>>+ rect->x - gap_loffset, >>>+ rect->y + gap_voffset - 3 * gap_height, >>>+ rect->width + gap_loffset + gap_roffset, >>>+ 3 * gap_height, GTK_POS_BOTTOM, >>>+ gap_loffset, rect->width); >>Need to also add gap_loffset here as you did below. > > Where the gap_loffset should be? The last parameter is a position rather than a width, and so needs to be gap_loffset + rect->width. >>>+ gtk_render_frame_gap(style, cr, >>>+ rect->x - gap_loffset, >>>+ rect->y + rect->height - gap_voffset, >>>+ rect->width + gap_loffset + gap_roffset, >>>+ 3 * gap_height, GTK_POS_TOP, >>>+ gap_loffset, gap_loffset+rect->width); >> Can you add spaces around the "+", please? > > Where? All "+" I see have spaces around. The last parameter is correct here, but there are no spaces around its "+". >>+ gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height); >> gtk_notebook_paint usually uses gtk_render_frame_gap. >> Why choose render_frame over render_frame_gap here? > > I haven't been able to decrypt the former code logic plus gtk_render_frame_gap() > does not allow to set negative gap start. gtk_render_frame() seems to work fine > too but if you have an idea how to transport the gap size to moz_gtk_tabpanels_paint() > I can modify it. The gap size is not needed in moz_gtk_tabpanels_paint because the gap will be painted with the foreground tab in moz_gtk_tab_paint. However, if moz_gtk_tabpanels_paint just uses gtk_render_frame(), the theme will think that there are no tabs and may draw something different. (Themes have done this in the past at least.) Hence the trick of using two clip regions, and drawing the gap outside each clip region, to get the correct frame for a tabpanel with tabs. With the API change from a cliprect parameter to a cairo_t, the clip rectangles for each half will need to be applied to the cairo_t, necessitating the use of cairo_save/restore. I suggest dividing rect into left and right halves instead of the approach for gtk2 of dividing the cliprect into two halves. (Getting the cliprect from cairo is harder and unnecessary.) For smaller tabpanels, the logic in the gtk2 port could lead to the gap being outside the rect. I suggest just using rect->width as the gap position in the render_frame_gap() call for the left half and 0 for the right half. >+static GtkStateFlags >+GetStateFlagFromGtkWidgetState(GtkWidgetState* state) Can you call this "GetStateFlagsFromGtkWidgetState", please? That would make it clearer that multiple flags may be set. >+ else >+ stateFlags |= GTK_STATE_FLAG_NORMAL; No need for this as GTK_STATE_FLAG_NORMAL is not a flag but 0, the empty state with no flags set.

Comment on attachment 564498 [details] [diff] [review] mozcontainer.c patch, v3 Backed out for causing build failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/62d490b0433f https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f09c204296f8 { In file included from ../../../../widget/src/gtk2/nsWindow.cpp:89:0: ../../../../widget/src/gtk2/gtk2compat.h:114:1: error: expected unqualified-id before '{' token ../../../../widget/src/gtk2/gtk2compat.h: In function 'void gtk_widget_set_can_focus(GtkWidget*, gboolean)': ../../../../widget/src/gtk2/gtk2compat.h:128:79: error: 'container' was not declared in this scope ../../../../widget/src/gtk2/gtk2compat.h:130:79: error: 'container' was not declared in this scope ../../../../widget/src/gtk2/gtk2compat.h: In function 'gboolean gtk_widget_get_visible(GtkWidget*)': ../../../../widget/src/gtk2/gtk2compat.h:134:1: error: redefinition of 'gboolean gtk_widget_get_visible(GtkWidget*)' ../../../../widget/src/gtk2/gtk2compat.h:95:1: error: 'gboolean gtk_widget_get_visible(GtkWidget*)' previously defined here ../../../../widget/src/gtk2/gtk2compat.h: At global scope: ../../../../widget/src/gtk2/gtk2compat.h:113:1: warning: 'void gtk_widget_set_allocation(GtkWidget*, const GtkAllocation*)' declared 'static' but never defined }

Uff, sorry for that, I really should build it on gtk2-2.10 box next time.

Comment on attachment 565231 [details] [diff] [review] mozcontainer.c patch, v5 >- widget->window = gdk_window_new (gtk_widget_get_parent_window (widget), >- &attributes, attributes_mask); >+ gtk_widget_set_window(widget, gdk_window_new (gtk_widget_get_parent_window (widget), >+ &attributes, attributes_mask)); Can you align &attributes with gtk_widget_get_parent_window, please?

Fixed indentation.

Comment on attachment 565464 [details] [diff] [review] mozcontainer.c patch, v6, fixed indentation In my queue, which is heading to try first then inbound :-) https://tbpl.mozilla.org/?tree=Try&rev=a8fbb2a76633

gtk_style_context_get_border uses the state wherever it's available now. > I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL) > calls are unnecessary. The pattern used in GTK is to create the style context > with default state and only ever set the state with a save/restore during > draw. Removing these would save a little code, but I don't feel strongly > about it, if you prefer to leave them in. I prefer to leave GTK_STATE_FLAG_NORMAL here for now, we can remove it / optimize later. I recall there were issues with rendering when GTK_STATE_FLAG_NORMAL was missing. > With the API change from a cliprect parameter to a cairo_t, the clip > rectangles for each half will need to be applied to the cairo_t, > necessitating the use of cairo_save/restore. I suggest dividing rect into > left and right halves instead of the approach for gtk2 of dividing the > cliprect into two halves. (Getting the cliprect from cairo is harder and > unnecessary.) > For smaller tabpanels, the logic in the gtk2 port could lead to the gap being > outside the rect. I suggest just using rect->width as the gap position in the > render_frame_gap() call for the left half and 0 for the right half. Updated. I hope I grasp the idea correctly.

Comment on attachment 565464 [details] [diff] [review] mozcontainer.c patch, v6, fixed indentation https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f8b4c3c12a

Comment on attachment 565519 [details] [diff] [review] gtk drawing patch v.3 It looks like many (but not all) of the changes between attachment 525346 [details] [diff] [review] and attachment 540027 [details] [diff] [review] have been reverted or lost in this version of the patch. I don't know whether or not you have a easy way of merging those back in. It may be easiest to go through attachment 533890 [details] again and check that each issue that you addressed is still addressed. (Separate vpaned/hpaned functions is fine.)

Uff, you're right, there were some missing parts from previous patch. Some remarks: >moz_gtk_entry_paint: >+ //gtk_render_background(style, cr, rect->x + x, rect->y + y, rect->width - 2*x, rect->height - 2*y); >+ //gtk_render_frame(style, cr, rect->x + x, rect->y + y, rect->width - 2*x, rect->height - 2*y); > Remove this if it is not used or add a TODO comment explaining why it is kept. > I can't make much sense of what is happening with the focus_width and the > rectangles in this function. The old logic looked closer to what we are going > to need, so I'd prefer to stay with that until this is sorted out. Removed. In moz_gtk_treeview_paint: >- gtk_widget_modify_bg(gTreeViewWidget, state_type, >- &gTreeViewWidget->style->base[state_type]); >+ gtk_style_context_get_background_color(style, state_type, &base_col); >+ gtk_widget_override_background_color(gScrolledWindowWidget, state_type, &base_col); > >Is this necessary? >If so, it looks like it should be applied to the TreeView widget. Not necessary, removed. >+ gtk_render_frame(style_tree, cr, >+ rect->x + xthickness, rect->y + ythickness, >+ rect->width - 2 * xthickness, >+ rect->height - 2 * ythickness); > >Is this necessary? >The only gtk_render_frame I see in GtkScrolledWindow is for the rubber band. Not necessary, removed. moz_gtk_tab_paint: > Are you sure about GTK_STATE_FLAG_INSENSITIVE for unselected tabs? Where does > that come from? I would have expected _NORMAL. It doesn't matter, changed to _NORMAL. > moz_gtk_menu_separator_paint: > + gtk_style_context_add_class(style, GTK_STYLE_CLASS_DEFAULT); > What is this for? Some leftover I guess, removed.

Note that http://hg.mozilla.org/releases/mozilla-beta/rev/875cb4f20eac seems to cause bug 709259 on GTK2 builds

Will those patches get merged to any UX build for public testing any time soon? :)

A minor query about gComboBoxEntryWidget = NULL; /* TODO - gtk_combo_box_entry_new();*/ in patch from comment 122. What would be wrong with using gtk_combo_box_new_with_entry there ?

I haven't quite looked at everything you changed here, but most of it, and I'll be on vacation for a week or two, so I'll post what I have.

Comment on attachment 540031 [details] [diff] [review] nsLookAndFeel.cpp patch I've been taking too long to get to these, so I've asked Chris to review these two patches. I'll review an updated gtk3drawing patch, as I've got to know that code quite well.

Thanks, there's an updated version. Some remarks: > Can you mention removal of the unused cliprect parameters to the TODO list > please? Added before moz_gtk_widget_paint(). >The old logic was different from what GetStateFlagsFromGtkWidgetState does, >which makes me think that GetStateFlagsFromGtkWidgetState should set >GTK_STATE_FLAG_PRELIGHT (when inHover) and GTK_STATE_FLAG_ACTIVE (when >depressed or active) independently of each other. Updated the GetStateFlagsFromGtkWidgetState(), it composes the flags now. > In moz_gtk_gripper_paint: > gtk_handle_box_paint also calls gtk_render_frame. Yeah. gtk_handle_box_paint draws gtk_render_handle() differently, regards to the handle_position. But the effective_handle_position() (from gtk_handle_box_paint) uses GtkHandleBoxPrivate which seems to be hidden/private.

(In reply to Rafał Mużyło from comment #125) > A minor query about > gComboBoxEntryWidget = NULL; /* TODO - gtk_combo_box_entry_new();*/ > in patch from comment 122. > What would be wrong with using gtk_combo_box_new_with_entry there ? I expect that would be the correct widget to use. However, I'm guessing it is not as simple as just changing the widget, but other code will also need changing. That can be sorted out separately.

Comment on attachment 595369 [details] [diff] [review] gtk drawing patch v.5 Thanks very much. Just a save/restore pair to remove in moz_gtk_combo_box_paint: >- gtk_paint_arrow(style, drawable, state_type, shadow_type, cliprect, >- gComboBoxArrowWidget, "arrow", GTK_ARROW_DOWN, TRUE, >- real_arrow_rect.x, real_arrow_rect.y, >- real_arrow_rect.width, real_arrow_rect.height); >- >+ style = gtk_widget_get_style_context(gComboBoxArrowWidget); >+ gtk_style_context_save(style); >+ >+ gtk_render_arrow(style, cr, ARROW_DOWN, >+ real_arrow_rect.x, real_arrow_rect.y, >+ real_arrow_rect.width); This is no longer applying the state when drawing the arrow, but that seems to be correct, because I can't see that GtkComboBox passes its state to the style context for gtk_render_arrow. > if (!gComboBoxSeparatorWidget) > return MOZ_GTK_SUCCESS; >+ gtk_style_context_restore(style); This save/restore is not balanced when taking the early return, but is unnecessary anyway because the style context was not changed. The save/restore for the style context of the gComboBoxSeparatorWidget is similarly unnecessary.

(In reply to Martin Stránský from comment #129) > Yeah. gtk_handle_box_paint draws gtk_render_handle() differently, > regards to the handle_position. But the effective_handle_position() (from > gtk_handle_box_paint) uses GtkHandleBoxPrivate which seems to be > hidden/private. The old gtk2drawing code was apparently only painting the box and not the handle. i.e. it didn't call gtk_paint_handle. So perhaps we should drop the gtk_render_handle from moz_gtk_gripper_paint to match that. I don't know where or if this is actually used.

Removed the unnecessary save/restore and gtk_render_handle().