Bug 130027 - [GLIB] Add GMainLoopSource class to wrap idle and timeout sources
Summary: [GLIB] Add GMainLoopSource class to wrap idle and timeout sources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 130078 130080 130081
  Show dependency treegraph
 
Reported: 2014-03-10 10:12 PDT by Carlos Garcia Campos
Modified: 2014-03-20 00:57 PDT (History)
21 users (show)

See Also:


Attachments
Patch (119.56 KB, patch)
2014-03-10 10:38 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (20.49 KB, patch)
2014-03-11 02:29 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (20.60 KB, patch)
2014-03-14 03:32 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff
New patch for landing (21.46 KB, patch)
2014-03-19 06:46 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-03-10 10:12:10 PDT
GLib main loop sources like idle and timeouts are sometimes unconvenient to use and it's very common to forget canceling the source when the object is destroyed or reset the source ID in the callback when called. We could add a wrapper class to make it easier to handle sources and also to avoid those typical mistakes.
Comment 1 Carlos Garcia Campos 2014-03-10 10:38:45 PDT
Created attachment 226318 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-10 10:39:57 PDT
Attachment 226318 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:374:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:378:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:388:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:532:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:551:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:588:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:608:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:649:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:682:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:88:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:104:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:126:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:144:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:153:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:76:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/gtk/webkit/webkitwebview.cpp:5337:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:539:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:547:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:689:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:719:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:748:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:218:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gtk/RunLoopGtk.cpp:118:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:110:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:117:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 38 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2014-03-10 10:53:19 PDT
It seems the style checker doesn't like std::function and lambdas.
Comment 4 Martin Robinson 2014-03-10 11:08:56 PDT
Comment on attachment 226318 [details]
Patch

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.
Comment 5 Carlos Garcia Campos 2014-03-10 11:27:52 PDT
(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.
Comment 6 Martin Robinson 2014-03-10 11:38:03 PDT
Comment on attachment 226318 [details]
Patch

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

>>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5338
>>> +    }
>> 
>> 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.

Well in the old code g_list_free is called on the subResources returned. In this version, the contents of the original list are freed, but not the actual list itself.

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

Oh, sorry. I missed the pairs! I don't think it's necessary.
Comment 7 Carlos Garcia Campos 2014-03-10 12:07:34 PDT
(In reply to comment #6)
> (From update of attachment 226318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226318&action=review
> 
> >>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5338
> >>> +    }
> >> 
> >> 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.
> 
> Well in the old code g_list_free is called on the subResources returned. In this version, the contents of the original list are freed, but not the actual list itself.

Where does that happen?

> >>> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:115
> >>> +    ref();
> >> 
> >> Ditto.
> > 
> > 
> 
> Oh, sorry. I missed the pairs! I don't think it's necessary.
Comment 8 Carlos Garcia Campos 2014-03-10 12:11:54 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 226318 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=226318&action=review
> > 
> > >>> Source/WebKit/gtk/webkit/webkitwebview.cpp:5338
> > >>> +    }
> > >> 
> > >> 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.
> > 
> > Well in the old code g_list_free is called on the subResources returned. In this version, the contents of the original list are freed, but not the actual list itself.
> 
> Where does that happen?

