[Webkit-unassigned] [Bug 130027] [GLIB] Add GMainLoopSource class to wrap idle and timeout source

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130027





--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2014-03-10 11:08:56 PST ---
(From update of attachment 226318)
View in context: https://bugs.webkit.org/attachment.cgi?id=226318&action=review

Very cool patch! I have a couple questions. I haven't looked at the GStreamer bits. I guess philn would be the best person for that. This is a big patch and we need to watch the bots closely when we land it. I'd prefer to have it split across a few patches that transition the source code in pieces to GMainLoopSource, but I'm sensitive to how annoying that is. Still, if it's possible, please consider it.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:46
> +GMainLoopSource::GMainLoopSource(bool deleteOnDestroy)
> +    : m_deleteOnDestroy(deleteOnDestroy)
> +{
> +}

You probably want to use an enum here instead of a boolean.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:60
> +        // Canceling the cancellable will make the source callback to be called
> +        // so the source is destroyed when it returns

So does this cause the source callback to be called? Not sure I understand the comment.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:270
> +    // We need a different source for this to not cancel the previous one.
> +    GMainLoopSource::create().scheduleAfterDelay("[WebKit] layerFlushTimer", std::bind(&AcceleratedCompositingContext::flushAndRenderLayers, this),
> +        std::chrono::milliseconds(500), GDK_PRIORITY_EVENTS);

This is a bit different because stopAnyPendingLayerFlush no longer cancels this. To be honest, I can't recall any longer whether we really want to replace any existing source or not.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5338
> +        GList* copiedList = g_list_copy(subResources);
> +        GMainLoopSource::create().scheduleAfterDelay("[WebKit] cleanupTemporarilyCachedSubresources",
> +            [copiedList]() { g_list_free_full(copiedList, g_object_unref); }, std::chrono::seconds(1));
> +    }

Aren't we leaking subResources now since the original list is not freed?

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:99
> +    m_socketEventSource.schedule("[WebKit] WorkQueue::SocketEventHandler", [function, closeFunction](GIOCondition condition) {
> +        if (condition & G_IO_HUP || condition & G_IO_ERR) {
> +            closeFunction();
> +            return GMainLoopSource::Stop;
> +        }
> +        if (condition & G_IO_IN) {
> +            function();
> +            return GMainLoopSource::Continue;
> +        }
> +        // EventSource has been cancelled, return false to destroy the source.
> +        return GMainLoopSource::Stop;
> +        }, socket.get(), G_IO_IN,
> +        [this]() { deref(); },
> +        m_eventContext.get());
>  }

Nice! I think we can add some newlines to this though.

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:108
> +    ref();

Probably want to mention where the corresponding deref is.

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:115
> +    ref();

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.