Created attachment 154133 [details] [review]
[gi] Add arg overrides

Adds arg overrides, which is a way to specificy that certain gi types
should use custom create/get and release functions. This is useful
to be able to integrate types from third-party libraries.
It makes no sense to override basic types as they are already handled
perfectly well by gjs itself. This also avoids the performance penalty
since we don't have to check all argument types.

It shall be noted that the arg overrides are registered when a native module is imported. For instance, consider the case of registering an override for cairo.Context/cairo_t:

  const Gdk = imports.gi.Gdk;
  var cairoContext = Gdk.cairo_create(..);

will raise an exception, but:

  const Cairo = imports.cairo;
  const Gdk = imports.gi.Gdk;
  var cairoContext = Gdk.cairo_create(..);

will work just fine.

I think the right solution for this is to create a namespace override, which will
be run when a namespace is registered, eg:

cairo_module_override_func() {
  ... import the 'imports.cairo' module.
}
gjs_ns_override_register(context, "cairo", "1.0", cairo_module_override_func);

Review of attachment 154133 [details] [review]:

It isn't clear to me how this differs from registering a boxed type - when do you use one or the other? Can any GITypeInfo be overridden in this way or only interface types?

::: gi/arg.c
@@ +1650,3 @@
 
+    if (gjs_arg_override_release_g_argument(context, transfer, type_info, arg))
+        return JS_TRUE;

shouldn't this be only if it's an interface type? i.e. we should avoid this lookup on booleans etc.

::: gi/override.c
@@ +76,3 @@
+
+static GjsArgOverride *
+gjs_arg_override_lookup(GITypeInfo *type_info)

if the lookup is always by GITypeInfo, maybe the hash table should be indexed by GITypeInfo? I guess GITypeInfo has no hash identity... maybe it should though, like g_type_info_get_hash_key or something

well maybe a later patch

@@ +95,3 @@
+    canonical[strlen(namespace)] = '.';
+    g_memmove(canonical+strlen(namespace)+1, type_name, strlen(type_name));
+    canonical[strlen(namespace)+1+strlen(type_name)] = '\0';

this is doing manual string operations for speed but then does strlen(namespace) 5 times and strlen(type_name) 3 times, maybe factor these out - will make it more readable too

gjs_ns_override_register seems like manual work that could be automated.

How is Gdk.cairo_create()'s return value annotated in g-i?

I think it would be nice if there were a default association between whatever that annotation is, and gjs native module names.

Maybe we even have a special namespace for types that are "hand-wrapped"?

imports.gicustom.cairo or something, I don't know

Thanks for the comments,

