Created attachment 163759 [details] [review]
[boxed] Make struct_is_simple public

Created attachment 163760 [details] [review]
[boxed] Rename Boxed to GjsBoxedPriv and make public

Created attachment 163761 [details] [review]
[boxed] Add public method gjs_boxed_get_copy_source

Created attachment 163762 [details] [review]
[gi] Add a new function for getting constructor

This figures out which will be the default constructor for
a given type.

Created attachment 163763 [details] [review]
[function] Add an additional raw_argument

Add an additional argument to gjs_invoke_c_function_uncached that
will skip the marshalling of return arguments and instead just
return a raw pointer. This is useful when calling the constructor
of a type so it doesn't recurse and attempt to create a new
javascript wrapper which we'll just throw away.

Created attachment 163764 [details] [review]
[fundamental] Add support for fundamental types

Fundamental types are a mix between object and boxed.
Like a box, but with reference counting, interfaces and
derivation. Most useful practical example is GstMiniObject which
is heavily used in GStreamer.

Created attachment 163831 [details] [review]
[fundamental] Add support for fundamental types

Fundamental types are a mix between object and boxed.
Like a box, but with reference counting, interfaces and
derivation. Most useful practical example is GstMiniObject which
is heavily used in GStreamer.

This version attaches constructors, makes sure construction works,
fixes up finalization, adds new logging category and adds a counters
so we can check refcounting.

Created attachment 163832 [details] [review]
[fundamental] Add support for fundamental types

Fundamental types are a mix between object and boxed.
Like a box, but with reference counting, interfaces and
derivation. Most useful practical example is GstMiniObject which
is heavily used in GStreamer.

(In reply to comment #4)
> Created an attachment (id=163762) [details] [review]
> [gi] Add a new function for getting constructor
> 
> This figures out which will be the default constructor for
> a given type.

This is not really needed and need a bit more work to function properly, I'll retract this.

Review of attachment 163759 [details] [review]:

is_simple seems like a pretty bad name. Maybe is_plain_old_data or is_assignable or _is_data_only or I don't know what. looks fine otherwise.

Review of attachment 163760 [details] [review]:

I'm missing where GjsBoxedPriv is then used in another file - I'm wondering if there's a better way to do whatever is done with it.

::: gi/boxed.c
@@ -55,3 @@
 } BoxedConstructInfo;
 
-static gboolean struct_is_simple(GIStructInfo *info);

removing struct_is_simple is an unrelated change, no?

Review of attachment 163761 [details] [review]:

Makes sense (does this mean GBoxedPriv doesn't need to be public?)

::: gi/boxed.c
@@ +1279,3 @@
+
+    proto_priv = priv_from_js(context, source_proto);
+    if (!boxed_get_copy_source(context, proto_priv, value, &source_priv))

this sets an exception on returning false always, right? (I am just too lazy to look)

Review of attachment 163763 [details] [review]:

Looks good. could this be used to fix https://bugzilla.gnome.org/show_bug.cgi?id=626682 ?

Review of attachment 163832 [details] [review]:

this all looks fine except I didn't have time to read fundamental.c yet in this patch, which is of course all the actual code ;-) I would guess most of it is copied from object.c ?

::: gi/arg.c
@@ +893,3 @@
                         }
                     }
