Bug 130027 – [GLIB] Add GMainLoopSource class to wrap idle and timeout sources

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 226318 [details]
Patch

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

It seems the style checker doesn't like std::function and lambdas.

[Martin Robinson]

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.

[Carlos Garcia Campos]

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

[Martin Robinson]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

(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

[Philippe Normand]

(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!

[Philippe Normand]

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?

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

I don't understand EFL build failures, could it be the compiler version of the EWS bots?

[Carlos Garcia Campos]

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.

[WebKit Commit Bot]

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.

[Raphael Kubo da Costa (:rakuco)]

(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));
  }

[Carlos Garcia Campos]

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

[Csaba Osztrogonác]

(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()>)

[Zan Dobersek]

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.

[Zan Dobersek]

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

[Carlos Garcia Campos]

(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); ?

[Philippe Normand]

What about WTF::callOnMainThread? I think actually that a lot of code in WebCore could use it.

[Carlos Garcia Campos]

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

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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!

[Carlos Garcia Campos]

Created attachment 226675 [details]
Updated patch

Addresses all review comments

[WebKit Commit Bot]

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.

[Zan Dobersek]

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.

[Martin Robinson]

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.

[Martin Robinson]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

(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!

[Carlos Garcia Campos]

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

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Committed r165952: <http://trac.webkit.org/changeset/165952>