Created attachment 124027 [details] [review]
split out gparam aspect of get_prop hook for GObject wrapper

Patch splits out the code that tries to resolve properties from the gparams of an object.

Follow-up patch would fall back to fields. In case of conflict, gparam should almost certainly win.

This is properly working nowadays. Closing.

I think GtkWidget.window works now because window was made a property :) But I don't think we ever added support for fields in GObjects

+    override = JSVAL_VOID;
+    JS_AddRoot(context, &override);
+    success = get_prop_from_g_param(context, obj, priv, name, &override);
+    if (!success)
+        goto out;
+
+    if (override != JSVAL_VOID) {
+        *value_p = override;
+        goto out;
+    }

I don't actually see the value of this over:

    success = get_prop_from_g_param(context, obj, priv, name, value_p);
    if (!success)
        goto out;

    if (*value_p != JSVAL_VOID)
        goto out;

Either way we assume *value_p is non-NULL and if we just use that location we avoid needing to add the root, even if we are being super-cautious.

Otherwise, the patch looks fine to me, though obviously doesn't do anything useful by itself.

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

Created attachment 342187 [details] [review]
object: Split out gparam aspect of get_prop hook for GObject wrapper

Split out the code that tries to resolve properties from the gparams of
an object.

The idea is to add a similar function fetching properties from fields.

(Stale patch, rebased and expanded to include the set_prop hook by
Philip Chimento eight years later.)

Created attachment 342188 [details] [review]
object: Support accessing fields on GObjects

This adds support for accessing fields in GObject instance structures as
if they were JS properties.

There are a few caveats:

- If there is a GObject property of the same name, the field is
  inaccessible.

- GObject Introspection doesn't support reading fields that are of a
  complex type, so we throw an exception if you try to read one of those.

- Unfortunately the /*< private >*/ annotation doesn't seem to show up in
  the GIR file, so there is no difference between private and public
  fields (I think that only gtk-doc uses that annotation.)

Created attachment 342189 [details] [review]
object: Handle setting GObject field

This doesn't add support for setting fields in GObject instance
structures because as far as I know GObject Introspection never exposes
them as writable.

However, if we don't handle them in our property setter hook, then
they'll just get set in JS as if they were normal JS properties. We don't
want that either.

This silently ignores an attempt to set a GObject field, except in
strict mode in which case it throws an exception. This matches the
behaviour of normal JS properties when they are read-only.

If there ever is a GObject field which is exposed as writable, we log a
message indicating that setting it is not yet implemented.

I ported Havoc's patch so it applies to master nowadays, and implemented getting fields. See the commit messages for a few caveats, as well as the reason why I didn't implement setting them.

Review of attachment 342187 [details] [review]:

Looks good

Review of attachment 342188 [details] [review]:

Looks good to me.

Review of attachment 342189 [details] [review]:

::: gi/object.cpp
@@ +528,3 @@
+        goto out;
+
+    ret = check_set_field_from_prop(context, priv, name, strict, value_p);

Don't you want to call this only when !g_param_was_set?

(In reply to Cosimo Cecchi from comment #12)
> Review of attachment 342189 [details] [review] [review]:
> 
> ::: gi/object.cpp
> @@ +528,3 @@
> +        goto out;
> +
> +    ret = check_set_field_from_prop(context, priv, name, strict, value_p);
> 
> Don't you want to call this only when !g_param_was_set?

You're correct. I assume you found the patch is otherwise good so I pushed it with this correction.

Attachment 342187 [details] pushed as b821473 - object: Split out gparam aspect of get_prop hook for GObject wrapper
Attachment 342188 [details] pushed as 8ebb803 - object: Support accessing fields on GObjects
Attachment 342189 [details] pushed as 655750c - object: Handle setting GObject field