[webkit-reviews] review requested: [Bug 79499] [GTK] Add support for nested event loops in RunLoop : [Attachment 129009] Patch Proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 79499: [GTK] Add support for nested event loops in RunLoop
https://bugs.webkit.org/show_bug.cgi?id=79499

Attachment 129009: Patch Proposal
https://bugs.webkit.org/attachment.cgi?id=129009&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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