Closed Bug 627699 (gtk3) Opened 13 years ago Closed 7 years ago

[meta] Port GTK2 to GTK3

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
blocking-basecamp -

People

(Reporter: jhorak, Assigned: stransky)

References

(Depends on 32 open bugs, Blocks 3 open bugs)

Details

(Keywords: meta, Whiteboard: [leave open as a tracking bug][please file new bugs for additional patches]])

Attachments

(26 files, 66 obsolete files)

5.44 KB, patch
Details | Diff | Splinter Review
96.89 KB, patch
emorley
: checkin+
Details | Diff | Splinter Review
842 bytes, patch
karlt
: review+
emorley
: checkin+
Details | Diff | Splinter Review
11.91 KB, patch
stransky
: review+
emorley
: checkin+
Details | Diff | Splinter Review
138.70 KB, patch
karlt
: review+
emorley
: checkin+
Details | Diff | Splinter Review
5.71 KB, patch
stransky
: review+
roc
: checkin+
Details | Diff | Splinter Review
669 bytes, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.73 KB, patch
benjamin
: review+
darktrojan
: checkin+
Details | Diff | Splinter Review
24.50 KB, patch
stransky
: review+
darktrojan
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
karlt
: review+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
25.18 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
838 bytes, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
22.17 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
115.96 KB, patch
Details | Diff | Splinter Review
2.76 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
25.52 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
43.52 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
723 bytes, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
5.34 KB, patch
Details | Diff | Splinter Review
1.15 KB, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
18.14 KB, patch
karlt
: review-
Details | Diff | Splinter Review
7.47 KB, patch
karlt
: review-
Details | Diff | Splinter Review
31.59 KB, patch
karlt
: review+
Details | Diff | Splinter Review
31.51 KB, patch
stransky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
846 bytes, patch
karlt
: review+
stransky
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review
Attached patch Very WIP patch (obsolete) — Splinter Review
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).
OS: Linux → Windows Server 2003
OS: Windows Server 2003 → Linux
Attached patch wip v2 (obsolete) — Splinter Review
updated version, against trunk, builds and runs. A gtk widget implementation is not finished yet, background is not rendered properly, sometimes crashes.
Attachment #505773 - Attachment is obsolete: true
Assignee: nobody → stransky
Attached patch wip v3 (obsolete) — Splinter Review
fixed background rendering and style borders
Attachment #513089 - Attachment is obsolete: true
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.
Attached patch wip v4 (obsolete) — Splinter Review
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.
Attachment #513457 - Attachment is obsolete: true
Attached patch WIP v5 (obsolete) — Splinter Review
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.
Attachment #515080 - Attachment is obsolete: true
Attached patch WIP v6 (obsolete) — Splinter Review
Wrong patch, reuploading.
Attachment #519093 - Attachment is patch: true
Attachment #519093 - Attachment mime type: application/octet-stream → text/plain
Attachment #518712 - Attachment is obsolete: true
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.
Attached patch WIP v7 (obsolete) — Splinter Review
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.
Attachment #519093 - Attachment is obsolete: true
Is the patch diffed against the RC tarball or latest HG? because I couldn't get it to patch against mozilla-central.
Status: NEW → ASSIGNED
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).
Depends on: 624422
(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.
Attached patch WIP v8 (obsolete) — Splinter Review
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.
Attachment #519919 - Attachment is obsolete: true
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.)
Attached patch WIP v9 (obsolete) — Splinter Review
Attaching rebased patch:
- Martin made gtk2 plugins in gtk3 code possible!
- Fixed custom mouse cursors
Attachment #521184 - Attachment is obsolete: true
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.
Attached patch gtk3 directory patch v1 (obsolete) — Splinter Review
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.
Attachment #525346 - Flags: review?(karlt)
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.
Attachment #523284 - Attachment is obsolete: true
Attachment #525346 - Attachment is obsolete: true
Attachment #525346 - Flags: review?(karlt)
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
Attached patch gtk drawing patch (obsolete) — Splinter Review
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.
Attachment #540027 - Flags: review?(karlt)
Attached patch mozcontainer.c patch (obsolete) — Splinter Review
patch to widget/src/gtk2/mozcontainer.c
Attachment #540030 - Flags: review?(karlt)
Attached patch nsLookAndFeel.cpp patch (obsolete) — Splinter Review
Attachment #540031 - Flags: review?(karlt)
Attached patch gtk2/nsWindow.cpp patch (obsolete) — Splinter Review
Attachment #540032 - Flags: review?(karlt)
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.
Attachment #540034 - Flags: review?(karlt)
Attached patch rest of gtk2 dir fixes (obsolete) — Splinter Review
Attachment #540036 - Flags: review?(karlt)
Attached file gtk3drawing review comments 2 (obsolete) —
Attachment #533890 - Attachment is obsolete: true
Attachment #540027 - Flags: review?(karlt) → review-
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.)
Attachment #540030 - Flags: review?(karlt) → review-
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
Attached file nsWindow drawing review comments (obsolete) —
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.
Attachment #540032 - Flags: review?(karlt) → review-
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.
Blocks: 397700
No longer blocks: 397700
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
Attachment #540034 - Flags: review?(karlt)
(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.
Attached patch v2. nsWindow/gtk2compat.h patch (obsolete) — Splinter Review
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.
Attachment #540032 - Attachment is obsolete: true
Attachment #557147 - Flags: review?(karlt)
Attached patch nsWindow.cpp - v3 (obsolete) — Splinter Review
sorry, there were some missing nits, fixed now.
Attachment #557147 - Attachment is obsolete: true
Attachment #557153 - Flags: review?(karlt)
Attachment #557147 - Flags: review?(karlt)
(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?
Attachment #543317 - Attachment is obsolete: true
Attachment #542725 - Attachment is obsolete: true
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.
Attachment #557153 - Flags: review?(karlt)
Attachment #557153 - Flags: review-
Attachment #557153 - Flags: feedback+
Attached file nsWindow review comments 2 (obsolete) —
Attached patch nsWindow.cpp, v4 (obsolete) — Splinter Review
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 :)
Attachment #557153 - Attachment is obsolete: true
Attachment #561682 - Flags: review?(karlt)
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.
Attachment #561682 - Flags: review?(karlt) → review+
Attachment #561418 - Attachment is obsolete: true
Attachment #561682 - Attachment is obsolete: true
Attachment #561701 - Flags: review+
Attachment #561701 - Flags: checkin?
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches]
Keywords: checkin-needed
added missing gtk2compat.h file
Attachment #561718 - Flags: review+
Attachment #561718 - Flags: checkin?
Attachment #561701 - Attachment is obsolete: true
Attachment #561701 - Flags: checkin?
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
}
Attachment #561718 - Flags: review+
Attachment #561718 - Flags: checkin?
Attached patch nsWindow.cpp, v6Splinter Review
I hope I address the build issues. Unfortunately I don't have GTK2 box handy right now, will check it later.
Attachment #561718 - Attachment is obsolete: true
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.
Attached patch build fix - startup notification (obsolete) — Splinter Review
This build fix is needed when MOZ_ENABLE_STARTUP_NOTIFICATION is enabled.
Attachment #561999 - Flags: review?(karlt)
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.
Attachment #561999 - Flags: review?(karlt) → review+
Fixed. I prefer to keep GDK_DRAWABLE() here, it's the original GTK 2.24 implementation.
Attachment #561999 - Attachment is obsolete: true
Attachment #562004 - Flags: review?(karlt)
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.
Attachment #562004 - Flags: review?(karlt) → review-
Okay, removed.
Attachment #562004 - Attachment is obsolete: true
Attachment #562011 - Flags: review?(karlt)
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.
Attachment #562011 - Flags: review?(karlt) → review+
Attachment #562011 - Flags: checkin?
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).
Keywords: checkin-needed
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.
Attached patch gtk drawing patch v.2 + comments (obsolete) — Splinter Review
It should address the remarks, my comments are at top of the patch.
Attachment #540027 - Attachment is obsolete: true
Attachment #563434 - Flags: feedback?(karlt)
Attached patch mozcontainer.c patch, v2 (obsolete) — Splinter Review
Attachment #540030 - Attachment is obsolete: true
Attachment #563817 - Flags: review?(karlt)
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?
Attachment #563817 - Flags: review?(karlt) → review+
Attached patch mozcontainer.c patch, v3 (obsolete) — Splinter Review
Fixed comments, ready to commit.
Attachment #563817 - Attachment is obsolete: true
Attachment #564498 - Flags: review+
Attachment #564498 - Flags: checkin?
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
}
Attachment #564498 - Flags: review-
Attachment #564498 - Flags: review+
Attachment #564498 - Flags: checkin?
Attached patch mozcontainer.c patch, v5 (obsolete) — Splinter Review
Uff, sorry for that, I really should build it on gtk2-2.10 box next time.
Attachment #564498 - Attachment is obsolete: true
Attachment #565231 - Flags: review+
Attachment #565231 - Flags: checkin?
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?
Attachment #565231 - Flags: checkin?
Fixed indentation.
Attachment #565231 - Attachment is obsolete: true
Attachment #565464 - Flags: review+
Attachment #565464 - Flags: checkin?
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
Attachment #565464 - Flags: checkin?
Keywords: checkin-needed
Attached patch gtk drawing patch v.3 (obsolete) — Splinter Review
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.
Attachment #542343 - Attachment is obsolete: true
Attachment #563434 - Attachment is obsolete: true
Attachment #565519 - Flags: review?(karlt)
Attachment #563434 - Flags: feedback?(karlt)
Attachment #533890 - Attachment is obsolete: false
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.)
Attachment #565519 - Flags: review?(karlt)
Attached patch gkt3drawing, v.4 (obsolete) — Splinter Review
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.
Attachment #565519 - Attachment is obsolete: true
Attachment #568338 - Flags: review?(karlt)
Depends on: 709259
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 ?
Attached file review comments on gkt3drawing, v.4 (obsolete) —
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.
Attachment #533890 - Attachment is obsolete: true
Attachment #588315 - Attachment is patch: false
Attachment #588315 - Attachment is obsolete: true
Attachment #568338 - Flags: review?(karlt) → review-
Blocks: 723754
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.
Attachment #540031 - Flags: review?(karlt) → review?(chrisccoulson)
Attachment #540036 - Flags: review?(karlt) → review?(chrisccoulson)
Attached patch gtk drawing patch v.5 (obsolete) — Splinter Review
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.
Attachment #568338 - Attachment is obsolete: true
Attachment #593982 - Attachment is obsolete: true
Attachment #595369 - Flags: review?(karlt)
(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.
Attachment #595369 - Flags: review?(karlt) → review-
(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.
Attached patch gkt3drawing, v.6Splinter Review
Removed the unnecessary save/restore and gtk_render_handle().
Attachment #595369 - Attachment is obsolete: true
Attachment #595699 - Flags: review?(karlt)
Attachment #595699 - Flags: review?(karlt) → review+
Attachment #595699 - Flags: checkin?
Attachment #595699 - Flags: checkin? → checkin+
Depends on: 726483
Blocks: wayland
(In reply to Diego Viola from 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.

You're right since GTK+2 is not Wayland-capable, unlike GTK+3.
Therefore having Firefox running on GTK+3 will ultimately allow Firefox to run on Wayland (which could be deployed in 2013 regarding how fast the project evolves at present time).

BTW, a live CD allows to test application on Wayland http://phoronix.com/forums/showthread.php?69394-Wayland-s-Weston-Lands-In-Ubuntu-12-04-LTS&p=254064#post254064
I guess another bug could be opened - once this one is closed - to check the Wayland compatibility of Firefox-gtk+3

End of the Wayland parenthesis. Thank you guys for your work on Firefox-GTK+ :-)
Sorry, I've had a few crazy weeks :(

I'll get to look at these patches this evening
I've just noticed that the epiphany (default Gnome3 browser) is built against GTK3 only and uses nspluginwrapper to run the gtk2 plugins. So it would be relatively easy to provide small wrapper based on GTK2 instead of the recent approach (an extra libxul for gkt2).
I thought that Flash plugin was broken with Epiphany-GTK+3 and that therefore they were waiting for WebkitGTK+2 to be able to run it in another process ?
I thought that Flash plugin was broken with Epiphany-GTK+3 and that therefore they were waiting for WebkitGTK+2 to be able to run it in another process ? (But maybe this unrelated to this bug)
Comment on attachment 540031 [details] [diff] [review]
nsLookAndFeel.cpp patch

Review of attachment 540031 [details] [diff] [review]:
-----------------------------------------------------------------

Hi,

Sorry for taking a while to get to this, I've had a crazy few weeks. A few things have changed since this patch was written, so it needs updating quite a bit (eg, bug 679226) . But I'll comment on what's here already.

::: widget/src/gtk2/nsLookAndFeel.h
@@ +112,5 @@
> +#else
> +        NS_ASSERTION(!mStyleWidget, "already initialized");
> +
> +        mStyleWidget = gtk_invisible_new();
> +        gtk_widget_realize(mStyleWidget);

The gtk_widget_realize isn't necesssary. In gtk2 it was required to guarantee that the widget had a valid GtkStyle, but it isn't required to guarantee that it has a GtkStyleContext in gtk3

@@ +116,5 @@
> +        gtk_widget_realize(mStyleWidget);
> +
> +        mStyle = gtk_widget_get_style_context(mStyleWidget);
> +#endif
> +    } 

This whole block has been moved in to nsLookAndFeel.cpp now, so we can drop the #ifdef from this header file

::: widget/src/gtk2/nsLookAndFeel.cpp
@@ +53,4 @@
>  #define GDK_COLOR_TO_NS_RGB(c) \
>      ((nscolor) NS_RGB(c.red>>8, c.green>>8, c.blue>>8))
> +#define GDK_RGBA_TO_NS_RGB(c) \
> +    ((nscolor) NS_RGBA((int)(c.red*255), (int)(c.green*255), (int)(c.blue*255), (int)(c.alpha*255)))

It looks like we can get rid of GDK_COLOR_TO_NS_RGB

@@ +104,4 @@
>  {
>      nsresult res = NS_OK;
> +    gtk_style_context_save(mStyle);
> +    aColor = NS_RGB(0xff, 0xff, 0x00);

This last line doesn't look like it's needed

@@ +118,1 @@
>          break;

This (and eColorID_TextBackground) used to use the same color as eColorID__moz_eventreerow and eColorID__moz_field, but now it uses the same color as eColorID_WidgetBackground. Was this change intentional?

One other thing to remember as well is that themes in gtk3 are able to define background images and gradients as opposed to solid colors, so I'm not sure we can make the assumption that the theme will set a solid color. There are already some widgets we are using for retrieving style data which are using gradients in Ambiance. I need to have a bit of a think about how to handle that, so we should probably have a follow-up bug for now

@@ +119,5 @@
>      case eColor_WindowForeground:
> +        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);
> +        gtk_style_context_get_color(mStyle, GTK_STATE_FLAG_NORMAL, &gdk_color);
> +        aColor = GDK_RGBA_TO_NS_RGB(gdk_color);
> +        aColor = NS_RGB(0, 0xff, 0);

I guess this last line is just leftover from debugging? There's a few more of these below too

As above, this (and eColorID_TextForeground) did used to use the same color as eColorID__moz_fieldtext, but now uses the same color as eColor_WidgetForeground. I'm not sure if that change was intentional?

Also, this doesn't work correctly on Ubuntu's default theme, which does this:

* {
  color: inherit;
}

GtkWindow {
  color: @fg_color;
}

... and there aren't any other selectors that match a GtkInvisible widget with just a background class (as this doesn't correspond to any real widget in gtk).