+                    else if (interface_type == GI_INFO_TYPE_OBJECT &&

it's confusing that gobject-introspection uses TYPE_OBJECT for any GTypeInstance but I guess that's a separate problem from this patch

(In reply to comment #14)
> Review of attachment 163832 [details] [review]:
> 
> this all looks fine except I didn't have time to read fundamental.c yet in this
> patch, which is of course all the actual code ;-) I would guess most of it is
> copied from object.c ?

Mostly copied yeah - I used both boxed.c and object.c as inspiration.

> +                    else if (interface_type == GI_INFO_TYPE_OBJECT &&
> 
> it's confusing that gobject-introspection uses TYPE_OBJECT for any
> GTypeInstance but I guess that's a separate problem from this patch

I wrote the patch in g-i to be able to test this in gjs.
Nobody should be using it yet - it's not too late to change, not sure it's worth adding another name, it's still an instantable and derivable object - just not a gobject.

*** Bug 705361 has been marked as a duplicate of this bug. ***

Created attachment 254718 [details] [review]
function: add gjs_invoke_constructor_from_c

Created attachment 254719 [details] [review]
gi: add fundamental type support

I attached a simplified version of the previous patches.

This work is largely based on the previous patches, although updated because of gjs got a lot more complex that it was back in 2010.
Also, I mean simplified because these new patches do not allow JS code to access the fields of the fundamental objects (but that could be added later on). The main reason not do support this, is to avoid dealing with the complexity of cloning proxies of boxed types.

In the end, it probably doesn't matter that much that we can't access fields, because most developers actually use fundamental types to hide the C structure of their objects (like ClutterPaintNode).

I've been using these patches successfully with Clutter and Cogl (+ a few introspection patches to create fundamental patches : https://git.gnome.org/browse/cogl/log/?h=lionel/introspection )
There again, fundamental types are used to hide the internals of the objects manipulated.

My typical Cogl use case :

----------------------------------------------

const Lang = imports.lang;
const Mainloop = imports.mainloop;
const GLib = imports.gi.GLib;
const Cogl = imports.gi.Cogl;

let ctx = new Cogl.Context(null);
let onscreen = new Cogl.Onscreen(ctx, 800, 600);
let cogl_source = Cogl.glib_source_new(ctx, GLib.PRIORITY_DEFAULT);
onscreen.show();

cogl_source.attach(GLib.MainContext.default());


let texture = Cogl.Texture.new_from_file(ARGV[0],
                                         Cogl.TextureFlags.NONE,
                                         Cogl.PixelFormat.ANY);

let pipeline = new Cogl.Pipeline(ctx);
pipeline.set_layer_texture(0, texture);

Mainloop.timeout_add(500, Lang.bind(this, function() {
    onscreen.clear4f(Cogl.BufferBit.COLOR, 0, 0, 0, 1);
    onscreen.draw_rectangle(pipeline, -1, 1, 1, -1);
    onscreen.swap_buffers();
    return true;
}));
Mainloop.run();

Created attachment 255612 [details] [review]
gi: object: add function to check if a JSObject contains a GObject

Just realized I forgot to attach a small patch required by the 2 previous ones.

Created attachment 255613 [details] [review]
function: add gjs_invoke_constructor_from_c

Created attachment 263922 [details] [review]
gi: object: add function to check if a JSObject contains a GObject

Created attachment 263923 [details] [review]
gi: function: add gjs_invoke_constructor_from_c

Created attachment 263925 [details] [review]
gi: add fundamental type support

Created attachment 270059 [details] [review]
gi: add fundamental type support

Update to fix a crash when arguments' types don't match the introspection's signature.

Review of attachment 270059 [details] [review]:

This looks good overall.  Just some very minor comments; feel free to commit after addressing.

::: gi/fundamental.cpp
@@ +59,3 @@
+/* #undef gjs_debug_jsprop */
+/* #endif */
+/* #define gjs_debug_jsprop(arg1,args...) g_message(#arg1 ": " args) */

Is this commented code likely to be used soon?  If not, just delete it?

@@ +89,3 @@
+} FundamentalInstance;
+
+static GHashTable *fundamental_to_js_object = NULL;

I think there should be one of these per GjsContext.

(Yes, we really only support one GjsContext per process...but let's try to contain our use of statics)

@@ +98,3 @@
+_ensure_mapping_table(void)
+{
+    if (G_UNLIKELY(fundamental_to_js_object == NULL))

If you hook this off the GjsContext it won't matter, but for other statics, use g_once_init_enter() please.

Comment on attachment 270059 [details] [review]
gi: add fundamental type support

Pushed with the modifications requested.