Bug 79496 - [GTK] Add GMainLoop and GMainContext to be handled by GRefPtr
Summary: [GTK] Add GMainLoop and GMainContext to be handled by GRefPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on:
Blocks: 79499 79500
  Show dependency treegraph
 
Reported: 2012-02-24 09:03 PST by Mario Sanchez Prada
Modified: 2012-02-28 11:35 PST (History)
5 users (show)

See Also:


Attachments
Patch proposal (19.50 KB, patch)
2012-02-24 09:10 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (18.72 KB, patch)
2012-02-27 01:26 PST, Mario Sanchez Prada
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2012-02-24 09:03:42 PST
This would be an useful addition IMHO for implementing new stuff (e.g. modal dialogs in WebKit2GTK+) but also for code already present.
Comment 1 Mario Sanchez Prada 2012-02-24 09:10:05 PST
Created attachment 128742 [details]
Patch proposal

Here we go
Comment 2 Carlos Garcia Campos 2012-02-24 09:25:19 PST
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
Comment 3 Mario Sanchez Prada 2012-02-27 01:26:38 PST
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.
Comment 4 Carlos Garcia Campos 2012-02-27 01:50:15 PST
(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.
Comment 5 Martin Robinson 2012-02-28 06:30:34 PST
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.
Comment 6 Mario Sanchez Prada 2012-02-28 11:35:33 PST
Committed r109129: <http://trac.webkit.org/changeset/109129>