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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 11 09:54:22 PDT 2014


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





--- Comment #24 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-03-11 09:51:13 PST ---
(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!

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



More information about the webkit-unassigned mailing list