(In reply to comment #3)
> Review of attachment 154133 [details] [review]:
> 
> It isn't clear to me how this differs from registering a boxed type - when do
> you use one or the other? Can any GITypeInfo be overridden in this way or only
> interface types?

It is similar to registering a boxed type but the intended audience is libraries with native gjs bindings that lacks GTypes. Registering boxed types for the cairo structs is trying to make the cairo library fit in the GObject world, while the semantics (memory management, inheritance) are quite different, I tried to do this, (see http://github.com/jdahlin/cairo-glib ) but it didn't really work out well.

> shouldn't this be only if it's an interface type? i.e. we should avoid this
> lookup on booleans etc.

Yeah, leftover, I initially allowed registration of all types, but I gave that
up because it would definitely hurt performance doing a custom type lookup on all arguments in all function calls.

> +static GjsArgOverride *
> +gjs_arg_override_lookup(GITypeInfo *type_info)
> 
> if the lookup is always by GITypeInfo, maybe the hash table should be indexed
> by GITypeInfo? I guess GITypeInfo has no hash identity... maybe it should
> though, like g_type_info_get_hash_key or something
> 
> well maybe a later patch

Yeah, it makes sense to hash by GITypeInfo, I'll investigate and see if it's easy to add hashing for that.

> +    canonical[strlen(namespace)] = '.';
> +    g_memmove(canonical+strlen(namespace)+1, type_name, strlen(type_name));
> +    canonical[strlen(namespace)+1+strlen(type_name)] = '\0';
> 
> this is doing manual string operations for speed but then does
> strlen(namespace) 5 times and strlen(type_name) 3 times, maybe factor these out
> - will make it more readable too

I'd bet my money that the compiler is actually optimizing this for us, but sure it will improve readability, so I'll change this.

(In reply to comment #4)
> gjs_ns_override_register seems like manual work that could be automated.

I'd like to know how :-) Actually gjs_ns_override_register() won't work as you can't put it in the overridden module. It's a chicken and egg problem. Perhaps
a static list inside gjs itself would suffice for now.

> How is Gdk.cairo_create()'s return value annotated in g-i?

It's not annotated at all, that's the best part.
It uses the handwritten cairo gir-file which specifies a "Context" struct mapping to cairo_t in the cairo namespace, see http://git.gnome.org/browse/gobject-introspection/tree/gir/cairo-1.0.gir

> Maybe we even have a special namespace for types that are "hand-wrapped"?
> 
> imports.gicustom.cairo or something, I don't know

The main intention of adding custom overrides is to map gi-types to a natively wrapped library. For instance if we discover that we want to be able to use C APIs that uses DBus types we could map it to the types in gjs dbus bindings.

Review of attachment 154133 [details] [review]:

::: gi/arg.c
@@ +751,3 @@
+                return JS_TRUE;
+                                                       arg_type, transfer, may_be_null, arg))
+            if (gjs_arg_override_convert_to_g_argument(context, value, type_info, arg_name,

This doesn't look right. If an argument (or rather, type?) is overridden but the conversion gets an error, it doesn't seem properly handled.

Shouldn't the overall logic be like:

if (overridden) {
  if (conversion_failed)
    return FALSE;
  return TRUE;
}

::: gi/override.c
@@ +48,3 @@
+JSBool
+gjs_arg_override_register(const char *namespace,
+                          const char *type_name,

Why doesn't this take GIBaseInfo ?

@@ +71,3 @@
+            (GDestroyNotify)gjs_arg_override_value_free);
+    }
+    g_hash_table_insert(arg_overrides_table, canonical_name, override);

Might want to use g_hash_table_replace() or otherwise ensure you don't leak the names if you override the overrides.

@@ +88,3 @@
+    namespace = g_base_info_get_namespace(base_info);
+    type_name = g_base_info_get_name(base_info);
+    g_base_info_unref(base_info);

This doesn't look right, if base_info gets freed because of the unref don't the namespace/type_name strings go with it?

@@ +90,3 @@
+    g_base_info_unref(base_info);
+
+    /* silly optimiazation, since this is rather frequent */

silly typo

Optimization could even be more silly and cache strlens, now it looks half finished :)

I guess GIBaseInfo isn't persistent enough to use the pointer as cache key?

@@ +92,3 @@
+    /* silly optimiazation, since this is rather frequent */
+    canonical = g_alloca(sizeof(char) * strlen(namespace) + 1 + strlen(type_name) + 1);
+    g_memmove(canonical, namespace, strlen(namespace));

there's no overlapping possible so memcpy would suffice

@@ +162,3 @@
+        return JS_FALSE;
+
+    if (!override->release_func(context, transfer, type_info, arg))

release_func is optional in register(), here it's used unconditionally.

I could imagine something like:

* GI_TYPE_TAG_CUSTOM or GI_TYPE_TAG_HANDCODED
* g_custom_info_get_namespace(GICustomInfo*)
* g_custom_info_get_typename(GICustomInfo*)

Then we annotate the return value from Gdk.cairo_create() as 
type "custom" and name "Cairo.Context"

If a custom/handcoded type is present, then a binding must supply handcoded bindings for that type in order to use the type.

This way it's possible to write a program "list all custom bindings I have to write to support GTK" for example, which would walk the introspection data and spit out the custom type names.

Next, in gjs we would import a native module to support custom types. So say we hit the above custom type, gjs would load imports.gicustom.Cairo

imports.gicustom.Cairo would be a hand-coded custom module that would invoke gjs_arg_override_register(), only we'd rename it gjs_custom_register() or something like that.

This way we "solve" (or at least structure) the problem for all bindings.

What isn't clear to me in your plan is what the "correct" way to annotate Gdk.cairo_create() would be, on the GTK level.

gjs would not need any special Cairo knowledge; the cairo module would just install to the gicustom/ directory.

Okay, perhaps parts of this patch should go into gobject-introspection itself, but I honestly think that requiring source level annotations is backwards. 
I mean Gtk/Clutter/Poppler etc shouldn't require annotations saying that this is a custom type. All libraries using a cairo_t* should be treated equally, eg they should work out of the box with no annotations. Moving the location of the custom annotation from callsite (eg Gdk.cairo_create) to the type definition (eg cairo_t ) makes a bit more sense.

Using a custom typelib tag/annotation poses another problem though. For instance the python bindings are rather complete even for libraries which have gobject-introspection support. It would make sense to instead of using gobject-introspection based bindings for say GIO and Gtk you would reuse the old/static ones.

I like the gicustom idea, but we'd obviously have to add two libraries, one to register the native module (cairoNative.so/imports.cairo) and one for the g-i integration (imports.gicustom.Cairo). Ideally I would like to avoid doing a search lookup for custom libraries each time a library is imported, perhaps there's a neat way to avoid the stat() fest, I just can't see it.

Seems I opened a can of worms :-)

