Bug 151632 - [GTK] Remove the remaining uses of GMainLoopSource
Summary: [GTK] Remove the remaining uses of GMainLoopSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 151633
  Show dependency treegraph
 
Reported: 2015-11-27 02:40 PST by Carlos Garcia Campos
Modified: 2015-11-27 08:55 PST (History)
7 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2015-11-27 02:42 PST, Carlos Garcia Campos
zan: review+
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 2015-11-27 02:40:03 PST
There are only a few places where we still use GMainLoopSource, once we migrate those, we could remove GMainLoopSource entirely.
Comment 1 Carlos Garcia Campos 2015-11-27 02:42:37 PST
Created attachment 266196 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-27 02:43:14 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Zan Dobersek 2015-11-27 06:00:29 PST
Comment on attachment 266196 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp:714
> +        WTF::GMutexLocker<GMutex>* lock;

This can be a simple reference instead of a pointer.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp:719
> +            auto* context = static_cast<UnlockServerSourceContext*>(userData);

You can dereference the casted user data immediately and bind it to a reference (auto&), instead of a pointer.

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:80
> +        g_source_set_ready_time(timeoutSource(), g_get_monotonic_time() + std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(timeout)).count());

That summation can overflow. It likely won't, but it's possible. Also, using std::chrono also maybe isn't a requirement here, G_USEC_PER_SEC would work just fine.
Comment 4 Zan Dobersek 2015-11-27 06:02:02 PST
Comment on attachment 266196 [details]
Patch

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

>> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:80
>> +        g_source_set_ready_time(timeoutSource(), g_get_monotonic_time() + std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(timeout)).count());
> 
> That summation can overflow. It likely won't, but it's possible. Also, using std::chrono also maybe isn't a requirement here, G_USEC_PER_SEC would work just fine.

The complete overflow protection (the one that roundtrips to std::chrono::microseconds) could be abstracted and put in WTF, in a small header. It's now used in maybe 5 places in the code.

Otherwise the patch looks fine.
Comment 5 Carlos Garcia Campos 2015-11-27 08:33:53 PST
Comment on attachment 266196 [details]
Patch

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

>>> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:80
>>> +        g_source_set_ready_time(timeoutSource(), g_get_monotonic_time() + std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(timeout)).count());
>> 
>> That summation can overflow. It likely won't, but it's possible. Also, using std::chrono also maybe isn't a requirement here, G_USEC_PER_SEC would work just fine.
> 
> The complete overflow protection (the one that roundtrips to std::chrono::microseconds) could be abstracted and put in WTF, in a small header. It's now used in maybe 5 places in the code.
> 
> Otherwise the patch looks fine.

I didn't add the overflow check because here the timeout is a fixed value, not something calculated, but I agree it would be better to move it a common place and use it everywhere else. Thanks for the review.
Comment 6 Carlos Garcia Campos 2015-11-27 08:55:02 PST
Committed r192782: <http://trac.webkit.org/changeset/192782>