Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
Widget: Gtk
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: Jan Horak, Assigned: Martin Stránský)

Tracking

(Depends on: 59 bugs, Blocks: 7 bugs, {meta})

Trunk
x86
Linux
Points:
---
1022241, 1147847, 1164674, 1169955, 1170342, 1174378, 1174509, 1186967, 1189189, 1189592, 1189923, 1190163, ship-gtk3, 1196777, 1197590, 1207700, 1207975, 1208372, 1213600, 1213601, 1224206, 1239213, 1246422, 1260061, 1260777, 1261064, 1261077, 1263427, gtk-3.20, 1267724, 1267815, 1267863, 1267982, 1268121, 1268234, 1268241, 1268338, 1268395, 1268599, 1268618, 1268894, 1269108, 1269145, 1269149, 1269166, 1270176, 1270421, 1270606, 1271305, 1271716, 1272902, 1273209, 1281364, 1281435, 1283086, 1284105, 1321332, 1329740, gtk3-pre-3.20, 624422, 694570, 709259, 722975, 726483, 772883, 791518, 791529, 792158, 877601, 877605, 877606, 877609, 877626, 878781, 879515, 879746, 879760, 884708, 884831, 886771, 887816, 888863, 888874, 890022, 897404, 908102, 928399, 939284, 943407, 959224, 972382, 975919, 978172, 979839, 980249, 982002, 982640, 982694, 982964, 982966, 983843, 983903, 984075, 984078, 984082, 984083, 984993, 991272, 991686, 995089, 996678, 1013552, try-gtk3, 1022123, 1022127, 1022135, 1022259, 1023004, 1025710, 1025715, 1025874, 1027000, 1027009, 1027034, 1027040, 1027138, 1027440, 1027448, 1027497, 1028137, 1031267, 1034064, 1039897, 1051209, 1063359, 1072902, 1073117, 1091136, 1092124, 1097592, 1101582, 1110211, 1126237, 1127752, 1129873, 1131978, 1135341, 1138295, 1143686, 1144643, 1144745, 1147848, 1157869, 1158076, 1159273, 1160154, 1161056, 1163954, 1163974, 1163993, 1165513, 1166741, 1167239, 1167720, 1168578, 1169232, 1169233, 1169370, 1169666, 1170158, 1170783, 1171011, 1171029, 1171696, 1171728, 1171972, 1173520, 1173907, 1173971, 1174248, 1174755, 1176109, 1176929, 1177171, 1177233, 1178896, 1179780, 1180234, 1180704, 1180840, 1182494, 1182632, 1186003, 1186229, 1186661, 1186748, 1186989, 1187166, 1187203, 1187208, 1187237, 1187252, 1187263, 1187271, 1187282, 1187385, 1187393, 1187406, 1187414, 1187485, 1188138, 1189028, 1190180, 1194044, 1194914, 1195002, 1197165, 1198557, 1198613, 1199164, 1204358, 1204406, 1209343, 1214957, 1216246, 1219717, 1221498, 1225044, 1225520, 1228281, 1228505, 1230065, 1234005, 1234158, 1236267, 1244305, 1245051, 1248974, 1250206, 1250704, 1255344, 1257811, 1258086, 1258989, 1259433, 1260902, 1261068, 1261095, 1261103, 1261576, 1263145, 1263791, 1265116, 1265254, 1265735, 1266338, 1267988, 1268244, 1268462, 1268465, 1268530, 1268784, 1268874, 1269058, 1269061, 1269121, 1269164, 1269243, 1269258, 1269306, 1269523, 1269561, 1270208, 1270473, 1270668, 1271362, 1271498, 1271579, 1271783, 1272140, 1272982, 1275700, 1275861, 1276596, 1279144, 1279419, 1279978, 1280226, 1280227, 1281710, 1281733, 1287036, 1287037, 1289063, 1301285, 1305367, 1354272
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

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

Attachments

