Created attachment 243461 [details] [review]
object: fix test suite after g_object_newv changes

Properties are now set in GObject class registration order, so
if we derive twice and install the js-context and js-object properties
twice, custom properties in the parent class get set before
js-context and js-object, which causes a crash.
Fix that by not installing the implementation properties in double
derived classes.

Review of attachment 243461 [details] [review]:

Blah.  I don't like these properties at all.  I'm inclined to just burn support for multiple GjsContext per process.  It sounds nice, but because of how toggle refs work, there can only be one GjsContext connecting to per-process shared infrastructure like the session GDBusConnection.  So we could just have gjs_context_get_default() -> gjs_context_get_native_context().

Instead of the js-object prop, could we have an internal API like GjsPrivate.set_constructing_object(this); that just sets a static variable, and then just grab the value inside a constructor for gjs object?

Anyways, gjs tests seem to pass at the moment.  Was this fixed by something else?

(In reply to comment #2)
> Review of attachment 243461 [details] [review]:
> 
> Blah.  I don't like these properties at all.  I'm inclined to just burn support
> for multiple GjsContext per process.  It sounds nice, but because of how toggle
> refs work, there can only be one GjsContext connecting to per-process shared
> infrastructure like the session GDBusConnection.  So we could just have
> gjs_context_get_default() -> gjs_context_get_native_context().
> 
> Instead of the js-object prop, could we have an internal API like
> GjsPrivate.set_constructing_object(this); that just sets a static variable, and
> then just grab the value inside a constructor for gjs object?

I tried implementing this. There is no problem with the context, but stuff breaks when you associate the JS object with the GObject in instance_init, because init is called once for each level of JS class derivation, but you only have one object.
I don't want to break the assumption that associate_js_gobject is only called on disassociated JS objects.
Also, I was trying to make the thing fully reentrant, so that we could call out to custom JS code from instance_init(), to support Gtk widget templates.

For now, I'm going with the ugly hack of only doing work in the most derived instance_init(), which we can find because we know from the JS object the final GType and we can just cross-reference it with what we get from instance_init().

Created attachment 244253 [details] [review]
GjsContext: add the concept of "current context"

Just a static global variable holding the GjsContext. There should
be at most once context at any time, so any new GjsContext is
immediately made current.
This will be used to avoid passing a JSContext where we can't easily.

Created attachment 244254 [details] [review]
object: stop using GObject properties to set up context and instance for derived classes

It was an ugly hack, that would easily break with derived classes.
Instead, use a global stack of constructing objects, that is
pushed when building the JS object and retrieved in instance_init()

Created attachment 244255 [details] [review]
object: add a custom hook to run at instance_init

This allows to run code that requires to be inside GTypeInstance
init(), such as gtk_widget_init_template()

Created attachment 244256 [details] [review]
Enable all test cases

There is no point in having tests that we don't run

Review of attachment 244253 [details] [review]:

"should" vs "must be" seems kind of important here.  Why not just gjs_context_get() ?  

Oh, right, test/gjs-tests.c would break.  But if we made the finalize function clear the cache (as we should), then I assume it'd work.

Review of attachment 244254 [details] [review]:

This looks in general a *lot* cleaner!

::: gi/object.c
@@ +88,3 @@
 } ToggleRefNotifyOperation;
 
+GSList *object_init_list;

static

@@ +2407,3 @@
+    }
+
+    object_init_list = prev_head->next;

I think this would be cleaner as:

object_list_init = g_slist_delete_link (object_list_init, object_list_init);

@@ +2418,3 @@
+    ensure_uses_toggle_ref(context, object);
+
+    g_slist_free_1(prev_head);

Then you could delete this line, as well as the prev_head bookkeeping.

@@ -2588,2 @@
     gjs_define_object_class(cx, module, NULL, instance_type, &constructor);
-=======

Some bad rebase going on here...

@@ +2587,3 @@
     g_param_spec_set_qdata(pspec, gjs_is_custom_property_quark(), GINT_TO_POINTER(1));
 
+    g_object_class_install_property(G_OBJECT_CLASS (priv->klass), 1, pspec);

Why not keep PROP_JS_HANDLED instead of 1?

Review of attachment 244256 [details] [review]:

Oops, my bad.  Yes, looks good!

Review of attachment 244255 [details] [review]:

This seems like it should go with bug 700347

Created attachment 244722 [details] [review]
GjsContext: add the concept of "current context"

Just a static global variable holding the GjsContext. There should
be at most one context at any time, and this is enforced by
make_current() checking the current context.
For backward compatibility, any new GjsContext is immediately made current,
which means that code that uses multiple context without handling
the current one manually will assert now. You can still use multiple
contexts if you take care to clear the current one.

This will be used to avoid passing a JSContext where we can't easily.

Created attachment 244723 [details] [review]
object: stop using GObject properties to set up context and instance for derived classes

It was an ugly hack, that would easily break with derived classes.
Instead, use a global stack of constructing objects, that is
pushed when building the JS object and retrieved in instance_init()

Review of attachment 244722 [details] [review]:

Looks simple and correct!

Review of attachment 244723 [details] [review]:

This looks a lot nicer I think.  Thanks for doing the cleanup!

Attachment 244256 [details] pushed as 83f8d97 - Enable all test cases
Attachment 244722 [details] pushed as 639e145 - GjsContext: add the concept of "current context"
Attachment 244723 [details] pushed as 62ac6bb - object: stop using GObject properties to set up context and instance for derived classes

Leaving open for the custom hook patch, unless you want me to move it to the other bug.

Yeah, let's move the hook to the widget bug, ok?

Moved.