Created attachment 269653 [details] [review]
gsocket: trivial typo fix

Created attachment 269654 [details] [review]
gsocket: use cancellable child source

Now that GCancellable's GSource is based on _set_ready_time() instead of
an fd, we should use it as a child source, instead of forcing the
creation of the fd and adding it as a poll.

Created attachment 269655 [details] [review]
gsocket: don't abuse GPollFD.revents field

We are reusing the GPollFD.revents field of the source to store a
temporary value.  Use a local variable for that instead.

This is a refactor to make the next commit easier to understand.

Created attachment 269656 [details] [review]
gsocket: make use of g_source_set_ready_time()

Drop our own hand-rolled version of the same functionality.

Created attachment 269657 [details] [review]
gsocket: use _add_unix_fd() instead of _add_poll()

Use g_source_add_unix_fd() on UNIX instead of using a GPollFD.

Created attachment 269658 [details] [review]
gsocket: use check/prepare only on win32

There is no longer any code left in the check/prepare functions on UNIX,
so put %NULL in the GSourceFuncs vtable.

Comment on attachment 269655 [details] [review]
gsocket: don't abuse GPollFD.revents field

>+#else
>+  events = socket_source->pollfd.revents;
> #endif
>   if (socket_source->socket->priv->timed_out)
>     socket_source->pollfd.revents |= socket_source->condition & (G_IO_IN | G_IO_OUT);

the last line needs to be changed to modify events, not pollfd.revents

(and if you want, it doesn't actually need to reference socket_source->condition, since events gets &'ed against that later anyway)

Comment on attachment 269656 [details] [review]
gsocket: make use of g_source_set_ready_time()

>+  timeout = g_source_get_ready_time (source);
>+  if (timeout >= 0 && timeout < g_source_get_time (source))
>+    {
>+      socket->priv->timed_out = TRUE;

This assumes g_source_set_ready_time() will never be used for any other purpose on a GSocketSource... 

I guess g_source_set_ready_time() is only meant to be used from within GSource implementations? You're not allowed to call it on an arbitrary source to cause it to fire? If so, its docs should mention that.

Comment on attachment 269657 [details] [review]
gsocket: use _add_unix_fd() instead of _add_poll()

>@@ -3219,10 +3223,10 @@ socket_source_prepare (GSource *source,
> 
> #ifdef G_OS_WIN32
>   socket_source->pollfd.revents = update_condition (socket_source->socket);
>-#endif
> 
>   if ((socket_source->condition & socket_source->pollfd.revents) != 0)
>     return TRUE;
>+#endif

pollfd.revents is being used as a temporary variable here too

Comment on attachment 269658 [details] [review]
gsocket: use check/prepare only on win32

>   if ((socket_source->condition & socket_source->pollfd.revents) != 0)
>     return TRUE;
>-#endif
> 
>   return FALSE;

can be simplified to just

  return ((socket_source->condition & socket_source->pollfd.revents) != 0);

Created attachment 269853 [details] [review]
gsocket: don't abuse GPollFD.revents field

Good catch!  Fixed.

Created attachment 269854 [details] [review]
GSource: mark some API as "implementation only"

Clarify that _add_poll() _remove_poll() _add_unix_fd(),
_modify_unix_fd(), _remove_unix_fd(), _query_unix_fd() and
_set_ready_time() are only intended to be used by the implementation of
a particular GSource -- not its consumers.

Created attachment 269855 [details] [review]
gsocket: make use of g_source_set_ready_time()

No changes, but rebased.

Created attachment 269856 [details] [review]
gsocket: use _add_unix_fd() instead of _add_poll()

Rebased only -- your suggested changes are in the next commit instead.

Created attachment 269857 [details] [review]
gsocket: use check/prepare only on win32

With the simplified logic as suggested, and also avoiding touching
revents.

Review of attachment 269854 [details] [review]:

Might make sense as well to add this notice for adding and removing child sources.  Probably people outside of a source implementation should not do that...

Attachment 269653 [details] pushed as d8263dd - gsocket: trivial typo fix
Attachment 269654 [details] pushed as ff96f88 - gsocket: use cancellable child source
Attachment 269853 [details] pushed as 1f71005 - gsocket: don't abuse GPollFD.revents field
Attachment 269854 [details] pushed as 12d65f2 - GSource: mark some API as "implementation only"
Attachment 269855 [details] pushed as 04aee2d - gsocket: make use of g_source_set_ready_time()
Attachment 269856 [details] pushed as e8f26ef - gsocket: use _add_unix_fd() instead of _add_poll()
Attachment 269857 [details] pushed as 8da795d - gsocket: use check/prepare only on win32