(26 attachments, 66 obsolete attachments)

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
Martin Stránský
: review+
emorley
: checkin+
Details | Diff | Splinter Review
138.70 KB, patch
karlt
: review+
emorley
: checkin+
Details | Diff | Splinter Review
5.71 KB, patch
Martin Stránský
: review+
roc
: checkin+
Details | Diff | Splinter Review
669 bytes, patch
Martin Stránský
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.73 KB, patch
bsmedberg
: review+
darktrojan
: checkin+
Details | Diff | Splinter Review
24.50 KB, patch
Martin Stránský
: review+
darktrojan
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
karlt
: review+
Ehsan
: checkin+
Details | Diff | Splinter Review
25.18 KB, patch
Martin Stránský
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
838 bytes, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
22.17 KB, patch
Martin Stránský
: 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
Martin Stránský
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
846 bytes, patch
karlt
: review+
Martin Stránský
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 505773 [details] [diff] [review]
Very WIP patch

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).
(Reporter)

Updated

7 years ago
OS: Linux → Windows Server 2003
(Assignee)

Updated

7 years ago
OS: Windows Server 2003 → Linux

Updated

7 years ago
Duplicate of this bug: 610234
(Assignee)

Comment 2

7 years ago
Created attachment 513089 [details] [diff] [review]
wip v2

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)

Updated

7 years ago
Assignee: nobody → stransky
(Assignee)

Comment 3

7 years ago
Created attachment 513457 [details] [diff] [review]
wip v3

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.
(Assignee)

Comment 5

7 years ago
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.
(Reporter)

Comment 6

7 years ago
Created attachment 515080 [details] [diff] [review]
wip v4

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
(Reporter)

Comment 7

6 years ago
Created attachment 518712 [details] [diff] [review]
WIP v5

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
(Reporter)

Comment 8

6 years ago
Created attachment 519093 [details] [diff] [review]
WIP v6

Wrong patch, reuploading.
(Reporter)

Updated

6 years ago
Attachment #519093 - Attachment is patch: true
Attachment #519093 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Updated

6 years ago
Attachment #518712 - Attachment is obsolete: true

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
Created attachment 519919 [details] [diff] [review]
WIP v7

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

Comment 12

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

Updated

6 years ago
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
(Reporter)

Comment 15