This seems to be the case with Adwaita too (Adwaita doesn't appear to have any background class selectors at all).

Rather than creating a GtkInvisible widget and using its style context, we could just manually create our own style context like this to make it look like a GtkWindow:

GtkStyleContext *context = gtk_style_context_new();

GtkWidgetPath *path = gtk_widget_path_new();
gtk_widget_path_append_type(path, GTK_TYPE_WINDOW);

gtk_style_context_set_path(path);

This seems to work here.

Note that all properties can be inherited from parent widgets in gtk3, which is an additional challenge over gtk2 (although, the use of inheritance seems to be restricted to foreground color in Adwaita and Ambiance. But I haven't looked at any other themes). I think the only way we can be confident that we get a valid color from a theme which uses inheritance is to use a style context from a widget that is either a GtkWindow, or inside a GtkWindow.

@@ +131,5 @@
>      case eColor_WidgetForeground:
> +        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);
> +        gtk_style_context_get_color(mStyle, GTK_STATE_FLAG_NORMAL, &gdk_color);
> +        aColor = GDK_RGBA_TO_NS_RGB(gdk_color);
> +        //aColor = GDK_COLOR_TO_NS_RGB(mStyle->fg[GTK_STATE_NORMAL]);

Can we drop the commented lines? This applies to a few places further down too

@@ +148,1 @@
>          break;

For consistency, these 2 (eColor_WidgetSelectBackground and eColor_WidgetSelectForeground) should probably use GTK_STYLE_CLASS_BACKGROUND (the same class as eColor_WidgetBackground and eColor_WidgetForeground)

@@ +161,1 @@
>          break;

See my earlier comment for eColor_WindowBackground

@@ +167,1 @@
>          break;

..and the earlier comment for eColor_WindowForeground

@@ +245,5 @@
>          // disabled text in windows, menus, etc.
> +        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BUTTON);
> +        gtk_style_context_get_color(mStyle, GTK_STATE_FLAG_INSENSITIVE, &gdk_color);
> +        aColor = GDK_RGBA_TO_NS_RGB(gdk_color);
> +        //aColor = GDK_COLOR_TO_NS_RGB(mStyle->fg[GTK_STATE_INSENSITIVE]);

Was there a particular reason for using GTK_STYLE_CLASS_BUTTON here?

@@ +265,1 @@
>          break;

For consistency, shouldn't these last 2 use GTK_STYLE_CLASS_VIEW?

@@ +307,2 @@
>          // scrollbar gray area
> +        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);

I think you want both GTK_STYLE_CLASS_SCROLLBAR and GTK_STYLE_CLASS_TROUGH here. This seems to work fine here on Adwaita. It doesn't work on Ambiance though, which uses a gradient (see my earlier comment). On Ambiance, this just returns solid white.

@@ +342,5 @@
>      case eColor_threeddarkshadow:
>          // 3-D shadow outer edge color
> +        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);
> +        gtk_style_context_get_border_color(mStyle, GTK_STATE_FLAG_NORMAL, &gdk_color);
> +        // TODO aColor = GDK_COLOR_TO_NS_RGB(mStyle->black);

You can just hardcode this to black, as it was already in gtk2

@@ +376,1 @@
>          break;

This produces the correct colors on Adwaita (white background), but fails on Ambiance because it only matches the universal selector (the background color should be white on Ambiance too, but it just ends up being the same color as the top-level GtkWindow). Ambiance seems to assume that all widgets with the "view" class are implementations of GtkIconView, GtkTextView, GtkTreeView or GtkCalendar.

The only way to make this work properly without modifying the theme is to use a more appropriate widget (eg, GtkTextView) or manually create the appropriate widget path. But you'd want to do that in InitLookAndFeel() to avoid doing it on each call of NativeGetColor().

@@ +376,2 @@
>          break;
> +    case eColor__moz_dialog: // FIXME RETURN BLACK

I'm not sure I understand the comment here :)

@@ +383,2 @@
>          break;
> +    case eColor__moz_dialogtext: // BLACK, seems to be ok

And here

@@ +431,1 @@
>          break;

See my earlier comment for eColor__moz_fieldtext (which also uses GTK_STYLE_CLASS_VIEW)

@@ +461,1 @@
>          res    = NS_ERROR_FAILURE;

I don't think that second assignment to aColor is meant to be there

@@ +481,3 @@
>      green *= 0.93;
>      blue *= 0.93;
> +    */

Please delete the commented out block

