Bug 120070 – [GTK] gdk threads deprecated functions calls should be refactored

• Status: REOPENED
• Component: WebKitGTK

[Anton Obzhirov]

Warnings: 'gdk_threads_enter/gdk_threads_leave is deprecated' should be removed.

[Anton Obzhirov]

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.

[Anton Obzhirov]

Created attachment 210376 [details]
Patch

[WebKit Commit Bot]

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

[Anton Obzhirov]

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.

[Mario Sanchez Prada]

Comment on attachment 210376 [details]
Patch

lgtm

[WebKit Commit Bot]

Comment on attachment 210376 [details]
Patch

Clearing flags on attachment: 210376

Committed r154989: <http://trac.webkit.org/changeset/154989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(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

[Mario Sanchez Prada]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120678

[Mario Sanchez Prada]

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.