If you mean the g_list_free(subResources); in line 5324, that's not the returned list, but the copied list passed to the callback. This patch does the same but using g_list_free_full instead of g_list_foreach + g_list_free
Comment 9 Philippe Normand 2014-03-10 12:20:30 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 226318 [details] [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?
> 

SGTM!
Comment 10 Philippe Normand 2014-03-10 12:22:36 PDT
Neat stuff btw!

I wonder if it'd be hard to make the style bot detect future bare g_timeout_ and friends calls so it could warn the patch author?
Comment 11 Carlos Garcia Campos 2014-03-11 00:53:53 PDT
(In reply to comment #10)
> Neat stuff btw!
> 
> I wonder if it'd be hard to make the style bot detect future bare g_timeout_ and friends calls so it could warn the patch author?

This is a great idea.
Comment 12 Carlos Garcia Campos 2014-03-11 01:11:27 PDT
I don't understand EFL build failures, could it be the compiler version of the EWS bots?
Comment 13 Carlos Garcia Campos 2014-03-11 02:29:45 PDT
Created attachment 226403 [details]
Patch

Patch with the WTF changes only and with review comments addressed. I'll file new bug reports for the rest of the original patch.
Comment 14 WebKit Commit Bot 2014-03-11 02:31:48 PDT
Attachment 226403 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:80:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gtk/RunLoopGtk.cpp:118:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:88:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:104:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:126:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:144:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:153:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 19 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Raphael Kubo da Costa (:rakuco) 2014-03-11 03:53:11 PDT
(In reply to comment #12)
> I don't understand EFL build failures, could it be the compiler version of the EWS bots?

Looks like you're right. I don't know which GCC version is installed on the bots, but the following snippet works with GCC 4.8.2 and fails with GCC 4.6.3:

  #include <functional>
  void f(std::function<void()>) {}
  void f(std::function<bool()>) {}
  void g(int) {}
  void h() {
      f(std::bind(g, 42));
  }
Comment 16 Carlos Garcia Campos 2014-03-11 03:59:18 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > I don't understand EFL build failures, could it be the compiler version of the EWS bots?
> 
> Looks like you're right. I don't know which GCC version is installed on the bots, but the following snippet works with GCC 4.8.2 and fails with GCC 4.6.3:
> 
>   #include <functional>
>   void f(std::function<void()>) {}
>   void f(std::function<bool()>) {}
>   void g(int) {}
>   void h() {
>       f(std::bind(g, 42));
>   }

Thanks for testing, I assume the EFL bots (not EWS) have a recent enough GCC. I'll keep watching your bots when these patches land to roll them out if needed.
Comment 17 Csaba Osztrogonác 2014-03-11 04:28:21 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > I don't understand EFL build failures, could it be the compiler version of the EWS bots?
> 
> Looks like you're right. I don't know which GCC version is installed on the bots, but the following snippet works with GCC 4.8.2 and fails with GCC 4.6.3:
> 
>   #include <functional>
>   void f(std::function<void()>) {}
>   void f(std::function<bool()>) {}
>   void g(int) {}
>   void h() {
>       f(std::bind(g, 42));
>   }

This snippet failed for me too with GCC 4.7.2 (default GCC on Ubuntu 12.10):
$ g++ 1.cpp -std=gnu++0x
1.cpp: In function ‘void h()’:
1.cpp:6:23: error: call of overloaded ‘f(std::_Bind_helper<false, void (&)(int), int>::type)’ is ambiguous
1.cpp:6:23: note: candidates are:
1.cpp:2:6: note: void f(std::function<void()>)
1.cpp:3:6: note: void f(std::function<bool()>)
Comment 18 Zan Dobersek 2014-03-11 05:05:44 PDT
Comment on attachment 226403 [details]
Patch

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

> Source/WTF/wtf/gobject/GMainLoopSource.h:56
> +    void scheduleAfterDelay(const char* name, std::function<void ()>, std::chrono::milliseconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::milliseconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> +    void scheduleAfterDelay(const char* name, std::function<void ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);

Can we simplify these into just two separate functions, differing in the type of the callback but both taking in std::chrono::milliseconds?

std::chrono::duration object should use the implicit constructor to convert between durations, so you could call sheduleAfterDelay() with std::chrono::seconds which would then get converted into milliseconds.

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:102
> +    GMainLoopSource::create().schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this));

Consider using a C++11 lambda, but remember to protect the RunLoop object through the reference counting mechanism:

    ref();
    GMainLoopSource::create().schedule(..., [this] {
        performWork();
        deref();
    });

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:118
> +    m_timerSource.scheduleAfterDelay("[WebKit] RunLoop::Timer", static_cast<std::function<bool ()>>([this, repeat]() { fired(); return repeat; }),

Consider simply using the std::function<bool ()> constructor instead of the static_cast to that type.
Comment 19 Zan Dobersek 2014-03-11 05:12:51 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > I don't understand EFL build failures, could it be the compiler version of the EWS bots?
> 
> Looks like you're right. I don't know which GCC version is installed on the bots, but the following snippet works with GCC 4.8.2 and fails with GCC 4.6.3:
> 
>   #include <functional>
>   void f(std::function<void()>) {}
>   void f(std::function<bool()>) {}
>   void g(int) {}
>   void h() {
>       f(std::bind(g, 42));
>   }

As a note, WebKit bumped the minimum required GCC version to 4.7, even if that obviously doesn't avoid bumping into such issues.

As a second note, Clang successfully compiles this code, so no issue there.
Comment 20 Carlos Garcia Campos 2014-03-11 06:07:21 PDT
(In reply to comment #18)
> (From update of attachment 226403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226403&action=review

Thanks for the comments.

> > Source/WTF/wtf/gobject/GMainLoopSource.h:56
> > +    void scheduleAfterDelay(const char* name, std::function<void ()>, std::chrono::milliseconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> > +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::milliseconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> > +    void scheduleAfterDelay(const char* name, std::function<void ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> > +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> 
> Can we simplify these into just two separate functions, differing in the type of the callback but both taking in std::chrono::milliseconds?

The implementation is not the same, when we receive milliseconds, we use g_timeout_source_new, and when we receive seconds we use g_timeout_source_new_seconds. They are different sources.

> std::chrono::duration object should use the implicit constructor to convert between durations, so you could call sheduleAfterDelay() with std::chrono::seconds which would then get converted into milliseconds.
> 
> > Source/WTF/wtf/gtk/RunLoopGtk.cpp:102
> > +    GMainLoopSource::create().schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this));
> 
> Consider using a C++11 lambda, but remember to protect the RunLoop object through the reference counting mechanism:
> 
>     ref();
>     GMainLoopSource::create().schedule(..., [this] {
>         performWork();
>         deref();
>     });

We were not getting a ref before, I'm trying to do what current code does (unless it's wrong of course). We can improve it later.

> > Source/WTF/wtf/gtk/RunLoopGtk.cpp:118
> > +    m_timerSource.scheduleAfterDelay("[WebKit] RunLoop::Timer", static_cast<std::function<bool ()>>([this, repeat]() { fired(); return repeat; }),
> 
> Consider simply using the std::function<bool ()> constructor instead of the static_cast to that type.

That would be something like std::function<bool ()>(lambda); ?
Comment 21 Philippe Normand 2014-03-11 07:15:53 PDT
What about WTF::callOnMainThread? I think actually that a lot of code in WebCore could use it.
Comment 22 Carlos Garcia Campos 2014-03-11 07:45:59 PDT
(In reply to comment #21)
> What about WTF::callOnMainThread? I think actually that a lot of code in WebCore could use it.

Well, I don't know how that exactly works, but it uses a functions queue protected by a mutex that ends up using a GMainLoopSource in the end. It seems to me that adds complexity that our GMainLoop already does for us. So, for non cross platform code, I think it's better to schedule sources directly.
Comment 23 Zan Dobersek 2014-03-11 09:33:37 PDT
Comment on attachment 226403 [details]
Patch

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

>>> Source/WTF/wtf/gobject/GMainLoopSource.h:56
>>> +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
>> 
>> Can we simplify these into just two separate functions, differing in the type of the callback but both taking in std::chrono::milliseconds?
>> 
>> std::chrono::duration object should use the implicit constructor to convert between durations, so you could call sheduleAfterDelay() with std::chrono::seconds which would then get converted into milliseconds.
> 
> The implementation is not the same, when we receive milliseconds, we use g_timeout_source_new, and when we receive seconds we use g_timeout_source_new_seconds. They are different sources.

Can we afford to only use g_timeout_source_new? Looking at the GLib code, the only difference g_timeout_source_new_seconds adds is a conditional branch in g_timeout_set_expiration that slightly modifies the expiration time of the source.

>>> Source/WTF/wtf/gtk/RunLoopGtk.cpp:102
>>> +    GMainLoopSource::create().schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this));
>> 
>> Consider using a C++11 lambda, but remember to protect the RunLoop object through the reference counting mechanism:
>> 
>>     ref();
>>     GMainLoopSource::create().schedule(..., [this] {
>>         performWork();
>>         deref();
>>     });
> 
> We were not getting a ref before, I'm trying to do what current code does (unless it's wrong of course). We can improve it later.

The current code doesn't guarantee (though ref-counting) that the RunLoop object will exist when the callback will be eventually fired.

>>> Source/WTF/wtf/gtk/RunLoopGtk.cpp:118
>>> +    m_timerSource.scheduleAfterDelay("[WebKit] RunLoop::Timer", static_cast<std::function<bool ()>>([this, repeat]() { fired(); return repeat; }),
>> 
>> Consider simply using the std::function<bool ()> constructor instead of the static_cast to that type.
> 
> That would be something like std::function<bool ()>(lambda); ?

Exactly.
Comment 24 Carlos Garcia Campos 2014-03-11 09:51:13 PDT
(In reply to comment #23)
> (From update of attachment 226403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226403&action=review
> 
> >>> Source/WTF/wtf/gobject/GMainLoopSource.h:56
> >>> +    void scheduleAfterDelay(const char* name, std::function<bool ()>, std::chrono::seconds, int priority = G_PRIORITY_DEFAULT, std::function<void ()> destroyFunction = nullptr, GMainContext* = nullptr);
> >> 
> >> Can we simplify these into just two separate functions, differing in the type of the callback but both taking in std::chrono::milliseconds?
> >> 
> >> std::chrono::duration object should use the implicit constructor to convert between durations, so you could call sheduleAfterDelay() with std::chrono::seconds which would then get converted into milliseconds.
> > 
> > The implementation is not the same, when we receive milliseconds, we use g_timeout_source_new, and when we receive seconds we use g_timeout_source_new_seconds. They are different sources.
> 
> Can we afford to only use g_timeout_source_new? Looking at the GLib code, the only difference g_timeout_source_new_seconds adds is a conditional branch in g_timeout_set_expiration that slightly modifies the expiration time of the source.

It's not exactly the same, with timeout_seconds the sources a grouped and dispatched together which is less accurate, but more efficient. See the documentation of g_timeout_add_seconds_full(). I added this, because we were using g_timeout_add_seconds somewhere, and I assumed it was used for this reason and not to avoid the conversion to milliseconds.

> >>> Source/WTF/wtf/gtk/RunLoopGtk.cpp:102
> >>> +    GMainLoopSource::create().schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this));
> >> 
> >> Consider using a C++11 lambda, but remember to protect the RunLoop object through the reference counting mechanism:
> >> 
> >>     ref();
> >>     GMainLoopSource::create().schedule(..., [this] {
> >>         performWork();
> >>         deref();
> >>     });
> > 
> > We were not getting a ref before, I'm trying to do what current code does (unless it's wrong of course). We can improve it later.
> 
> The current code doesn't guarantee (though ref-counting) that the RunLoop object will exist when the callback will be eventually fired.

So, it's a different bug.

> >>> Source/WTF/wtf/gtk/RunLoopGtk.cpp:118
> >>> +    m_timerSource.scheduleAfterDelay("[WebKit] RunLoop::Timer", static_cast<std::function<bool ()>>([this, repeat]() { fired(); return repeat; }),
> >> 
> >> Consider simply using the std::function<bool ()> constructor instead of the static_cast to that type.
> > 
> > That would be something like std::function<bool ()>(lambda); ?
> 
> Exactly.

I'll try that way, thanks!
Comment 25 Carlos Garcia Campos 2014-03-14 03:32:15 PDT
Created attachment 226675 [details]
Updated patch

Addresses all review comments
Comment 26 WebKit Commit Bot 2014-03-14 03:33:25 PDT
Attachment 226675 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:80:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gtk/RunLoopGtk.cpp:120:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:88:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:104:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:126:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:144:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:153:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 19 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Zan Dobersek 2014-03-14 05:32:26 PDT
Comment on attachment 226675 [details]
Updated patch

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

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:104
> +        G_PRIORITY_DEFAULT, [this]() { deref(); });

Nit: you can omit the () parentheses: '[this] { deref(); }'

> Source/WTF/wtf/gtk/RunLoopGtk.cpp:120
> +    m_timerSource.scheduleAfterDelay("[WebKit] RunLoop::Timer", std::function<bool ()>([this, repeat]() { fired(); return repeat; }),

Ditto.
Comment 28 Martin Robinson 2014-03-15 20:32:16 PDT
Comment on attachment 226675 [details]
Updated patch

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

Looks good for landing with Zan's suggestions.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:46
> +GMainLoopSource& GMainLoopSource::create()
> +{
> +    return *new GMainLoopSource(DeleteOnDestroy);
> +}
> +
> +GMainLoopSource::GMainLoopSource()
> +    : m_deleteOnDestroy(DoNotDeleteOnDestroy)
> +{
> +}
> +
> +GMainLoopSource::GMainLoopSource(DeleteOnDestroyType deleteOnDestroy)
> +    : m_deleteOnDestroy(deleteOnDestroy)
> +{
> +}

This is quite clever.

> Source/WTF/wtf/gobject/GMainLoopSource.cpp:194
> +    bool retval = m_socketCallback(condition);
> +    if (!retval && source == m_source.get())
> +        destroy();

Nit: I think you should use returnValue instead of retval.
Comment 29 Martin Robinson 2014-03-15 21:16:43 PDT
Comment on attachment 226675 [details]
Updated patch

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

>> Source/WTF/wtf/gobject/GMainLoopSource.cpp:46
>> +}
> 
> This is quite clever.

Just thinking about this, the ::create thing is a bit confusing, because it doesn't actually return something that you are supposed to delete (like other create methods). I wonder if you can just make static versions of the schedule methods that take care of creating the source? So when you call it, it would look something like:

GMainLoopSource::schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this), G_PRIORITY_DEFAULT, [this]() { deref(); });

One issue with this is that "schedule" cannot be both static and an instance method, so we'd need to pick one name for the static version and one for the instance version.

I think another option would be to rename "create" to something else that makes it clear that this isn't a traditional factory method.
Comment 30 Carlos Garcia Campos 2014-03-17 02:20:49 PDT
(In reply to comment #29)
> (From update of attachment 226675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226675&action=review
> 
> >> Source/WTF/wtf/gobject/GMainLoopSource.cpp:46
> >> +}
> > 
> > This is quite clever.
> 
> Just thinking about this, the ::create thing is a bit confusing, because it doesn't actually return something that you are supposed to delete (like other create methods).

hmm, this is a good point.

> I wonder if you can just make static versions of the schedule methods that take care of creating the source? So when you call it, it would look something like:
> 
> GMainLoopSource::schedule("[WebKit] RunLoop work", std::bind(&RunLoop::performWork, this), G_PRIORITY_DEFAULT, [this]() { deref(); });
> 
> One issue with this is that "schedule" cannot be both static and an instance method, so we'd need to pick one name for the static version and one for the instance version.

I think this is a good idea, the problem is indeed choosing good names :-) scheduleAndDeleteOnDestroy? scheduleDeletingOnDestroy? ...

> I think another option would be to rename "create" to something else that makes it clear that this isn't a traditional factory method.

Well, I agree this is not consistent with other cases where we use create(), but we are actually creating a new object, so I can't think of a better name. createdAutoDeleted? ...
Comment 31 Carlos Garcia Campos 2014-03-18 07:17:56 PDT
I think it will be easier and simpler if I rename the create static method instead of duplicate all schedule methods with a static variant and a different name. What do you think about createAndDeleteOnDestroy()?
Comment 32 Martin Robinson 2014-03-18 07:50:25 PDT
(In reply to comment #31)
> I think it will be easier and simpler if I rename the create static method instead of duplicate all schedule methods with a static variant and a different name. What do you think about createAndDeleteOnDestroy()?

Sounds good to me!
Comment 33 Carlos Garcia Campos 2014-03-19 06:46:04 PDT
Created attachment 227177 [details]
New patch for landing

I've updated the patch because I've noticed some issues in edge cases while porting WebCore. The current patch prevents cancel from being called twice when the destroy function cancels the source. Also handles the case where the destroy function deletes the source. And I've noticed that in some cases we want to know if the source is scheduled, but in other we want to know if it's active (for example for boolean sources that are dispatched several times). So, I have added an internal status and exposed both isScheduled() and isActive().
Comment 34 WebKit Commit Bot 2014-03-19 06:51:24 PDT
Attachment 227177 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.h:86:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gtk/RunLoopGtk.cpp:120:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:102:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:110:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:118:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:155:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:164:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/gobject/GMainLoopSource.cpp:173:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 19 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Carlos Garcia Campos 2014-03-20 00:57:13 PDT
Committed r165952: <http://trac.webkit.org/changeset/165952>