right, I'm not sure where the annotation goes. I just think it has to be clear what gdk_cairo_create() should look like in the typelib, and there should be some defined way to recognize that cairo_t requires a handcoded module.

Then each binding can implement mechanisms to go from "I have a thing called cairo_t that must be handcoded" to "the actual handcoded module" - there could be mechanisms to reuse existing old/static bindings, or more standardized methods for new code.

I don't think we need both imports.cairo and imports.gicustom.Cairo ... there are various possibilities here:
 a) imports.gicustom.Cairo is the only one
 b) we just use imports.gi.Cairo and assume custom types won't namespace-collide,
   so Cairo looks like any other gi module from application code
 c) imports.cairo is the only one and we assume custom types won't collide
   with the toplevel namespace
 d) we add some sort of "symlink" type mechanism where imports.gicustom.Cairo 
   somehow just redirects gjs to imports.cairo (maybe literally with a symlink,
   maybe with a module that just loads and then runs some redirect code)
 e) we add some sort of "symlink" type mechanism in the other direction, where
   imports.cairo just loads imports.gicustom.Cairo
 f) to be invented

Here's another half-baked idea. The scanner would generate this when it sees a type it doesn't know about:
 GI_TYPE_TAG_FOREIGN, c_name=cairo_t

and then imports.foreign.cairo_t or something like that would exist, and contain: 
  imports.foreign.cairo_t.preloadModule = 'imports.cairo'
  imports.foreign.cairo_t.jsType = 'imports.cairo.Context'

I don't know if imports.foreign goes with a directory full of modules, or if imports.foreign is just a js file full of hardcoded module names.

To handle a foreign type, gjs would have to load the preloadModule, and then look up the foreign type argument-conversion functions as in your patch, using the jsType name for the lookup.

just brainstorming, I don't know.

A good solution I think would:
 * make very clear the "correct" typelib data that gdk_cairo_create() should have
 * give us an automated way to "list all things that require handcoding in GTK" so you can know if you have 100% support for the stuff GTK uses in your binding

Created attachment 157083 [details] [review]
Add support for foreign structs

Foreign structs are special in the sense that there might
be native bindings (for instance PyCairo for PyGI) that provides
the same functionallity as the introspected variant.

Created attachment 157084 [details] [review]
[cairo] Mark add structs as foreign

Created attachment 157085 [details] [review]
[gi] Add support for foreign structs

Add support for foreign structs, which is a way to specificy that structs
should use custom create/get/release functions. This is useful
to be able to integrate types from third-party libraries.
It makes no sense to override basic types as they are already handled
perfectly well by gjs itself. This also avoids the performance penalty
since we don't have to check all argument types.

Created attachment 157086 [details] [review]
[cairo] Add foreign support for Cairo.Context

Review of attachment 157083 [details] [review]:

This looks fine except for the FIXME

::: girepository/gtypelib.h
@@ +670,3 @@
  * @alignment: The byte boundary that the struct is aligned to in memory
  * @is_gtype_struct: Whether this structure is the class or interface layout for a GObject
+ * @foreign: FIXME

fix!

Review of attachment 157084 [details] [review]:

