GNOME Bugzilla – Bug 609121
[dbus] Add getCurrentMessageContext, remove old _dbus_sender
Last modified: 2010-02-08 16:18:06 UTC
The dbus binding had inconsistent and pretty broken support for retrieving the sender of a DBusMessage. The function gjs_js_add_dbus_props, when passed an object, added a _dbus_sender property. However, in many cases it won't be passed an object, such as a service method implementation. There are a number of cases here; the primary ones that we need to support are service methods, signals, and asynchronous callbacks. This patch regresses one case, which is that previously if you did a synchronous call that *also* returned multiple values, you could access _dbus_sender. This patch has no support for synchronous calls, however there is a workaround - convert them to asynchronous.
Created attachment 153105 [details] [review] [dbus] Add getCurrentMessageContext, remove old _dbus_sender
Review of attachment 153105 [details] [review]: ::: modules/dbus.c @@ +100,3 @@ + /* Don't need to take a ref here, if someone forgets to unset the message, + * that's a bug. + */ maybe assert that _gjs_current_dbus_message is null on entry? Does this need to be a stack instead of a single value? hopefully not If we set the current message on the JSContext instead of in a global variable it would be threadsafe ... but maybe there's no way to store anything on the JSContext @@ +1545,3 @@ + if (!JS_DefineProperty(context, context_obj, + "sender", + sender_str ? STRING_TO_JSVAL(sender_str) : JSVAL_VOID, better to just not define the prop if sender is NULL, rather than defining it but setting it to undefined or else define it as null rather than undefined, perhaps (JSVAL_NULL instead of JSVAL_VOID I think is null vs. undefined) @@ +1547,3 @@ + sender_str ? STRING_TO_JSVAL(sender_str) : JSVAL_VOID, + NULL, NULL, + GJS_MODULE_PROP_FLAGS)) GJS_MODULE_PROP_FLAGS is supposed to be for properties of modules, so doesn't make a lot of semantic sense here, even if it happens to be the right value ::: modules/dbus.js @@ +35,3 @@ +// sender (string): The unique connection name of the message sender +function getCurrentMessageContext() { + return imports.dbusNative.currentMessageContext; didn't we also copy currentMessageContext into the module on the line above? Sort of odd to have both getCurrentMessageContext and currentMessageContext (the second one probably doesn't work, either)
Created attachment 153112 [details] [review] [dbus] Add getCurrentMessageContext, remove old _dbus_sender One unforunate thing - we really need some way to document functions defined natively. In the future some documentation extractor that looks for JS_DefineFunction? Should investigate what Mozilla does.
Review of attachment 153112 [details] [review]: Looks good. I think make check probably isn't working. It tests _dbus_sender. Fine to commit if you port the tests over to test the new stuff instead.
Attachment 153112 [details] pushed as ed6ee5f - [dbus] Add getCurrentMessageContext, remove old _dbus_sender