Bug 79496 – [GTK] Add GMainLoop and GMainContext to be handled by GRefPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

This would be an useful addition IMHO for implementing new stuff (e.g. modal dialogs in WebKit2GTK+) but also for code already present.

[Mario Sanchez Prada]

Created attachment 128742 [details]
Patch proposal

Here we go

[Carlos Garcia Campos]

Comment on attachment 128742 [details]
Patch proposal

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

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

Same for main_loop_unref

> 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. I wouldn't use a GRefPtr for this.

> 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

[Mario Sanchez Prada]

Created attachment 128980 [details]
Patch proposal

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.

[Carlos Garcia Campos]

(In reply to comment #3)
> > 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.

The default main context is *never* unrefed, so you will always have a valid context. Taking a reference is harmless in any case.

[Martin Robinson]

Comment on attachment 128980 [details]
Patch proposal

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

Looks good, but please fix the small things below.

> Source/WebCore/platform/gtk/RunLoopGtk.cpp:31
> +#include <wtf/gobject/GRefPtr.h>

This include isn't necessary in the header, so why is it necessary here?

> Source/WebKit2/Platform/WorkQueue.h:53
>  typedef struct _GMainContext GMainContext;
>  typedef struct _GMainLoop GMainLoop;

You can remove these now.

[Mario Sanchez Prada]

Committed r109129: <http://trac.webkit.org/changeset/109129>