Bug 130081

Summary: [GTK] Use GMainLoopSource for idle and timeout sources in WebKit2
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gns, pnormand, svillar
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 130027, 131220, 133348    
Bug Blocks: 132326    
Attachments:
Description Flags
Patch
none
Rebased patch
none
Rebased patch mrobinson: review+

Description Carlos Garcia Campos 2014-03-11 02:52:49 PDT
ssia
Comment 1 Carlos Garcia Campos 2014-03-11 02:54:14 PDT
Created attachment 226408 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-11 02:55:29 PDT
Attachment 226408 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2014-03-21 02:43:26 PDT
Created attachment 227404 [details]
Rebased patch
Comment 4 WebKit Commit Bot 2014-03-21 02:45:19 PDT
Attachment 227404 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Carlos Garcia Campos 2014-05-27 03:25:01 PDT
Created attachment 232121 [details]
Rebased patch
Comment 6 WebKit Commit Bot 2014-05-27 03:28:18 PDT
Attachment 232121 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:119:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Martin Robinson 2014-05-28 06:51:32 PDT
Comment on attachment 232121 [details]
Rebased patch

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

Nice. Just consider fixing the small style nit and the source naming issue that we discussed earlier when landing.

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:89
> +        if (condition & G_IO_HUP || condition & G_IO_ERR || condition & G_IO_NVAL) {
> +            closeFunction();
> +            return GMainLoopSource::Stop;
> +        }

Nit: Maybe one more level of indentation here?

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:98
> +        }, socket.get(), G_IO_IN,

It might make this a little less strange.
Comment 8 Martin Robinson 2014-05-28 06:51:32 PDT
Comment on attachment 232121 [details]
Rebased patch

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

Nice. Just consider fixing the small style nit and the source naming issue that we discussed earlier when landing.

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:89
> +        if (condition & G_IO_HUP || condition & G_IO_ERR || condition & G_IO_NVAL) {
> +            closeFunction();
> +            return GMainLoopSource::Stop;
> +        }

Nit: Maybe one more level of indentation here?

> Source/WebKit2/Platform/gtk/WorkQueueGtk.cpp:98
> +        }, socket.get(), G_IO_IN,

It might make this a little less strange.
Comment 9 Carlos Garcia Campos 2014-05-28 08:35:31 PDT
Committed r169423: <http://trac.webkit.org/changeset/169423>
Comment 10 WebKit Commit Bot 2014-05-28 09:19:55 PDT
Re-opened since this is blocked by bug 133348
Comment 11 Carlos Garcia Campos 2014-05-28 09:22:43 PDT
I'm pretty sure the assert is harmless, I've been using this patch for a long time with release builds and it has never crashed. I think the problem is in GMainLoopSource itself, not in this patch, or simply the assert is wrong. I'll debug it.
Comment 12 Carlos Garcia Campos 2014-05-28 09:23:06 PDT
See also https://bugs.webkit.org/show_bug.cgi?id=131220
Comment 13 Carlos Garcia Campos 2014-05-29 03:33:40 PDT
Committed r169444: <http://trac.webkit.org/changeset/169444>