sure

To write a quick summary of how things will work with this patch applied;

Each time a module is imported a search for all foreign types is done, this is probably a bit of a performance penalty, didn't measure it though.
If a type which is foreign is detected to be used by a library it will look at a hard coded mapping between typelib namespace and native module inside gjs/gi/foreign.c. It'll then import then native module which will register the cairo_t<->JSObject wrapper marshallers.

Review of attachment 157085 [details] [review]:

::: gi/foreign.c
@@ +36,3 @@
+    gboolean loaded;
+} foreign_modules[] = {
+    { "cairo", "cairo", FALSE },

would still be nicer if this could be distributed separately from gjs, but I guess ok for now

@@ +61,3 @@
+            NULL);
+    }
+    g_hash_table_insert(struct_foreigns_table, canonical_name, info);

does this adopt a reference on GjsForeignInfo? would be more normal to add a ref here and make caller unref

@@ +76,3 @@
+                                 g_strdup(g_base_info_get_namespace(interface)),
+                                 interface);
+            return;

just } else { 
  unref(interface);
}

would be a lot more clear, I thought the refcounting was broken at first

@@ +135,3 @@
+                                GIRepository *repository,
+                                const gchar  *namespace)
+{

why do we have to import them all up front? couldn't we just import when we see a foreign struct?

@@ +176,3 @@
+            continue;
+        // FIXME: Find a way to check if a module is imported
+        //        and only execute this statement if isn't

could add a c function to gjs for this that would live in the importer code and take "imports" as argument and return if a module was defined

@@ +180,3 @@
+        if (!gjs_context_eval(JS_GetContextPrivate(context), script, strlen(script),
+                              "<internal>", &code,
+                              &error)) {

log (and free) error?

Review of attachment 157086 [details] [review]:

::: modules/cairo-context.c
@@ +837,3 @@
+        return JS_FALSE;
+    if (transfer == GI_TRANSFER_EVERYTHING)
+        cairo_destroy(cr);

shouldn't this be in release() ?

(In reply to comment #17)
> Review of attachment 157085 [details] [review]:
> 
> ::: gi/foreign.c
> @@ +36,3 @@
> +    gboolean loaded;
> +} foreign_modules[] = {
> +    { "cairo", "cairo", FALSE },
> 
> would still be nicer if this could be distributed separately from gjs, but I
> guess ok for now

Agreed, but a lot of extra work :-)

> @@ +61,3 @@
> +            NULL);
> +    }
> +    g_hash_table_insert(struct_foreigns_table, canonical_name, info);
> 
> does this adopt a reference on GjsForeignInfo? would be more normal to add a
> ref here and make caller unref

Well, GjsForeignInfo is expected to be static & const in most situations, so I'm not sure if it's worth dealing with memory management here. It's not something that's going to be passed around a lot.


> would be a lot more clear, I thought the refcounting was broken at first

That's nuked.

> why do we have to import them all up front? couldn't we just import when we see
> a foreign struct?

Good idea, makes it faster at import time and avoids the complete traversal.
But it adds a small performance penalty when converting cairo arguments.

> 
> @@ +176,3 @@
> +            continue;
> +        // FIXME: Find a way to check if a module is imported
> +        //        and only execute this statement if isn't
> 
> could add a c function to gjs for this that would live in the importer code and
> take "imports" as argument and return if a module was defined

Yeah, that's what I wanted. Needs this in a couple of other places. I'll file a separate bug for this.


> @@ +180,3 @@
> +        if (!gjs_context_eval(JS_GetContextPrivate(context), script,
> strlen(script),
> +                              "<internal>", &code,
> +                              &error)) {
> 
> log (and free) error?

Yup.

(In reply to comment #18)
> Review of attachment 157086 [details] [review]:
> 
> ::: modules/cairo-context.c
> @@ +837,3 @@
> +        return JS_FALSE;
> +    if (transfer == GI_TRANSFER_EVERYTHING)
> +        cairo_destroy(cr);
> 
> shouldn't this be in release() ?

gjs_cairo_context_get_context creates a new reference per default, we're just neutralizing that one when the context is ours.

I pushed all the patches above to both g-i and gjs, with fixes for the comments Havoc suggested.