Last Comment Bug 627699 - (gtk3) Port GTK2 to GTK3
(gtk3)
: Port GTK2 to GTK3
Status: ASSIGNED
[leave open as a tracking bug][please...
: meta
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- normal with 153 votes (vote)
: ---
Assigned To: Martin Stránský
:
:
Mentors:
: 610234 (view as bug list)
Depends on: 959224 1022241 1147847 1158076 1164674 1169955 1170342 1174378 1174509 1186967 1189189 1189592 1189923 1190163 ship-gtk3 1196777 1197590 1207700 1207975 1208372 1213600 1213601 1224206 1239213 1246422 1250206 1260061 1260777 1261064 1261077 1263427 gtk-3.20 1265254 1267724 1267815 1267863 1267982 1268121 1268234 1268241 1268338 1268395 1268599 1268618 1268894 1269108 1269145 1269149 1269166 1270176 1270208 1270421 1270606 1271305 1271716 1272902 1272982 1273209 1281364 1281435 1283086 1284105 1289063 1321332 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 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 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 1250704 1255344 1257811 1258086 1258989 1259433 1260902 1261068 1261095 1261103 1261576 1263145 1263791 1265116 1265735 1266338 1267988 1268244 1268462 1268465 1268530 1268784 1268874 1269058 1269061 1269121 1269164 1269243 1269258 1269306 1269523 1269561 1270473 1270668 1271362 1271498 1271579 1271783 1272140 1275700 1275861 1276596 1279144 1279419 1279978 1280226 1280227 1281710 1281733 1287036 1287037 1301285 1305367
Blocks: 513159 635134 720523 813578 1038800 1175029 1181610 1213609 723754 738937 882036 884634 958868 978679 1015218 1043146 1268412
  Show dependency treegraph
 
Reported: 2011-01-21 06:18 PST by Jan Horak
Modified: 2016-11-30 13:52 PST (History)
211 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Very WIP patch (333.66 KB, patch)
2011-01-21 06:18 PST, Jan Horak
no flags Details | Diff | Splinter Review
wip v2 (250.10 KB, patch)
2011-02-17 05:09 PST, Martin Stránský
no flags Details | Diff | Splinter Review
wip v3 (248.48 KB, patch)
2011-02-18 07:32 PST, Martin Stránský
no flags Details | Diff | Splinter Review
wip v4 (364.00 KB, patch)
2011-02-25 08:15 PST, Jan Horak
no flags Details | Diff | Splinter Review
WIP v5 (35.17 KB, patch)
2011-03-11 07:20 PST, Jan Horak
no flags Details | Diff | Splinter Review
WIP v6 (400.89 KB, patch)
2011-03-14 00:14 PDT, Jan Horak
no flags Details | Diff | Splinter Review
WIP v7 (1.21 MB, patch)
2011-03-17 08:19 PDT, Jan Horak
no flags Details | Diff | Splinter Review
WIP v8 (547.99 KB, patch)
2011-03-23 08:24 PDT, Jan Horak
no flags Details | Diff | Splinter Review
WIP v9 (763.18 KB, patch)
2011-03-31 05:22 PDT, Jan Horak
no flags Details | Diff | Splinter Review
gtk3 directory patch v1 (446.35 KB, patch)
2011-04-12 03:03 PDT, Jan Horak
no flags Details | Diff | Splinter Review
specific comments on gtk3drawing (9.97 KB, text/plain)
2011-05-19 22:32 PDT, Karl Tomlinson (:karlt)
no flags Details
complete patch against latest trunk (393.40 KB, patch)
2011-06-09 06:11 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gtk drawing patch (115.45 KB, patch)
2011-06-17 04:10 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
mozcontainer.c patch (7.02 KB, patch)
2011-06-17 04:15 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
nsLookAndFeel.cpp patch (30.02 KB, patch)
2011-06-17 04:17 PDT, Martin Stránský
chrisccoulson: review-
Details | Diff | Splinter Review
gtk2/nsWindow.cpp patch (53.83 KB, patch)
2011-06-17 04:21 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
gtk2 makefile patch (5.44 KB, patch)
2011-06-17 04:36 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
rest of gtk2 dir fixes (24.65 KB, patch)
2011-06-17 04:40 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
gtk3drawing review comments 2 (6.57 KB, text/plain)
2011-06-27 17:45 PDT, Karl Tomlinson (:karlt)
no flags Details
nsWindow except drawing review comments (18.34 KB, text/plain)
2011-06-28 22:30 PDT, Karl Tomlinson (:karlt)
no flags Details
nsWindow drawing review comments (9.34 KB, text/plain)
2011-06-30 18:26 PDT, Karl Tomlinson (:karlt)
no flags Details
v2. nsWindow/gtk2compat.h patch (92.64 KB, patch)
2011-08-31 06:21 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
nsWindow.cpp - v3 (92.63 KB, patch)
2011-08-31 06:55 PDT, Martin Stránský
karlt: review-
karlt: feedback+
Details | Diff | Splinter Review
nsWindow review comments 2 (5.46 KB, text/plain)
2011-09-21 02:58 PDT, Karl Tomlinson (:karlt)
no flags Details
nsWindow.cpp, v4 (96.58 KB, patch)
2011-09-22 01:49 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
nsWindow.cpp, v4, commit friendly version, r=karlt (92.48 KB, patch)
2011-09-22 04:52 PDT, Martin Stránský
stransky: review+
Details | Diff | Splinter Review
nsWindow.cpp, v5, commit friendly version, r=karlt (96.78 KB, patch)
2011-09-22 06:33 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
nsWindow.cpp, v6 (96.89 KB, patch)
2011-09-22 08:37 PDT, Martin Stránský
emorley: checkin+
Details | Diff | Splinter Review
build fix - startup notification (823 bytes, patch)
2011-09-23 02:24 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
build fix - startup notification, v2 (857 bytes, patch)
2011-09-23 03:06 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
build fix - startup notification, v3 (842 bytes, patch)
2011-09-23 03:47 PDT, Martin Stránský
karlt: review+
emorley: checkin+
Details | Diff | Splinter Review
gtk drawing patch v.2 + comments (136.00 KB, patch)
2011-09-29 09:04 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
mozcontainer.c patch, v2 (11.28 KB, patch)
2011-09-30 13:03 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
mozcontainer.c patch, v3 (12.01 KB, patch)
2011-10-04 02:45 PDT, Martin Stránský
emorley: review-
Details | Diff | Splinter Review
mozcontainer.c patch, v5 (11.89 KB, patch)
2011-10-06 08:17 PDT, Martin Stránský
stransky: review+
Details | Diff | Splinter Review
mozcontainer.c patch, v6, fixed indentation (11.91 KB, patch)
2011-10-07 01:48 PDT, Martin Stránský
stransky: review+
emorley: checkin+
Details | Diff | Splinter Review
gtk drawing patch v.3 (133.79 KB, patch)
2011-10-07 07:03 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gkt3drawing, v.4 (140.26 KB, patch)
2011-10-20 01:57 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
review comments on gkt3drawing, v.4 (5.20 KB, text/plain)
2012-01-12 22:19 PST, Karl Tomlinson (:karlt)
no flags Details
review comments on gkt3drawing, v.4 (completed) (7.13 KB, text/plain)
2012-02-02 15:01 PST, Karl Tomlinson (:karlt)
no flags Details
gtk drawing patch v.5 (138.93 KB, patch)
2012-02-08 03:31 PST, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
gkt3drawing, v.6 (138.70 KB, patch)
2012-02-09 03:13 PST, Martin Stránský
karlt: review+
emorley: checkin+
Details | Diff | Splinter Review
fix drawing&window parts (6.21 KB, patch)
2012-05-23 02:12 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
fix drawing&window parts, v.2 (6.96 KB, patch)
2012-05-24 04:34 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
fix drawing&window parts, v.2, for check-in (5.71 KB, patch)
2012-05-25 02:19 PDT, Martin Stránský
stransky: review+
roc: checkin+
Details | Diff | Splinter Review
nsLookAndFeel v.2 (34.57 KB, patch)
2012-05-29 00:50 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gfx patch (31.46 KB, patch)
2012-05-29 08:15 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gfx, v.2 (31.40 KB, patch)
2012-05-30 00:35 PDT, Martin Stránský
roc: review+
Details | Diff | Splinter Review
patch to /dom, without plugin changes (28.64 KB, patch)
2012-05-30 06:02 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
xpcom gtk3 update (2.74 KB, patch)
2012-05-30 06:40 PDT, Martin Stránský
benjamin: review-
Details | Diff | Splinter Review
gtl3 patch to /toolkit (26.93 KB, patch)
2012-05-30 06:54 PDT, Martin Stránský
benjamin: review-
Details | Diff | Splinter Review
minimal configure patch (492 bytes, patch)
2012-06-06 12:29 PDT, Martin Stránský
benjamin: review+
Details | Diff | Splinter Review
minimal configure patch, for check-in (669 bytes, patch)
2012-06-12 04:29 PDT, Martin Stránský
stransky: review+
ryanvm: checkin+
Details | Diff | Splinter Review
gfx, v.3 (30.24 KB, patch)
2012-06-12 05:56 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
xpcom gtk3 v2 (2.70 KB, patch)
2012-06-13 01:25 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gfx, v.4 (30.32 KB, patch)
2012-06-13 01:30 PDT, Martin Stránský
vladimir: review+
emorley: checkin-
Details | Diff | Splinter Review
xpcom gtk3 v3 (2.73 KB, patch)
2012-06-13 05:45 PDT, Martin Stránský
benjamin: review+
geoff: checkin+
Details | Diff | Splinter Review
toolkit gtk3 v.2 (24.42 KB, patch)
2012-06-13 07:36 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
gfx gtk3, v.5 (30.47 KB, patch)
2012-06-14 03:28 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
dom gtk3 v.3 (27.03 KB, patch)
2012-06-15 05:01 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
toolkit gtk3 v.3 (24.50 KB, patch)
2012-06-15 07:17 PDT, Martin Stránský
stransky: review+
geoff: checkin+
Details | Diff | Splinter Review
gfx, v.6 (30.47 KB, patch)
2012-06-15 07:40 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
export gtk2compat.h (1.24 KB, patch)
2012-06-18 08:06 PDT, Martin Stránský
karlt: review+
ehsan: checkin+
Details | Diff | Splinter Review
dom, v.4 (25.35 KB, patch)
2012-06-19 07:06 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
gfx, v.7 (22.56 KB, patch)
2012-06-19 08:26 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
dom, v.4 for check-in (25.18 KB, patch)
2012-06-20 06:41 PDT, Martin Stránský
stransky: review+
ryanvm: checkin+
Details | Diff | Splinter Review
gtk2compat fix for dom, v.4 (838 bytes, patch)
2012-06-26 07:39 PDT, Martin Stránský
karlt: review+
ryanvm: checkin+
Details | Diff | Splinter Review
gfx, v.7 for check-in (22.17 KB, patch)
2012-06-26 08:31 PDT, Martin Stránský
stransky: review+
ryanvm: checkin+
Details | Diff | Splinter Review
complete patch against latest trunk, v2 (115.96 KB, patch)
2012-08-24 00:52 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
rest of gtk2, v2 (39.33 KB, patch)
2012-08-24 05:05 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
rest of gtk2, v3 (39.05 KB, patch)
2012-09-05 03:37 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
gtk2/nsBidiKeyboard patch (2.99 KB, patch)
2012-09-05 06:32 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
gtk2/bidi v2 (2.98 KB, patch)
2012-09-05 06:48 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
rest of gtk2, v4 (37.86 KB, patch)
2012-09-06 04:26 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
gtk2/bidi v2 for check-in (2.76 KB, patch)
2012-09-13 04:25 PDT, Martin Stránský
ryanvm: checkin+
Details | Diff | Splinter Review
rest of gtk2, v4, for check-in (25.52 KB, patch)
2012-09-13 04:28 PDT, Martin Stránský
ryanvm: checkin+
Details | Diff | Splinter Review
nsLookAndFeel v.2 color changes (21.29 KB, patch)
2012-09-17 07:17 PDT, Martin Stránský
karlt: feedback+
Details | Diff | Splinter Review
gtk3drawing - gtk_entry clip (1.88 KB, patch)
2013-02-05 12:41 PST, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
remove ThemeRenderer(gfxGdkNativeRenderer) from gtk3 (4.14 KB, patch)
2013-02-05 14:09 PST, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
ThemeRenderer, v2 (43.48 KB, patch)
2013-02-19 02:26 PST, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
ThemeRenderer, v3, for check-in (43.52 KB, patch)
2013-02-20 01:06 PST, Martin Stránský
ryanvm: checkin+
Details | Diff | Splinter Review
menubar is transparent in gtk3 (723 bytes, patch)
2013-02-26 05:20 PST, Martin Stránský
karlt: review+
ryanvm: checkin+
Details | Diff | Splinter Review
add padding to widget borders (1.59 KB, patch)
2013-02-26 05:22 PST, Martin Stránský
no flags Details | Diff | Splinter Review
widget clipping patch (5.34 KB, patch)
2013-03-20 07:55 PDT, Martin Stránský
no flags Details | Diff | Splinter Review
radio button fix (1.15 KB, patch)
2013-03-20 08:17 PDT, Martin Stránský
karlt: review+
ryanvm: checkin+
Details | Diff | Splinter Review
widget padding patch (18.14 KB, patch)
2013-03-21 05:37 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
tab widget rendering (7.47 KB, patch)
2013-03-21 08:03 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
nsLookAndFeel v.3 (30.44 KB, patch)
2013-04-11 08:30 PDT, Martin Stránský
karlt: review-
Details | Diff | Splinter Review
nsLookAndFeel v.4 (31.59 KB, patch)
2013-05-27 03:44 PDT, Martin Stránský
karlt: review+
Details | Diff | Splinter Review
nsLookAndFeel v.4 for check-in (31.51 KB, patch)
2013-05-28 06:13 PDT, Martin Stránský
stransky: review+
ryanvm: checkin+
Details | Diff | Splinter Review
GTK+3 build fixes (2.49 KB, patch)
2013-06-13 05:35 PDT, Quentin "Sardem FF7" Glidic
no flags Details | Diff | Splinter Review
Fix build with --disable-dbus (846 bytes, patch)
2014-01-07 13:32 PST, ojab
karlt: review+
stransky: feedback+
ryanvm: checkin+
Details | Diff | Splinter Review

Description Jan Horak 2011-01-21 06:18:10 PST
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).
Comment 1 Hussam Al-Tayeb 2011-01-25 09:31:43 PST
*** Bug 610234 has been marked as a duplicate of this bug. ***
Comment 2 Martin Stránský 2011-02-17 05:09:26 PST
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.
Comment 3 Martin Stránský 2011-02-18 07:32:27 PST
Created attachment 513457 [details] [diff] [review]
wip v3

