This is something needed for properly implementing modal dialogs in WebKit2 at least, and something that other ports (e.g. Qt, Mac) already have, so it would be a good addition. Actually, it would be more than just "a good addition" but something needed if we want to implement modal dialogs in WebKit2GTK+, since I have observed (while working in the patch for modal dialogs in WK2GTK) that it would be very hard (impossible?) to implement proper unit tests for such a patch since the WebProcess would die right after closing the modal dialog, since WebCore would quit the only event loop once the dialog was closed, which would result in the WebProcess being terminated, causing great trouble and pain in the form of the unit tests hanging after that. So, a patch would be nice here.
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>