GNOME Bugzilla – Bug 646162
media-player-keys: Port to GDBus
Last modified: 2011-04-05 09:58:20 UTC
.
Created attachment 184631 [details] [review] media-player-keys: Port to GDBus And handle gnome-settings-daemon restarting.
Review of attachment 184631 [details] [review]: ::: src/plugins/media-player-keys/totem-media-player-keys.c @@ +100,3 @@ + return FALSE; + + variant = g_dbus_proxy_call_sync (pi->priv->proxy, Why isn't this async? Surely this could cause a nasty delay on focussing in if the call doesn't complete immediately? @@ +156,3 @@ } + variant = g_dbus_proxy_call_sync (pi->priv->proxy, Perhaps the two GrabMediaPlayerKeys calls could be factored out into a new function? @@ +200,3 @@ + g_object_unref (pi->priv->proxy); + pi->priv->proxy = NULL; + } What if name_vanished_cb() gets called between name_appeared_cb() being called and got_proxy_cb() being called (i.e. proxy==NULL and cancellable!=NULL)? I guess in that case, the call to create the proxy will fail at around the same time and everything will be handled nicely. You can probably ignore this comment then. @@ +256,3 @@ + g_cancellable_cancel (pi->priv->cancellable); + g_object_unref (pi->priv->cancellable); + pi->priv->cancellable = NULL; The cancellable gets unreffed and destroyed in got_proxy_cb(), which should be called as a result of g_cancellable_cancel(); so I think this unref and set-to-null is unnecessary.
Created attachment 184675 [details] [review] media-player-keys: Port to GDBus And handle gnome-settings-daemon restarting.
Review of attachment 184675 [details] [review]: ::: src/plugins/media-player-keys/totem-media-player-keys.c @@ +115,3 @@ + g_variant_new ("(su)", "Totem", 0), + G_DBUS_CALL_FLAGS_NONE, + -1, pi->priv->cancellable, It seems icky to me to re-use the same GCancellable instance for two different D-Bus calls. For example, if one call to GrabMediaPlayerKeys is cancelled, the GCancellable will remain marked as cancelled for all subsequent calls to GrabMediaPlayerKeys, so they'll all fail. Another consequence is that the GCancellable is kept around in memory all the time, even when no D-Bus calls are being made. It also means that the same GCancellable will be used for concurrent calls to GrabMediaPlayerKeys (e.g. if on_window_focus_in_event() gets called twice in quick succession), but due to the design of GCancellable only one of the async calls will be cancelled if the GCancellable is cancelled (it can't be used on parallel asynchronous calls). @@ +205,3 @@ + g_cancellable_cancel (pi->priv->cancellable); + g_object_unref (pi->priv->cancellable); + pi->priv->cancellable = NULL; Continuing from my above point, I suggest that fresh GCancellable instances are used for each async call and that this unref and set-to-null are moved back into the _finish() callbacks for the async calls. That seems to have worked neatly in the YouTube plugin, at least. @@ +265,1 @@ } This is just a hunch, but it might be better to put the cancellable cancellation code before the proxy destroy code, so that if cancellation ends up being a no-op (due to an operation finishing concurrently with g_cancellable_cancel() being called), the resulting proxy instance is still unreffed correctly.
Created attachment 185136 [details] [review] media-player-keys: Port to GDBus And handle gnome-settings-daemon restarting. With help from Philip Withnall.
Attachment 184675 [details] pushed as e116133 - media-player-keys: Port to GDBus
Created attachment 185164 [details] [review] media-player-keys: Fix potential double-unrefs in cancellation handling This should remove the potential double unrefs that I think exist in the plugin. How does it look?
Review of attachment 185164 [details] [review]: Looks good to me.
Comment on attachment 185164 [details] [review] media-player-keys: Fix potential double-unrefs in cancellation handling commit 0bf512defe6a547187c91faa9c6860e845eba0a0 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Apr 5 01:44:38 2011 +0100 media-player-keys: Fix potential double-unrefs in cancellation handling .../media-player-keys/totem-media-player-keys.c | 44 ++++++++++++-------- 1 files changed, 26 insertions(+), 18 deletions(-)