Bug 79499

Created attachment 128747 [details] Patch Proposal This patch mimics the implementation for the Qt port, and works pretty nice with the patch I already have in my local machine for supporting modal dialogs in WebKit2GTK+ (that will serve as the test for this, actually). I will be reporting here the bug for the modal dialogs in WK2GTK as soon as I filed it (in a few minutes)

This patch is written assuming we have support for GMainLoop in GRefPtr, so that's why I'm setting it as depending on bug 79496

Comment on attachment 128747 [details] Patch Proposal View in context: https://bugs.webkit.org/attachment.cgi?id=128747&action=review One thing that worries me about this patch is that RunLoop::mainLoop will return the wrong GMainLoop when you are in an interior loop. Perhapsm_runLoopMain should be replaced with a stack of loops? > Source/WebCore/platform/gtk/RunLoopGtk.cpp:50 > +static GRefPtr<GMainLoop> currentEventLoop; I think this should be a member of RunLoop. Isn't it important that this can be used across different threads? You should probably call this currentMainLoop to match the type. > Source/WebCore/platform/gtk/RunLoopGtk.cpp:55 > + static bool mainEventLoopIsRunning = false; > + if (!mainEventLoopIsRunning) { mainEventLoopIsRunning can be replaced with a call to g_main_loop_is_running.