fixed background rendering and style borders
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-24 21:27:46 PST
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.
Comment 5 Martin Stránský 2011-02-25 00:56:01 PST
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.
Comment 6 Jan Horak 2011-02-25 08:15:11 PST
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.
Comment 7 Jan Horak 2011-03-11 07:20:08 PST
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.
Comment 8 Jan Horak 2011-03-14 00:14:43 PDT
Created attachment 519093 [details] [diff] [review]
WIP v6

Wrong patch, reuploading.
Comment 9 Hussam Al-Tayeb 2011-03-14 09:12:36 PDT
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.
Comment 10 Martin Stránský 2011-03-15 00:42:04 PDT
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.
Comment 11 Jan Horak 2011-03-17 08:19:39 PDT
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.
Comment 12 Hussam Al-Tayeb 2011-03-19 11:01:56 PDT
Is the patch diffed against the RC tarball or latest HG? because I couldn't get it to patch against mozilla-central.
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-03-20 13:43:06 PDT
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.
Comment 14 Karl Tomlinson (:karlt) 2011-03-20 14:05:35 PDT
Using "hg copy" would make the duplication easier to follow (and may make splitting the modifications unnecessary).
Comment 15 Jan Horak 2011-03-21 01:03:05 PDT
(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.
Comment 16 Jan Horak 2011-03-21 01:05:50 PDT
(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.
Comment 17 Karl Tomlinson (:karlt) 2011-03-21 21:28:09 PDT
(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 Michael Ventnor 2011-03-23 07:49:17 PDT
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.
Comment 19 Martin Stránský 2011-03-23 08:03:18 PDT
Sure, the code should be optimized and refactorised. Our goal is to create a minimal working patch and then improve it.
Comment 20 Jan Horak 2011-03-23 08:04:32 PDT
(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.
Comment 21 Jan Horak 2011-03-23 08:24:00 PDT
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.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-23 14:16:30 PDT
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 Michael Ventnor 2011-03-23 19:28:09 PDT
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.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-23 20:05:23 PDT
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.
Comment 25 Karl Tomlinson (:karlt) 2011-03-23 21:40:43 PDT
(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.
Comment 26 Martin Stránský 2011-03-24 02:22:41 PDT
(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 Michael Ventnor 2011-03-24 03:45:01 PDT
(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...
Comment 28 Martin Stránský 2011-03-28 03:31:17 PDT
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?
Comment 29 Christopher Aillon (sabbatical, not receiving bugmail) 2011-03-28 12:08:56 PDT
We just disabled building without ipc (bug 638755), so you should expect it to be there, yes.
Comment 30 Martin Stránský 2011-03-29 04:45:16 PDT
Some plugins are still run in-process only (like Adobe reader) regardless of the ipc state.
Comment 31 Karl Tomlinson (:karlt) 2011-03-29 21:40:27 PDT
(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.)
Comment 32 Jan Horak 2011-03-31 05:22:08 PDT
Created attachment 523284 [details] [diff] [review]
WIP v9

Attaching rebased patch:
- Martin made gtk2 plugins in gtk3 code possible!
- Fixed custom mouse cursors
Comment 33 Michael Ventnor 2011-04-07 19:50:29 PDT
jhorak, any updates on the patch or its status?
Comment 34 Martin Stránský 2011-04-11 00:38:42 PDT
(In reply to comment #33)
> jhorak, any updates on the patch or its status?

Are you looking for something particular?
Comment 35 Michael Ventnor 2011-04-11 03:45:37 PDT
(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 Chris Coulson 2011-04-11 09:39:52 PDT
(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 :)
Comment 37 Martin Stránský 2011-04-12 02:23:01 PDT
> > 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.
Comment 38 Jan Horak 2011-04-12 03:03:45 PDT
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.
Comment 39 Karl Tomlinson (:karlt) 2011-05-19 22:31:47 PDT
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.
Comment 40 Karl Tomlinson (:karlt) 2011-05-19 22:32:44 PDT
Created attachment 533890 [details]
specific comments on gtk3drawing
Comment 41 Martin Stránský 2011-05-19 22:48:47 PDT
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.
Comment 42 Karl Tomlinson (:karlt) 2011-05-22 20:25:28 PDT
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 Javier Jardón 2011-05-23 04:38:08 PDT
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 Chris Coulson 2011-05-23 04:48:06 PDT
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.
Comment 45 Martin Stránský 2011-05-23 05:01:13 PDT
(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 wavded 2011-05-29 20:02:01 PDT
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 Eric Appleman 2011-06-06 07:34:22 PDT
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.
Comment 48 Martin Stránský 2011-06-09 06:11:04 PDT
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.
Comment 49 Hussam Al-Tayeb 2011-06-11 02:37:46 PDT
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
Comment 50 Martin Stránský 2011-06-17 04:10:38 PDT
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.
Comment 51 Martin Stránský 2011-06-17 04:15:25 PDT
Created attachment 540030 [details] [diff] [review]
mozcontainer.c patch

patch to widget/src/gtk2/mozcontainer.c
Comment 52 Martin Stránský 2011-06-17 04:17:52 PDT
Created attachment 540031 [details] [diff] [review]
nsLookAndFeel.cpp patch
Comment 53 Martin Stránský 2011-06-17 04:21:59 PDT
Created attachment 540032 [details] [diff] [review]
gtk2/nsWindow.cpp patch
Comment 54 Martin Stránský 2011-06-17 04:36:00 PDT
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.
Comment 55 Martin Stránský 2011-06-17 04:40:45 PDT
Created attachment 540036 [details] [diff] [review]
rest of gtk2 dir fixes
Comment 56 Karl Tomlinson (:karlt) 2011-06-27 17:45:39 PDT
Created attachment 542343 [details]
gtk3drawing review comments 2
Comment 57 Karl Tomlinson (:karlt) 2011-06-27 18:54:05 PDT
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.)
Comment 58 Karl Tomlinson (:karlt) 2011-06-28 22:30:22 PDT
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.
Comment 59 Karl Tomlinson (:karlt) 2011-06-29 19:33:59 PDT
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
Comment 60 Karl Tomlinson (:karlt) 2011-06-30 18:26:26 PDT
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.
Comment 61 Diego Viola 2011-07-11 16:11:06 PDT
Does this means that if Firefox is ported to GTK+ 3, Firefox will run on Wayland?

Please support Wayland.

Thanks.
Comment 62 Andre Klapper 2011-07-11 17:37:36 PDT
(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 Diego Viola 2011-07-11 18:05:58 PDT
(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 Diego Viola 2011-07-11 21:55:21 PDT
(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.
Comment 65 Karl Tomlinson (:karlt) 2011-07-11 22:56:57 PDT
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.
Comment 66 Denis Washington 2011-08-23 01:45:20 PDT
Just out of curiosity, has any progress on this been made in the last month?
Comment 67 Martin Stránský 2011-08-23 01:54:04 PDT
I'm porting it to latest trunk right now.
Comment 68 Karl Tomlinson (:karlt) 2011-08-23 17:25:36 PDT
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
Comment 69 Martin Stránský 2011-08-23 23:05:54 PDT
(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.
Comment 70 Karl Tomlinson (:karlt) 2011-08-24 23:06:27 PDT
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.
Comment 71 Martin Stránský 2011-08-24 23:20:33 PDT
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.
Comment 72 Martin Stránský 2011-08-24 23:23:42 PDT
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)
Comment 73 Karl Tomlinson (:karlt) 2011-08-24 23:41:44 PDT
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.
Comment 74 Martin Stránský 2011-08-24 23:56:33 PDT
(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.
Comment 75 Karl Tomlinson (:karlt) 2011-08-25 00:25:34 PDT
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.
Comment 76 Martin Stránský 2011-08-25 00:32:20 PDT
(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.
Comment 77 Martin Stránský 2011-08-31 06:21:43 PDT
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.
Comment 78 Martin Stránský 2011-08-31 06:55:44 PDT
Created attachment 557153 [details] [diff] [review]
nsWindow.cpp - v3

sorry, there were some missing nits, fixed now.
Comment 79 Karl Tomlinson (:karlt) 2011-08-31 17:24:51 PDT
(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 Hussam Al-Tayeb 2011-09-01 16:15:40 PDT
what patches do I need to try this against latest trunk?
Comment 81 Karl Tomlinson (:karlt) 2011-09-21 02:56:03 PDT
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.
Comment 82 Karl Tomlinson (:karlt) 2011-09-21 02:58:53 PDT
Created attachment 561418 [details]
nsWindow review comments 2
Comment 83 Martin Stránský 2011-09-22 01:49:03 PDT
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 :)
Comment 84 Karl Tomlinson (:karlt) 2011-09-22 02:58:45 PDT
Comment on attachment 561682 [details] [diff] [review]
nsWindow.cpp, v4

I think this can land for mozilla-central now
(but please check it passes try server).

(In reply to Martin Stránský from comment #83)
> If this or any other window is destroyed,
> get_window_for_gdk_window() or gdk_window_get_children() just returns NULL,
> right?

Yes, provided their GdkWindow parameter still exists.

> Or are you concerned with already obtained list of windows from
> children list?

Yes, and gdk_window_get_children() is called after dispatching the event.

Even events dispatched for painting can flush layout, causing JS notifications, and so anything can happen.
Comment 85 Martin Stránský 2011-09-22 04:52:02 PDT
Created attachment 561701 [details] [diff] [review]
nsWindow.cpp, v4, commit friendly version, r=karlt
Comment 86 Ed Morley [:emorley] 2011-09-22 05:34:10 PDT
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
Comment 87 Martin Stránský 2011-09-22 06:33:40 PDT
Created attachment 561718 [details] [diff] [review]
nsWindow.cpp, v5, commit friendly version, r=karlt

added missing gtk2compat.h file
Comment 88 Ed Morley [:emorley] 2011-09-22 06:46:31 PDT
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 89 Ed Morley [:emorley] 2011-09-22 07:20:37 PDT
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
}
Comment 90 Martin Stránský 2011-09-22 08:37:04 PDT
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.
Comment 91 Ed Morley [:emorley] 2011-09-22 09:45:43 PDT
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 Hussam Al-Tayeb 2011-09-22 10:04:24 PDT
(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 93 Ed Morley [:emorley] 2011-09-22 13:48:13 PDT
Comment on attachment 561753 [details] [diff] [review]
nsWindow.cpp, v6

Passed try. Happy for me to land now?
Comment 94 Martin Stránský 2011-09-22 14:00:35 PDT
(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 95 Ed Morley [:emorley] 2011-09-22 14:01:10 PDT
Comment on attachment 561753 [details] [diff] [review]
nsWindow.cpp, v6

https://hg.mozilla.org/integration/mozilla-inbound/rev/875cb4f20eac
Comment 96 Martin Stránský 2011-09-23 02:24:39 PDT
Created attachment 561999 [details] [diff] [review]
build fix - startup notification

This build fix is needed when MOZ_ENABLE_STARTUP_NOTIFICATION is enabled.
Comment 97 Karl Tomlinson (:karlt) 2011-09-23 02:40:11 PDT
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.
Comment 98 Martin Stránský 2011-09-23 03:06:00 PDT
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.
Comment 99 Karl Tomlinson (:karlt) 2011-09-23 03:43:10 PDT
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.
Comment 100 Martin Stránský 2011-09-23 03:47:14 PDT
Created attachment 562011 [details] [diff] [review]
build fix - startup notification, v3

Okay, removed.
Comment 101 Karl Tomlinson (:karlt) 2011-09-23 03:51:04 PDT
Comment on attachment 562011 [details] [diff] [review]
build fix - startup notification, v3

Thanks.

(In reply to Karl Tomlinson (:karlt) from comment #99)
> GDK_DRAWABLE() adds code for an extra function call per caller.

Actually two function calls: gdk_drawable_get_type and g_type_check_instance.
Comment 102 Ed Morley [:emorley] 2011-09-23 04:54:13 PDT
Comment on attachment 561753 [details] [diff] [review]
nsWindow.cpp, v6

nsWindow.cpp part: https://hg.mozilla.org/mozilla-central/rev/875cb4f20eac
Comment 103 Benjamin Otte 2011-09-23 05:01:49 PDT
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 105 Ed Morley [:emorley] 2011-09-25 06:24:14 PDT
Comment on attachment 562011 [details] [diff] [review]
build fix - startup notification, v3

startup notification part: https://hg.mozilla.org/mozilla-central/rev/2137c5bd3c36
Comment 106 Karl Tomlinson (:karlt) 2011-09-26 16:51:51 PDT
(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.
Comment 107 Martin Stránský 2011-09-29 09:04:29 PDT
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.
Comment 108 Martin Stránský 2011-09-30 13:03:49 PDT
Created attachment 563817 [details] [diff] [review]
mozcontainer.c patch, v2
Comment 109 Karl Tomlinson (:karlt) 2011-10-03 21:03:27 PDT
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?
Comment 110 Martin Stránský 2011-10-04 02:45:38 PDT
Created attachment 564498 [details] [diff] [review]
mozcontainer.c patch, v3

Fixed comments, ready to commit.
Comment 111 Karl Tomlinson (:karlt) 2011-10-04 22:12:00 PDT
Comment on attachment 563434 [details] [diff] [review]
gtk drawing patch v.2 + comments

>>>         if (interior_focus) {
>>>+            GtkBorder border;
>>>+            gtk_style_context_get_border(style, 0, &border);
>>Don't we want to pass the state flags here?
>
> What do you mean?

gtk_style_context_get_border() "Gets the border for a given state [...]" and
uses the "state" parameter rather than the state on the StyleContext.  However,
this code is not passing anything for the GtkStateFlags state parameter.

Most, but not all, gtk_style_context_get_border calls in GTK widgets pass the
state flags.

>> gtk_tree_view_column_create_button uses GTK_SHADOW_IN.
>
> gtk_tree_view_column_create_button()? I don't see it anywhere.

http://git.gnome.org/browse/gtk+/tree/gtk/gtktreeviewcolumn.c?id=3.2.0#n876

>> In moz_gtk_tab_paint:
>>+        gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
>> I assume _NORMAL is the default, so this is unnecessary.
>
> I don't see GTK_STATE_FLAG_NORMAL in moz_gtk_tab_paint(), it's only in:
>
> moz_gtk_progressbar_paint
> moz_gtk_progress_chunk_paint
> moz_gtk_tabpanels_paint
> moz_gtk_check_menu_item_paint

I must have pasted something in the wrong place there.

I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL)
calls are unnecessary.  The pattern used in GTK is to create the style context
with default state and only ever set the state with a save/restore during
draw.  Removing these would save a little code, but I don't feel strongly
about it, if you prefer to leave them in.

>>>+            gtk_render_frame_gap(style, cr,
>>>+                                 rect->x - gap_loffset,
>>>+                                 rect->y + gap_voffset - 3 * gap_height,
>>>+                                 rect->width + gap_loffset + gap_roffset,
>>>+                                 3 * gap_height, GTK_POS_BOTTOM,
>>>+                                 gap_loffset, rect->width);        
>>Need to also add gap_loffset here as you did below.
>
> Where the gap_loffset should be?

The last parameter is a position rather than a width, and so needs to be
gap_loffset + rect->width.

>>>+            gtk_render_frame_gap(style, cr,
>>>+                                 rect->x - gap_loffset,
>>>+                                 rect->y + rect->height - gap_voffset,
>>>+                                 rect->width + gap_loffset + gap_roffset,
>>>+                                 3 * gap_height, GTK_POS_TOP,
>>>+                                 gap_loffset, gap_loffset+rect->width);        
>> Can you add spaces around the "+", please?
>
> Where? All "+" I see have spaces around.

The last parameter is correct here, but there are no spaces around its "+".

>>+    gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
>> gtk_notebook_paint usually uses gtk_render_frame_gap.
>> Why choose render_frame over render_frame_gap here?
>
> I haven't been able to decrypt the former code logic plus gtk_render_frame_gap()
> does not allow to set negative gap start. gtk_render_frame() seems to work fine 
> too but if you have an idea how to transport the gap size to moz_gtk_tabpanels_paint()
> I can modify it.

The gap size is not needed in moz_gtk_tabpanels_paint because the gap will be
painted with the foreground tab in moz_gtk_tab_paint.

However, if moz_gtk_tabpanels_paint just uses gtk_render_frame(), the theme
will think that there are no tabs and may draw something different.  (Themes
have done this in the past at least.)  Hence the trick of using two clip
regions, and drawing the gap outside each clip region, to get the correct
frame for a tabpanel with tabs.

With the API change from a cliprect parameter to a cairo_t, the clip
rectangles for each half will need to be applied to the cairo_t,
necessitating the use of cairo_save/restore.  I suggest dividing rect into
left and right halves instead of the approach for gtk2 of dividing the
cliprect into two halves.  (Getting the cliprect from cairo is harder and
unnecessary.)

For smaller tabpanels, the logic in the gtk2 port could lead to the gap being
outside the rect.  I suggest just using rect->width as the gap position in the
render_frame_gap() call for the left half and 0 for the right half.

>+static GtkStateFlags
>+GetStateFlagFromGtkWidgetState(GtkWidgetState* state)

Can you call this "GetStateFlagsFromGtkWidgetState", please?
That would make it clearer that multiple flags may be set.

>+    else
>+        stateFlags |= GTK_STATE_FLAG_NORMAL;

No need for this as GTK_STATE_FLAG_NORMAL is not a flag but 0, the empty state
with no flags set.
Comment 113 Ed Morley [:emorley] 2011-10-06 03:27:57 PDT
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
}
Comment 114 Martin Stránský 2011-10-06 08:17:32 PDT
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.
Comment 115 Karl Tomlinson (:karlt) 2011-10-06 16:57:54 PDT
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?
Comment 116 Martin Stránský 2011-10-07 01:48:51 PDT
Created attachment 565464 [details] [diff] [review]
mozcontainer.c patch, v6, fixed indentation

Fixed indentation.
Comment 117 Ed Morley [:emorley] 2011-10-07 03:30:33 PDT
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
Comment 118 Martin Stránský 2011-10-07 07:03:30 PDT
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.
Comment 119 Ed Morley [:emorley] 2011-10-07 08:18:20 PDT
Comment on attachment 565464 [details] [diff] [review]
mozcontainer.c patch, v6, fixed indentation

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f8b4c3c12a
Comment 120 Matt Brubeck (:mbrubeck) 2011-10-07 12:38:46 PDT
https://hg.mozilla.org/mozilla-central/rev/f9f8b4c3c12a
Comment 121 Karl Tomlinson (:karlt) 2011-10-16 21:07:08 PDT
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.)
Comment 122 Martin Stránský 2011-10-20 01:57:02 PDT
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.
Comment 123 Chris Coulson 2011-12-09 12:58:46 PST
Note that http://hg.mozilla.org/releases/mozilla-beta/rev/875cb4f20eac seems to cause bug 709259 on GTK2 builds
Comment 124 alexhultman 2011-12-27 06:10:04 PST
Will those patches get merged to any UX build for public testing any time soon? :)
Comment 125 Rafał Mużyło 2011-12-31 12:01:27 PST
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 ?
Comment 126 Karl Tomlinson (:karlt) 2012-01-12 22:19:30 PST
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.
Comment 127 Karl Tomlinson (:karlt) 2012-02-02 15:01:12 PST
Created attachment 593982 [details]
review comments on gkt3drawing, v.4 (completed)
Comment 128 Karl Tomlinson (:karlt) 2012-02-07 17:44:53 PST
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.
Comment 129 Martin Stránský 2012-02-08 03:31:59 PST
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.
Comment 130 Karl Tomlinson (:karlt) 2012-02-08 15:23:38 PST
(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 131 Karl Tomlinson (:karlt) 2012-02-08 16:04:22 PST
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.
Comment 132 Karl Tomlinson (:karlt) 2012-02-08 16:25:59 PST
(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.
Comment 133 Martin Stránský 2012-02-09 03:13:56 PST
Created attachment 595699 [details] [diff] [review]
gkt3drawing, v.6

Removed the unnecessary save/restore and gtk_render_handle().
Comment 135 Ed Morley [:emorley] 2012-02-10 19:43:36 PST
Comment on attachment 595699 [details] [diff] [review]
gkt3drawing, v.6

https://hg.mozilla.org/mozilla-central/rev/5d76c57d1e8c
Comment 136 antistress 2012-03-15 06:37:53 PDT
(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 Chris Coulson 2012-03-19 12:27:24 PDT
Sorry, I've had a few crazy weeks :(

I'll get to look at these patches this evening
Comment 138 Martin Stránský 2012-04-04 23:36:32 PDT
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 antistress 2012-04-19 00:40:06 PDT
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 antistress 2012-04-19 00:41:04 PDT
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 Chris Coulson 2012-05-08 18:45:14 PDT
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.
Comment 142 Martin Stránský 2012-05-23 02:12:09 PDT
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.
Comment 143 Karl Tomlinson (:karlt) 2012-05-23 17:53:32 PDT
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)
Comment 144 Martin Stránský 2012-05-24 04:34:37 PDT
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.
Comment 145 Karl Tomlinson (:karlt) 2012-05-24 18:26:50 PDT
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.
Comment 146 Martin Stránský 2012-05-25 02:19:53 PDT
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().
Comment 147 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-27 15:42:52 PDT
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
Comment 148 Ed Morley [:emorley] 2012-05-28 10:12:58 PDT
Comment on attachment 627143 [details] [diff] [review]
fix drawing&window parts, v.2, for check-in

https://hg.mozilla.org/mozilla-central/rev/6c1dbb75d232
Comment 149 Martin Stránský 2012-05-29 00:50:25 PDT
Created attachment 627866 [details] [diff] [review]
nsLookAndFeel v.2

The updated one.
Comment 150 Martin Stránský 2012-05-29 08:15:35 PDT
Created attachment 627965 [details] [diff] [review]
gfx patch

Patch to gfx, without makefile changes. I wonder who is a right reviewer here, maybe roc?
Comment 151 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 15:56:19 PDT
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
Comment 152 Martin Stránský 2012-05-30 00:35:55 PDT
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.
Comment 153 Martin Stránský 2012-05-30 03:06:31 PDT
Comment on attachment 628255 [details] [diff] [review]
gfx, v.2

Thanks!
Comment 154 Martin Stránský 2012-05-30 06:02:47 PDT
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.
Comment 155 Martin Stránský 2012-05-30 06:40:18 PDT
Created attachment 628324 [details] [diff] [review]
xpcom gtk3 update

Patch to /xpcom, without makefile changes. Benjamin, can you check it please?
Comment 156 Martin Stránský 2012-05-30 06:54:28 PDT
Created attachment 628327 [details] [diff] [review]
gtl3 patch to /toolkit

Patch to toolkit.
Comment 157 Benjamin Smedberg [:bsmedberg] 2012-06-05 10:30:47 PDT
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.
Comment 158 Martin Stránský 2012-06-06 06:15:14 PDT
(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.
Comment 159 Martin Stránský 2012-06-06 12:29:04 PDT
Created attachment 630669 [details] [diff] [review]
minimal configure patch

A minimal patch needed for the MOZ_WIDGET_GTK change.
Comment 160 Martin Stránský 2012-06-12 04:29:31 PDT
Created attachment 632197 [details] [diff] [review]
minimal configure patch, for check-in
Comment 161 Martin Stránský 2012-06-12 05:56:15 PDT
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
Comment 162 Ryan VanderMeulen [:RyanVM] 2012-06-12 14:07:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4f8a07e4d9
Comment 163 Martin Stránský 2012-06-13 01:25:33 PDT
Created attachment 632596 [details] [diff] [review]
xpcom gtk3 v2

xpcom patch with MOZ_WIDGET_GTK
Comment 164 Martin Stránský 2012-06-13 01:30:05 PDT
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).
Comment 165 Martin Stránský 2012-06-13 01:37:21 PDT
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)
Comment 166 Martin Stránský 2012-06-13 05:45:28 PDT
Created attachment 632655 [details] [diff] [review]
xpcom gtk3 v3

fixed defines
Comment 167 Ed Morley [:emorley] 2012-06-13 05:59:35 PDT
https://hg.mozilla.org/mozilla-central/rev/4e4f8a07e4d9
Comment 168 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-13 07:23:55 PDT
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

macro changes look fine to me
Comment 169 Martin Stránský 2012-06-13 07:36:37 PDT
Created attachment 632688 [details] [diff] [review]
toolkit gtk3 v.2

toolkit patch with MOZ_WIDGET_GTK macro.
Comment 170 Martin Stránský 2012-06-13 07:39:56 PDT
Comment on attachment 632599 [details] [diff] [review]
gfx, v.4

Thanks!
Comment 173 Martin Stránský 2012-06-13 15:09:57 PDT
(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.
Comment 174 Karl Tomlinson (:karlt) 2012-06-13 15:23:27 PDT
Yes, I'm guessing gdkx.h doesn't pull in gdk.h (or gdkdisplay.h).
Comment 175 Andre Klapper 2012-06-14 02:12:17 PDT
(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."
Comment 176 Martin Stránský 2012-06-14 03:28:11 PDT
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!
Comment 177 Benjamin Smedberg [:bsmedberg] 2012-06-14 14:12:24 PDT
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
Comment 178 Karl Tomlinson (:karlt) 2012-06-14 14:56:02 PDT
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 179 Karl Tomlinson (:karlt) 2012-06-14 15:23:28 PDT
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.
Comment 180 Rafał Mużyło 2012-06-15 01:35:52 PDT
(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.
Comment 181 Chris Lord [:cwiiis] 2012-06-15 02:28:09 PDT
(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).
Comment 182 Martin Stránský 2012-06-15 05:01:23 PDT
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?
Comment 183 Martin Stránský 2012-06-15 07:17:43 PDT
Created attachment 633508 [details] [diff] [review]
toolkit gtk3 v.3

Thanks, updated with requested changes.
Comment 184 Martin Stránský 2012-06-15 07:40:12 PDT
Created attachment 633520 [details] [diff] [review]
gfx, v.6

The gfx patch, with g_object_unref (G_OBJECT(data)) in do_gdk_drawable_unref().
Comment 187 Karl Tomlinson (:karlt) 2012-06-17 20:08:21 PDT
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?
Comment 188 Martin Stránský 2012-06-18 07:44:13 PDT
Comment on attachment 633520 [details] [diff] [review]
gfx, v.6

Let's use gtk2compat.h there too.
Comment 189 Martin Stránský 2012-06-18 08:06:06 PDT
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.
Comment 190 Martin Stránský 2012-06-19 07:06:26 PDT
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.
Comment 192 Martin Stránský 2012-06-19 08:26:33 PDT
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?
Comment 193 Karl Tomlinson (:karlt) 2012-06-19 22:18:39 PDT
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.
Comment 194 Hubert Figuiere [:hub] 2012-06-19 22:25:44 PDT
(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.
Comment 195 Hubert Figuiere [:hub] 2012-06-19 22:39:24 PDT
Forget my last comment. I missed gtk2compat.

Sorry for the noise.
Comment 196 Ed Morley [:emorley] 2012-06-20 02:22:15 PDT
https://hg.mozilla.org/mozilla-central/rev/2e8089e80e0e
Comment 197 Martin Stránský 2012-06-20 06:41:41 PDT
Created attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

Dom patch for check-in, updated with the comments.
Comment 198 Ryan VanderMeulen [:RyanVM] 2012-06-21 17:46:16 PDT
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
Comment 199 Ryan VanderMeulen [:RyanVM] 2012-06-21 17:57:37 PDT
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
Comment 200 Karl Tomlinson (:karlt) 2012-06-21 20:39:15 PDT
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.
Comment 201 Martin Stránský 2012-06-26 07:39:59 PDT
Created attachment 636708 [details] [diff] [review]
gtk2compat fix for dom, v.4

Ahh, we're missing gtk_widget_is_toplevel() in gtk2compat.h
Comment 202 Martin Stránský 2012-06-26 08:31:46 PDT
Created attachment 636722 [details] [diff] [review]
gfx, v.7 for check-in

The gfx patch with requested changes.
Comment 203 Ryan VanderMeulen [:RyanVM] 2012-06-27 17:18:24 PDT
Comment on attachment 634895 [details] [diff] [review]
dom, v.4 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ed2abf47cc
Comment 204 Ryan VanderMeulen [:RyanVM] 2012-06-27 17:18:45 PDT
Comment on attachment 636708 [details] [diff] [review]
gtk2compat fix for dom, v.4

https://hg.mozilla.org/integration/mozilla-inbound/rev/44931b8274b4
Comment 205 Ryan VanderMeulen [:RyanVM] 2012-06-27 17:18:55 PDT
Comment on attachment 636722 [details] [diff] [review]
gfx, v.7 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0ba9f18f3d
Comment 207 Karl Tomlinson (:karlt) 2012-08-15 21:28:12 PDT
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.
Comment 208 Karl Tomlinson (:karlt) 2012-08-16 20:21:29 PDT
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.
Comment 209 aditya3098 2012-08-18 04:22:17 PDT
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 Hussam Al-Tayeb 2012-08-18 06:02:35 PDT
(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.
Comment 211 Martin Stránský 2012-08-24 00:52:55 PDT
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.
Comment 212 Martin Stránský 2012-08-24 00:54:31 PDT
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 Hussam Al-Tayeb 2012-08-24 03:12:10 PDT
not all of it applies to latest trunk but I managed to apply the missing parts manually apart from widget/gtk2/nsFilePicker.cpp
Comment 214 Martin Stránský 2012-08-24 05:05:20 PDT
Created attachment 654976 [details] [diff] [review]
rest of gtk2, v2

Thanks, there's an updated one.
Comment 215 Karl Tomlinson (:karlt) 2012-08-29 21:49:23 PDT
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.
Comment 216 Martin Stránský 2012-09-05 03:37:42 PDT
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!
Comment 217 Martin Stránský 2012-09-05 06:32:49 PDT
Created attachment 658468 [details] [diff] [review]
gtk2/nsBidiKeyboard patch

The nsBidiKeyboard patch
Comment 218 Martin Stránský 2012-09-05 06:48:04 PDT
Created attachment 658473 [details] [diff] [review]
gtk2/bidi v2

Updated initialization in nsBidiKeyboard::nsBidiKeyboard(), it's closer to the original.
Comment 219 Karl Tomlinson (:karlt) 2012-09-05 19:04:21 PDT
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.
Comment 220 Martin Stránský 2012-09-06 04:26:03 PDT
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!
Comment 221 Karl Tomlinson (:karlt) 2012-09-06 21:51:49 PDT
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 222 Karl Tomlinson (:karlt) 2012-09-06 21:52:43 PDT
Comment on attachment 658841 [details] [diff] [review]
rest of gtk2, v4

The rest is good, thanks.
Comment 223 Martin Stránský 2012-09-13 04:25:35 PDT
Created attachment 660786 [details] [diff] [review]
gtk2/bidi v2 for check-in
Comment 224 Martin Stránský 2012-09-13 04:28:03 PDT
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).
Comment 225 Hussam Al-Tayeb 2012-09-13 14:14:00 PDT
I just did a build with attachment 660788 [details] [diff] [review] and attachment 660786 [details] [diff] [review] and it builds and runs well.
Comment 226 Ryan VanderMeulen [:RyanVM] 2012-09-13 17:22:05 PDT
Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=a86c3014db5e
Comment 227 Ryan VanderMeulen [:RyanVM] 2012-09-13 18:58:26 PDT
Comment on attachment 660786 [details] [diff] [review]
gtk2/bidi v2 for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dd45dc4a1a
Comment 228 Ryan VanderMeulen [:RyanVM] 2012-09-13 18:58:57 PDT
Comment on attachment 660788 [details] [diff] [review]
rest of gtk2, v4, for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3817afd0374
Comment 230 Martin Stránský 2012-09-17 07:17:50 PDT
Created attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

Color changes only (plus new file creation).
Comment 231 Marti Raudsepp 2012-09-23 13:26:09 PDT
Can GTK3 be enabled in nightly builds please?
Comment 232 Marc-Antoine Perennou 2012-12-22 04:12:52 PST
Any news or ETA on this? It seemed to be quite active for a while but then totally stopped.
Comment 233 Martin Stránský 2013-01-02 05:55:34 PST
Comment on attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

Yeah, you're right, we should move it a bit. Karl, can you please check this one? It's just color changes in the GTK3 code and reflects the Cris requests.
Comment 234 Martin Stránský 2013-02-05 12:41:23 PST
Created attachment 710347 [details] [diff] [review]
gtk3drawing - gtk_entry clip

GtkEntry drawing needs clipping around the element.
Comment 235 Martin Stránský 2013-02-05 14:09:48 PST
Created attachment 710377 [details] [diff] [review]
remove ThemeRenderer(gfxGdkNativeRenderer) from gtk3
Comment 236 yangtsesu 2013-02-17 06:59:48 PST
Why still no gtk3 branch?
Comment 237 Karl Tomlinson (:karlt) 2013-02-17 21:13:08 PST
Comment on attachment 710347 [details] [diff] [review]
gtk3drawing - gtk_entry clip

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

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

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

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

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

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

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

It shouldn't be necessary to use cliprect here as there should already be a
clip region on the cairo_t that is a subregion of the dirtyRect.
Adding just rect as another clip should be sufficient.
Comment 238 Karl Tomlinson (:karlt) 2013-02-17 21:14:38 PST
Comment on attachment 710377 [details] [diff] [review]
remove ThemeRenderer(gfxGdkNativeRenderer) from gtk3

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

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

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

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

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

I filled https://bugzilla.gnome.org/show_bug.cgi?id=694086 for that with more details. Benjamin Otte promised to help with that so let's leave it for further update.
Comment 240 Martin Stránský 2013-02-19 02:26:04 PST
Created attachment 715429 [details] [diff] [review]
ThemeRenderer, v2

It also removes the cliprect parameter from gtk3drawing.c
Comment 241 Karl Tomlinson (:karlt) 2013-02-19 13:33:07 PST
Comment on attachment 715429 [details] [diff] [review]
ThemeRenderer, v2

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

Need to pass gdk_rect for the rect parameter here.
Comment 242 Martin Stránský 2013-02-20 01:06:46 PST
Created attachment 715914 [details] [diff] [review]
ThemeRenderer, v3, for check-in
Comment 243 Martin Stránský 2013-02-20 02:52:08 PST
Comment on attachment 715914 [details] [diff] [review]
ThemeRenderer, v3, for check-in

Try submission for this patch: https://tbpl.mozilla.org/?tree=Try&rev=84118866603d
Comment 244 Ryan VanderMeulen [:RyanVM] 2013-02-20 05:56:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8e1a272c78
Comment 245 Ryan VanderMeulen [:RyanVM] 2013-02-21 06:57:26 PST
https://hg.mozilla.org/mozilla-central/rev/0c8e1a272c78
Comment 246 Martin Stránský 2013-02-26 05:20:01 PST
Created attachment 718386 [details] [diff] [review]
menubar is transparent in gtk3
Comment 247 Martin Stránský 2013-02-26 05:22:07 PST
Created attachment 718387 [details] [diff] [review]
add padding to widget borders

According to Benjamin Otte, we should use padding+border for widget sizes.
Comment 248 Martin Stránský 2013-02-26 08:02:17 PST
Comment on attachment 718387 [details] [diff] [review]
add padding to widget borders

The padding needs to be added to more places so I'll attach a complete patch for that later.
Comment 249 Martin Stránský 2013-03-20 07:55:24 PDT
Created attachment 727206 [details] [diff] [review]
widget clipping patch

This is a workaround for https://bugzilla.gnome.org/show_bug.cgi?id=694086
Comment 250 Martin Stránský 2013-03-20 08:17:39 PDT
Created attachment 727215 [details] [diff] [review]
radio button  fix

All radio buttons are rendered as active now, this patch fixes it.
Comment 251 Martin Stránský 2013-03-21 05:37:08 PDT
Created attachment 727638 [details] [diff] [review]
widget padding patch

Some elements (especially GtkEntry and menu separators) needs padding, which is reflected by this patch.
Comment 252 Martin Stránský 2013-03-21 08:03:21 PDT
Created attachment 727682 [details] [diff] [review]
tab widget rendering

This patch fixes tabs size and tab background rendering and removes obsolete background rendering code.
Comment 253 Karl Tomlinson (:karlt) 2013-04-08 21:39:45 PDT
Comment on attachment 661786 [details] [diff] [review]
nsLookAndFeel v.2 color changes

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

These changes should land with the other parts that were in attachment 627866 [details] [diff] [review], so just giving feedback here for now.  Shall I look at attachment 627866 [details] [diff] [review] or have you made changes since then?
Comment 254 Martin Stránský 2013-04-09 12:36:08 PDT
Thanks! I'll check them.

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

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

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

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

I mean using a single file with #if (MOZ_WIDGET_GTK == 2) for the methods that are specific to GTK2 and #else for those specific to GTK3.
Comment 256 Martin Stránský 2013-04-11 08:30:51 PDT
Created attachment 736306 [details] [diff] [review]
nsLookAndFeel v.3

Thanks. There's a complete nsLookAndFeel patch with the changes you requested.
Comment 257 texou 2013-05-13 16:41:39 PDT
Hi,

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

Thanks very much for your help.

Regards,
Comment 258 Karl Tomlinson (:karlt) 2013-05-14 23:43:53 PDT
Comment on attachment 736306 [details] [diff] [review]
nsLookAndFeel v.3

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

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

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

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

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

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

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

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

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


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

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

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

It feels like there could be more sharing of StyleContexts between InitStyle
and InitLookAndFeel.  Perhaps that is easier with more color variables
initialized in InitLookAndFeel rather than keeping StyleContexts around.
Comment 259 Karl Tomlinson (:karlt) 2013-05-14 23:46:34 PDT
Comment on attachment 727206 [details] [diff] [review]
widget clipping patch

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

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

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

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

Oh, that would be because of bug 867047.

Re GtkButton and GtkNotebook, does GTK clip before calling the render functions, or does the clipping come from the GdkWindows for the widgets?
Comment 262 Karl Tomlinson (:karlt) 2013-05-16 21:48:54 PDT
Comment on attachment 727638 [details] [diff] [review]
widget padding patch

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Isn't this going to mean that the minimum entry size depends on GTK's default font size?
What bug is this fixing?
Comment 263 Karl Tomlinson (:karlt) 2013-05-16 21:51:27 PDT
Comment on attachment 727682 [details] [diff] [review]
tab widget rendering

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Spaces around the '&' operator, please.

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

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

>+        gtk_style_context_add_region(style, GTK_STYLE_REGION_TAB, region_flags); 

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

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

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

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

Please remove the additional whitespace added after the comment.
Comment 264 Karl Tomlinson (:karlt) 2013-05-20 02:26:32 PDT
(In reply to Karl Tomlinson (:karlt) from comment #261)
> Re GtkButton and GtkNotebook, does GTK clip before calling the render
> functions, or does the clipping come from the GdkWindows for the widgets?

These are all NO_WINDOW widgets, so please ignore that question.
Comment 265 Martin Stránský 2013-05-27 03:44:27 PDT
Created attachment 754403 [details] [diff] [review]
nsLookAndFeel v.4

Thanks, there's an updated patch, it also removes the static members.

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

eColorID__moz_dragtargetzone uses the same color as eColorID_WidgetSelectBackground in gtk2 so let's keep it in gtk3 too. I don't know why we used the cell class here - it may be some leftover from color tests.
Comment 266 Karl Tomlinson (:karlt) 2013-05-27 19:43:53 PDT
Comment on attachment 754403 [details] [diff] [review]
nsLookAndFeel v.4

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

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

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

>+    sMozFieldBackground = GDK_RGBA_TO_NS_RGB(color);

>+    sMozFieldText = GDK_RGBA_TO_NS_RGB(color);

GDK_RGBA_TO_NS_RGBA

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

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

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

I don't see any :selected rules in gtk-default.css that would match the path
and class here.  mViewStyle may be more appropriate and GTK_STATE_FLAG_FOCUSED
may be required to get what we want, but that can be addressed later.  Let's
get this landed now, thanks.
Comment 267 Martin Stránský 2013-05-28 06:13:07 PDT
Created attachment 754789 [details] [diff] [review]
nsLookAndFeel v.4 for check-in

Updated. Try run for this patch:  https://tbpl.mozilla.org/?tree=Try&rev=ad67cba4a14a
Comment 270 Martin Stránský 2013-06-13 04:47:57 PDT
The last build patches hit latest trunk so you can build the gtk3 browser now. It's still a bit raw - there are bugs in color/widget/menu rendering - but it runs. Use this build flags:

ac_add_options --enable-system-cairo
ac_add_options --enable-default-toolkit=cairo-gtk3
Comment 271 Quentin "Sardem FF7" Glidic 2013-06-13 05:35:17 PDT
Created attachment 761985 [details] [diff] [review]
GTK+3 build fixes

In order to compile Firefox with GTK+3 support, I had to apply this patch.

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

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

I don't see that. Please file a bug for that and attach your mozconfig there. Thanks!
Comment 274 Martin Stránský 2013-06-17 02:27:31 PDT
(In reply to Rafał Mużyło from comment #272)
> How does such build work in regard of what I've reported in bug 862422 ?
> To reproduce you may (and do) need to use cairo 1.12 on the system.

The recent gtk3 port does not support NPAPI plugins (which are gtk2 only) now so the flash plugin does not work there.
Comment 275 Seif Lotfy 2013-12-26 15:39:55 PST
(In reply to Martin Stránský from comment #274)
> (In reply to Rafał Mużyło from comment #272)
> > How does such build work in regard of what I've reported in bug 862422 ?
> > To reproduce you may (and do) need to use cairo 1.12 on the system.
> 
> The recent gtk3 port does not support NPAPI plugins (which are gtk2 only)
> now so the flash plugin does not work there.

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

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

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

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

It's covered by Bug 624422 where two libxul are build and plugin-container is linked to the gtk2 one.
Comment 277 ojab 2014-01-07 13:32:20 PST
Created attachment 8356778 [details] [diff] [review]
Fix build with --disable-dbus
Comment 278 Martin Stránský 2014-01-08 06:21:04 PST
Comment on attachment 8356778 [details] [diff] [review]
Fix build with --disable-dbus

Looks okay.
Comment 279 Ryan VanderMeulen [:RyanVM] 2014-01-09 10:39:10 PST
Comment on attachment 8356778 [details] [diff] [review]
Fix build with --disable-dbus

https://hg.mozilla.org/integration/mozilla-inbound/rev/713c3832f82b
Comment 280 Wes Kocher (:KWierso) 2014-01-09 16:15:34 PST
https://hg.mozilla.org/mozilla-central/rev/713c3832f82b
Comment 281 Martin Stránský 2014-01-17 08:17:05 PST
Test packages for Fedora based on trunk can be downloaded here - http://copr-fe.cloud.fedoraproject.org/coprs/stransky/FirefoxGtk3/
Comment 282 Daniel 2014-01-18 01:43:08 PST
How can It be built for other flavour?
Comment 283 Brion Vibber 2014-02-23 18:44:04 PST
I've filed bug 975919 to cover making sure that Gtk+ 3's HiDPI mode is properly supported.
Comment 284 Cork 2014-03-14 08:33:09 PDT
Is bug reports on this feature of interest or is that too early for that yet? (i have a long list)
Comment 285 Hubert Figuiere [:hub] 2014-03-14 09:19:43 PDT
(In reply to Cork from comment #284)
> Is bug reports on this feature of interest or is that too early for that
> yet? (i have a long list)

Please file new bugs and make them block this one for tracking purpose.
Comment 286 Cork 2014-03-16 00:44:07 PDT
There, the other bugs I see appear to be theme bugs and not firefox bugs.
Comment 287 Martin Stránský 2014-04-01 05:49:57 PDT
(In reply to Wiseacre from comment #282)
> How can It be built for other flavour?

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

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

and build as usually.
Comment 288 kxra 2014-05-19 22:27:58 PDT
I've been running this on Fedora 21 (Rawhide) and Electrolysis (separate UI and content processes / tab sandboxing) doesn't work (browser window is empty when I turn it on)

https://wiki.mozilla.org/Electrolysis

Is there an open bug for this issue?
Comment 289 Hubert Figuiere [:hub] 2014-05-20 12:50:24 PDT
If you don't find a duplicate, don't hesitate to file a bug. Make it block this one so it can be tracked. 

Thanks.
Comment 290 kxra 2014-05-20 13:18:15 PDT
Thanks, i filed it as bug 1013552
Comment 291 kxra 2014-05-23 22:31:09 PDT Comment hidden (offtopic)
Comment 292 Hubert Figuiere [:hub] 2014-05-24 09:35:35 PDT Comment hidden (offtopic)
Comment 294 Hussam Al-Tayeb 2015-08-09 20:48:06 PDT
It is possible someone can take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1190163 ?
It's just a matter of making the tab area in 'certificate viewer' a bit darker than the rest of window and making the tab text a bit grey.
(Note: This is not about Firefox browser tabs).
Comment 295 Artem S. Tashkinov 2015-09-23 13:44:55 PDT
Am I correct thinking that there's no way to run the official Firefox (>=42) Linux builds without GTK3?

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

gtk2/libmozgtk.so ?

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

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

There's no way to run gtk3 pages as gtk2. gtk2/libmozgtk.so is only used for gtk2 plugins.
Comment 297 Simón 2015-11-06 03:51:15 PST
I have installed the official Firefox 42 on Ubuntu 14.04 64 bits and it's compiled with gtk2:
Configure arguments
--enable-update-channel=release --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --with-google-oauth-api-keyfile=/builds/google-oauth-api.key --with-mozilla-api-keyfile=/builds/mozilla-desktop-geoloc-api.key --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-default-toolkit=cairo-gtk2 --enable-warnings-as-errors --enable-official-branding --enable-verify-mar

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

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

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

I will try to work with gtk developers to get this fixed, but I can't use any updated Firefox version in the meanwhile.
Comment 302 Jeff Buhrt 2016-01-07 13:16:38 PST
Is there a way to make Firefox use GTK2 vs GTK3 for a quick test?

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

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

Thanks,

-Jeff
Comment 303 Storm 2016-04-13 01:20:59 PDT
Could someone please raise the importance of https://bugzilla.mozilla.org/show_bug.cgi?id=1263427 from normal, because it renders several sites completely unusable requiring to use another browser.
Comment 304 Hussam Al-Tayeb 2016-04-29 08:05:49 PDT
Would this be of any interest?  bug 1268894. The toolbar menu items already use "up arrows" or whatever they are called which is basically similar to native gtk+ popovers. Perhaps theme them like gtk+ popovers?
Comment 305 Hussam Al-Tayeb 2016-05-10 12:40:53 PDT
If possible, please also take a look at bug 1271523 and bug 1271524. I am trying to find areas there themeing is missing.
Comment 306 Hussam Al-Tayeb 2016-05-15 17:26:36 PDT
If time permits, can you please verify bug 1273013? Download progress bar is invisible in my gtk3 build.
Thank you.

Note You need to log in before you can comment on or make changes to this bug.