It uses dbus-glib just for the mainloop integration.

I think we would like to keep support for older glib for a little while at least, so the reliance on gdbus should be optional

gdbus will be a total rewrite, since gjs is using libdbus directly for the most part.

It might be better to bind gdbus through gobject-introspection, and then if desired do the nicer custom dbus API that gjs has entirely in JavaScript. Some of the custom stuff in gjs would be obsoleted by similar gdbus APIs probably, such as name watching.

Basically I'd look at this as a new dbus API based on gdbus g-i bindings, rather than porting the current dbus API.

Note using gdbus would depend on gvariant bindings for gjs.

Created attachment 186461 [details] [review]
Add a JS GDBus convenience layer

This only covers the proxy (client) side, for the skeleton (server)
side we still need glue code (but it is easy to generate it using
gdbus-binding-tool, for example)

Created attachment 186525 [details] [review]
Complete porting to GDBus

Add a GjsDBusImplementation, which is just a standalone, signal-driven
GDBusInterfaceSkeleton (from gdbus-codegen branch in glib) to libgjs-dbus.
This is exposed, thanks to overrides, as Gio.DBusExportedObject,
which also gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Created attachment 186526 [details] [review]
Add GDBus example

Shows how to access GDBusConnection objects, to create GDBusInterfaceInfo,
to create GDBusProxy and to implement/export GDBusExportedObjects
(GjsDBusImplementation, actually)

Created attachment 186527 [details] [review]
Port DBus tests to GDBus

Don't work very well, because GDBus doesn't seem to support talking
to himself, but should pass.

Created attachment 186624 [details] [review]
Complete porting to GDBus

Add a GjsDBusImplementation, which is just a standalone, signal-driven
GDBusInterfaceSkeleton (from gdbus-codegen branch in glib) to libgjs-dbus.
This is exposed, thanks to overrides, as Gio.DBusExportedObject,
which also gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Review of attachment 186461 [details] [review]:

::: modules/overrides/GLib.js
@@ +21,3 @@
 
+var GLib;
+var originalVariantClass;

Hm; why does this patch go back from let->var?  I'm actually in favor of doing this in the long term, but not this cycle probably.

Comment on attachment 186461 [details] [review]
Add a JS GDBus convenience layer

Attachment 186461 [details] pushed as b3efadd - Add a JS GDBus convenience layer

Comment on attachment 186461 [details] [review]
Add a JS GDBus convenience layer

I dropped the let->var conversion and pushed this.

Does the client-side support D-Bus properties?

One of the convenient things about GDBusProxy is that it does this - including support for the (recently added) PropertiesChanged signal. It makes it a lot easier to work with (well-written [1]) D-Bus services since you don't have to worry about async issues as all properties are loaded as part of object construction.

Oh, while we're at it, it would also be nice with support for the recently added ObjectManager interface :-) - maybe the GIO types for this (GDBusObjectManager{,Client,Server}), see

 http://developer.gnome.org/gio/unstable/GDBusObjectManager.html
 http://developer.gnome.org/gio/unstable/GDBusObjectManagerClient.html
 http://developer.gnome.org/gio/unstable/GDBusObjectManagerServer.html

are good enough for this? I guess GDBusProxyTypeFunc isn't really that bindable...

[1] : That is, D-Bus services that actually use D-Bus properties and the PropertiesChanged signal. Sadly, many programmers still add a GetXYZ() methods instead of making it a property. And others roll their own properties interface...