@@ +732,5 @@
>  nsLookAndFeel::InitLookAndFeel()
>  {
> +    GtkWidget *style_widget = gtk_invisible_new();
> +    gtk_widget_realize(style_widget);
> +    GtkStyleContext *context = gtk_widget_get_style_context(style_widget);

This doesn't need to be realized. Also, is there any reason not to just use the existing style context (mStyle)?

@@ +742,5 @@
> +    gtk_style_context_get_background_color(context, GTK_STATE_FLAG_NORMAL, &color);
> +    sInfoBackground = GDK_RGBA_TO_NS_RGB(color);
> +    gtk_style_context_get_color(context, GTK_STATE_FLAG_NORMAL, &color);
> +    sInfoText = GDK_RGBA_TO_NS_RGB(color);
> +    gtk_style_context_restore(context);

This is another one which uses a gradient on Ambiance :(

@@ +754,1 @@
>  

On both Ambiance and Adwaita, this only matches the universal selector, which sets "color: inherit", and ends up returning white. The "accelerator" class isn't all that meaningful on its own without any other context (like, what is the container widget?).

What we really want here is menu text color, and the selector in Adwaita that sets the menu text color is ".menuitem .accelerator". To make sure this works properly for most themes, you need to set up an appropriate widget path on the style context by creating a GtkMenuItem and adding a GtkAccelLabel to it (a bit like before).

@@ +770,1 @@
>  

The same applies to the menu and menuitem styling

@@ +791,1 @@
>  

Adwaita seems to have specific styling for the combobox which isn't picked up here. The only way to do that is to create a real combobox, or manually set the correct widget path (creating a combobox is easier).

@@ +795,5 @@
> +    gtk_style_context_get_color(context, GTK_STATE_FLAG_NORMAL, &color);
> +    sMenuBarText = GDK_RGBA_TO_NS_RGB(color);
> +    gtk_style_context_get_color(context, GTK_STATE_FLAG_ACTIVE, &color);
> +    sMenuBarHoverText = GDK_RGBA_TO_NS_RGB(color);
> +    gtk_style_context_restore(context);

TBH, I'd change this section to do pretty much what used to happen before (creating real widget hierarchies), as this is the most reliable way to ensure you end up with the style that's expected. Remember, you don't actually need to realize any widgets in gtk3 to guarantee the existence of a style context.

@@ +834,5 @@
> +    // Get even row background color
> +    gtk_style_context_save(context);
> +    gtk_style_context_add_class(context, GTK_STYLE_CLASS_VIEW);
> +    gtk_style_context_add_region(context, GTK_STYLE_REGION_ROW, GTK_REGION_EVEN);
> +    gtk_style_context_get_background_color(context, GTK_STATE_FLAG_NORMAL, &color);

This is really only applicable to GtkTreeView widgets, whereas the "view" class is used for other widgets too. This means it is quite likely that themes will use a type selector for this (and Ambiance does seem to), so you would be better off just creating a real GtkTreeView again.
Attachment #540031 - Flags: review?(chrisccoulson) → review-
Attached patch fix drawing&window parts (obsolete) — Splinter Review
I managed to update the whole patch to the latest trunk. There are some fixes needed in the already checked code, would be great to have it there.
Attachment #626372 - Flags: review?(karlt)
Comment on attachment 626372 [details] [diff] [review]
fix drawing&window parts

>-    if (!mShell || !mShell->window ||
>-        !gdk_property_get(mShell->window,
>+    if (!mShell || !gtk_widget_get_window(mShell) ||
>+        !gdk_property_get(gtk_widget_get_window(mShell),

Please don't add two function calls to get the same information when the
information won't change.  Store the window in a local variable.

You may be able to use something like this here:

  if (!mShell || !(GdkWindow* window = gtk_widget_get_window(mShell)) ||
      !gdk_property_get(window,
      

>-        /* TODO */
>-        gTreeHeaderCellWidget = NULL;
>-        gTreeHeaderSortArrowWidget = NULL;
>+        /* TODO, but they can't be NULL */
>+        gTreeHeaderCellWidget = gtk_button_new_with_label("M");
>+        gTreeHeaderSortArrowWidget = gtk_button_new();

Please either leave gTreeHeaderCellWidget and gTreeHeaderSortArrowWidget null
at this stage, or add the necessary destroy calls to moz_gtk_shutdown.
(Comment 56)

>+GdkVisual*
>+moz_gtk_widget_get_visual()

This shouldn't be necessary.
(Comment 39)
Attachment #626372 - Flags: review?(karlt) → review-
Attached patch fix drawing&window parts, v.2 (obsolete) — Splinter Review
Thanks, this should fix that.

>>+GdkVisual*
>>+moz_gtk_widget_get_visual()
>> This shouldn't be necessary. (Comment 39)

It's used by gfxXlibNativeRenderer::Draw(), we derive Visual* and Screen* from it so it's necessary now and the code crashes with NULL pointer passed there.
Attachment #626372 - Attachment is obsolete: true
Attachment #626756 - Flags: review?(karlt)
Comment on attachment 626756 [details] [diff] [review]
fix drawing&window parts, v.2

> >>+GdkVisual*
> >>+moz_gtk_widget_get_visual()
> >> This shouldn't be necessary. (Comment 39)
> 
> It's used by gfxXlibNativeRenderer::Draw(), we derive Visual* and Screen*
> from it so it's necessary now and the code crashes with NULL pointer passed
> there.

Ah, OK. Draw() does need a visual, but it doesn't need to match that of any
GtkWidget.  The default Screen* and Visual* will be fine.  (GDK refers to the
default visual the system_visual.)

With GTK3, we don't actually need gfxXlibNativeRenderer at all, because any
cairo_t is fine, but that can be a later incremental improvement.

r+ on the other changes.
Attachment #626756 - Flags: review?(karlt) → review+
A patch for check-in, removed the moz_gtk_widget_get_visual().
Attachment #626756 - Attachment is obsolete: true
Attachment #627143 - Flags: review+
Attachment #627143 - Flags: checkin?
Comment on attachment 627143 [details] [diff] [review]
fix drawing&window parts, v.2, for check-in

Review of attachment 627143 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1dbb75d232
Attachment #627143 - Flags: checkin? → checkin+
Attached patch nsLookAndFeel v.2 (obsolete) — Splinter Review
The updated one.
Attachment #540031 - Attachment is obsolete: true
Attachment #627866 - Flags: review?(chrisccoulson)
Attached patch gfx patch (obsolete) — Splinter Review
Patch to gfx, without makefile changes. I wonder who is a right reviewer here, maybe roc?
Attachment #627965 - Flags: review?(roc)
Comment on attachment 627965 [details] [diff] [review]
gfx patch

Review of attachment 627965 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderEGL.cpp
@@ +7,5 @@
>  // please add new includes below Qt, otherwise it break Qt build due malloc wrapper conflicts
>  
>  #if defined(XP_UNIX)
>  
> +#if defined(MOZ_WIDGET_GTK2) || defined(MOZ_WIDGET_GTK3)

How about defining MOZ_WIDGET_GTK somewhere and using that?

Or even making it a numeric macro and using #if MOZ_WIDGET_GTK >= 2?

::: gfx/thebes/gfxXlibNativeRenderer.cpp
@@ +434,5 @@
> +PRBool
> +gfxXlibNativeRenderer::DrawOntoTempSurface(cairo_t *cr,
> +                                           nsIntPoint offset)
> +{
> +    PRBool tt = cairo_version() < CAIRO_VERSION_ENCODE(1, 4, 0);

tt is unused?

@@ +438,5 @@
> +    PRBool tt = cairo_version() < CAIRO_VERSION_ENCODE(1, 4, 0);
> +
> +	cairo_surface_t *target = cairo_get_group_target (cr);
> +
> +	cairo_surface_flush (target);

Fix indent
Attached patch gfx, v.2 (obsolete) — Splinter Review
Removed the cairo check (I'm going to move it to configure) and fixed indentation. 

> How about defining MOZ_WIDGET_GTK somewhere and using that?
> Or even making it a numeric macro and using #if MOZ_WIDGET_GTK >= 2?

Sounds good. But we have already some code checked in with MOZ_WIDGET_GTK2/MOZ_WIDGET_GTK3 combo so I prefer to use it for now and then change it at once.
Attachment #627965 - Attachment is obsolete: true
Attachment #627965 - Flags: review?(roc)
Attachment #628255 - Flags: review?(roc)
Comment on attachment 628255 [details] [diff] [review]
gfx, v.2

Thanks!
Attachment #628255 - Flags: checkin?
This one adds basic GTK3 support to DOM code. It does not contain changes for plug-ins, the recent trunk patch implements it as an extra plugin-container with extra libxul which should be done better.
Attachment #628318 - Flags: review?(jst)
Attached patch xpcom gtk3 update (obsolete) — Splinter Review
Patch to /xpcom, without makefile changes. Benjamin, can you check it please?
Attachment #628324 - Flags: review?(benjamin)
Attached patch gtl3 patch to /toolkit (obsolete) — Splinter Review
Patch to toolkit.
Attachment #628327 - Flags: review?(benjamin)
Comment on attachment 628324 [details] [diff] [review]
xpcom gtk3 update

I talked with Ted about this, and I don't see any reason to change this to MOZ_WIDGET_GTK3 and then change it again, since the MOZ_WIDGET_GTK3 support hasn't landed in configure yet. Please submit a patch with MOZ_WIDGET_GTK.
Attachment #628324 - Flags: review?(benjamin) → review-
Attachment #628327 - Flags: review?(benjamin) → review-
Attachment #628255 - Flags: checkin?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #157)
> I talked with Ted about this, and I don't see any reason to change this to
> MOZ_WIDGET_GTK3 and then change it again, since the MOZ_WIDGET_GTK3 support
> hasn't landed in configure yet. Please submit a patch with MOZ_WIDGET_GTK.

The proposed MOZ_WIDGET_GTK is not compatible with the recent MOZ_WIDGET_GTK2 macro, it may break the gtk2 build. So we have to check-in a path to configure which defines MOZ_WIDGET_GTK 2 (and maybe 3) first.
Attached patch minimal configure patch (obsolete) — Splinter Review
A minimal patch needed for the MOZ_WIDGET_GTK change.
Attachment #630669 - Flags: review?(benjamin)
Attachment #630669 - Flags: review?(benjamin) → review+
Attached patch gfx, v.3 (obsolete) — Splinter Review
The same as v2 but with MOZ_WIDGET_GTK macro, so all lines with:

defined(MOZ_WIDGET_GTK2) || defined(MOZ_WIDGET_GTK3)

are replaced with:

MOZ_WIDGET_GTK >= 2
Attachment #628255 - Attachment is obsolete: true
Attachment #632218 - Flags: review?(roc)
Attachment #628318 - Flags: review?(jst)
Attachment #632197 - Flags: checkin? → checkin+
Attached patch xpcom gtk3 v2 (obsolete) — Splinter Review
xpcom patch with MOZ_WIDGET_GTK
Attachment #628324 - Attachment is obsolete: true
Attachment #632596 - Flags: review?(benjamin)
Attached patch gfx, v.4 (obsolete) — Splinter Review
Ahh, sorry, I missed the convention for macros, this one adds braces around (MOZ_WIDGET_GTK >= 2).
Attachment #632218 - Attachment is obsolete: true
Attachment #632218 - Flags: review?(roc)
Attachment #632599 - Flags: review?
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

Vladimir, can you please check this one? It has r+ already from Roc (comment 151) but contains a change in the GTK macro (defined(MOZ_WIDGET_GTK2) || defined(MOZ_WIDGET_GTK3)) => (MOZ_WIDGET_GTK >= 2)
Attachment #632599 - Flags: review? → review?(vladimir)
Attached patch xpcom gtk3 v3Splinter Review
fixed defines
Attachment #632596 - Attachment is obsolete: true
Attachment #632596 - Flags: review?(benjamin)
Attachment #632655 - Flags: review?(benjamin)
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

macro changes look fine to me
Attachment #632599 - Flags: review?(vladimir) → review+
Attached patch toolkit gtk3 v.2 (obsolete) — Splinter Review
toolkit patch with MOZ_WIDGET_GTK macro.
Attachment #628327 - Attachment is obsolete: true
Attachment #632688 - Flags: review?(benjamin)
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

Thanks!
Attachment #632599 - Flags: checkin?
(In reply to Ed Morley [:edmorley] from comment #172)
> Comment on attachment 632599 [details] [diff] [review]
> gfx, v.4
> 
> Backed out for compilation errors:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12624103&tree=Mozilla-
> Inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0ae73dafcf

Hm, strange, gdk_display_get_default() should be there since 2.2. It may be a problem with different headers.
Yes, I'm guessing gdkx.h doesn't pull in gdk.h (or gdkdisplay.h).
(In reply to Karl Tomlinson (:karlt) from comment #174)
> Yes, I'm guessing gdkx.h doesn't pull in gdk.h (or gdkdisplay.h).

http://developer.gnome.org/gtk/stable/gtk-compiling.html says:

"The recommended way of using GTK+ has always been to only include the toplevel headers gtk.h, gdk.h, gdk-pixbuf.h. If you want to make sure that your program follows this recommended practise, you can define the preprocessor symbols GTK_DISABLE_SINGLE_INCLUDES and GDK_PIXBUF_DISABLE_SINGLE_INCLUDES to make GTK+ generate an error when individual headers are directly included. 
There are some exceptions: 
gdkkeysyms.h is not included in gdk.h because the file is quite large. 
gdkx.h must be included independently because It's platform-specific. 
The same for gtkunixprint.h if you use the non-portable GtkPrintUnixDialog API."
Attached patch gfx gtk3, v.5 (obsolete) — Splinter Review
An updated patch, with:

- added <gdk/gdk.h> to X11Util.h
- added gdk_visual_get_depth() for old GTK (gfxPlatformGtk.cpp)
- simplified usage of MOZ_WIDGET_GTK macros

Thanks!
Attachment #632599 - Attachment is obsolete: true
Attachment #633089 - Flags: review?(vladimir)
Attachment #632655 - Flags: review?(benjamin) → review+
Comment on attachment 632688 [details] [diff] [review]
toolkit gtk3 v.2

somebody who knows GTK needs to review the non-mechanical changes in nsGTKRemoteService etc
Attachment #632688 - Flags: review?(benjamin) → review?(karlt)
Comment on attachment 633089 [details] [diff] [review]
gfx gtk3, v.5

Yes, removing ">= 2" is tidier, thanks.

> static void do_gdk_drawable_unref (void *data)
> {
>+#if (MOZ_WIDGET_GTK == 2)
>     GdkDrawable *d = (GdkDrawable*) data;
>+#else
>+    GdkWindow *d = (GdkWindow*) data;
>+#endif
>     g_object_unref (d);
> }

Better to make put this whole function within #if (MOZ_WIDGET_GTK == 2) as with GTK3 SetGdkDrawable doesn't exist.
Comment on attachment 632688 [details] [diff] [review]
toolkit gtk3 v.2

>+#if (MOZ_WIDGET_GTK == 2)
>+    XID window = GDK_WINDOW_XWINDOW(pevent->window);
>+#else
>+    XID window = gdk_x11_window_get_xid(gtk_widget_get_window(GTK_WIDGET(aWidget)));
>+#endif

No need for the GTK_WIDGET() cast here.

This kind of change is OK if there are not too many of them, but I do wonder
if there are going to many similar places, in which case we want to put
gtk2compat.h somewhere that other files can find it.
Perhaps widget can export the header (or perhaps it could be exported from gfx
if useful in gfx, image, dom, and/or view).

>-#ifdef MOZ_X11
>+#if defined(MOZ_X11) && (MOZ_WIDGET_GTK == 2)
>   if (!gnome_program_get()) {
>     gnome_program_init("Gecko", "1.0", libgnomeui_module_info_get(), gArgc, gArgv, NULL);
>   }
>-#endif /* MOZ_X11 */
>+#endif /* MOZ_X11 && (MOZ_WIDGET_GTK == 2) */

>-#ifdef MOZ_X11
>+#if defined(MOZ_X11) && (MOZ_WIDGET_GTK == 2)

>   GnomeClient *client = gnome_master_client();
>   g_signal_connect(client, "save-yourself", G_CALLBACK(save_yourself_cb), NULL);

>   if (argv1) {
>     gnome_client_set_restart_command(client, 1, &argv1);
>   }
>-#endif /* MOZ_X11 */
>+#endif /* MOZ_X11 && (MOZ_WIDGET_GTK == 2) */

Looks like there should be a "TODO GTK3" here and a reference to bug 694570.
I haven't checked whether gnome_program_init does anything beyond XSMP.
Attachment #632688 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #178)
> Comment on attachment 633089 [details] [diff] [review]
> gfx gtk3, v.5
> 
> Yes, removing ">= 2" is tidier, thanks.
> 
> > static void do_gdk_drawable_unref (void *data)
> > {
> >+#if (MOZ_WIDGET_GTK == 2)
> >     GdkDrawable *d = (GdkDrawable*) data;
> >+#else
> >+    GdkWindow *d = (GdkWindow*) data;
> >+#endif
> >     g_object_unref (d);
> > }
> 
> Better to make put this whole function within #if (MOZ_WIDGET_GTK == 2) as
> with GTK3 SetGdkDrawable doesn't exist.

Actually, the cast looks like it should be simply (GObject *) regardless of gtk version.
(In reply to Rafał Mużyło from comment #180)
> (In reply to Karl Tomlinson (:karlt) from comment #178)
> > Comment on attachment 633089 [details] [diff] [review]
> > gfx gtk3, v.5
> > 
> > Yes, removing ">= 2" is tidier, thanks.
> > 
> > > static void do_gdk_drawable_unref (void *data)
> > > {
> > >+#if (MOZ_WIDGET_GTK == 2)
> > >     GdkDrawable *d = (GdkDrawable*) data;
> > >+#else
> > >+    GdkWindow *d = (GdkWindow*) data;
> > >+#endif
> > >     g_object_unref (d);
> > > }
> > 
> > Better to make put this whole function within #if (MOZ_WIDGET_GTK == 2) as
> > with GTK3 SetGdkDrawable doesn't exist.
> 
> Actually, the cast looks like it should be simply (GObject *) regardless of
> gtk version.

Actually, g_object_unref takes a gpointer, and so the cast is entirely unnecessary. I kind of wonder what the point of the function is at all if it just calls g_object_unref...? It'd be a lot cleaner to just remove it and call g_object_unref directly (without any casts).

If you want to cast for the sake of debugging, cast with the G_OBJECT() macro and it'll likely catch situations where the object isn't a GObject and give you a warning you can break more easily on (before subsequently crashing).
Attached patch dom gtk3 v.3 (obsolete) — Splinter Review
A patch to dom, the changes are pretty straightforward, Karl can you please check the gtk changes?
Attachment #628318 - Attachment is obsolete: true
Attachment #633482 - Flags: review?(karlt)
Attachment #632655 - Flags: checkin?
Attached patch toolkit gtk3 v.3Splinter Review
Thanks, updated with requested changes.
Attachment #632688 - Attachment is obsolete: true
Attachment #633508 - Flags: review+
Attachment #633508 - Flags: checkin?
Attached patch gfx, v.6 (obsolete) — Splinter Review
The gfx patch, with g_object_unref (G_OBJECT(data)) in do_gdk_drawable_unref().
Attachment #633089 - Attachment is obsolete: true
Attachment #633089 - Flags: review?(vladimir)
Attachment #633520 - Flags: review?(vladimir)
Attachment #633508 - Flags: checkin? → checkin+
Attachment #632655 - Flags: checkin? → checkin+
Comment on attachment 633482 [details] [diff] [review]
dom gtk3 v.3

>-#ifdef MOZ_WIDGET_GTK2
>+#if (MOZ_WIDGET_GTK == 2)
> #include <gdk/gdk.h>
> #include <gdk/gdkx.h>
> #include "gtk2xtbin.h"
>+#elif (MOZ_WIDGET_GTK == 3)
>+#include <gdk/gdk.h>
>+#include <gdk/gdkx.h>
> #endif

I would prefer only listing each header file once.
i.e. change "#ifdef MOZ_WIDGET_GTK2" to "#ifdef MOZ_WIDGET_GTK" and wrap
gtk2xtbin in "if (MOZ_WIDGET_GTK == 3)"

>-#ifdef MOZ_WIDGET_GTK2
>+#ifdef MOZ_WIDGET_GTK
>   // This is the visual used by the widgets, 24-bit if available.
>-  GdkVisual* gdkVisual = gdk_rgb_get_visual();
>+  GdkVisual* gdkVisual = gdk_screen_get_system_visual(gdk_screen_get_default());
>   Visual* visual = gdk_x11_visual_get_xvisual(gdkVisual);
>   Screen* screen =
>     gdk_x11_screen_get_xscreen(gdk_visual_get_screen(gdkVisual));
> #else
>   Display* dpy = mozilla::DefaultXDisplay();
>   Screen* screen = DefaultScreenOfDisplay(dpy);
>   Visual* visual = DefaultVisualOfScreen(screen);
> #endif

I don't really object to the change from the rgb visual to the default visual
(because the OOP code uses the default visual), but the comment is no longer
accurate with this change and the generic non-GTK2 code would give the same
result.  Please use the generic Xlib code with all toolkits.

>+#if (MOZ_WIDGET_GTK == 3)
>+nsresult
>+nsPluginInstanceOwner::Renderer::DrawWithXlib(cairo_t* cr,
>+                                              nsIntPoint offset,
>+                                              nsIntRect *clipRects, 
>+                                              PRUint32 numClipRects)
>+#else
> nsresult
> nsPluginInstanceOwner::Renderer::DrawWithXlib(gfxXlibSurface* xsurface, 
>                                               nsIntPoint offset,
>                                               nsIntRect *clipRects, 
>                                               PRUint32 numClipRects)
>+#endif
> {
>+#if (MOZ_WIDGET_GTK == 3)
>+  cairo_surface_t * target = cairo_get_target(cr);
>+  nsRefPtr<gfxXlibSurface> xsurface = gfxASurface::Wrap(target);
>+  Screen *screen = cairo_xlib_surface_get_screen(target);
>+#else
>   Screen *screen = cairo_xlib_surface_get_screen(xsurface->CairoSurface());
>+#endif

What is the reason for the change in API here?
I wonder whether the target is necessarily a Pixmap, but the old API seemed
more appropriate.
gfxASurface::Wrap returns already_AddRefed<gfxASurface>.
I didn't expect nsRefPtr<gfxXlibSurface> to have a compatible constructor.

>   // The GtkSocket has a visible window, but the plugin's XEmbed plug will
>   // cover this window.  Normally GtkSockets let the X server paint their
>   // background and this would happen immediately (before the plug is
>   // created).  Setting the background to None prevents the server from
>   // painting this window, avoiding flicker.
>-  gdk_window_set_back_pixmap(mSocketWidget->window, NULL, FALSE);
>+#if (MOZ_WIDGET_GTK == 2)
>+  gdk_window_set_back_pixmap(gtk_widget_get_window(mSocketWidget), NULL, FALSE);
>+#endif

Need a TODO GTK here.  Preferably file a bug.

>+  mWsInfo.colormap = NULL;

colormap is an XID, so use None.

>+#if (MOZ_WIDGET_GTK == 2)
>+  Window w = gdk_x11_drawable_get_xid(socket_window);
>+#else
>+  GdkWindow *w = gdk_x11_window_get_xid(socket_window);
>+#endif

gdk_x11_window_get_xid returns a Window, not a GdkWindow*.

Use the same gdk_x11_window_get_xid function for both toolkits.

>+#if (MOZ_WIDGET_GTK == 2)
>     xevent.xbutton.window = GDK_WINDOW_XWINDOW(socket_window);
>     xevent.xbutton.root = GDK_WINDOW_XWINDOW(gdk_screen_get_root_window(screen));
>     xevent.xbutton.subwindow = GDK_WINDOW_XWINDOW(plug_window);
>+#else
>+    xevent.xbutton.window = gdk_x11_window_get_xid(socket_window);
>+    xevent.xbutton.root = gdk_x11_window_get_xid(gdk_screen_get_root_window(screen));
>+    xevent.xbutton.subwindow = gdk_x11_window_get_xid(plug_window);
>+#endif

Use the same function for both toolkits here too.

>-    GdkWindow* socket_window = GTK_PLUG(widget)->socket_window;
>+    GdkWindow* socket_window = gtk_plug_get_socket_window(GTK_PLUG(widget));

This won't build on our GTK+ 2.10 builders.

Can gtk2compat.h be used so simplify these changes?
Attachment #633482 - Flags: review?(karlt) → review-
Comment on attachment 633520 [details] [diff] [review]
gfx, v.6

Let's use gtk2compat.h there too.
Attachment #633520 - Flags: review?(vladimir)
We'd need to export gtk2compat.h plus implement gtk_plug_get_socket_window() for the dom/gfx changes there.
Attachment #634040 - Flags: review?(karlt)
Attachment #634040 - Flags: review?(karlt) → review+
Attachment #634040 - Flags: checkin?
Attached patch dom, v.4 (obsolete) — Splinter Review
Fixed the comments. I removed the API change in DrawWithXlib(), it was because of changes in gfx but I reworked it a bit.
Attachment #633482 - Attachment is obsolete: true
Attachment #634390 - Flags: review?(karlt)
Keywords: checkin-needed
Attached patch gfx, v.7 (obsolete) — Splinter Review
Gfx patch, compatible with changes in the dom patch. Karl, can you please check the GTK parts?
Attachment #633520 - Attachment is obsolete: true
Attachment #634430 - Flags: review?(karlt)
Comment on attachment 634390 [details] [diff] [review]
dom, v.4

> nsresult nsPluginNativeWindowGtk2::CreateXEmbedWindow() {
>   NS_ASSERTION(!mSocketWidget,"Already created a socket widget!");
>-
>+#if (MOZ_WIDGET_GTK == 2)
>   GdkWindow *parent_win = gdk_window_lookup((XID)window);
>+#else
>+  GdkDisplay *display = gdk_display_get_default();
>+  GdkWindow *parent_win = gdk_x11_window_lookup_for_display(display, (XID)window);
>+#endif

Might as well use the same code with GTK2 here.

> nsresult nsPluginNativeWindowGtk2::CreateXtWindow() {

>+#if (MOZ_WIDGET_GTK == 2)
>   GdkWindow *gdkWindow = gdk_window_lookup((XID)window);
>+#else
>+  GdkDisplay *display = gdk_display_get_default();
>+  GdkWindow *gdkWindow = gdk_x11_window_lookup_for_display(display, (XID)window);
>+#endif

And here.

>   for (unsigned int i = 0; i < nchildren; ++i) {
>     Window child = children[i];
>+#if (MOZ_WIDGET_GTK == 2)
>     if (!gdk_window_lookup(child)) {
>+#else
>+    if (!gdk_x11_window_lookup_for_display(gdkDisplay, child)) {
>+#endif

And here.

>+#if (MOZ_WIDGET_GTK == 2)
>     GdkScreen* screen = gdk_drawable_get_screen(socket_window);

>+#else
>+    GdkScreen* screen = gdk_window_get_screen(socket_window);
>+#endif

And here.

> wrap_gtk_plug_embedded(GtkPlug* plug) {
>-    GdkWindow* socket_window = plug->socket_window;
>+    GdkWindow* socket_window = gtk_plug_get_socket_window(GTK_PLUG(plug));

No need for the GTK_PLUG cast here.

r+ with those changes.
Attachment #634390 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #193)
> Comment on attachment 634390 [details] [diff] [review]
> dom, v.4
> 
> > nsresult nsPluginNativeWindowGtk2::CreateXEmbedWindow() {
> >   NS_ASSERTION(!mSocketWidget,"Already created a socket widget!");
> >-
> >+#if (MOZ_WIDGET_GTK == 2)
> >   GdkWindow *parent_win = gdk_window_lookup((XID)window);
> >+#else
> >+  GdkDisplay *display = gdk_display_get_default();
> >+  GdkWindow *parent_win = gdk_x11_window_lookup_for_display(display, (XID)window);
> >+#endif
> 
> Might as well use the same code with GTK2 here.

gdk_x11_window_lookup_for_display() is 2.24

> > nsresult nsPluginNativeWindowGtk2::CreateXtWindow() {
> 
> >+#if (MOZ_WIDGET_GTK == 2)
> >   GdkWindow *gdkWindow = gdk_window_lookup((XID)window);
> >+#else
> >+  GdkDisplay *display = gdk_display_get_default();
> >+  GdkWindow *gdkWindow = gdk_x11_window_lookup_for_display(display, (XID)window);
> >+#endif
> 
> And here.

same

> >   for (unsigned int i = 0; i < nchildren; ++i) {
> >     Window child = children[i];
> >+#if (MOZ_WIDGET_GTK == 2)
> >     if (!gdk_window_lookup(child)) {
> >+#else
> >+    if (!gdk_x11_window_lookup_for_display(gdkDisplay, child)) {
> >+#endif
> 
> And here.
> 
> >+#if (MOZ_WIDGET_GTK == 2)
> >     GdkScreen* screen = gdk_drawable_get_screen(socket_window);
> 
> >+#else
> >+    GdkScreen* screen = gdk_window_get_screen(socket_window);
> >+#endif
> 
> And here.

gdk_window_get_screen() is 2.24

Since we still check for gtk 2.10 in configure, using the same code in both case might fail.
Forget my last comment. I missed gtk2compat.

Sorry for the noise.
Dom patch for check-in, updated with the comments.
Attachment #634390 - Attachment is obsolete: true
Attachment #634895 - Flags: review+
Attachment #634895 - Flags: checkin?
Comment on attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

(In reply to Martin Stránský from comment #197)
> Created attachment 634895 [details] [diff] [review]
> dom, v.4 for check-in
> 
> Dom patch for check-in, updated with the comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a07e9d98a7c6
Attachment #634895 - Flags: checkin? → checkin+
Comment on attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

Backed out due to build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d77afd530ba

https://tbpl.mozilla.org/php/getParsedLog.php?id=12881833&tree=Mozilla-Inbound

PluginScriptableObjectParent.cpp
/usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o PluginScriptableObjectParent.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /builds/slave/m-in-lnx/build/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -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 -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_LINUX=1 -DOS_POSIX=1  -DFORCE_PR_LOG -I/builds/slave/m-in-lnx/build/dom/plugins/ipc/../base -I/builds/slave/m-in-lnx/build/xpcom/base/  -I/builds/slave/m-in-lnx/build/ipc/chromium/src -I/builds/slave/m-in-lnx/build/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/builds/slave/m-in-lnx/build/dom/plugins/ipc -I. -I../../../dist/include  -I/builds/slave/m-in-lnx/build/obj-firefox/dist/include/nspr -I/builds/slave/m-in-lnx/build/obj-firefox/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -finline-limit=50 -fno-omit-frame-pointer  -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0   -I/builds/slave/m-in-lnx/build/obj-firefox/dist/include/cairo   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/PluginScriptableObjectParent.pp /builds/slave/m-in-lnx/build/dom/plugins/ipc/PluginScriptableObjectParent.cpp
../../../../dom/plugins/ipc/PluginModuleChild.cpp: In function 'gboolean gtk_plug_scroll_event(GtkWidget*, GdkEventScroll*)':
../../../../dom/plugins/ipc/PluginModuleChild.cpp:276:39: error: 'gtk_widget_is_toplevel' was not declared in this scope
make[6]: *** [PluginModuleChild.o] Error 1
Attachment #634895 - Flags: checkin+ → checkin-
Comment on attachment 634430 [details] [diff] [review]
gfx, v.7

>-#ifdef MOZ_ENABLE_GTK2
>+#ifdef MOZ_WIDGET_GTK

-DMOZ_ENABLE_GTK2 should be removed from the Makefile too:
http://hg.mozilla.org/mozilla-central/annotate/da85d45f39ef/gfx/src/Makefile.in#l80

> static cairo_user_data_key_t cairo_gdk_drawable_key;
> static void do_gdk_drawable_unref (void *data)
> {
>-    GdkDrawable *d = (GdkDrawable*) data;
>-    g_object_unref (d);
>+    g_object_unref (G_OBJECT(data));
> }

This still leaves an unused function with GTK3.  Best solution is to replace
do_gdk_drawable_unref with g_object_unref in SetGdkDrawable.
I expect cairo_user_data_key_t is also unused and should be removed.

>--- a/gfx/thebes/gfxGdkNativeRenderer.cpp
>+++ b/gfx/thebes/gfxGdkNativeRenderer.cpp

gfxGdkNativeRender no longer needs to use an X surface and so no longer needs
to be a gfxXlibNativeRenderer.

In fact, nsNativeThemeGTK.cpp shouldn't even need a gfxGdkNativeRenderer as it
should be able to use ctx->GetCairo() and pass that through to
moz_gtk_widget_paint.

I suggest leaving the NativeRenderer changes out of this patch (or just
disabling NativeRenderer in the Makefile, but this patch doesn't include
Makefile changes), and addressing nsNativeThemeGTK separately.
The rest is good so r+ if you are happy to do that.
Attachment #634430 - Flags: review?(karlt) → review+
Blocks: 711711
Ahh, we're missing gtk_widget_is_toplevel() in gtk2compat.h
Attachment #636708 - Flags: review?(karlt)
The gfx patch with requested changes.
Attachment #634430 - Attachment is obsolete: true
Attachment #636722 - Flags: review+
Attachment #636722 - Flags: checkin?
Attachment #636708 - Flags: review?(karlt) → review+
Attachment #636708 - Flags: checkin?
Depends on: 772883
Comment on attachment 540036 [details] [diff] [review]
rest of gtk2 dir fixes

I'll have a look at this patch.

Chris, if you can look at the updated nsLookAndFeel patch, please, that would be very helpful.
Attachment #540036 - Flags: review?(chrisccoulson) → review?(karlt)
Comment on attachment 540036 [details] [diff] [review]
rest of gtk2 dir fixes

I need more context in the patch to do a proper review all the bits, but here
are some general things to address:

Keep code within 80 columns.

If using the same object data more than once in the same function, then only
call the getter function once.

Some specifics:

>+    GList *l, *list = gdk_device_manager_list_devices (device_manager, GDK_DEVICE_TYPE_MASTER);
>+    for (l = list; l; l = l->next) {
>+      if (gdk_device_get_source (l->data) == GDK_SOURCE_MOUSE) {
>+        event.button.device = l->data;
>+        break;
>+      }
>+    }

gdk_device_manager_get_client_pointer, I assume.

Mozilla style is no space between function names and the '('.

>+#else
>+    GdkPixbuf *pixbuf = gdk_pixbuf_get_from_surface(
>+          aSurface->CairoSurface(), 0, 0, dragRect.width, dragRect.height);
>+    if (!pixbuf)
>+      return PR_FALSE;
>+    
>+     // The drag transaction addrefs the pixmap, so we can just unref it from us here
>+    gtk_drag_set_icon_pixbuf(aContext, pixbuf, aXOffset, aYOffset);
>+    g_object_unref(pixbuf);
>+    return PR_TRUE;
>+#endif

This function is what renders dragged text on a transparent background
antialiased without bitmap outlines, when a compositing window manager is
running.

I would have guessed that the replacement for gtk_drag_set_icon_pixmap would
be gtk_drag_set_icon_surface, but it looks like that just uses the shape
extension instead of an ARGB translucent window.

When this method returns false, gtk_drag_set_icon_pixbuf is used, so it
looks like there is no point adding this code, as returning false will fall
back to the same effect.

Perhaps something similar to the old behaviour could be implemented from
scratch using gtk_drag_set_icon_widget, but that might be a bit of work.

I suggest marking this TODO GTK3, with just a return false.

>+#if defined(MOZ_WIDGET_GTK2) || defined(MOZ_PLUGIN_GTK2)
>     void          CreateSharedGC(void);
>     GdkGC         *GetSharedGC(void);
>-    
>+#endif
>+
>     /**
>      * Get/set our value of DESKTOP_STARTUP_ID. When non-empty, this is applied
>@@ -80,5 +82,7 @@ public:
> 
> private:
>+#if defined(MOZ_WIDGET_GTK2) || defined(MOZ_PLUGIN_GTK2)
>     GdkGC         *mSharedGC;
>+#endif

GetSharedGC and NS_NATIVE_GRAPHIC are not used.
Can you remove them altogether, please?

>+#else
>+    GtkIMContext *slave = NULL; //multicontext->slave; TODO this doesn'st look good
>+#endif

Please use "TODO GTK3" or whatever is consistent with other comments, or file
bugs, so we know what needs doing.

But I hope we can drop these workarounds altogether now.
Do people need to change code in their IM modules to work with GTK3?
If so, I think we drop these workarounds for GTK3 and hope the modules are
written better now.  If the modules just need a recompile, then we should try
to continue the workaround, so leave the TODO GTK3.

>+  nsresult DrawWithGDK(cairo_t * cr, gint offsetX, gint offsetY,

As indicated in comment 200, the appropriate approach for GTK3 is quite
different and this isn't taking us in the right direction.  I suggest leaving
nsNativeThemeGTK.cpp changes out of this patch, using ctx->GetCairo() in a
separate patch.

>-  GdkVisual * rgb_visual = gdk_rgb_get_visual();
>-  *aPixelDepth = rgb_visual->depth;
>+  GdkVisual * rgb_visual = gdk_screen_get_system_visual(gdk_screen_get_default());
>+  *aPixelDepth = gdk_visual_get_depth(rgb_visual);

Change the name of the variable to |visual|, now that it is not the rgb visual.
Attachment #540036 - Flags: review?(karlt) → review-
Sorry for the total noob question, but how do I utilize this? I don't mind unstable...
P.S: Am on Ubuntu 12.04 gnome shell
(In reply to aditya3098 from comment #209)
> Sorry for the total noob question, but how do I utilize this? I don't mind
> unstable...
> P.S: Am on Ubuntu 12.04 gnome shell

you need to fetch the source code https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial
then compile it https://developer.mozilla.org/en-US/docs/Build_and_Install
but as far as I can tell, the makefile/configure portions of the gtk3 build are not in yet.
If anyone want to try it there's a patch for latest trunk. It does not contain all fixes (look&feel, plugins) but works.
Attachment #538243 - Attachment is obsolete: true
Comment on attachment 654957 [details] [diff] [review]
complete patch against latest trunk, v2

Note: It needs this .mozconfig options:

ac_add_options --enable-default-toolkit=cairo-gtk3
ac_add_options --enable-system-cairo
not all of it applies to latest trunk but I managed to apply the missing parts manually apart from widget/gtk2/nsFilePicker.cpp
Attached patch rest of gtk2, v2 (obsolete) — Splinter Review
Thanks, there's an updated one.
Attachment #540036 - Attachment is obsolete: true
Attachment #654976 - Flags: review?(karlt)
Comment on attachment 654976 [details] [diff] [review]
rest of gtk2, v2

> nsBidiKeyboard::nsBidiKeyboard()
> {
>+#if (MOZ_WIDGET_GTK == 2)

What's the plan here?  See comment 42.

>+                    ConvertHTMLtoUCS2((guchar *)gtk_selection_data_get_data(selectionData), 
>+                                      length, &htmlBody, htmlBodyLen);
>                     // Try next data format?
>                     if (!htmlBodyLen)
>                         continue;
>                     data = (guchar *)htmlBody;
>                     length = htmlBodyLen * 2;
>                 } else {
>                     data = (guchar *)nsMemory::Alloc(length);
>                     if (!data)
>                         break;
>-                    memcpy(data, selectionData->data, length);
>+                    memcpy(data, gtk_selection_data_get_data(selectionData), length);

Move the gtk_selection_data_get_data call outside the if/else blocks, so that
only one call is required.

>-    if (aSelectionData->selection == GDK_SELECTION_PRIMARY)
>+    if (gtk_selection_data_get_selection(aSelectionData) == GDK_SELECTION_PRIMARY)
>         whichClipboard = kSelectionClipboard;
>-    else if (aSelectionData->selection == GDK_SELECTION_CLIPBOARD)
>+    else if (gtk_selection_data_get_selection(aSelectionData) == GDK_SELECTION_CLIPBOARD)

Similarly here.

Please go through and check that there are no other similar situations to
address.  I haven't looked at the rest of the patch.
Attachment #654976 - Flags: review?(karlt) → review-
Attached patch rest of gtk2, v3 (obsolete) — Splinter Review
I'm going to address nsBidiKeyboard changes in a different patch, with gtk2compat.h update and so.

The gtk_selection_data_get_data() is called only once, because of the if/else...but I put it above as you requested.

The GDK_SELECTION_* is also updated - it was the only place I hesitated about the change because it was called only twice. But it's updated now.

Thanks!
Attachment #654976 - Attachment is obsolete: true
Attachment #658436 - Flags: review?(karlt)
Attached patch gtk2/nsBidiKeyboard patch (obsolete) — Splinter Review
The nsBidiKeyboard patch
Attachment #658468 - Flags: review?(karlt)
Attached patch gtk2/bidi v2 (obsolete) — Splinter Review
Updated initialization in nsBidiKeyboard::nsBidiKeyboard(), it's closer to the original.
Attachment #658468 - Attachment is obsolete: true
Attachment #658468 - Flags: review?(karlt)
Attachment #658473 - Flags: review?(karlt)
Comment on attachment 658436 [details] [diff] [review]
rest of gtk2, v3

(In reply to Martin Stránský from comment #216)
> The gtk_selection_data_get_data() is called only once, because of the
> if/else...but I put it above as you requested.

Thanks.  Sharing the same call means less code generated. 

>+                guchar *clipboardData = (guchar *)gtk_selection_data_get_data(selectionData);
>+                length = gtk_selection_data_get_length(selectionData);
>                 // Special case text/html since we can convert into UCS2
>                 if (!strcmp(flavorStr, kHTMLMime)) {
>                     PRUnichar* htmlBody= nullptr;
>                     int32_t htmlBodyLen = 0;
>                     // Convert text/html into our unicode format
>-                    ConvertHTMLtoUCS2((guchar *)selectionData->data, length,
>+                    ConvertHTMLtoUCS2(clipboardData, length, 
>                                       &htmlBody, htmlBodyLen);

I'd prefer the const_cast at the ConvertHTMLtoUCS2 call (and have
const guchar *clipboardData) as that looks like a bug in ConvertHTMLtoUCS2
requiring mutable data.

>-    if (gtk_targets_include_image(&aSelectionData->target, 1, TRUE)) {
>+    GdkAtom targets[] = {selectionTarget, NULL};
>+    if (gtk_targets_include_image(targets, 1, TRUE)) {

gtk_targets_include_image doesn't require NULL terminated targets, so
passing &selectionTarget is fine here.

>     TargetResetData();
>     mTargetDragDataReceived = true;
>-    if (aSelectionData->length > 0) {
>-        mTargetDragDataLen = aSelectionData->length;
>+    if (gtk_selection_data_get_length(aSelectionData) > 0) {
>+        mTargetDragDataLen = gtk_selection_data_get_length(aSelectionData);
>         mTargetDragData = g_malloc(mTargetDragDataLen);
>-        memcpy(mTargetDragData, aSelectionData->data, mTargetDragDataLen);
>+        memcpy(mTargetDragData, gtk_selection_data_get_data(aSelectionData), 
>+               mTargetDragDataLen);
>     }
>     else {
>         PR_LOG(sDragLm, PR_LOG_DEBUG,
>                ("Failed to get data.  selection data len was %d\n",
>-                aSelectionData->length));
>+                gtk_selection_data_get_length(aSelectionData)));
>     }

Multiple gtk_selection_data_get_length calls.
TargetResetData sets mTargetDragDataLen = 0, so it is fine to use that to
store the result of gtk_selection_data_get_length.

> }
>-
> static void
> invisibleSourceDragBegin(GtkWidget        *aWidget,

Keep the blank line.

>-        _XnrmIsActive(GDK_DISPLAY())) {
>-      screenInfo = _XnrmQueryScreens(GDK_DISPLAY(), &numScreens);
>+        _XnrmIsActive(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()))) {
>+      screenInfo = _XnrmQueryScreens(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()), 
>+                                     &numScreens);

Repeated calls to get the same display.
Attachment #658436 - Flags: review?(karlt) → review-
Attachment #658473 - Flags: review?(karlt) → review+
Attached patch rest of gtk2, v4 (obsolete) — Splinter Review
Ahh, sorry about the gtk_selection_data_get_length() in Drag&Drop. There's a new one. Thanks!
Attachment #658436 - Attachment is obsolete: true
Attachment #658841 - Flags: review?(karlt)
Comment on attachment 658841 [details] [diff] [review]
rest of gtk2, v4

>+                const guchar *clipboardData = (const guchar *)gtk_selection_data_get_data(selectionData);

Remove the cast here because gtk_selection_data_get_data returns
(const guchar *).
Comment on attachment 658841 [details] [diff] [review]
rest of gtk2, v4

The rest is good, thanks.
Attachment #658841 - Flags: review?(karlt) → review+
rest of gtk2, with build fixes (gtk2compat.h update, added gtk2compat.h includes).
Attachment #660788 - Flags: checkin?
Whiteboard: [leave open for remaining patches] → [leave open for remaining patches][check linux try build before checkin]
I just did a build with attachment 660788 [details] [diff] [review] and attachment 660786 [details] [diff] [review] and it builds and runs well.
Depends on: 791529
Depends on: 791518
Attached patch nsLookAndFeel v.2 color changes (obsolete) — Splinter Review
Color changes only (plus new file creation).
Attachment #627866 - Attachment is obsolete: true
Attachment #627866 - Flags: review?(chrisccoulson)
Attachment #661786 - Flags: review?(chrisccoulson)
Depends on: 722975
Depends on: 792158
Can GTK3 be enabled in nightly builds please?
Blocks: 813578
Any news or ETA on this? It seemed to be quite active for a while but then totally stopped.
Comment on attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

Yeah, you're right, we should move it a bit. Karl, can you please check this one? It's just color changes in the GTK3 code and reflects the Cris requests.
Attachment #661786 - Flags: review?(chrisccoulson) → review?(karlt)
Attached patch gtk3drawing - gtk_entry clip (obsolete) — Splinter Review
GtkEntry drawing needs clipping around the element.
Attachment #710347 - Flags: review?(karlt)
Why still no gtk3 branch?
Comment on attachment 710347 [details] [diff] [review]
gtk3drawing - gtk_entry clip

(In reply to Martin Stránský from comment #234)
> Created attachment 710347 [details] [diff] [review]
> gtk3drawing - gtk_entry clip
> 
> GtkEntry drawing needs clipping around the element.

Can you be more specific about what exactly needs this clipping, please?

Is it gtk_render_background, gtk_render_frame, or gtk_render_focus?
Is it an issue with the theme engine or does GTK's default theme have the
same problem.

I wouldn't have expected this to be necessary because each gtk_render_* method
is passed a rectangle, but if it is necessary then perhaps we should be
clipping to the drawingRect for all widget drawing.

Knowing the source of the problem would help us pick the appropriate solution.

>+    /* GtkEntry needs clipping around the element, otherwise it fills whole parent 
>+     * window with background color */  

>+    GdkRectangle clipped_rect;
>+    gdk_rectangle_intersect(rect, cliprect, &clipped_rect);

It shouldn't be necessary to use cliprect here as there should already be a
clip region on the cairo_t that is a subregion of the dirtyRect.
Adding just rect as another clip should be sufficient.
Attachment #710347 - Flags: review?(karlt) → review-
Comment on attachment 710377 [details] [diff] [review]
remove ThemeRenderer(gfxGdkNativeRenderer) from gtk3

>   // GtkStyles (used by the widget drawing backend) are created for a
>   // particular colormap/visual.
>+#if defined(MOZ_WIDGET_GTK2)
>+  ThemeRenderer renderer(state, gtkWidgetType, flags, direction,

>+
>   GdkColormap* colormap = moz_gtk_widget_get_colormap();
>-
>   renderer.Draw(ctx, drawingRect.Size(), rendererFlags, colormap);

Please leave the comment adjacent to the colormap declaration, and leave the
following blank line to make it clearer that the comment applies to the
colormap line.

>+  cairo_clip_extents(cr,&x1,&y1,&x2,&y2);
>+  
>+  GdkRectangle surfaceRect;
>+  surfaceRect.x = x1;
>+  surfaceRect.y = y1;
>+  surfaceRect.width = x2-x1;
>+  surfaceRect.height = y2-y1;
>+  
>+  gdk_rectangle_intersect(&gdk_clip, &surfaceRect, &gdk_clip);
>+  moz_gtk_widget_paint(gtkWidgetType, cr, &gdk_rect, &gdk_clip,
>+                      &state, flags, direction);

The cliprect parameter to moz_gtk_widget_paint() should be unused (and best removed).  The clip is already on the cairo_t.  That means that this code is not required and the gdk_clip declaration can be moved to the GTK2 block.
Attachment #710377 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #237)
> Comment on attachment 710347 [details] [diff] [review]
> gtk3drawing - gtk_entry clip
> > GtkEntry drawing needs clipping around the element.
> 
> Can you be more specific about what exactly needs this clipping, please?

I filled https://bugzilla.gnome.org/show_bug.cgi?id=694086 for that with more details. Benjamin Otte promised to help with that so let's leave it for further update.
Attached patch ThemeRenderer, v2 (obsolete) — Splinter Review
It also removes the cliprect parameter from gtk3drawing.c
Attachment #710377 - Attachment is obsolete: true
Attachment #715429 - Flags: review?(karlt)
Comment on attachment 715429 [details] [diff] [review]
ThemeRenderer, v2

>+  moz_gtk_widget_paint(gtkWidgetType, ctx->GetCairo(), &state, flags, direction);

Need to pass gdk_rect for the rect parameter here.
Attachment #715429 - Flags: review?(karlt) → review+
Comment on attachment 715914 [details] [diff] [review]
ThemeRenderer, v3, for check-in

Try submission for this patch: https://tbpl.mozilla.org/?tree=Try&rev=84118866603d
Attachment #715914 - Flags: checkin?
Attachment #715429 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8e1a272c78
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches][check linux try build before checkin] → [leave open for remaining patches][check linux try build before requesting checkin]
Attachment #715914 - Flags: checkin? → checkin+
Attachment #658473 - Attachment is obsolete: true
Attachment #658841 - Attachment is obsolete: true
Attachment #630669 - Attachment is obsolete: true
Attached patch add padding to widget borders (obsolete) — Splinter Review
According to Benjamin Otte, we should use padding+border for widget sizes.
Attachment #718387 - Flags: review?(karlt)
Comment on attachment 718387 [details] [diff] [review]
add padding to widget borders

The padding needs to be added to more places so I'll attach a complete patch for that later.
Attachment #718387 - Attachment is obsolete: true
Attachment #718387 - Flags: review?(karlt)
Attachment #718386 - Flags: review?(karlt) → review+
This is a workaround for https://bugzilla.gnome.org/show_bug.cgi?id=694086
Attachment #710347 - Attachment is obsolete: true
Attachment #727206 - Flags: review?(karlt)
Attached patch radio button fixSplinter Review
All radio buttons are rendered as active now, this patch fixes it.
Attachment #727215 - Flags: review?(karlt)
Some elements (especially GtkEntry and menu separators) needs padding, which is reflected by this patch.
Attachment #727638 - Flags: review?(karlt)
This patch fixes tabs size and tab background rendering and removes obsolete background rendering code.
Attachment #727682 - Flags: review?(karlt)
Comment on attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

This seems to be doing the right thing in at least most situations but there is some repeated code that I'd like to see reduced.

One part of this is that GetIntImpl, GetFloatImpl, and GetSystemFontInfo make up
close to half of this file, and they are being duplicated, unchanged I assume,
in two files now.  The simplest way to deal with this is probably to use #if
(MOZ_WIDGET_GTK == 2) preprocessor conditionals.

The other part is that NativeGetColor() contains quite a bit of code that is
either repeated or similar, but it takes a bit of study to work out which
parts are identical.

I expect it would be less code if mStyle is replaced with a mBackgroundStyle
that is initialized with GTK_STYLE_CLASS_BACKGROUND.  Colors requiring a
style with different class could be initialized in InitLookAndFeel() and kept
in new static variables.  That would mean no gtk_style_context_add_class calls
would be required from NativeGetColor() and the gtk_style_context_save/restore
would not be required.  It would also make it easier to adjust the widget
paths should we find that some themes use them.
eColorID_(cell)highlight(text) could use the existing widget path for
eColorID__moz_field(text), and
eColorID__moz_buttondefault/buttonhoverface/buttonhovertext the path for
eColorID_buttonface.

Perhaps there are few enough different colors obtained even with
GTK_STYLE_CLASS_BACKGROUND that it is not necessary to even keep a
GtkStyleContext at all and all colors could be initialized in
InitLookAndFeel().  I'm not sure.

There are a number of ColorID cases that could fall through to pick up the
same color as the next case, also saving on code.
e.g. eColorID_activecaption, eColorID_appworkspace, and eColorID_background
are all the same color.  Some code duplication already existed, but there is
more code in the new version, and more ColorIDs now share the same colors.

nsLookAndFeel.h contains this comment, which should probably be replaces with
just "Cached colors" now:

    // Cached colors, we have to create a dummy widget to actually
    // get the style

InitWidget() should probably be renamed to InitStyle() if you keep the
GtkStyleContext for GTK_STYLE_CLASS_BACKGROUND.

>+#define GDK_RGBA_TO_NS_RGB(c) \
>+    ((nscolor) NS_RGBA((int)((c).red*255), (int)((c).green*255), \
>+                       (int)((c).blue*255), (int)((c).alpha*255)))

I'd prefer GDK_RGBA_TO_NSCOLOR, or perhaps GDK_RGBA_TO_NS_RGBA(c) if you'd
like to be consistent with the existing code, to make it clear that the color
remains translucent.

> nsLookAndFeel::~nsLookAndFeel()
> {
>+  if(mStyle)

mStyle should always be set at this point, so no need for this check.

>     case eColorID_threeddarkshadow:
>         // 3-D shadow outer edge color
>-        aColor = GDK_COLOR_TO_NS_RGB(mStyle->black);
>+        gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);
>+        gtk_style_context_get_border_color(mStyle, GTK_STATE_FLAG_NORMAL, &gdk_color);
>+        // Hardcode to black
>+        aColor = 0;
>         break;

No need for the gtk_style_context_* calls if black is what will be used.

File style is a space after "if" (though there is one exception).

>-        g_object_ref_sink(GTK_OBJECT(menu));
>+        g_object_ref_sink(G_OBJECT(menu));

The parameter for g_object_ref_sink() has gpointer type, so no cast is
necessary here.

These changes should land with the other parts that were in attachment 627866 [details] [diff] [review], so just giving feedback here for now.  Shall I look at attachment 627866 [details] [diff] [review] or have you made changes since then?
Attachment #661786 - Flags: review?(karlt) → feedback+
Thanks! I'll check them.

> One part of this is that GetIntImpl, GetFloatImpl, and GetSystemFontInfo
> make up
> close to half of this file, and they are being duplicated, unchanged I
> assume,
> in two files now.  The simplest way to deal with this is probably to use #if
> (MOZ_WIDGET_GTK == 2) preprocessor conditionals.

How do you mean that? The code is duplicated, but there's one copy in each file (nsLookAndFeelGtk3.cpp and nsLookAndFeel.cpp) so I don't understand how (MOZ_WIDGET_GTK == 2) can help here. Or would you like to move it to an extra file?

> These changes should land with the other parts that were in attachment
> 627866 [details] [diff] [review], so just giving feedback here for now. 
> Shall I look at attachment 627866 [details] [diff] [review] or have you made
> changes since then?

Yes, there are some changes here, let's leave it after this one.
(In reply to Martin Stránský from comment #254)
> > One part of this is that GetIntImpl, GetFloatImpl, and GetSystemFontInfo
> > make up
> > close to half of this file, and they are being duplicated, unchanged I
> > assume,
> > in two files now.  The simplest way to deal with this is probably to use #if
> > (MOZ_WIDGET_GTK == 2) preprocessor conditionals.
> 
> How do you mean that? The code is duplicated, but there's one copy in each
> file (nsLookAndFeelGtk3.cpp and nsLookAndFeel.cpp) so I don't understand how
> (MOZ_WIDGET_GTK == 2) can help here. Or would you like to move it to an
> extra file?

I mean using a single file with #if (MOZ_WIDGET_GTK == 2) for the methods that are specific to GTK2 and #else for those specific to GTK3.
Attached patch nsLookAndFeel v.3 (obsolete) — Splinter Review
Thanks. There's a complete nsLookAndFeel patch with the changes you requested.
Attachment #661786 - Attachment is obsolete: true
Attachment #736306 - Flags: review?(karlt)
Hi,

Sorry to open a thread as non-dev, but reading this bug, I don't understand the current situation. Could you tell me if, noawadays, Firefox can be built against GTK3 and without GTK2? What kind of release should I take to have such feasure, and how to fetch it via http (I've not Mercurial).

Thanks very much for your help.

Regards,
Attachment #727215 - Flags: review?(karlt) → review+
Comment on attachment 736306 [details] [diff] [review]
nsLookAndFeel v.3

>+    GtkStyleContext *style = gtk_style_context_new();
>+    GtkWidgetPath *path = gtk_widget_path_new();
> 
>+    gtk_widget_path_append_type(path, GTK_TYPE_WINDOW);
>+    gtk_style_context_set_path(style, path);
>+  
>+    // Text colors
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    sMozFieldBackground = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_get_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    sMozFieldText = GDK_RGBA_TO_NS_RGBA(color);

Attachment 627866 [details] [diff] used a GtkStyleContext from a GtkTextView, but this version
is using a GTK_TYPE_WINDOW path with no STYLE_CLASS.  I liked the previous
approach because the goal is to mimick what GTK does and so get the same
result (almost) regardless of what themes choose to do.

I suspect that the StyleContext here will not give the right color with
gtk-default.css as it will use background-color: @bg_color; from GtkWindow but
.view uses @base_color.  Perhaps it is easiest to use mViewStyle.

>+    // GTK's guide to fancy odd row background colors:
>+    // 1) Check if a theme explicitly defines an odd row color
>+    // 2) If not, check if it defines an even row color, and darken it
>+    //    slightly by a hardcoded value (gtkstyle.c)
>+    // 3) If neither are defined, take the base background color and
>+    //    darken that by a hardcoded value
>+    style = gtk_widget_get_style_context(treeView);
>+    
>+    // Get odd row background color
>+    gtk_style_context_save(style);
>+    gtk_style_context_add_region(style, GTK_STYLE_REGION_ROW, GTK_REGION_ODD);
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    sOddCellBackground = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_restore(style);
>+
>+    // Get even row background color
>+    gtk_style_context_save(style);
>+    gtk_style_context_add_region(style, GTK_STYLE_REGION_ROW, GTK_REGION_EVEN);
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    if (sOddCellBackground == GDK_RGBA_TO_NS_RGBA(color)) {
>+      GdkRGBA darkenColor;
>+      darken_gdk_rgba_color(&color, &darkenColor);
>+      sOddCellBackground = GDK_RGBA_TO_NS_RGBA(darkenColor);
>+    }
>+    gtk_style_context_restore(style);

The GTK2 version of this code was mimicking part of
gtk_default_draw_flat_box(), I assume, but there is no similar code in GTK3,
so I think its better to just use what the theme gives us.  i.e. remove the
code comparing with the GTK_REGION_EVEN color and darkening.

>+#if (MOZ_WIDGET_GTK == 3)
>+static struct _GtkStyleContext *
>+create_background_context(GtkWidgetPath *path)
>+{
>+    struct _GtkStyleContext * mStyle = gtk_style_context_new();
>+    gtk_style_context_set_path(mStyle, path);
>+    gtk_style_context_add_class(mStyle, GTK_STYLE_CLASS_BACKGROUND);
>+    return(mStyle);
>+}
>+#endif

>+    mViewStyle = create_background_context(path);
>+    gtk_style_context_add_class(mViewStyle, GTK_STYLE_CLASS_VIEW);
>+    
>+    mScrollBarStyle = create_background_context(path);
>+    gtk_style_context_add_class(mScrollBarStyle, GTK_STYLE_CLASS_SCROLLBAR);
>+    gtk_style_context_add_class(mScrollBarStyle, GTK_STYLE_CLASS_TROUGH);
>+    
>+    mCellStyle = create_background_context(path);
>+    gtk_style_context_add_class(mCellStyle, GTK_STYLE_CLASS_CELL);
>+    
>+    mButtonStyle = create_background_context(path);
>+    gtk_style_context_add_class(mButtonStyle, GTK_STYLE_CLASS_BUTTON);

AIUI it makes sense to add GTK_STYLE_CLASS_BACKGROUND to mBackgroundStyle
only.  GTK adds CLASS_BACKGROUND only when drawing GtkWindows.  It doesn't
make sense on the other StyleContexts.

mCellStyle is used only for eColorID__moz_dragtargetzone, so it would be more
efficient to use a single color variable (initialized in InitLookAndFeel())
than keeping the StyleContext as done here.  I don't see -moz-dragtargetzone
used in Firefox, so I don't know much about its intended use.  What was the
reason for choosing CLASS_CELL here?
Could this just use the same code as eColorID_highlight with mViewStyle?


I would like to see the above comments addressed before landing this.
The remaining comments are just suggested optional improvements:

>+    // Window colors
>+    gtk_style_context_save(style);
>+    gtk_style_context_add_class(style, GTK_STYLE_CLASS_BACKGROUND);
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    sMozWindowBackground = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_get_color(style, GTK_STATE_FLAG_NORMAL, &color);
>+    sMozWindowText = GDK_RGBA_TO_NS_RGBA(color);
>+
>+    // Selected text and background
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_SELECTED, &color);
>+    sMozWindowSelectedBackground = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_get_color(style, GTK_STATE_FLAG_SELECTED, &color);
>+    sMozWindowSelectedText = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_restore(style);

If mBackgroundStyle is created in InitWidget() then that can be used here.
InitLookAndFeel() can be made an object method by removing its static keyword
and removing sInitialized from the constructor.  sInitialized didn't make
sense anyway because RefreshImpl() couldn't be called while there was no
nsLookAndFeel object.  nsLookAndFeel is a singleton class now so the
distinction between member and static variables is minor.
http://hg.mozilla.org/mozilla-central/annotate/7b8ed29c6bc0/widget/xpwidgets/nsXPLookAndFeel.cpp#l237

It feels like there could be more sharing of StyleContexts between InitStyle
and InitLookAndFeel.  Perhaps that is easier with more color variables
initialized in InitLookAndFeel rather than keeping StyleContexts around.
Attachment #736306 - Flags: review?(karlt) → review-
Comment on attachment 727206 [details] [diff] [review]
widget clipping patch

>@@ -1417,20 +1426,28 @@ moz_gtk_caret_paint(cairo_t *cr, GdkRect
>                     GtkTextDirection direction)
> {
>     GdkRectangle location = *rect;
> 
>     if (direction == GTK_TEXT_DIR_RTL) {
>         /* gtk_draw_insertion_cursor ignores location.width */
>         location.x = rect->x + rect->width;
>     }
>+    
>+    /* A workaround for Gtk3 clipping bug:
>+     * https://bugzilla.gnome.org/show_bug.cgi?id=694086
>+     */
>+    cairo_save(cr);
>+    gdk_cairo_rectangle(cr,&location);
>+    cairo_clip(cr);
> 
>     ensure_entry_widget();
>     gtk_draw_insertion_cursor(gEntryWidget, cr,
>                               &location, TRUE, direction, FALSE);
>+    cairo_restore(cr);

I don't have a moz_gtk_caret_paint() function in my source code, so I guess
there is another patch somewhere.

draw_insertion_cursor doesn't look like it is meant to paint beyond the
cursor.  We must be missing something here.
https://git.gnome.org/browse/gtk+/tree/gtk/gtkstylecontext.c?id=3.8.2#n4498

Can you step through there, please, and call gdk_flush() after each cairo
drawing operation, to see what is going wrong and check the parameters passed?
Attachment #727206 - Flags: review?(karlt)
This bug is too long to follow all the issues and changes here.  I'll look at
the remaining patches here, but any additional bugs or workarounds should be
addressed in new bug reports marked as blocking this one.  Use this bug for
build system changes if you like as discussion has already started here.
(In reply to Karl Tomlinson (:karlt) from comment #259)
> I don't have a moz_gtk_caret_paint() function in my source code, so I guess
> there is another patch somewhere.

Oh, that would be because of bug 867047.

Re GtkButton and GtkNotebook, does GTK clip before calling the render functions, or does the clipping come from the GdkWindows for the widgets?
Comment on attachment 727638 [details] [diff] [review]
widget padding patch

>@@ -1290,33 +1290,35 @@ moz_gtk_scale_paint(cairo_t *cr, GdkRect

gtk_range_draw uses the margin, so I suspect that is more appropriate than
border + padding.

>@@ -1709,18 +1711,21 @@ moz_gtk_combo_box_paint(cairo_t *cr, Gdk

>+            GtkBorder padding;
>+            GtkStateFlags state_flags = GetStateFlagsFromGtkWidgetState(state);
>+            gtk_style_context_get_border(style, state_flags, &border);
>+            gtk_style_context_get_padding(style, state_flags, &padding);
>+            arrow_rect.x -= (border.left +  padding.left);

gtk_separator_draw has replaced xthickness (in gtk_separator_expose) with
padding.left, so I assume only the padding is needed here.

>@@ -1928,20 +1933,21 @@ moz_gtk_toolbar_separator_paint(cairo_t 

Similarly, _gtk_toolbar_paint_space_line has replaced xthickness with
padding.left, so again only padding is necessary.

>@@ -2404,51 +2410,53 @@ moz_gtk_menu_separator_paint(cairo_t *cr

>         gtk_render_frame(style, cr,
>-                          rect->x + horizontal_padding + border.left,
>-                          rect->y + (rect->height - separator_height - border.top) / 2,
>-                          rect->width - 2 * (horizontal_padding + border.left),
>+                          rect->x + horizontal_padding + border.left + padding.left,
>+                          rect->y + (rect->height - separator_height - border.top - padding.top) / 2,
>+                          rect->width - 2 * (horizontal_padding + border.left + padding.left),

padding.left + padding.right rather than 2 * padding.left in the width.

>                           separator_height);        
>     } else {
>-        paint_height = border.top;
>+        paint_height = border.top + padding.top;
>         if (paint_height > rect->height)
>             paint_height = rect->height;
>         
>         gtk_render_line(style, cr,
>-                        rect->x + horizontal_padding + border.left,
>-                        rect->y + (rect->height - border.top) / 2,
>-                        rect->x + rect->width - horizontal_padding - border.left - 1,
>-                        rect->y + (rect->height - border.top) / 2);
>+                        rect->x + horizontal_padding + border.left + padding.left,
>+                        rect->y + (rect->height - border.top - padding.top) / 2,
>+                        rect->x + rect->width - horizontal_padding - border.left - padding.left - 1,

padding.right instead of .left for the right hand end.

>+                        rect->y + (rect->height - border.top - padding.top) / 2);
>     }

Note also that gtk_menu_item_draw uses gtk_container_get_border_width()
instead of gtk_style_context_get_border().  I don't know where the
border_width gets set.  The vertical dimensions are also a bit different now.

>@@ -2539,19 +2548,20 @@ moz_gtk_check_menu_item_paint(cairo_t *c

>     gtk_style_context_set_state(style, state_flags);
>     gtk_style_context_get_border(style, state_flags, &border);
>+    gtk_style_context_get_padding(style, state_flags, &padding);
> 
>     offset = gtk_container_get_border_width(GTK_CONTAINER(gCheckMenuItemWidget)) +
>-                                            border.left + 2;
>+                                            border.left + padding.left + 2;

gtk_real_check_menu_item_draw_indicator does not use
include border.left from gtk_style_context_get_border().

I haven't checked the rest, so please compare with GTK sources.

>+moz_gtk_get_entry_height(gint* size)
>+{
>+    GtkRequisition requisition;
>+    ensure_entry_widget();
>+
>+    gtk_widget_size_request(gEntryWidget, &requisition);

Isn't this going to mean that the minimum entry size depends on GTK's default font size?
What bug is this fixing?
Attachment #727638 - Flags: review?(karlt) → review-
Comment on attachment 727682 [details] [diff] [review]
tab widget rendering

>+    rect->width += 2 * tab_curvature - tab_overlap;
>+    rect->x -= (2 * tab_curvature - tab_overlap)/2;

I'm not following this.  Can you explain, please?

I would have thought that the rect passed in here is currently from a set of
frame rectangles that do not overlap.  Therefore I would expect that the
overlap needs to be added to the rect so as to provide the overlap.

There should be no overlap between the first/last tab and the end of the tab
panel.  To avoid inconsistency in the position of labels on tabs relative to
the tab borders, I expect nsNativeThemeGTK::GetWidgetPadding() will need to do
something different for the left of the first tab and for the right of the
last tab.  I suspect GetWidgetPadding() should include the tab_curvature and
border and padding (including focus width and padding) on both sides of all
tabs and subtract half the overlap for sides that are adjacent to other tabs
(all expect first and last).  Mark this as a TODO GTK3 if you like.  It gets
more complicated if the overlap is greater than the curvature and padding.
See gtk_notebook_page_allocate().

If this might increase the size of rect then
nsNativeThemeGTK::GetExtraSizeForWidget() needs to return the extra margin,
or are you wanting to clip the tab at |rect|?

Should the clip depend on whether the tab is the selected tab or to the right
or left of the selected tab?

I can think of two approaches to making the tabs overlap with the borders
appearing to overlap in the right order:

1) Rearrange the order of drawing tabs (like gtk_notebook_paint) and use
   GetExtraSizeForWidget() to indicate the overlap.

   I don't know how hard it is to rearrange the order.
   This might be the more efficient method if that is practical.
   No (additional) clipping of drawing should be required.

2) Pass something to moz_gtk_tab_paint in the GtkTabFlags to indicate whether
   the tab is to the selected tab to the left or right of the selected tab.
   Foreground tabs draw the edges of adjacent background tabs.

   GetExtraSizeForWidget() doesn't need to return anything non-zero.
   Clipping is used to ensure that background tabs don't overdraw foreground
   tabs.

   GetWidgetPadding() needs to return different values for selected/left/right
   tabs, which will lead to reflow of the tab bar, despite looking similar.
   The overlap is only subtracted off the padding for background tabs.

>+    /* Adwaita theme engine needs that to pick up correct tab background.
>+     * Actual flags (GTK_REGION_FIRST/GTK_REGION_LAST, 
>+     * GTK_REGION_ODD/GTK_REGION_EVEN) are not used by the theme right now.
>+     */

I'm not clear what 'that' refers to here.  Do you mean it needs GTK_REGION_ODD
or GTK_REGION_EVEN to be set?  Or just that it needs a region?

>+    int region_flags = (GTK_REGION_ODD | GTK_REGION_LAST);
>+    if (flags&MOZ_GTK_TAB_FIRST)
>+        region_flags |= GTK_REGION_FIRST;
>+

Spaces around the '&' operator, please.

I can imagine Adwaita might need one of GTK_REGION_ODD, GTK_REGION_EVEN to pick
up styling, but does Adwaita really need GTK_REGION_LAST?  Surely it must draw
a background even for tabs that are neither first nor last?

>     if ((flags & MOZ_GTK_TAB_SELECTED) == 0) {
>         /* Only draw the tab */
>         gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
>+        gtk_style_context_add_region(style, GTK_STYLE_REGION_TAB, region_flags); 
>         gtk_render_extension(style, cr,
>                              rect->x, rect->y, rect->width, rect->height,
>                             (flags & MOZ_GTK_TAB_BOTTOM) ?
>                                 GTK_POS_TOP : GTK_POS_BOTTOM );
>     } else {

>+        gtk_style_context_add_region(style, GTK_STYLE_REGION_TAB, region_flags); 

Please move the two _add_region calls into a single call before the
conditional.

>         if (flags & MOZ_GTK_TAB_BOTTOM) {
>             /* Draw the tab on bottom */
>             focusRect.y += gap_voffset;
>             focusRect.height -= gap_voffset;
> 
>             gtk_render_extension(style, cr,
>                                  rect->x, rect->y + gap_voffset, rect->width,
>                                  rect->height - gap_voffset, GTK_POS_TOP);
> 
>-            /* Draw the gap; erase with background color before painting in
>-             * case theme does not */
>-            gtk_render_background(style, cr,
>-                                 rect->x,
>-                                 rect->y + gap_voffset
>-                                         - gap_height,
>-                                 rect->width, gap_height);
>-            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, gap_loffset + rect->width);
>-                                 
>         } else {
>-            /* Draw the tab on top */
>-            focusRect.height -= gap_voffset;
>+            /* Draw the tab on top */            

Why is drawing the gap no longer necessary here?
What draws the gap?

Why do top tabs no longer need focusRect adjustment, but bottom tabs still do?

Please remove the additional whitespace added after the comment.
Attachment #727682 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #261)
> Re GtkButton and GtkNotebook, does GTK clip before calling the render
> functions, or does the clipping come from the GdkWindows for the widgets?

These are all NO_WINDOW widgets, so please ignore that question.
Thanks, there's an updated patch, it also removes the static members.

> mCellStyle is used only for eColorID__moz_dragtargetzone, so it would be more
> efficient to use a single color variable (initialized in InitLookAndFeel())
> than keeping the StyleContext as done here.  I don't see -moz-dragtargetzone
> used in Firefox, so I don't know much about its intended use.  What was the
> reason for choosing CLASS_CELL here?
> Could this just use the same code as eColorID_highlight with mViewStyle?

eColorID__moz_dragtargetzone uses the same color as eColorID_WidgetSelectBackground in gtk2 so let's keep it in gtk3 too. I don't know why we used the cell class here - it may be some leftover from color tests.
Attachment #736306 - Attachment is obsolete: true
Attachment #754403 - Flags: review?(karlt)
Depends on: 876553
Comment on attachment 754403 [details] [diff] [review]
nsLookAndFeel v.4

>+static struct _GtkStyleContext *
>+create_context(GtkWidgetPath *path)
>+{
>+    struct _GtkStyleContext * mStyle = gtk_style_context_new();

Please rename |mStyle| to |style| as the |m| prefix is used to indicate member
variables.

I'd prefer "static GtkStyleContext *" and "GtkStyleContext *style" over
"struct _GtkStyleContext * ".

>+    sMozFieldBackground = GDK_RGBA_TO_NS_RGB(color);

>+    sMozFieldText = GDK_RGBA_TO_NS_RGB(color);

GDK_RGBA_TO_NS_RGBA

>+    GtkWidgetPath *path = gtk_widget_path_new();
>+    gtk_widget_path_append_type(path, GTK_TYPE_WINDOW);

>+    // Window colors
>+    style = create_context(path);
>+    gtk_style_context_save(style);
>+    gtk_style_context_add_class(style, GTK_STYLE_CLASS_BACKGROUND);

>+    // Selected text and background
>+    gtk_style_context_get_background_color(style, GTK_STATE_FLAG_SELECTED, &color);
>+    sMozWindowSelectedBackground = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_get_color(style, GTK_STATE_FLAG_SELECTED, &color);
>+    sMozWindowSelectedText = GDK_RGBA_TO_NS_RGBA(color);
>+    gtk_style_context_restore(style);

I don't see any :selected rules in gtk-default.css that would match the path
and class here.  mViewStyle may be more appropriate and GTK_STATE_FLAG_FOCUSED
may be required to get what we want, but that can be addressed later.  Let's
get this landed now, thanks.
Attachment #754403 - Flags: review?(karlt) → review+
No longer depends on: 876553
Attachment #754789 - Flags: checkin?
Attachment #727215 - Flags: checkin?
Attachment #718386 - Flags: checkin?
Attachment #718386 - Flags: checkin? → checkin+
Attachment #727215 - Flags: checkin? → checkin+
Attachment #754789 - Flags: checkin? → checkin+
Depends on: 877601
Depends on: 877605
Depends on: 877606
Depends on: 877609
Depends on: 877626
Depends on: 878781
Depends on: 879515
Depends on: 879746
Depends on: 879760
Blocks: 882036
The last build patches hit latest trunk so you can build the gtk3 browser now. It's still a bit raw - there are bugs in color/widget/menu rendering - but it runs. Use this build flags:

ac_add_options --enable-system-cairo
ac_add_options --enable-default-toolkit=cairo-gtk3
In order to compile Firefox with GTK+3 support, I had to apply this patch.

I am using GLib 2.37.1 GTK+ 3.9.2.
(In reply to Martin Stránský from comment #270)
> The last build patches hit latest trunk so you can build the gtk3 browser
> now. It's still a bit raw - there are bugs in color/widget/menu rendering -
> but it runs. Use this build flags:
> 
> ac_add_options --enable-system-cairo
> ac_add_options --enable-default-toolkit=cairo-gtk3

How does such build work in regard of what I've reported in bug 862422 ?
To reproduce you may (and do) need to use cairo 1.12 on the system.
(In reply to Quentin "Sardem FF7" Glidic from comment #271)
> Created attachment 761985 [details] [diff] [review]
> GTK+3 build fixes
> 
> In order to compile Firefox with GTK+3 support, I had to apply this patch.
> 
> I am using GLib 2.37.1 GTK+ 3.9.2.

I don't see that. Please file a bug for that and attach your mozconfig there. Thanks!
(In reply to Rafał Mużyło from comment #272)
> How does such build work in regard of what I've reported in bug 862422 ?
> To reproduce you may (and do) need to use cairo 1.12 on the system.

The recent gtk3 port does not support NPAPI plugins (which are gtk2 only) now so the flash plugin does not work there.
Blocks: 884634
Depends on: 884708
Depends on: 884831
Depends on: 886771
Depends on: 887816
Depends on: 888863
Depends on: 888874
Depends on: 890022
Depends on: 897404
Depends on: 908102
Blocks: 900740
No longer blocks: 900740
Depends on: 928399
Depends on: 943407
(In reply to Martin Stránský from comment #274)
> (In reply to Rafał Mużyło from comment #272)
> > How does such build work in regard of what I've reported in bug 862422 ?
> > To reproduce you may (and do) need to use cairo 1.12 on the system.
> 
> The recent gtk3 port does not support NPAPI plugins (which are gtk2 only)
> now so the flash plugin does not work there.

AFAIK Firefox is planning on moving to a multi-process architecture. If so it should be possible to get Flash working with Gtk3 :P (and other plugins that depend on Gtk2)

WebKitGtk3-2.0 already has a fix that we could adopt (if applicable):
"The WebkitGTK+ solution for the Flash problem was implemented in version 2.0 by having a second process linked against GTK2 to run browser plugins while Webkit itself is linked to GTK3. This makes Flash work again."

http://lzone.de/How%20to%20Get%20Flash%20Working%20with%20WebkitGTK3

What do you think?
Cheers
(In reply to Seif Lotfy from comment #275)
> WebKitGtk3-2.0 already has a fix that we could adopt (if applicable):
> "The WebkitGTK+ solution for the Flash problem was implemented in version
> 2.0 by having a second process linked against GTK2 to run browser plugins
> while Webkit itself is linked to GTK3. This makes Flash work again."

It's covered by Bug 624422 where two libxul are build and plugin-container is linked to the gtk2 one.
Attachment #8356778 - Flags: review?(stransky)
Attachment #8356778 - Flags: review?(karlt)
Comment on attachment 8356778 [details] [diff] [review]
Fix build with --disable-dbus

Looks okay.
Attachment #8356778 - Flags: review?(stransky) → feedback+
Attachment #8356778 - Flags: review?(karlt) → review+
Whiteboard: [leave open for remaining patches][check linux try build before requesting checkin] → [leave open as a tracking bug][please file new bugs for additional patches]]
Attachment #8356778 - Flags: checkin?(ryanvm)
See Also: → 958811
Depends on: 959224
See Also: 958811
Test packages for Fedora based on trunk can be downloaded here - http://copr-fe.cloud.fedoraproject.org/coprs/stransky/FirefoxGtk3/
How can It be built for other flavour?
Blocks: 958868
Depends on: 972382
I've filed bug 975919 to cover making sure that Gtk+ 3's HiDPI mode is properly supported.
Depends on: 975919
Depends on: 978172
Depends on: 978679
Depends on: 979839
Depends on: 980249
Depends on: 982002
Depends on: 982640
Depends on: 982694
Depends on: 982964, 982966
Is bug reports on this feature of interest or is that too early for that yet? (i have a long list)
(In reply to Cork from comment #284)
> Is bug reports on this feature of interest or is that too early for that
> yet? (i have a long list)

Please file new bugs and make them block this one for tracking purpose.
Depends on: 983843
Depends on: 983903
Depends on: 984075
Depends on: 984078
Depends on: 984082
Depends on: 984083
There, the other bugs I see appear to be theme bugs and not firefox bugs.
Depends on: 984993
Depends on: 989819
(In reply to Wiseacre from comment #282)
> How can It be built for other flavour?

Check out the latest trunk, create a .mozconfig file with:

. $topsrcdir/browser/config/mozconfig
ac_add_options --enable-default-toolkit=cairo-gtk3
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/objdir
ac_add_options --without-system-nspr
ac_add_options --without-system-nss

and build as usually.
No longer depends on: 989819
Depends on: 991272
Depends on: 991686
Depends on: 995089
Depends on: 996678
Blocks: 720523
Depends on: 939284
I've been running this on Fedora 21 (Rawhide) and Electrolysis (separate UI and content processes / tab sandboxing) doesn't work (browser window is empty when I turn it on)

https://wiki.mozilla.org/Electrolysis

Is there an open bug for this issue?
If you don't find a duplicate, don't hesitate to file a bug. Make it block this one so it can be tracked. 

Thanks.
Depends on: 1013552
Thanks, i filed it as bug 1013552
Alias: gtk3
Depends on: 1022123
Depends on: 1022127
Depends on: 1022135
Depends on: 1022259
Depends on: 1023004
Depends on: 1025715
Depends on: 1025710
Depends on: 1025874
Depends on: 1027000
Depends on: try-gtk3
Depends on: 1027009
Depends on: 1027034
Depends on: 1027040
Depends on: 1027138
Depends on: 1027440
Depends on: 1027448
Depends on: 1027497
Depends on: 1028137
Depends on: 1031267
Keywords: meta
Blocks: 1034064
No longer blocks: 1034064
Depends on: 1034064
Depends on: 1039897
Depends on: 1041345
No longer depends on: 1041345
Depends on: 1022241
Depends on: 1051209
Depends on: 1072902
Depends on: 1091136
Depends on: 1092124
Depends on: 1097592
Depends on: 1101582
Depends on: 1110211
Depends on: 1127752
Depends on: 1126237
Depends on: 1129873
Depends on: 1131978
Depends on: 1138295
Depends on: 1140832
No longer depends on: 1140832
Depends on: 1143686
Depends on: 1144643
Depends on: 1144745
Depends on: 1147847
Depends on: 1147848
Depends on: 1157869
Depends on: 1158076
Depends on: 1160154
Depends on: 1159273
Depends on: 1161056
Depends on: 1163954
Depends on: 1163974
Depends on: 1163993
Depends on: 1165513
Depends on: 1166741
Depends on: 1167239
Depends on: 1167720
Depends on: 1168578
Depends on: 1169232
Depends on: 1169233
Depends on: 1169370
Depends on: 1168464
No longer depends on: 1168464
Depends on: 1169666
Depends on: 1169955
Depends on: 1170158
Depends on: 1170342
Depends on: 1170783
Depends on: 1170800
No longer depends on: 1170800
Depends on: 1171011
Depends on: 1171029
Depends on: 1171696
Depends on: 1171728
Depends on: 1171972
Depends on: 1173520
Depends on: 1173907
Depends on: 1173971
Depends on: 1174248
Blocks: 1043146
Depends on: 1174755
Depends on: 1174509
Depends on: 1175029
Depends on: 1176109
Depends on: 1176929
Depends on: 1177171
Depends on: 1177233
Depends on: 1178896
Depends on: 1179780
Depends on: 1180234
Depends on: 1180704
Depends on: 1180840
Depends on: 1182494
Depends on: 1182632
Depends on: 1186003
Depends on: 1186229
Depends on: 1186661
Depends on: 1186748
Depends on: 1187203
Depends on: 1187208
Depends on: 1187252
Depends on: 1187271
Depends on: 1187282
Depends on: 1187385
Depends on: 1187393
Depends on: 1187406
Depends on: 1187414
Depends on: 1188138
Depends on: 1189028
Depends on: 1189189
Depends on: 1189592
Depends on: 1187237
Depends on: 1189923
Depends on: 1190180
Depends on: 1135341
Depends on: 1191795
It is possible someone can take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1190163 ?
It's just a matter of making the tab area in 'certificate viewer' a bit darker than the rest of window and making the tab text a bit grey.
(Note: This is not about Firefox browser tabs).
Depends on: 1190163
No longer depends on: 1191795
Depends on: 1186989
Depends on: 1187485
Depends on: 1194914
Depends on: 1196777
Depends on: 1197590
Depends on: 1195002
No longer depends on: 1193570
Depends on: 1198557
Depends on: 1194044
Depends on: 1204358
No longer depends on: 726483
Depends on: 726483
Am I correct thinking that there's no way to run the official Firefox (>=42) Linux builds without GTK3?

If so, what's the point of the following file:

gtk2/libmozgtk.so ?

It surely looks like there's a way to run Firefox with GTK2 but ./firefox --help doesn't run since it cannot find GTK3 libraries (on CentOS 6.7).

Actually it's the stuff for an extra bug report: ./firefox --help should have the least amount of dependencies to show its startup options/flags.
(In reply to Artem S. Tashkinov from comment #295)
> Am I correct thinking that there's no way to run the official Firefox (>=42)
> Linux builds without GTK3?

There's no way to run gtk3 pages as gtk2. gtk2/libmozgtk.so is only used for gtk2 plugins.
Depends on: 1199164
Depends on: 1164674
Depends on: 1207700
Depends on: 1207975
Depends on: 1174378
Depends on: 1208372
Depends on: 1209343
No longer blocks: 1213601
Depends on: 1213601
Depends on: 1214170
Depends on: 1214957
Blocks: 1175029
No longer depends on: 1175029
Blocks: 978679
No longer depends on: 978679
No longer depends on: 1214170
Depends on: 1216246
Depends on: 1219717
Depends on: 1220050
Depends on: 1220386
No longer depends on: 1220050
No longer depends on: 1220386
I have installed the official Firefox 42 on Ubuntu 14.04 64 bits and it's compiled with gtk2:
Configure arguments
--enable-update-channel=release --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --with-google-oauth-api-keyfile=/builds/google-oauth-api.key --with-mozilla-api-keyfile=/builds/mozilla-desktop-geoloc-api.key --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-default-toolkit=cairo-gtk2 --enable-warnings-as-errors --enable-official-branding --enable-verify-mar

For when a stable version with gtk3?
(In reply to Simón from comment #297)
> I have installed the official Firefox 42 on Ubuntu 14.04 64 bits and it's
> compiled with gtk2:
> --enable-stdcxx-compat --enable-default-toolkit=cairo-gtk2
Your distribution specified --enable-default-toolkit=cairo-gtk2 (meaning they are specifying that want a gtk2 build)
You should probably talk to them about removing that configure option if you want a gtk3 build.
(In reply to Hussam Al-Tayeb from comment #298)
> (In reply to Simón from comment #297)
> > I have installed the official Firefox 42 on Ubuntu 14.04 64 bits and it's
> > compiled with gtk2:
> > --enable-stdcxx-compat --enable-default-toolkit=cairo-gtk2
> Your distribution specified --enable-default-toolkit=cairo-gtk2 (meaning
> they are specifying that want a gtk2 build)
> You should probably talk to them about removing that configure option if you
> want a gtk3 build.

I downloaded my Firefox release from mozilla.org, no from package manager of Ubuntu.
(In reply to Simón from comment #299)
> (In reply to Hussam Al-Tayeb from comment #298)
> > (In reply to Simón from comment #297)
> > > I have installed the official Firefox 42 on Ubuntu 14.04 64 bits and it's
> > > compiled with gtk2:
> > > --enable-stdcxx-compat --enable-default-toolkit=cairo-gtk2
> > Your distribution specified --enable-default-toolkit=cairo-gtk2 (meaning
> > they are specifying that want a gtk2 build)
> > You should probably talk to them about removing that configure option if you
> > want a gtk3 build.
> 
> I downloaded my Firefox release from mozilla.org, no from package manager of
> Ubuntu.

Ah. sorry then. I misunderstood. Looks like delayed to 43 according to to https://bugzilla.mozilla.org/show_bug.cgi?id=1186003
Depends on: 1225520
Depends on: 1228281
Depends on: 1230065
Depends on: 1234158
Depends on: 1186967
Depends on: 1204406
Depends on: 1234805
Hello, I now have a problem launching Firefox at all. This seems to be a gtk3 issue on my system, because even zenity or gtk3-demo won't start on it.
Bissected the issue to https://hg.mozilla.org/integration/mozilla-inbound/rev/939320b957c5 with mozregression. I just want you to be aware of some problems caused by the transition to gtk3.

I will try to work with gtk developers to get this fixed, but I can't use any updated Firefox version in the meanwhile.
Depends on: 1235520
No longer depends on: 1235520
Depends on: 1236267
Depends on: 1228505
Is there a way to make Firefox use GTK2 vs GTK3 for a quick test?

This is the ticket for where Firefox went to a moderately high CPU use after an update:
https://bugzilla.mozilla.org/show_bug.cgi?id=508427#c111

Today I saw the same type issue with Eclipse. After running 'strace -f eclipse' it loops on the same type of polling/timeout as Firefox is per ticket 508427.

Thanks,

-Jeff
Depends on: 1239213
Depends on: 1224206
Depends on: 1244305
Depends on: 1245051
Depends on: 1245167
No longer depends on: 1245167
Depends on: 1246422
Depends on: 1248974
Depends on: 1250704
Depends on: 1197165
Depends on: 1255344
Depends on: 1257811
Depends on: 1258086
Depends on: 1258989
Depends on: 1259433
Depends on: 1260061
Depends on: 1260777
Depends on: 1260902
Depends on: 1261068
Depends on: 1261077
Depends on: 1261103
Depends on: 1261576
Depends on: 1262136
No longer depends on: 1262136
Depends on: 1250206
Depends on: 1263145
Depends on: 1263427
Depends on: 1263791
Depends on: gtk-3.20
Could someone please raise the importance of https://bugzilla.mozilla.org/show_bug.cgi?id=1263427 from normal, because it renders several sites completely unusable requiring to use another browser.
Depends on: 1265116
Depends on: 1265254
Depends on: 1265735
Depends on: 1266338
Depends on: 1267815
Depends on: 1267988
Depends on: 1267863
Depends on: 1267982
Blocks: 1268412
Depends on: 1268462
Depends on: 1268395
Depends on: 1268338
Depends on: 1268121
Depends on: 1268244
Depends on: 1268784
Depends on: 1268599
Would this be of any interest?  bug 1268894. The toolbar menu items already use "up arrows" or whatever they are called which is basically similar to native gtk+ popovers. Perhaps theme them like gtk+ popovers?
Depends on: 1269121
Depends on: 1269108
Depends on: 1269164
Depends on: 1269149
Depends on: 1267724
Blocks: 1213609
No longer depends on: 1213609
Depends on: 1269258
Depends on: 1269243
Depends on: 1268894
Depends on: 1269306
No longer depends on: 1268590
Depends on: 1268241
Depends on: 1269561
Depends on: 1269523
Depends on: 1270208
Depends on: 1270176
Depends on: 1270421
Depends on: 1270606
Depends on: 1270473
Depends on: 1270668
Depends on: 1271305
Depends on: 1271362
Depends on: 1269166
Depends on: 1271579
Depends on: 1271498
If possible, please also take a look at bug 1271523 and bug 1271524. I am trying to find areas there themeing is missing.
Depends on: 1271716
Depends on: 1272140
Depends on: 1269145
Depends on: 1272332
Depends on: 1271783
No longer depends on: 1272332
If time permits, can you please verify bug 1273013? Download progress bar is invisible in my gtk3 build.
Thank you.
Depends on: 1273013
No longer depends on: 1273013
Depends on: 1272982
Depends on: 1272902
Depends on: 1273209
Depends on: 1273746
No longer depends on: 1273746
Depends on: 694570
Depends on: 1275407
No longer depends on: 1275407
Depends on: 1275861
Depends on: 1275700
Depends on: 1276534
No longer depends on: 1276534
Depends on: 1225044
Depends on: 1263171
Depends on: 1278108
No longer depends on: 1278108
Depends on: 1279144
Depends on: 1279419
Depends on: 1280227
Depends on: 1279978
No longer depends on: 1234805
No longer depends on: 1263171
Depends on: 1281435
Depends on: 1281364
Depends on: 1281710
Depends on: 1281733
Depends on: 1283086
Depends on: 1280226
Depends on: 1284105
Depends on: 1287036
Depends on: 1287037
Depends on: 1289063
Depends on: 1301285
Depends on: 1305367
Depends on: 1321332
We moved to gtk3, we remove the GTK2 code in bug 1278282, let's close it \o/
Thanks to all for the hard work!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard field says:  [leave open as a tracking bug][please... 

Would you mind reopening? this is a tracker bug. Plenty of gtk3 related bugs are still open. The gtk3 port is far from complete.
Perhaps also change the Subject to something like pre GTK3.20 GTK3 open issues.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Port GTK2 to GTK3 → pre GTK3.20 open issues
Status: REOPENED → NEW
Won't argue for long but the port itself is completed. The "leave open" is 3 years old.

And, instead of renaming it, we should open a new meta bug to deal with the remaining issues from the migration.
Bugs are cheap.

Do you agree?
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Summary: pre GTK3.20 open issues → Port GTK2 to GTK3
Either choice would be very appreciated.
If you do open a new bug, please post the bug report id here so I can CC myself.
Thank you very much.
(In reply to Sylvestre Ledru [:sylvestre] from comment #310)
> Won't argue for long but the port itself is completed. The "leave open" is 3
> years old.
> 
> And, instead of renaming it, we should open a new meta bug to deal with the
> remaining issues from the migration.
> Bugs are cheap.
> 
> Do you agree?

Well my reason for just changing the subject and leaving it open is that the new subject is more representative of the way this bug has been used by people working on gtk3 since gtk3.20 was released.  We had 2 tracking bugs open.  one for gtk3 issues from before the version 3.20 and one for thing introduced by the changes in gtk3.20.  I can certainly open a new bug copying all the cc's and all the unresolved bugs as being tracked.  I just thought redefining it to the way hit has been being used for a awhile was easier.  I will leave it to karlt: and stransky:, who have been the main people working on gtk3 lately, to sort this.
Depends on: 1354272
Depends on: 1383118
Summary: Port GTK2 to GTK3 → [meta] Port GTK2 to GTK3
No longer blocks: 720523
You need to log in before you can comment on or make changes to this bug.