Created attachment 129009 [details] Patch Proposal (In reply to comment #3) > (From update of attachment 128747 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128747&action=review > > One thing that worries me about this patch is that RunLoop::mainLoop will return the wrong GMainLoop when you are in an interior loop. Perhapsm_runLoopMain should be replaced with a stack of loops? As I said in a previous comment, I just took Qt's implementation as an example and translated it to Gtk+. Still, I agree with you there might be issues with this approach and so the idea of having a stack of loops sounds good to me too, so here it's a new patch implementing that approach. Also, I renamed RunLoop::mainLoop() to RunLoop::rootMainLoop() to make clear that it will return always the initial main loop (the one created on object construction), which is what we need to make sure it's running before attempting to create a new and nested loop. > > Source/WebCore/platform/gtk/RunLoopGtk.cpp:50 > > +static GRefPtr<GMainLoop> currentEventLoop; > > I think this should be a member of RunLoop. Isn't it important that this can be used across different threads? You should probably call this currentMainLoop to match the type. I just removed this variable now that I hold a Vector of main loops per instance of the RunLoop class. We could add a new RunLoop::currentMainLoop() to get the last one in the stack, but the truth is that I don't see it necessary at all (see the patch). > > Source/WebCore/platform/gtk/RunLoopGtk.cpp:55 > > + static bool mainEventLoopIsRunning = false; > > + if (!mainEventLoopIsRunning) { > > mainEventLoopIsRunning can be replaced with a call to g_main_loop_is_running. Very true. Fixed

Comment on attachment 129009 [details] Patch Proposal View in context: https://bugs.webkit.org/attachment.cgi?id=129009&action=review With a one small fix and a rename, this looks good to me. > Source/WebCore/platform/gtk/RunLoopGtk.cpp:54 > + for (size_t i = 0; i < m_runLoopMainLoops.size(); ++i) { > + if (!g_main_loop_is_running(m_runLoopMainLoops[i].get())) > + continue; > + g_main_loop_quit(m_runLoopMainLoops[i].get()); > + } You should probably traverse the list backward since if a loop > 1 is running, the ones before it are running too. > Source/WebCore/platform/gtk/RunLoopGtk.cpp:75 > +GMainLoop* RunLoop::rootMainLoop() I think I prefer the name innermostRunLoop. Would you mind changing this before landing.

Comment on attachment 129009 [details] Patch Proposal View in context: https://bugs.webkit.org/attachment.cgi?id=129009&action=review > Source/WebCore/platform/gtk/RunLoopGtk.cpp:72 > + GDK_THREADS_ENTER(); > + g_main_loop_run(nestedMainLoop); > + GDK_THREADS_LEAVE(); I don't think we should add this here. Note that RunLoop might be used to create run loops on any thread, that's why there's main() and current() static methods. So this can be used by a thread that doesn't even use GDK at all. So, I think the user should decide whether to use GDK_THREADS_ENTER/LEAVE, the UI process in the case of showModalDialog()

Sorry, I didn't meant to change the review flag

Comment on attachment 129009 [details] Patch Proposal View in context: https://bugs.webkit.org/attachment.cgi?id=129009&action=review >> Source/WebCore/platform/gtk/RunLoopGtk.cpp:72 >> + GDK_THREADS_LEAVE(); > > I don't think we should add this here. Note that RunLoop might be used to create run loops on any thread, that's why there's main() and current() static methods. So this can be used by a thread that doesn't even use GDK at all. So, I think the user should decide whether to use GDK_THREADS_ENTER/LEAVE, the UI process in the case of showModalDialog() Also, once the main loop has finished, you don't want to keep it in the vector, just remove it here

Comment on attachment 129009 [details] Patch Proposal View in context: https://bugs.webkit.org/attachment.cgi?id=129009&action=review >> Source/WebCore/platform/gtk/RunLoopGtk.cpp:72 >> + GDK_THREADS_LEAVE(); > > I don't think we should add this here. Note that RunLoop might be used to create run loops on any thread, that's why there's main() and current() static methods. So this can be used by a thread that doesn't even use GDK at all. So, I think the user should decide whether to use GDK_THREADS_ENTER/LEAVE, the UI process in the case of showModalDialog() I think I agree with cgarcia here. Do you mind removing these before landing?

(In reply to comment #8) > Also, once the main loop has finished, you don't want to keep it in the vector, just remove it here Presumably at that time ::stop has already been called and it's already removed...

(In reply to comment #10) > (In reply to comment #8) > > > Also, once the main loop has finished, you don't want to keep it in the vector, just remove it here > > Presumably at that time ::stop has already been called and it's already removed... But here you have already checked it's not the main one, so that in stop you can simply call main_loop_quit, and you don't need to check whether vector size is > 1.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > > > Also, once the main loop has finished, you don't want to keep it in the vector, just remove it here > > > > Presumably at that time ::stop has already been called and it's already removed... > > But here you have already checked it's not the main one, so that in stop you can simply call main_loop_quit, and you don't need to check whether vector size is > 1. Good point. It does seem a bit cleaner to do that here rather than in ::stop.

Thank you Carlos and Martin for reviewing the patches. I will apply all the suggestions before landing, but before I need to get patch for bug 79496 reviewed too. So... "ping, reviewers"? :-)

(In reply to comment #12) >[...] > > But here you have already checked it's not the main one, so that in stop you can simply call main_loop_quit, and you don't need to check whether vector size is > 1. > > Good point. It does seem a bit cleaner to do that here rather than in ::stop. There's a problem with this: RunLoop::run() function is static, thus I don't have access to the m_runLoopMainLoops Vector there and I'd need a removeLoastMainLoop function in a similar way I needed addNestedMainLoop(). That's why I do it in the stop function: I just quit the main loop and remove it from the Vector. I think it's equivalent and more clean, since ::stop is not an static function, and so I have access to the private Vector from there.

(In reply to comment #14) > There's a problem with this: RunLoop::run() function is static, thus I don't have access to the m_runLoopMainLoops Vector there and I'd need a removeLoastMainLoop function in a similar way I needed addNestedMainLoop(). A pair of functions sounds reasonable. Why not rename addNestedMainLoop(...) to pushNestedMainLoop and then add popNestedMainLoop. It's more obvious what they do then.

(In reply to comment #15) > (In reply to comment #14) > > > There's a problem with this: RunLoop::run() function is static, thus I don't have access to the m_runLoopMainLoops Vector there and I'd need a removeLoastMainLoop function in a similar way I needed addNestedMainLoop(). > > A pair of functions sounds reasonable. Why not rename addNestedMainLoop(...) to pushNestedMainLoop and then add popNestedMainLoop. It's more obvious what they do then. I just tried to minimize the number of new functions to add, but this idea of push and pop sounds good to me too. Will change before landing then. Thanks

Committed r109161: <http://trac.webkit.org/changeset/109161>