[webkit-reviews] review requested: [Bug 79496] [GTK] Add GMainLoop and GMainContext to be handled by GRefPtr : [Attachment 128980] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 79496: [GTK] Add GMainLoop and GMainContext to be handled by GRefPtr
https://bugs.webkit.org/show_bug.cgi?id=79496

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching a new patch after addressing some issues. See some comments below...

(In reply to comment #2)
> (From update of attachment 128742 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=128742&action=review
> 
> > Source/JavaScriptCore/wtf/gobject/GRefPtr.cpp:49
> > +	 g_main_context_unref(ptr);
> 
> g_main_context_unref doesn't accept NULL, you should check the pointer

Fixed.

> > Source/JavaScriptCore/wtf/gobject/GRefPtr.cpp:61
> > +	 g_main_loop_unref(ptr);
> 
> Same for main_loop_unref

Fixed.
 
> > Source/WebCore/platform/gtk/RunLoopGtk.cpp:37
> > +	 m_runLoopContext = adoptGRef(g_main_context_default());
> 
> g_main_context_default() is transfer none, we shouldn't adopt the ref.
Previous code works because the context is unrefed only once, I think.

Oops! Fixed.

> I wouldn't use a GRefPtr for this.

I think I prefer to keep using a GRefPtr just in case, to ensure we have a
valid context in functions like RunLoop::wakeUp() and RunLoop::start() when
needed, even if it sounds unlikely to get any trouble with that.

> > Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:138
> > +	 if (m_eventContext)
> > +	     m_eventContext.clear();
> 
> clear already checks the pointer before unref, so you don't need the check

Fixed.