Bug 107887 – [Gtk] RunLoop::run shuold run current thread's run loop.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Seulgi Kim]

When off-the main thread use RunLoop with Timer, Timer doesn't fire callback.

[Seulgi Kim]

Created attachment 184933 [details]
Patch

[Seulgi Kim]

Created attachment 184936 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 184936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184936&action=review

> Source/WebCore/ChangeLog:15
> +        No new tests. There is no case that uses RunLoop in off the main thread yet.

I guess you have some specific use cases for this change already in mind, and also that you probably have tested this manually in some way. If so, would you mind sharing those use cases / manual tests here? Also, I really think it would be worth trying to provide some kind of test along with this patch (probably an unit test).

I don't have much experience with this part of the code besides the implementation of modal dialogs in WebKit2GTK+, which I remember used this part of the code to control "modality" of the dialogs, so that's where my interest comes from.

[Seulgi Kim]

(In reply to comment #3)

Now my team is developing threaded model of coordinated graphics in Gtk (https://bugs.webkit.org/show_bug.cgi?id=100341).
And RunLoop is needed in compositor thread.
I'll share prototype sometime this week.

[Mario Sanchez Prada]

(In reply to comment #4)
> (In reply to comment #3)
> 
> Now my team is developing threaded model of coordinated graphics in Gtk (https://bugs.webkit.org/show_bug.cgi?id=100341).
> And RunLoop is needed in compositor thread.
> I'll share prototype sometime this week.

Thanks for the clarification and for sharing that prototype, that will be really useful to properly understand this in a better way.

BTW, I'm also adding Denis Nomiyama to CC here as he's working in something quite related to this and so he might be interested, I guess.

[Seulgi Kim]

I uploaded the prototype at https://github.com/DorothyBrowser/webkit/tree/threaded-compositor.
I welcome your comments.

Thank you.

[Martin Robinson]

Comment on attachment 184936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184936&action=review

Looks pretty reasonable, but there is a memory leak in this new code, I think.

> Source/WebCore/platform/gtk/RunLoopGtk.cpp:35
>  #include <glib.h>
>  
> +#include <wtf/MainThread.h>
> +
>  namespace WebCore {

You should have these includes be all one block.

> Source/WebCore/platform/gtk/RunLoopGtk.cpp:40
> +    m_runLoopContext = isMainThread() ? g_main_context_default() : g_main_context_new();

g_main_context_default does not return a new reference while g_main_context_new does. You are leaking the new main loop here. You should do:

m_runLoopContext = isMainThread() ? g_main_context_default() : adoptGRef(g_main_context_new());

[Seulgi Kim]

Created attachment 185912 [details]
Patch

[Martin Robinson]

Comment on attachment 185912 [details]
Patch

Okay. This seem pretty reasonable.

[WebKit Review Bot]

Comment on attachment 185912 [details]
Patch

Clearing flags on attachment: 185912

Committed r142219: <http://trac.webkit.org/changeset/142219>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.