Created attachment 209436 [details] [review]
Don't pass the object as the first argument to a vfunc

The object is already passed as this, there is no need to pass it
as the first argument, and it actually breaks expectations from
API users.

Review of attachment 209436 [details] [review]:

If you're absolutely curious, the reason that I did this was to support a case like:

    const CustomContainer = new Lang.Class({
         Extends: St.Widget,

         vfunc_paint: St.Widget.paint_background,
    }):

Or something like that, because vfunc chaining is a mess... this is a very special edge case, so if we want the API change for the better, let's land this before we start using it.

Review of attachment 209436 [details] [review]:

(Accidentally hit Publish before changing the status)

(In reply to comment #2)
> Review of attachment 209436 [details] [review]:
> 
> If you're absolutely curious, the reason that I did this was to support a case
> like:
> 
>     const CustomContainer = new Lang.Class({
>          Extends: St.Widget,
> 
>          vfunc_paint: St.Widget.paint_background,
>     }):
> 
> Or something like that, because vfunc chaining is a mess... this is a very
> special edge case, so if we want the API change for the better, let's land this
> before we start using it.

Uhm... That's St.Widget.prototype.paint_background, and it wants the widget as this + nothing for argv.
I don't think there is a useful case of a function taking the object it operates on as the first argument without being a method, so I'm pushing the patch.

Attachment 209436 [details] pushed as c0d38f4 - Don't pass the object as the first argument to a vfunc

(In reply to comment #4)
> Uhm... That's St.Widget.prototype.paint_background, and it wants the widget as
> this + nothing for argv.

That's because I remembered wrong. I was testing with a variant of the "st-container: Implement standard pick and paint methods" patch from bug #649631 where the _st_container_paint/_st_container_pick methods were public to introspection. As I was implementing a container (replacing DashItemContainer, to be exact) with that patch applied, I needed to have the paint/pick methods as otherwise my container wouldn't paint anything.

Since st_container_paint/st_container_pick have a ClutterActor * as their first argument, it would be introspected as a function on St.Container, so instead of:

    const DashItemContainer = new Lang.Class({
        // ...
        vfunc_paint: function() {
            St.Container.paint(this);
        },

        vfunc_pick: function() {
            St.Container.pick(this);
        }
    });

I chose to just add the argument (or not omit it, rather). Now that I look back on it, I made a horrible decision (optimizing for one very minor use case in exchange for the loss of sanity), and this patch is a the right one.