6 years ago
(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.
(Reporter)

Comment 16

6 years ago
(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

Comment 18

6 years ago
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.
(Assignee)

Comment 19

6 years ago
Sure, the code should be optimized and refactorised. Our goal is to create a minimal working patch and then improve it.
(Reporter)

Comment 20

6 years ago
(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.
(Reporter)

Comment 21

6 years ago
Created attachment 521184 [details] [diff] [review]
WIP v8

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).

Comment 23

6 years ago
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.
(Assignee)

Comment 26

6 years ago
(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.

Comment 27

6 years ago
(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...
(Assignee)

Comment 28

6 years ago
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.
(Assignee)

Comment 30

6 years ago
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.)
(Reporter)

Comment 32

6 years ago
Created attachment 523284 [details] [diff] [review]
WIP v9

Attaching rebased patch:
- Martin made gtk2 plugins in gtk3 code possible!
- Fixed custom mouse cursors
Attachment #521184 - Attachment is obsolete: true

Comment 33

6 years ago
jhorak, any updates on the patch or its status?
(Assignee)

Comment 34

6 years ago
(In reply to comment #33)
> jhorak, any updates on the patch or its status?

Are you looking for something particular?

Comment 35

6 years ago
(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.

Comment 36

6 years ago
(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 :)
(Assignee)

Comment 37

6 years ago
> > 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.
(Reporter)

Comment 38

6 years ago
Created attachment 525346 [details] [diff] [review]
gtk3 directory patch v1

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.
Created attachment 533890 [details]
specific comments on gtk3drawing
(Assignee)

Comment 41

6 years ago
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

Comment 43

6 years ago
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?

Comment 44

6 years ago
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.
(Assignee)

Comment 45

6 years ago
(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.

Comment 46

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

Comment 47

6 years ago
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.
(Assignee)

Comment 48

6 years ago
Created attachment 538243 [details] [diff] [review]
complete patch against latest trunk

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)

Comment 49

6 years ago
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
(Assignee)

Comment 50

6 years ago
Created attachment 540027 [details] [diff] [review]
gtk drawing patch

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)
(Assignee)

Comment 51

6 years ago
Created attachment 540030 [details] [diff] [review]
mozcontainer.c patch

patch to widget/src/gtk2/mozcontainer.c
Attachment #540030 - Flags: review?(karlt)
(Assignee)

Comment 52

6 years ago
Created attachment 540031 [details] [diff] [review]
nsLookAndFeel.cpp patch
Attachment #540031 - Flags: review?(karlt)
(Assignee)

Comment 53

6 years ago
Created attachment 540032 [details] [diff] [review]
gtk2/nsWindow.cpp patch
Attachment #540032 - Flags: review?(karlt)
(Assignee)

Comment 54

6 years ago
Created attachment 540034 [details] [diff] [review]
gtk2 makefile patch

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)
(Assignee)

Comment 55

6 years ago
Created attachment 540036 [details] [diff] [review]
rest of gtk2 dir fixes
Attachment #540036 - Flags: review?(karlt)
Created attachment 542343 [details]
gtk3drawing review comments 2
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-
Created attachment 542725 [details]
nsWindow except drawing review comments

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
Created attachment 543317 [details]
nsWindow drawing review comments

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-

Comment 61

6 years ago
Does this means that if Firefox is ported to GTK+ 3, Firefox will run on Wayland?

Please support Wayland.

Thanks.

Comment 62

6 years ago
(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.

Comment 63

6 years ago
(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.

Comment 64

6 years ago
(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

Comment 66

6 years ago
Just out of curiosity, has any progress on this been made in the last month?
(Assignee)

Comment 67

6 years ago
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)
(Assignee)

Comment 69

6 years ago
(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.
(Assignee)

Comment 71

6 years ago
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.
(Assignee)

Comment 72

6 years ago
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.
(Assignee)

Comment 74

6 years ago
(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.
(Assignee)

Comment 76

6 years ago
(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.
(Assignee)

Comment 77

6 years ago
Created attachment 557147 [details] [diff] [review]
v2. nsWindow/gtk2compat.h patch

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)
(Assignee)

Comment 78

6 years ago
Created attachment 557153 [details] [diff] [review]
nsWindow.cpp - v3

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)

Comment 80

6 years ago
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+
Created attachment 561418 [details]
nsWindow review comments 2
(Assignee)

Comment 83

6 years ago
Created attachment 561682 [details] [diff] [review]
nsWindow.cpp, v4

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
(Assignee)

Comment 85

6 years ago
Created attachment 561701 [details] [diff] [review]
nsWindow.cpp, v4, commit friendly version, r=karlt
Attachment #561682 - Attachment is obsolete: true
Attachment #561701 - Flags: review+
Attachment #561701 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches]
Comment on attachment 561701 [details] [diff] [review]
nsWindow.cpp, v4, commit friendly version, r=karlt

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=59eea1f86cc9

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 87

6 years ago
Created attachment 561718 [details] [diff] [review]
nsWindow.cpp, v5, commit friendly version, r=karlt

added missing gtk2compat.h file
Attachment #561718 - Flags: review+
Attachment #561718 - Flags: checkin?
(Assignee)

Updated

6 years ago
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

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

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

Comment 90

6 years ago
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.
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

:-)

Comment 92

6 years ago
(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?
(Assignee)

Comment 94

6 years ago
(In reply to Ed Morley [:edmorley] from comment #93)
> Comment on attachment 561753 [details] [diff] [review]
> nsWindow.cpp, v6
> 
> Passed try. Happy for me to land now?

Yes, Please.

(In reply to Hussam Al-Tayeb from comment #92)
> I finished a gtk2 build with mozilla-central and only this patch. what other
> patches do I need to make a gtk3 build?

You need a complete patch, this one is just a small step. The easiest way is to grab the complete one attached here (attachment #538243 [details] [diff] [review]), download mozilla-central from 2011-06-09, patch it and build.
Comment on attachment 561753 [details] [diff] [review]
nsWindow.cpp, v6

https://hg.mozilla.org/integration/mozilla-inbound/rev/875cb4f20eac
Attachment #561753 - Flags: checkin+
(Assignee)

Comment 96

6 years ago
Created attachment 561999 [details] [diff] [review]
build fix - startup notification

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+
(Assignee)

Comment 98

6 years ago
Created attachment 562004 [details] [diff] [review]
build fix - startup notification, v2

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-
(Assignee)

Comment 100

6 years ago
Created attachment 562011 [details] [diff] [review]
build fix - startup notification, v3

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+
(Assignee)

Updated

6 years ago
Attachment #562011 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Comment on attachment 561753 [details] [diff] [review]
nsWindow.cpp, v6

nsWindow.cpp part: https://hg.mozilla.org/mozilla-central/rev/875cb4f20eac

Comment 103

6 years ago
small intermission from a lurker:

(In reply to Karl Tomlinson (:karlt) from comment #101)
> > GDK_DRAWABLE() adds code for an extra function call per caller.
> 
> Actually two function calls: gdk_drawable_get_type and g_type_check_instance.

Only if you don't compile with -DG_DISABLE_CAST_CHECKS which people definitely should do for releases. In that case those casts are just casts.

FWIW, using GNOME coding style, I'd make every cast use the macro both because of style ad because I know there's no performance difference, but huge debuggability improvements (once you turn it on).
Comment on attachment 562011 [details] [diff] [review]
build fix - startup notification, v3

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=003fcee02889

https://hg.mozilla.org/integration/mozilla-inbound/rev/2137c5bd3c36
Attachment #562011 - Flags: checkin? → checkin+

Updated

6 years ago
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.
(Assignee)

Comment 107

6 years ago
Created attachment 563434 [details] [diff] [review]
gtk drawing patch v.2 + comments

It should address the remarks, my comments are at top of the patch.
Attachment #540027 - Attachment is obsolete: true
Attachment #563434 - Flags: feedback?(karlt)
(Assignee)

Comment 108

6 years ago
Created attachment 563817 [details] [diff] [review]
mozcontainer.c patch, v2
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+
(Assignee)

Comment 110

6 years ago
Created attachment 564498 [details] [diff] [review]
mozcontainer.c patch, v3

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.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/f09c204296f8
Keywords: checkin-needed
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?
(Assignee)

Comment 114

6 years ago
Created attachment 565231 [details] [diff] [review]
mozcontainer.c patch, v5

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?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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?
Keywords: checkin-needed
(Assignee)

Comment 116

6 years ago
Created attachment 565464 [details] [diff] [review]
mozcontainer.c patch, v6, fixed indentation

Fixed indentation.
Attachment #565231 - Attachment is obsolete: true
Attachment #565464 - Flags: review+
Attachment #565464 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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?

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 118

6 years ago
Created attachment 565519 [details] [diff] [review]
gtk drawing patch v.3

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)
Comment on attachment 565464 [details] [diff] [review]
mozcontainer.c patch, v6, fixed indentation

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f8b4c3c12a
Attachment #565464 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/f9f8b4c3c12a
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)
(Assignee)

Comment 122

6 years ago
Created attachment 568338 [details] [diff] [review]
gkt3drawing, v.4

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)

Comment 123

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

Comment 124

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

Comment 125

6 years ago
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 ?
Created attachment 588315 [details]
review comments on gkt3drawing, v.4

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
Created attachment 593982 [details]
review comments on gkt3drawing, v.4 (completed)
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)
(Assignee)

Comment 129

6 years ago
Created attachment 595369 [details] [diff] [review]
gtk drawing patch v.5

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.
(Assignee)

Comment 133

6 years ago
Created attachment 595699 [details] [diff] [review]
gkt3drawing, v.6

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+
(Assignee)

Updated

6 years ago
Attachment #595699 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/5d76c57d1e8c
Keywords: checkin-needed

Updated

6 years ago
Attachment #595699 - Flags: checkin? → checkin+
Comment on attachment 595699 [details] [diff] [review]
gkt3drawing, v.6

https://hg.mozilla.org/mozilla-central/rev/5d76c57d1e8c
Depends on: 726483

Updated

6 years ago
Blocks: 635134

Updated

5 years ago

Updated

5 years ago

Comment 136

5 years ago
(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+ :-)

Comment 137

5 years ago
Sorry, I've had a few crazy weeks :(

I'll get to look at these patches this evening
Blocks: 738937
(Assignee)

Comment 138

5 years ago
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).

Comment 139

5 years ago
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 ?

Comment 140

5 years ago
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 141

5 years ago
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-
(Assignee)

Comment 142

5 years ago
Created attachment 626372 [details] [diff] [review]
fix drawing&window parts

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-
(Assignee)

Comment 144

5 years ago
Created attachment 626756 [details] [diff] [review]
fix drawing&window parts, v.2

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+
(Assignee)

Comment 146

5 years ago
Created attachment 627143 [details] [diff] [review]
fix drawing&window parts, v.2, for check-in

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+
Comment on attachment 627143 [details] [diff] [review]
fix drawing&window parts, v.2, for check-in

https://hg.mozilla.org/mozilla-central/rev/6c1dbb75d232
(Assignee)

Comment 149

5 years ago
Created attachment 627866 [details] [diff] [review]
nsLookAndFeel v.2

The updated one.
Attachment #540031 - Attachment is obsolete: true
Attachment #627866 - Flags: review?(chrisccoulson)
(Assignee)

Comment 150

5 years ago
Created attachment 627965 [details] [diff] [review]
gfx patch

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
(Assignee)

Comment 152

5 years ago
Created attachment 628255 [details] [diff] [review]
gfx, v.2

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)
Attachment #628255 - Flags: review?(roc) → review+
(Assignee)

Comment 153

5 years ago
Comment on attachment 628255 [details] [diff] [review]
gfx, v.2

Thanks!
Attachment #628255 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 154

5 years ago
Created attachment 628318 [details] [diff] [review]
patch to /dom, without plugin changes

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)
(Assignee)

Comment 155

5 years ago
Created attachment 628324 [details] [diff] [review]
xpcom gtk3 update

Patch to /xpcom, without makefile changes. Benjamin, can you check it please?
Attachment #628324 - Flags: review?(benjamin)
(Assignee)

Comment 156

5 years ago
Created attachment 628327 [details] [diff] [review]
gtl3 patch to /toolkit

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-
blocking-basecamp: --- → -
(Assignee)

Updated

5 years ago
Attachment #628255 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 158

5 years ago
(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.
(Assignee)

Comment 159

5 years ago
Created attachment 630669 [details] [diff] [review]
minimal configure patch

A minimal patch needed for the MOZ_WIDGET_GTK change.
Attachment #630669 - Flags: review?(benjamin)
Attachment #630669 - Flags: review?(benjamin) → review+
(Assignee)

Comment 160

5 years ago
Created attachment 632197 [details] [diff] [review]
minimal configure patch, for check-in
Attachment #632197 - Flags: review+
Attachment #632197 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 161

5 years ago
Created attachment 632218 [details] [diff] [review]
gfx, v.3

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)
(Assignee)

Updated

5 years ago
Attachment #628318 - Flags: review?(jst)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4f8a07e4d9
Keywords: checkin-needed
Attachment #632197 - Flags: checkin? → checkin+
(Assignee)

Comment 163

5 years ago
Created attachment 632596 [details] [diff] [review]
xpcom gtk3 v2

xpcom patch with MOZ_WIDGET_GTK
Attachment #628324 - Attachment is obsolete: true
Attachment #632596 - Flags: review?(benjamin)
(Assignee)

Comment 164

5 years ago
Created attachment 632599 [details] [diff] [review]
gfx, v.4

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?
(Assignee)

Comment 165

5 years ago
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)
(Assignee)

Comment 166

5 years ago
Created attachment 632655 [details] [diff] [review]
xpcom gtk3 v3

fixed defines
Attachment #632596 - Attachment is obsolete: true
Attachment #632596 - Flags: review?(benjamin)
Attachment #632655 - Flags: review?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/4e4f8a07e4d9
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

macro changes look fine to me
Attachment #632599 - Flags: review?(vladimir) → review+
(Assignee)

Comment 169

5 years ago
Created attachment 632688 [details] [diff] [review]
toolkit gtk3 v.2

toolkit patch with MOZ_WIDGET_GTK macro.
Attachment #628327 - Attachment is obsolete: true
Attachment #632688 - Flags: review?(benjamin)
(Assignee)

Comment 170

5 years ago
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

Thanks!
Attachment #632599 - Flags: checkin?
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

https://hg.mozilla.org/integration/mozilla-inbound/rev/a55eea2c294d
Attachment #632599 - Flags: checkin? → checkin+
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
Attachment #632599 - Flags: checkin+ → checkin-
(Assignee)

Comment 173

5 years ago
(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).

Comment 175

5 years ago
(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."
(Assignee)

Comment 176

5 years ago
Created attachment 633089 [details] [diff] [review]
gfx gtk3, v.5

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+

Comment 180

5 years ago
(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).
(Assignee)

Comment 182

5 years ago
Created attachment 633482 [details] [diff] [review]
dom gtk3 v.3

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)
(Assignee)

Updated

5 years ago
Attachment #632655 - Flags: checkin?
(Assignee)

Comment 183

5 years ago
Created attachment 633508 [details] [diff] [review]
toolkit gtk3 v.3

Thanks, updated with requested changes.
Attachment #632688 - Attachment is obsolete: true
Attachment #633508 - Flags: review+
Attachment #633508 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 184

5 years ago
Created attachment 633520 [details] [diff] [review]
gfx, v.6

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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e87bb0023f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/88d69928e3b1
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e87bb0023f1
https://hg.mozilla.org/mozilla-central/rev/88d69928e3b1
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-
(Assignee)

Comment 188

5 years ago
Comment on attachment 633520 [details] [diff] [review]
gfx, v.6

Let's use gtk2compat.h there too.
Attachment #633520 - Flags: review?(vladimir)
(Assignee)

Comment 189

5 years ago
Created attachment 634040 [details] [diff] [review]
export gtk2compat.h

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+
(Assignee)

Updated

5 years ago
Attachment #634040 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 190

5 years ago
Created attachment 634390 [details] [diff] [review]
dom, v.4

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)
Comment on attachment 634040 [details] [diff] [review]
export gtk2compat.h

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8089e80e0e
Attachment #634040 - Flags: checkin? → checkin+
Keywords: checkin-needed
(Assignee)

Comment 192

5 years ago
Created attachment 634430 [details] [diff] [review]
gfx, v.7

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.
https://hg.mozilla.org/mozilla-central/rev/2e8089e80e0e
(Assignee)

Comment 197

5 years ago
Created attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

Dom patch for check-in, updated with the comments.
Attachment #634390 - Attachment is obsolete: true
Attachment #634895 - Flags: review+
Attachment #634895 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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+
Keywords: checkin-needed
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
(Assignee)

Comment 201

5 years ago
Created attachment 636708 [details] [diff] [review]
gtk2compat fix for dom, v.4

Ahh, we're missing gtk_widget_is_toplevel() in gtk2compat.h
Attachment #636708 - Flags: review?(karlt)
(Assignee)

Comment 202

5 years ago
Created attachment 636722 [details] [diff] [review]
gfx, v.7 for check-in

The gfx patch with requested changes.
Attachment #634430 - Attachment is obsolete: true
Attachment #636722 - Flags: review+
Attachment #636722 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Attachment #636708 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Attachment #636708 - Flags: checkin?
Comment on attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ed2abf47cc
Attachment #634895 - Flags: checkin- → checkin+
Comment on attachment 636708 [details] [diff] [review]
gtk2compat fix for dom, v.4

https://hg.mozilla.org/integration/mozilla-inbound/rev/44931b8274b4
Attachment #636708 - Flags: checkin? → checkin+
Comment on attachment 636722 [details] [diff] [review]
gfx, v.7 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0ba9f18f3d
Attachment #636722 - Flags: checkin? → checkin+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0ed2abf47cc
https://hg.mozilla.org/mozilla-central/rev/44931b8274b4
https://hg.mozilla.org/mozilla-central/rev/ab0ba9f18f3d

Updated

5 years ago
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-

Comment 209

5 years ago
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

Comment 210

5 years ago
(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.
(Assignee)

Comment 211

5 years ago
Created attachment 654957 [details] [diff] [review]
complete patch against latest trunk, v2

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
(Assignee)

Comment 212

5 years ago
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

Comment 213

5 years ago
not all of it applies to latest trunk but I managed to apply the missing parts manually apart from widget/gtk2/nsFilePicker.cpp
(Assignee)

Comment 214

5 years ago
Created attachment 654976 [details] [diff] [review]
rest of gtk2, v2

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-
(Assignee)

Comment 216

5 years ago
Created attachment 658436 [details] [diff] [review]
rest of gtk2, v3

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)
(Assignee)

Comment 217

5 years ago
Created attachment 658468 [details] [diff] [review]
gtk2/nsBidiKeyboard patch

The nsBidiKeyboard patch
Attachment #658468 - Flags: review?(karlt)
(Assignee)

Comment 218

5 years ago
Created attachment 658473 [details] [diff] [review]
gtk2/bidi v2

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+
(Assignee)

Comment 220

5 years ago
Created attachment 658841 [details] [diff] [review]
rest of gtk2, v4

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+
(Assignee)

Comment 223

5 years ago
Created attachment 660786 [details] [diff] [review]
gtk2/bidi v2 for check-in
Attachment #660786 - Flags: checkin?
(Assignee)

Comment 224

5 years ago
Created attachment 660788 [details] [diff] [review]
rest of gtk2, v4, for check-in

rest of gtk2, with build fixes (gtk2compat.h update, added gtk2compat.h includes).
Attachment #660788 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches] → [leave open for remaining patches][check linux try build before checkin]

Comment 225

5 years ago
I just did a build with attachment 660788 [details] [diff] [review] and attachment 660786 [details] [diff] [review] and it builds and runs well.
Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=a86c3014db5e
Comment on attachment 660786 [details] [diff] [review]
gtk2/bidi v2 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dd45dc4a1a
Attachment #660786 - Flags: checkin? → checkin+
Comment on attachment 660788 [details] [diff] [review]
rest of gtk2, v4, for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3817afd0374
Attachment #660788 - Flags: checkin? → checkin+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9dd45dc4a1a
https://hg.mozilla.org/mozilla-central/rev/e3817afd0374

Updated

5 years ago
Depends on: 791529

Updated

5 years ago
Depends on: 791518
(Assignee)

Comment 230

5 years ago
Created attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

Color changes only (plus new file creation).
Attachment #627866 - Attachment is obsolete: true
Attachment #627866 - Flags: review?(chrisccoulson)
Attachment #661786 - Flags: review?(chrisccoulson)
(Assignee)

Updated

5 years ago
Depends on: 722975

Updated

5 years ago
Depends on: 792158

Comment 231

5 years ago
Can GTK3 be enabled in nightly builds please?
Blocks: 813578