(In reply to comment #13)
> Does the client-side support D-Bus properties?
> 
> One of the convenient things about GDBusProxy is that it does this - including
> support for the (recently added) PropertiesChanged signal. It makes it a lot
> easier to work with (well-written [1]) D-Bus services since you don't have to
> worry about async issues as all properties are loaded as part of object
> construction.

Yes, because it internally creates a GDBusProxy and monkey patches it with connectSignal / disconnectSignal for connecting indiviual signals, and MethodName functions for directly calling.
(It would be easier if ::g-signal was detailed with the signal name instead)

It still has the problem that you cannot use Gio.DBusProxy.new and Gio.DBusProxy.new_sync, instead you must manually call proxy.init or proxy.async_init. A proper solution for that would involve GObject inheritance from JS code and dynamic class generation.

(In reply to comment #14)
> (In reply to comment #13)
> > Does the client-side support D-Bus properties?
> > 
> > One of the convenient things about GDBusProxy is that it does this - including
> > support for the (recently added) PropertiesChanged signal. It makes it a lot
> > easier to work with (well-written [1]) D-Bus services since you don't have to
> > worry about async issues as all properties are loaded as part of object
> > construction.
> 
> Yes, because it internally creates a GDBusProxy and monkey patches it with
> connectSignal / disconnectSignal for connecting indiviual signals, and
> MethodName functions for directly calling.

So, yeah, I get the signal and method bits from your patch. But I was asking about properties - it would be nice if the D-Bus properties was available as properties on the JS object that represents the proxy.

Here's an example of a D-Bus object with two interfaces that has properties:

$ gdbus introspect --system --dest org.freedesktop.UDisks2 --object-path /org/freedesktop/UDisks2/block_devices/mmcblk0p1 --only-properties 
node /org/freedesktop/UDisks2/block_devices/mmcblk0p1 {
  interface org.freedesktop.UDisks2.Filesystem {
    properties:
      readonly aay MountPoints = [];
  };
  interface org.freedesktop.UDisks2.BlockDevice {
    properties:
      readonly ay Device = b'/dev/mmcblk0p1';
      readonly aay Symlinks = [b'/dev/disk/by-id/mmc-SD04G_0x50261705-part1', b'/dev/disk/by-path/pci-0000:05:00.2-part1', b'/dev/disk/by-uuid/4DC8-A870', b'/dev/disk/by-label/FatStuff'];
      readonly x Major = 179;
      readonly x Minor = 1;
      readonly t Size = 1999986688;
      readonly ay PreferredDevice = b'/dev/mmcblk0p1';
      readonly o Drive = '/org/freedesktop/UDisks2/drives/SD04G_0x50261705';
      readonly s IdUsage = 'filesystem';
      readonly s IdType = 'vfat';
      readonly s IdVersion = 'FAT32';
      readonly ay IdLabel = b'FatStuff';
      readonly ay IdUUID = b'4DC8-A870';
      readonly b PartTable = false;
      readonly s PartTableScheme = '';
      readonly b PartEntry = true;
      readonly u PartEntryNumber = 1;
      readonly s PartEntryScheme = 'mbr';
      readonly s PartEntryType = '0x0c';
      readonly s PartEntryFlags = '';
      readonly t PartEntryOffset = 8192;
      readonly t PartEntrySize = 1999986688;
      readonly ay PartEntryLabel = b'';
      readonly ay PartEntryUUID = b'';
      readonly o PartEntryTable = '/org/freedesktop/UDisks2/block_devices/mmcblk0';
      readonly o CryptoBackingDevice = '/';
      readonly ay LoopBackingFile = b'';
  };
};

Comment on attachment 186461 [details] [review]
Add a JS GDBus convenience layer

I dropped the let->var conversion and pushed this.

Review of attachment 186624 [details] [review]:

I'm not a big fan of E4X (and neither are the SpiderMonkey maintainers mostly).

Is there a reason we need to remove the old dbus bindings at the same time as adding gdbus?

::: Makefile-gjs-dbus.am
@@ +34,3 @@
+GjsDBus_1_0_gir_INCLUDES = GObject-2.0 Gio-2.0
+GjsDBus_1_0_gir_FILES = gjs-dbus/dbus.h gjs-dbus/dbus.c
+GjsDBus_1_0_gir_SCANNERFLAGS = --identifier-prefix=GjsDBus --symbol-prefix=gjs_dbus --warn-all

It might make sense to structure this as a generic private .gir/.typelib for gjs, rather than being specific to GjsDBus.

(I'd like to use this to move some other current hand-written modules to be based on g-i, like gettext).

::: gi/ns.c
@@ +119,2 @@
             */
+            ret = JS_TRUE;

We did this bit in another patch.

::: gi/repo.c
@@ +115,3 @@
+                           NULL, NULL,
+                           GJS_MODULE_PROP_FLAGS))
+        gjs_fatal("no memory to define ns property");

This should be a separate patch.

::: modules/lang.js
@@ +115,3 @@
 }
 
+function defineAccessorProperty(object, name, getter, setter) {

Hmm...ugly =(   Oh well, we can add it to list of things to kill after we hard require SpiderMonkey >= 1.8.5

::: modules/overrides/Gio.js
@@ +104,3 @@
+
+	// silence a warning
+	return undefined;

Oh, I fixed this already.

(In reply to comment #15)
> [...]
> 
> So, yeah, I get the signal and method bits from your patch. But I was asking
> about properties - it would be nice if the D-Bus properties was available as
> properties on the JS object that represents the proxy.
> 
> Here's an example of a D-Bus object with two interfaces that has properties:
> 
> [...]

What I meant is that it is a full GDBusProxy and we use what it has for properties (g_dbus_proxy_get_cached_property), so getting is possible, as well as receiving property notifications (g-properties-changed). Setting is not possible, but that seems a Gio problem.

But actually I forgot what the second patch adds, which is accessor properties mapping dbus properties, and GetRemote / SetRemote invocations, for async getting and setting.

(In reply to comment #17)
> Review of attachment 186624 [details] [review]:
> 
> I'm not a big fan of E4X (and neither are the SpiderMonkey maintainers mostly).
> 
> Is there a reason we need to remove the old dbus bindings at the same time as
> adding gdbus?

Well, they don't share the same connection, for one, so you cannot use dbus-glib and gdbus at the same time. This is the very reason I wrote this: I need to port the shell to GDBus for GApplication.

> ::: Makefile-gjs-dbus.am
> @@ +34,3 @@
> +GjsDBus_1_0_gir_INCLUDES = GObject-2.0 Gio-2.0
> +GjsDBus_1_0_gir_FILES = gjs-dbus/dbus.h gjs-dbus/dbus.c
> +GjsDBus_1_0_gir_SCANNERFLAGS = --identifier-prefix=GjsDBus
> --symbol-prefix=gjs_dbus --warn-all
> 
> It might make sense to structure this as a generic private .gir/.typelib for
> gjs, rather than being specific to GjsDBus.
> 
> (I'd like to use this to move some other current hand-written modules to be
> based on g-i, like gettext).

Uhm...
Mainloop could be in GLib (once we have GMainLoop as a boxed).
Gettext is provided by GLib, just needs some overrides to do domain binding.
Lang should be deprecated by EcmaScript 5 / JS 1.8.
Console and Debugger can't be made introspected (they use JS API).
Anything else?

> ::: gi/repo.c
> @@ +115,3 @@
> +                           NULL, NULL,
> +                           GJS_MODULE_PROP_FLAGS))
> +        gjs_fatal("no memory to define ns property");
> 
> This should be a separate patch.

Ok

> ::: modules/lang.js
> @@ +115,3 @@
>  }
> 
> +function defineAccessorProperty(object, name, getter, setter) {
> 
> Hmm...ugly =(   Oh well, we can add it to list of things to kill after we hard
> require SpiderMonkey >= 1.8.5

Like the whole Lang module, I guess.

(In reply to comment #18)
> 
> (In reply to comment #17)
> > Review of attachment 186624 [details] [review] [details]:
> > Is there a reason we need to remove the old dbus bindings at the same time as
> > adding gdbus?
> 
> Well, they don't share the same connection, for one, so you cannot use
> dbus-glib and gdbus at the same time. This is the very reason I wrote this: I
> need to port the shell to GDBus for GApplication.

Absolutely, and I really appreciate that!  It'll be super nice to be using GDBus.  However, for client code (think say telepathy-glib) we can't rebase off of dbus-glib yet. 

All your patch for telepathyGLib.js in the shell does is:

-const DBus = imports.dbus;

And do we really need to do that right now?  Basically while I know the current dbus code is crufty and ugly, let's just keep it alive for a while longer since it can't be too hard to do?

(In reply to comment #18)
> (In reply to comment #15)
> > [...]
> > 
> > So, yeah, I get the signal and method bits from your patch. But I was asking
> > about properties - it would be nice if the D-Bus properties was available as
> > properties on the JS object that represents the proxy.
> > 
> > Here's an example of a D-Bus object with two interfaces that has properties:
> > 
> > [...]
> 
> What I meant is that it is a full GDBusProxy and we use what it has for
> properties (g_dbus_proxy_get_cached_property), so getting is possible, as well
> as receiving property notifications (g-properties-changed). Setting is not
> possible, but that seems a Gio problem.

It's not a problem, it's just how it's designed. Higher-level code, such as the code generated by gdbus-codegen(1), allows setting properties. In particular, that code in generated so setting isn't some async operation that requires a callback - the docs at http://developer.gnome.org/gio/unstable/gdbus-codegen.html say

  Note that all property access is via GDBusProxy's property cache
  so no I/O is ever done when reading properties. Also note that
  setting a property will cause the org.freedesktop.DBus.Properties.Set
  method to be called on the remote object. This call, however, is
  asynchronous so setting a property won't block. Further, the change
  is delayed and no error checking is possible.

This is very much on purpose - see below for more discussion

> But actually I forgot what the second patch adds, which is accessor properties
> mapping dbus properties, and GetRemote / SetRemote invocations, for async
> getting and setting.

Why would you want async getting? The whole point of GDBusProxy (I'd even say: the goal of D-Bus properties) is to enable a programming model where properties are cached in each client. You definitely do not want the overhead of sending a D-Bus message every time you read a property - that's just way too expensive.

FWIW, I don't think you want async setting either - if you assume that setting a property never fails (which is a fine assumption), then you don't have to make it async. For the few cases where setting a property can fail, you can manually call into the org.freedesktop.DBus.Properties interface.

Thanks,
David

(In reply to comment #19)
> (In reply to comment #18)
> > 
> > (In reply to comment #17)
> > > Review of attachment 186624 [details] [review] [details] [details]:
> > > Is there a reason we need to remove the old dbus bindings at the same time as
> > > adding gdbus?
> > 
> > Well, they don't share the same connection, for one, so you cannot use
> > dbus-glib and gdbus at the same time. This is the very reason I wrote this: I
> > need to port the shell to GDBus for GApplication.
> 
> Absolutely, and I really appreciate that!  It'll be super nice to be using
> GDBus.  However, for client code (think say telepathy-glib) we can't rebase off
> of dbus-glib yet.

That's a different story, I think If telepathy-glib (or libnm-glib, or libgdmuser, or even gvfs) use dbus-glib that's less of a problem, because those libraries are like black boxes, and DBus is an implementation detail for them. (sort of big detail, in case of telepathy, but still not relevant for the library user).
On the other hand, if NotificationDaemon keeps using imports.dbus while libgnome-shell is ported to GDBus, org.freedesktop.Notification will be claimed on the wrong connection and notifications won't be shown. Same applies for org.gnome.Shell.

> All your patch for telepathyGLib.js in the shell does is:
> 
> -const DBus = imports.dbus;
> 
> And do we really need to do that right now?  Basically while I know the current
> dbus code is crufty and ugly, let's just keep it alive for a while longer since
> it can't be too hard to do?

You need to move all JS code, atomically, once you port name owning code from dbus-glib to GDBus, because it exports objects and wants them on the right connection.
That doesn't necessarily mean that gjs can't export two dbus modules for a little while, although I would prefer to have just one.

(In reply to comment #20)
> > [...]
> > 
> > What I meant is that it is a full GDBusProxy and we use what it has for
> > properties (g_dbus_proxy_get_cached_property), so getting is possible, as well
> > as receiving property notifications (g-properties-changed). Setting is not
> > possible, but that seems a Gio problem.
> 
> It's not a problem, it's just how it's designed. Higher-level code, such as the
> code generated by gdbus-codegen(1), allows setting properties. In particular,
> that code in generated so setting isn't some async operation that requires a
> callback - the docs at
> http://developer.gnome.org/gio/unstable/gdbus-codegen.html say
> 
>   Note that all property access is via GDBusProxy's property cache
>   so no I/O is ever done when reading properties. Also note that
>   setting a property will cause the org.freedesktop.DBus.Properties.Set
>   method to be called on the remote object. This call, however, is
>   asynchronous so setting a property won't block. Further, the change
>   is delayed and no error checking is possible.
> 
> This is very much on purpose - see below for more discussion

So the accessor properties should just invoke org.freedesktop.DBus.Properties.Set with a dummy callback and forget about the rest? Ok then.

> > But actually I forgot what the second patch adds, which is accessor properties
> > mapping dbus properties, and GetRemote / SetRemote invocations, for async
> > getting and setting.
> 
> Why would you want async getting? The whole point of GDBusProxy (I'd even say:
> the goal of D-Bus properties) is to enable a programming model where properties
> are cached in each client. You definitely do not want the overhead of sending a
> D-Bus message every time you read a property - that's just way too expensive.
> 
> FWIW, I don't think you want async setting either - if you assume that setting
> a property never fails (which is a fine assumption), then you don't have to
> make it async. For the few cases where setting a property can fail, you can
> manually call into the org.freedesktop.DBus.Properties interface.

Ok, I'll remove both async getting and setting.

(In reply to comment #21)
> So the accessor properties should just invoke
> org.freedesktop.DBus.Properties.Set with a dummy callback and forget about the
> rest? Ok then.

I think it would probably nice to warn the user if setting does fail - gdbus-codegen(1) generates this code for the callback

 static void
 foo_bar_proxy_set_property_cb (GDBusProxy *proxy,
   GAsyncResult *res,
   gpointer      user_data)
 {
   const _ExtendedGDBusPropertyInfo *info = user_data;
   GError *error;
   error = NULL;
   if (!g_dbus_proxy_call_finish (proxy, res, &error))
     {
       g_warning ("Error setting property `%s' on interface org.project.Bar: %s (%s, %d)",
                  info->parent_struct.name, 
                  error->message, g_quark_to_string (error->domain), error->code);
       g_error_free (error);
    }
 }

Btw, there is also the question of how to notify when a D-Bus property changes. Can you still do it the GObject way, by emitting a "notify::DBusPropertyName" signal even if the JS proxy object isn't a GObject? Or is there a better way to do this?

(In reply to comment #22)
> (In reply to comment #21)
> > So the accessor properties should just invoke
> > org.freedesktop.DBus.Properties.Set with a dummy callback and forget about the
> > rest? Ok then.
> 
> I think it would probably nice to warn the user if setting does fail -
> gdbus-codegen(1) generates this code for the callback
> 
>  static void
>  foo_bar_proxy_set_property_cb (GDBusProxy *proxy,
>    GAsyncResult *res,
>    gpointer      user_data)
>  {
>    const _ExtendedGDBusPropertyInfo *info = user_data;
>    GError *error;
>    error = NULL;
>    if (!g_dbus_proxy_call_finish (proxy, res, &error))
>      {
>        g_warning ("Error setting property `%s' on interface org.project.Bar: %s
> (%s, %d)",
>                   info->parent_struct.name, 
>                   error->message, g_quark_to_string (error->domain),
> error->code);
>        g_error_free (error);
>     }
>  }

Ok, let's go for the warning then.

(In reply to comment #23)
> Btw, there is also the question of how to notify when a D-Bus property changes.
> Can you still do it the GObject way, by emitting a "notify::DBusPropertyName"
> signal even if the JS proxy object isn't a GObject? Or is there a better way to
> do this?

I could emit a notify signal, with details and everything, but I would need a GParamSpec for the property (which of course I don't have). I don't know how well GObject would react to use a dummy paramspec, so I think that g-properties-changed is enough for now. It is still an improvement from current situation, though.

Created attachment 190462 [details] [review]
Complete porting to GDBus

Introduces a new introspected library, libgjs-gdbus, which hosts
GjsDBusImplementation, a standalone, signal-driven GDBusInterfaceSkeleton.
This is exposed, thanks to overrides, as Gio.DBusExportedObject, where
it gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Created attachment 190463 [details] [review]
GIRepositoryNamespace: define properties early to avoid reentrancy

It could happen that an override module requires a class which
inherits from another defined in the overridden module. In the case,
defining the imported class would reimport the namespace and obtain
an undefined object, causing an invalid prototype.

Review of attachment 190463 [details] [review]:

Sounds good.

Review of attachment 190462 [details] [review]:

A few quick comments.

::: configure.ac
@@ +218,3 @@
 CFLAGS="$save_CFLAGS"
 
+GOBJECT_INTROSPECTION_CHECK([0.9.6])

Should really be 1.29.0 nowadays.

::: gi/repo.c
@@ +209,3 @@
     priv = g_slice_new0(Repo);
 
+    g_irepository_prepend_search_path(PKGLIBDIR);

Separate patch, and I don't think this is right to do here.  Probably around line 672 in context.c, right after we initialize gi there?

::: gjs-dbus/gdbus.c
@@ +7,3 @@
+
+#include <string.h>
+#include <stdlib.h>

What needs stdlib.h here?

::: gjs-dbus/gdbus.h
@@ +1,1 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */

I don't like calling this gdbus.h.  How about gjs-gdbus-wrapper.h ?

::: modules/overrides/Gio.js
@@ +185,3 @@
+    var iname = info.name;
+    return function(bus, name, object, asyncCallback, cancellable) {
+	var obj = new Gio.DBusProxy({ g_connection: Gio.DBus.session,

Shouldn't hardcode the session bus.

(In reply to comment #28)
>
> +    g_irepository_prepend_search_path(PKGLIBDIR);
> 
> Separate patch, and I don't think this is right to do here.  Probably around
> line 672 in context.c, right after we initialize gi there?

Nevermind, it can go in this patch; it's necessary for the GjsDBus stuff which is in the same patch.  But I'd still prefer it to be in context.c (and also it could use a comment like:  /* For GjsDBus */ )

Comment on attachment 190463 [details] [review]
GIRepositoryNamespace: define properties early to avoid reentrancy

Attachment 190463 [details] pushed as 327399a - GIRepositoryNamespace: define properties early to avoid reentrancy

Created attachment 190533 [details] [review]
Complete porting to GDBus

Introduces a new introspected library, libgjs-gdbus, which hosts
GjsDBusImplementation, a standalone, signal-driven GDBusInterfaceSkeleton.
This is exposed, thanks to overrides, as Gio.DBusExportedObject, where
it gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Review of attachment 190533 [details] [review]:

Ok - I think I'd like to see this go in with some test cases.  This would help me better understand how the wrapper C class works.  Your previous patch ported over the old dbus.js tests, but in the new model, we keep them around.  So I think we'll need test/js/testGDbus.js then.

::: gjs-dbus/gjs-gdbus-wrapper.c
@@ +1,2 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/* Copyright 2008 litl, LLC. All Rights Reserved. */

This is your code isn't it?

::: modules/lang.js
@@ +103,3 @@
 
+    if (callback.bind && arguments.length == 2) // ECMAScript 5 (but only if not passing any bindArguments)
+	return callback.bind(obj);

Optional but it might be nice to split this to a separate patch; better for the release notes.  Just something like:

Lang.bind(): Use standard Function.bind() if available

(And actually, I'd like to grep gnome-shell sources for Lang.bind() which also gives arguments, and make them stop doing that).

@@ +115,3 @@
 }
 
+function defineAccessorProperty(object, name, getter, setter) {

Similar:

Lang.defineAccessorProperty(): New API which wraps Object.defineProperty

::: test/js/testJSDefault.js
@@ -3,3 @@
 // application/javascript;version=ECMAv3
 
-var GLib = imports.gi.GLib;

Want to just go ahead and push this fix?

Created attachment 191060 [details] [review]
Add GDBus example

Shows how to access GDBusConnection objects, to create GDBusInterfaceInfo,
to create GDBusProxy and to implement/export GDBusExportedObjects
(GjsDBusImplementation, actually)

Refactored to have more comments and be like a real example

Created attachment 191065 [details] [review]
Complete porting to GDBus

Introduces a new introspected library, libgjs-gdbus, which hosts
GjsDBusImplementation, a standalone, signal-driven GDBusInterfaceSkeleton.
This is exposed, thanks to overrides, as Gio.DBusExportedObject, where
it gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Some bugs fixed in the async paths

Created attachment 191066 [details] [review]
Add GDBus example

Shows how to access GDBusConnection objects, to create GDBusInterfaceInfo,
to create GDBusProxy and to implement/export GDBusExportedObjects
(GjsDBusImplementation, actually)

Fixed example of async proxy.

Created attachment 191067 [details] [review]
Port DBus tests to GDBus

Make a copy of DBus tests and port them to GDBus. Also rename
the old tests for consistency.

Created attachment 191068 [details] [review]
gdbusintrospection: fix introspection annotations

g_dbus_interface_info_lookup_* were incorrectly considered
(transfer full) by introspected bindings, and this caused memory
corruptions.

Created attachment 191093 [details] [review]
Complete porting to GDBus

Introduces a new introspected library, libgjs-gdbus, which hosts
GjsDBusImplementation, a standalone, signal-driven GDBusInterfaceSkeleton.
This is exposed, thanks to overrides, as Gio.DBusExportedObject, where
it gains some JS code to expose normal JS objects on the bus
and some glue to use E4X in place of XML strings for introspection.

Forgot to fix copyright notices

Review of attachment 191068 [details] [review]:

Looks obviously fine.

Review of attachment 191068 [details] [review]:

Committed.

Review of attachment 191093 [details] [review]:

One API issue; otherwise, I think this is OK.

::: modules/overrides/Gio.js
@@ +177,3 @@
+	Lang.defineAccessorProperty(this, name,
+				    Lang.bind(this, _propertyGetter, name),
+				    Lang.bind(this, _propertySetter, name, signature));

There's a possible namespace collision issue here, similar to the GVariant one:

https://bugzilla.gnome.org/show_bug.cgi?id=622344#c22

We may want to use have a .dbusProperties object or something?

Review of attachment 191067 [details] [review]:

Need to update Makefile.am too.

::: modules/overrides/Gio.js
@@ +286,3 @@
 		}
 		invocation.return_dbus_error(e.name, e.message);
+		return;

Should be rolled into the porting commit.

Review of attachment 191066 [details] [review]:

Need to update Makefile.am, otherwise looks fine.

Getting the following from "make valgrind-check", investigating.

==2497== Invalid read of size 4
==2497==    at 0x6002DB1: g_dbus_property_info_unref (gdbusintrospection.c:358)
==2497==    by 0x60022E5: free_null_terminated_array (gdbusintrospection.c:244)
==2497==    by 0x6002D88: g_dbus_interface_info_unref (gdbusintrospection.c:389)
==2497==    by 0x5FEB325: exported_interface_free (gdbusconnection.c:3682)
==2497==    by 0x571491B: g_hash_table_remove_internal (ghash.c:1175)
==2497==    by 0x5FF34BD: g_dbus_connection_unregister_object (gdbusconnection.c:4800)
==2497==    by 0x6008E5E: g_dbus_interface_skeleton_unexport (gdbusinterfaceskeleton.c:704)
==2497==    by 0x6008FE4: g_dbus_interface_skeleton_finalize (gdbusinterfaceskeleton.c:81)
==2497==    by 0x52861BC: g_object_unref (gobject.c:2746)
==2497==    by 0x4C2EECA: ??? (object.c:790)
==2497==    by 0x5AB66A6: JSCompartment::finalizeObjectArenaLists(JSContext*) (in /src/build/jhbuild/lib64/libmozjs185.so.1.0.0)
==2497==    by 0x5AB9547: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind) (in /src/build/jhbuild/lib64/libmozjs185.so.1.0.0)
==2497==  Address 0xd367580 is 0 bytes inside a block of size 40 free'd
==2497==    at 0x4A055FE: free (vg_replace_malloc.c:366)
==2497==    by 0x572D012: g_free (gmem.c:263)
==2497==    by 0x528192A: g_boxed_free (gboxed.c:391)
==2497==    by 0x4C299C9: ??? (boxed.c:611)
==2497==    by 0x5AB642E: JSCompartment::finalizeObjectArenaLists(JSContext*) (in /src/build/jhbuild/lib64/libmozjs185.so.1.0.0)
==2497==    by 0x5AB9547: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind) (in /src/build/jhbuild/lib64/libmozjs185.so.1.0.0)
==2497==    by 0x5A72E0E: js_DestroyContext(JSContext*, JSDestroyContextMode) (in /src/build/jhbuild/lib64/libmozjs185.so.1.0.0)
==2497==    by 0x4C1CB99: ??? (context.c:364)
==2497==    by 0x528613A: g_object_unref (gobject.c:2709)
==2497==    by 0x401B27: teardown (gjs-unit.c:83)
==2497==    by 0x574A43E: g_test_run_suite_internal (gtestutils.c:1237)
==2497==    by 0x574A565: g_test_run_suite_internal (gtestutils.c:1291)

(In reply to comment #44)
> Getting the following from "make valgrind-check", investigating.

Ok, that was because I hadn't pulled the new glib annotations into g-i.  Fixed now.

Ok, let's just go with this; we can gain further experience with the system.

One other concern I have is how we're calling .deep_unpack() internally.  While this clearly optimizes for convenience of usage, in practice it may happen that one wants to know exactly *what* type one got over dbus (for example, was that entry in an a{sv} an integer or double?).

Attachment 191067 [details] pushed as 81cfc7d - Port DBus tests to GDBus
Attachment 191093 [details] pushed as e5b6e96 - Complete porting to GDBus

(In reply to comment #46)
> Ok, let's just go with this; we can gain further experience with the system.
> 
> One other concern I have is how we're calling .deep_unpack() internally.  While
> this clearly optimizes for convenience of usage, in practice it may happen that
> one wants to know exactly *what* type one got over dbus (for example, was that
> entry in an a{sv} an integer or double?).

For an a{sv}, unpack returns an array of {sv} variants, deep_unpack an object keyed to the string and whose values are the actual variants (not the "v" typed variants). In both cases, the caller has to unpack them explicitly.