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

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from Zan Dobersek <zandobersek at gmail.com>  2014-03-11 09:33:37 PST ---
(From update of attachment 226403)
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.

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