Created attachment 185227 [details] [review]
GIRepositoryFunction: make it inherit from Function

Set the prototype of GIRepositoryFunction to Function.prototype, so
methods bind(), call() and apply() are available, and instanceof
reports correct results.

Review of attachment 185227 [details] [review]:

I thought bug 627287 made call() and apply() available. How are these two bugs related?

In any case, misses the tests to prove what commit message claims :-)

::: gi/function.c
@@ +969,3 @@
 
+static JSBool
+function_to_string (JSContext *context,

it's the longest part of the implementation in this patch and doesn't even get mentioned in the commit message :)

I'm not completely sure it's necessary though, but the code comment should probably mention whether or not the intention of toString() here is to mimic the Function.toString() output.

@@ +988,3 @@
+    priv = priv_from_js (context, self);
+
+    jsval retval;

I'm a bit fuzzy on the semantics here, but is this even possible?

maybe related, doesn't this also get called for the prototype in which case there is no code as such and might be better for toString() to not claim so?

@@ +993,3 @@
+    } else {
+        string = g_strdup_printf("function %s(){\n\t[native code]\n}",
+    self = JS_THIS_OBJECT(context, vp);

in some cases I imagine it would be useful to include the fully qualified name of the function somewhere

@@ +1040,3 @@
 
+/* The original Function.prototype.toString complains when
+   given a GIRepository function as an argument */

curious. why does toString() complain (and how?) while apply(), bind(), call() presumably work without fail?

(In reply to comment #2)
> I thought bug 627287 made call() and apply() available. How are these two bugs
> related?

No relation; I think Giovanni and I are just both looking at old FIXMEs in gnome-shell this week.

> In any case, misses the tests to prove what commit message claims :-)

The tests from 627287 can be used here if this approach is preferable to the one there.

(In reply to comment #2)
> Review of attachment 185227 [details] [review]:
> 
> I thought bug 627287 made call() and apply() available. How are these two bugs
> related?

I just didn't search before opening a new bug. Mark it duplicate if you prefer the other implementation.
 
> In any case, misses the tests to prove what commit message claims :-)
>
> ::: gi/function.c
> @@ +969,3 @@
> 
> +static JSBool
> +function_to_string (JSContext *context,
> 
> it's the longest part of the implementation in this patch and doesn't even get
> mentioned in the commit message :)

Will fix.

> I'm not completely sure it's necessary though, but the code comment should
> probably mention whether or not the intention of toString() here is to mimic
> the Function.toString() output.

Ok

> @@ +988,3 @@
> +    priv = priv_from_js (context, self);
> +
> +    jsval retval;
> 
> I'm a bit fuzzy on the semantics here, but is this even possible?

priv == NULL means "this is the prototype, not an instance"
 
> maybe related, doesn't this also get called for the prototype in which case
> there is no code as such and might be better for toString() to not claim so?

Ok, I'll change to do what Function.prototype.toString() does.

> @@ +993,3 @@
> +    } else {
> +        string = g_strdup_printf("function %s(){\n\t[native code]\n}",
> +    self = JS_THIS_OBJECT(context, vp);
> 
> in some cases I imagine it would be useful to include the fully qualified name
> of the function somewhere

So g_function_info_get_symbol instead of g_base_info_get_name? Or that exposes a C implementation detail?

> @@ +1040,3 @@
> 
> +/* The original Function.prototype.toString complains when
> +   given a GIRepository function as an argument */
> 
> curious. why does toString() complain (and how?) while apply(), bind(), call()
> presumably work without fail?

TypeError: Function.prototype.toString called on incompatible object
whereas apply(), bind() and call() just work.
Don't ask me why...

Created attachment 185439 [details] [review]
GIRepositoryFunction: make it inherit from Function

Set the prototype of GIRepositoryFunction to Function.prototype, so
methods bind(), call() and apply() are available, and instanceof
reports correct results.
Also ensures that method invocations are applied on GObjects of the
correct type, and adds an explicit toString() method to GIRepositoryFunction,
since Function.prototype.toString() throws when called on objects
of the wrong class.

Review of attachment 185439 [details] [review]:

::: gi/function.c
@@ +491,3 @@
+                failed = TRUE;
+                goto release;
+            }

This should be a separate patch.

@@ +982,3 @@
+    self = JS_THIS_OBJECT(context, vp);
+    if (!self) {
+        gjs_throw (context, "this cannot be null");

No space between identifier and paren.

@@ +989,3 @@
+
+    if (priv == NULL) {
+        string = "function () {\n}";

Hmm...can this actually happen?  If it can't, we should just assert.

(In reply to comment #6)
> Review of attachment 185439 [details] [review]:
> 
> ::: gi/function.c
> @@ +491,3 @@
> +                failed = TRUE;
> +                goto release;
> +            }
> 
> This should be a separate patch.

It's not really: you can get a this pointer of the wrong class only by using .call / .apply (unless you do some black magic).

> @@ +982,3 @@
> +    self = JS_THIS_OBJECT(context, vp);
> +    if (!self) {
> +        gjs_throw (context, "this cannot be null");
> 
> No space between identifier and paren.

Really? That's unusual. Will fix anyway.

> @@ +989,3 @@
> +
> +    if (priv == NULL) {
> +        string = "function () {\n}";
> 
> Hmm...can this actually happen?  If it can't, we should just assert.

GIRepository.prototype.toString()
(corner case, but in general JS code should never crash or assert)

(In reply to comment #7)
> (In reply to comment #6)
> > Review of attachment 185439 [details] [review] [details]:
> > 
> > ::: gi/function.c
> > @@ +491,3 @@
> > +                failed = TRUE;
> > +                goto release;
> > +            }
> > 
> > This should be a separate patch.
> 
> It's not really: you can get a this pointer of the wrong class only by using
> .call / .apply (unless you do some black magic).

Okay.

> > @@ +982,3 @@
> > +    self = JS_THIS_OBJECT(context, vp);
> > +    if (!self) {
> > +        gjs_throw (context, "this cannot be null");
> > 
> > No space between identifier and paren.
> 
> Really? That's unusual. Will fix anyway.

Yeah, that's the gjs style (also, indent of 4).  I know it sucks switching between C code of different indentation styles, but that's life =/

 
> GIRepository.prototype.toString()
> (corner case, but in general JS code should never crash or assert)

Ah of course, right.  And your toString here is matching Function.prototype.toString().  Looks fine to me then.

Created attachment 186426 [details] [review]
GIRepositoryFunction: make it inherit from Function

Set the prototype of GIRepositoryFunction to Function.prototype, so
methods bind(), call() and apply() are available, and instanceof
reports correct results.
Also ensures that method invocations are applied on GObjects of the
correct type, and adds an explicit toString() method to GIRepositoryFunction,
since Function.prototype.toString() throws when called on objects
of the wrong class.

Review of attachment 186426 [details] [review]:

Looks fine

Attachment 186426 [details] pushed as cb55783 - GIRepositoryFunction: make it inherit from Function

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