[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 #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-03-10 11:27:52 PST ---
(In reply to comment #4)
> (From update of attachment 226318 [details])
> 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.

Thanks, it's actually split :-P I left the Tools dir out. Anyway, I guess I can add patches for WTF, WebCore, WEbKit and WebKit2, for example?

> > 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.

Well, this is private and only used here, but I will changed it.

> > 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.

Yes, this is specific to socket sources that use a cancellable. This can be called from any thread, so when the source is cancelled using the cancellable, we wait for the callback to be called with a 0 condition, that makes the function return FALSE and the source is destroyed.

> > 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.

I didn't know what to do here because the thing is a bit confusing to me, current code uses the same member to handle both sources. scheduleLayerFlush schedules a new source, and this once was scheduled without checking if there was a previous one, but using the same member, so the previous one is "leaked".

> > 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?

The function doesn't have documentation, I assumed the return value is transfer container, so the user frees the returned list. This is doing the same than the current code, it copies the list (the container) and frees everything in an idle.

> > 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.

Really? it's in the next line :-)

> > 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.