120070 2013-08-20 09:48:11 -0700 [GTK] gdk threads deprecated functions calls should be refactored 2017-03-11 10:45:04 -0800 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Other Linux REOPENED P2 Normal --- 120678 1 obzhirov obzhirov berto bugs-noreply cgarcia commit-queue gustavo mario mrobinson oldest_to_newest 919087 0 obzhirov 2013-08-20 09:48:11 -0700 Warnings: 'gdk_threads_enter/gdk_threads_leave is deprecated' should be removed. 919900 1 obzhirov 2013-08-22 09:19:18 -0700 We have 2 places where gdk_threads_leave()/gdk_threads_enter() is used to unblock GDK lock to run nested loop for modal dialogs - void WebPopupMenuProxyGtk::showPopupMenu and webkitWebViewRunAsModal. It is not possible to use gdk_threads_add_* API here because we need to block in these functions. May be popup menu can be refactored not to be modal dialog (For QT port for example it is not). In webkitWebViewRunAsModal it has to be modal unless there is some way to block somewhere else (need to check with Mario since he added this function). Or it should be possible to create new thread, make current thread join new one and run main loop in the new thread. 924026 2 210376 obzhirov 2013-09-03 08:46:24 -0700 Created attachment 210376 Patch 924027 3 commit-queue 2013-09-03 08:48:38 -0700 Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 924031 4 obzhirov 2013-09-03 08:56:04 -0700 After extensive manual testing and running Layout tests it seems that these functions calls can be removed for good. There is no more checks for threads lock in GTK 3.6. In both places (WebPopupMenuProxyGtk::showPopupMenu and webkitWebViewRunAsModal) new loop is created in the context of the thread running main loop so it should be safe and I didn't notice any changes in code behaviour. 924051 5 210376 mario 2013-09-03 09:54:18 -0700 Comment on attachment 210376 Patch lgtm 924061 6 210376 commit-queue 2013-09-03 10:07:13 -0700 Comment on attachment 210376 Patch Clearing flags on attachment: 210376 Committed r154989: <http://trac.webkit.org/changeset/154989> 924062 7 commit-queue 2013-09-03 10:07:16 -0700 All reviewed patches have been landed. Closing bug. 924357 8 cgarcia 2013-09-04 04:26:59 -0700 (In reply to comment #4) > After extensive manual testing and running Layout tests it seems that these functions calls can be removed for good. There is no more checks for threads lock in GTK 3.6. In both places (WebPopupMenuProxyGtk::showPopupMenu and webkitWebViewRunAsModal) new loop is created in the context of the thread running main loop so it should be safe and I didn't notice any changes in code behaviour. Are you sure? leave/enter is still used by gtk itself in current git master for every nested run loop 924370 9 mario 2013-09-04 05:39:29 -0700 (In reply to comment #8) > [...] > Are you sure? leave/enter is still used by gtk itself in current git master for every nested run loop In theory those two functions are deprecated as of GTK 3.6. This is what can be red from gtk-doc in gdk/gdk.c: * Deprecated:3.6: All GDK and GTK+ calls should be made from the main * thread We tested this locally and did not presented any issue, but there might be perhaps something we did not considered properly? If that's the case, feel free to roll it out and apologies for perhaps rushing it too much. 924387 10 commit-queue 2013-09-04 06:13:53 -0700 Re-opened since this is blocked by bug 120678 924391 11 mario 2013-09-04 06:17:51 -0700 I talked to Carlos by IRC and there's actually a valid point here: as webkitgtk+ is not an app, but a library (like gtk+), we need to protect against apps not using webkitgtk+ from the main thread by keeping those gdk_threads_enter/leave calls in place for modal dialogs and popups. This is exactly what gtk+ does and why it internally still uses those functions everywhere, so we need to do the same, even if that means getting some warnings because of have them marked as deprecated functions. Sorry again for the too fast review. 210376 2013-09-03 08:46:24 -0700 2013-09-03 10:07:13 -0700 Patch bug-120070-20130903164623.patch text/plain 2358 obzhirov U3VidmVyc2lvbiBSZXZpc2lvbjogMTU0OTg0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggYTBmZjAxZDllN2YxNTUw YWM5MzBjMTgzMmQzZDU0NDY3MGM3ZmY2NC4uMWFjNjhhMTBlZDI4MmUzMjhiMWJkNDE0MzNjZDli OTEyMzNhYjZjMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEzLTA5LTAzICBBbnRv biBPYnpoaXJvdiAgPGEub2J6aGlyb3ZAc2Ftc3VuZy5jb20+CisKKyAgICAgICAgW0dUS10gZ2Rr IHRocmVhZHMgZGVwcmVjYXRlZCBmdW5jdGlvbnMgY2FsbHMgc2hvdWxkIGJlIHJlZmFjdG9yZWQK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyMDA3MAor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZWQg ZGVwcmVjYXRlZCBmdW5jdGlvbnMgZ2RrX3RocmVhZHNfbGVhdmUoKS9nZGtfdGhyZWFkc19lbnRl cigpIGZ1bmN0aW9ucyBzaW5jZQorICAgICAgICB0aGVyZSBpcyBubyBtb3JlIGNoZWNrcyBmb3Ig dGhyZWFkcyBsb2NrIGluIEdUSyAzLjYuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9X ZWJLaXRXZWJWaWV3LmNwcDoKKyAgICAgICAgKHdlYmtpdFdlYlZpZXdSdW5Bc01vZGFsKToKKyAg ICAgICAgKiBVSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcDoKKyAgICAgICAg KFdlYktpdDo6V2ViUG9wdXBNZW51UHJveHlHdGs6OnNob3dQb3B1cE1lbnUpOgorCiAyMDEzLTA5 LTAyICBEYXJpbiBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAgICBDdXQgZG93biBv biBkb3VibGUgaGFzaGluZyBhbmQgY29kZSBuZWVkbGVzc2x5IHVzaW5nIGhhc2ggdGFibGUgaXRl cmF0b3JzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRXZWJWaWV3LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdl YlZpZXcuY3BwCmluZGV4IDcyZmU2NmE0MmVhZDIyZmFmMmQ5YTQ4NWE5OTEyMjUzMjYyMmZlZDIu LjMxODcxNjI0YWU5MmY5NzIwMjAwZjcyZTY2YjdhMjQwNmI0OTBjNzggMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXcuY3BwCisrKyBiL1Nv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYlZpZXcuY3BwCkBAIC0xNTYw LDkgKzE1NjAsNyBAQCB2b2lkIHdlYmtpdFdlYlZpZXdSdW5Bc01vZGFsKFdlYktpdFdlYlZpZXcq IHdlYlZpZXcpCiAgICAgZ19zaWduYWxfZW1pdCh3ZWJWaWV3LCBzaWduYWxzW1JVTl9BU19NT0RB TF0sIDAsIE5VTEwpOwogCiAgICAgd2ViVmlldy0+cHJpdi0+bW9kYWxMb29wID0gYWRvcHRHUmVm KGdfbWFpbl9sb29wX25ldygwLCBGQUxTRSkpOwotICAgIGdka190aHJlYWRzX2xlYXZlKCk7CiAg ICAgZ19tYWluX2xvb3BfcnVuKHdlYlZpZXctPnByaXYtPm1vZGFsTG9vcC5nZXQoKSk7Ci0gICAg Z2RrX3RocmVhZHNfZW50ZXIoKTsKIH0KIAogdm9pZCB3ZWJraXRXZWJWaWV3Q2xvc2VQYWdlKFdl YktpdFdlYlZpZXcqIHdlYlZpZXcpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nl c3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vz cy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuY3BwCmluZGV4IGE0MTgzOWEzZWI5NzY0ZWQ0NzM0 YWYyZTk3Njg0NzdlOTBlZjM0MjcuLjExMTRhMTg0MDU3MmU0YjFmMjEwZjk4NTg4NTVhM2I0MmYx NWQyZmIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2ViUG9wdXBN ZW51UHJveHlHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2ViUG9w dXBNZW51UHJveHlHdGsuY3BwCkBAIC05Nyw5ICs5Nyw3IEBAIHZvaWQgV2ViUG9wdXBNZW51UHJv eHlHdGs6OnNob3dQb3B1cE1lbnUoY29uc3QgSW50UmVjdCYgcmVjdCwgVGV4dERpcmVjdGlvbiB0 ZXh0CiAgICAgLy8gbWVudSByaWdodCBhZnRlciBjYWxsaW5nIFdlYlBvcHVwTWVudVByb3h5Ojpz aG93UG9wdXBNZW51KCkuCiAgICAgbV9ydW5Mb29wID0gYWRvcHRHUmVmKGdfbWFpbl9sb29wX25l dygwLCBGQUxTRSkpOwogCi0gICAgZ2RrX3RocmVhZHNfbGVhdmUoKTsKICAgICBnX21haW5fbG9v cF9ydW4obV9ydW5Mb29wLmdldCgpKTsKLSAgICBnZGtfdGhyZWFkc19lbnRlcigpOwogCiAgICAg bV9ydW5Mb29wLmNsZWFyKCk7CiAK