Created attachment 356666 [details] [review]
Revert "js: Workaround for function with custom prototype"

This reverts commit 72c0298c5f9df9036ed67fd504db84cbc028daaa.
The warning that was the reason for this code in the first place was
removed in SpiderMonkey 52. According to
https://bugzilla.mozilla.org/show_bug.cgi?id=1049041 the warning was out
of date, and there are only performance effects "if the object escapes to
somewhere interesting before getting its final __proto__".

In addition, client code should move to ES6 classes which do not have
this performance problem.

Edited to resolve conflicts due to code being moved around in the
meantime, and to use Object.setPrototypeOf() instead of the deprecated
__proto__ property.

Created attachment 356667 [details] [review]
Revert "build: Allow compiling without RTTI"

This reverts commit eccf3a14b9b0d6aae89baeaa32dd53f07b2b77d7.

We don't need to match SpiderMonkey here anymore, since we don't inherit
from a SpiderMonkey-defined class.

Created attachment 356668 [details] [review]
class: Stop using custom __name__ property

Since ES6 classes are functions, if you declare 'class Foo {}' then
'Foo.name === "Foo"'. We had a custom __name__ property on Lang.Class
instances, but instead mimic the normal JS class and put a name property
on the class.

That is, the equivalent to 'instance.__name__' is now
'instance.constructor.name'.

This could potentially break client code if any was using this, but the
double underscore made it clear enough that the property should have been
internal.

Created attachment 356669 [details] [review]
lang: Move all legacy Lang.Class code

This moves Lang.Class into a separate Legacy module, which is not meant
to be imported directly; instead, Lang imports it and exports the same
members that it always did.

Also moves all tests for the legacy Lang.Class into a separate file. It
is named testLegacyClass in order to indicate that it tests legacy code.

Created attachment 356670 [details] [review]
tests: Add ES6 class inheriting from legacy class

This should be supported, so add tests for it.

Created attachment 356671 [details] [review]
class: Move to ES6 classes in internal code

Where possible, move usage of Lang.Class within GJS to use ES6 classes.
That is currently possible for any classes which don't inherit from
GObject classes and don't implement Lang.Interfaces.

Remove the documentation for the Lang.Class class framework, as it's now
become outdated.

Created attachment 356672 [details] [review]
GObject: Move all legacy GObject class code

This moves the GObject.Class and GObject.Interface code into the Legacy
module along with Lang.Class and Lang.Interface.

Also moves all tests for the legacy GObject code into a separate file.
It is named testLegacyGObject in order to indicate that it tests legacy
code.

Review of attachment 356666 [details] [review]:

Nice

Review of attachment 356667 [details] [review]:

OK

Review of attachment 356668 [details] [review]:

Looks good, but the __name__ removal should probably be documented in the NEWS file anyway.

::: modules/lang.js
@@ +435,3 @@
     if (unfulfilledReqs.length > 0) {
         throw new Error('The following interfaces must be implemented before ' +
+            `${this.constructor.name}: ${unfulfilledReqs.join(', ')}`);

Should this.prototype.constructor.name not be used here instead?

@@ +443,3 @@
     .filter((p) => !(p in proto) || proto[p] === Interface.UNIMPLEMENTED);
     if (unimplementedFns.length > 0)
+        throw new Error(`The following members of ${this.constructor.name}` +

Ditto

Created attachment 356893 [details] [review]
class: Stop using custom __name__ property

Since ES6 classes are functions, if you declare 'class Foo {}' then
'Foo.name === "Foo"'. We had a custom __name__ property on Lang.Class
instances, but instead mimic the normal JS class and put a name property
on the class.

That is, the equivalent to 'instance.__name__' is now
'instance.constructor.name'.

This could potentially break client code if any was using this, but the
double underscore made it clear enough that the property should have been
internal.

Created attachment 356894 [details] [review]
lang: Move all legacy Lang.Class code

This moves Lang.Class into a separate Legacy module, which is not meant
to be imported directly; instead, Lang imports it and exports the same
members that it always did.

Also moves all tests for the legacy Lang.Class into a separate file. It
is named testLegacyClass in order to indicate that it tests legacy code.

Created attachment 356895 [details] [review]
class: Move to ES6 classes in internal code

Where possible, move usage of Lang.Class within GJS to use ES6 classes.
That is currently possible for any classes which don't inherit from
GObject classes and don't implement Lang.Interfaces.

Remove the documentation for the Lang.Class class framework, as it's now
become outdated.

Created attachment 356896 [details] [review]
GObject: Move all legacy GObject class code

This moves the GObject.Class and GObject.Interface code into the Legacy
module along with Lang.Class and Lang.Interface.

Also moves all tests for the legacy GObject code into a separate file.
It is named testLegacyGObject in order to indicate that it tests legacy
code.

Created attachment 356897 [details] [review]
GObject: Adapt GObject class framework to ES6

This moves our GObject class framework to use ES6 classes. GObject.Object
and GObject.Interface gain static _classInit() methods which register
GTypes and perform other such magic that used to be performed in the
metaclasses. When defining the class you must call
GObject.registerClass() on the class object, with an optional metadata
object as the first argument. (The metadata object works exactly like the
meta properties from Lang.Class, except that Name and Extends are not
present.)

Old:

    var MyClass = new Lang.Class({
        Name: 'MyClass',
        Extends: GObject.Object,
        Signals: { 'event': {} },
        _init(props={}) {
            this._private = [];
            this.parent(props);
        },
    });

New:

    var MyClass = GObject.registerClass({
        Signals: { 'event': {} },
    }, class MyClass extends GObject.Object {
        _init(props={}) {
            this._private = [];
            super._init(props);
        }
    });

It is forward compatible with the following syntax requiring decorators
and class fields, which are not in the JS standard yet:

    @GObject.registerClass
    class MyClass extends GObject.Object {
        static [GObject.signals] = { 'event': {} }
        _init(props={}) {
            this._private = [];
            super._init(props);
        }
    }

We also now add a [Symbol.hasInstance]() method to interfaces so that
instanceof will finally work for interfaces.

One limitation is that GObject ES6 classes can't have constructor()
methods, they must do any setup in an _init() method.

Created attachment 356898 [details] [review]
class: Move to ES6 GObject classes in internal code

This moves all remaining uses of Lang.Class (except for where the legacy
class framework is explicitly tested) from the codebase, in favour of
GObject.registerClass().

Created attachment 356899 [details] [review]
Gtk: Use GObject.registerClass() for Gtk.Widgets

This extends the GObject.registerClass() API to deal with the magic
properties from Gtk.WidgetClass: CssName, Template, Children, and
InternalChildren.

Created attachment 356900 [details] [review]
tests: Add ES6 class inheriting from legacy class

This should be supported, so add tests for it.

(In reply to Cosimo Cecchi from comment #10)
> Review of attachment 356668 [details] [review] [review]:
> ::: modules/lang.js
> @@ +435,3 @@
>      if (unfulfilledReqs.length > 0) {
>          throw new Error('The following interfaces must be implemented
> before ' +
> +            `${this.constructor.name}: ${unfulfilledReqs.join(', ')}`);
> 
> Should this.prototype.constructor.name not be used here instead?

I don't believe so; this.prototype.constructor === this, by definition.

Review of attachment 356893 [details] [review]:

::: modules/lang.js
@@ +443,3 @@
     .filter((p) => !(p in proto) || proto[p] === Interface.UNIMPLEMENTED);
     if (unimplementedFns.length > 0)
+        throw new Error(`The following members of ${this.constructor.name}` +

What I meant is that elsewhere you have this replacement:
- this.__name__ -> this.constructor.name
- this.prototype.__name__ -> this.prototype.constructor.name

However here you're changing this.prototype.__name__ -> this.constructor.name, where I'd expect this.prototype.constructor.name to be used instead.

Review of attachment 356894 [details] [review]:

OK

Review of attachment 356895 [details] [review]:

OK

Review of attachment 356896 [details] [review]:

OK

Review of attachment 356897 [details] [review]:

Nice!

::: modules/overrides/GObject.js
@@ +31,3 @@
+var properties = Symbol('GObject properties');
+var signals = Symbol('GObject signals');
+var requires = Symbol('GObject interface requires');

Nit: alphabetical order

Review of attachment 356898 [details] [review]:

OK

Review of attachment 356899 [details] [review]:

::: installed-tests/js/testGtk.js
@@ +44,3 @@
+// Sadly, putting this in the body of the class will prevent calling
+// get_template_child, since MyComplexGtkSubclass will be bound to the ES6
+// class name without the GObject goodies in it

Not sure I fully understand this... Will it be a problem in practice for other GTK classes out there?

Review of attachment 356900 [details] [review]:

OK

Attachment 356894 [details] pushed as 78a021e - lang: Move all legacy Lang.Class code
Attachment 356895 [details] pushed as 4cbfe50 - class: Move to ES6 classes in internal code
Attachment 356896 [details] pushed as bdb6667 - GObject: Move all legacy GObject class code
Attachment 356897 [details] pushed as 3017008 - GObject: Adapt GObject class framework to ES6
Attachment 356898 [details] pushed as 1fb1583 - class: Move to ES6 GObject classes in internal code
Attachment 356900 [details] pushed as 4c2719f - tests: Add ES6 class inheriting from legacy class

(In reply to Cosimo Cecchi from comment #24)
> Review of attachment 356897 [details] [review] [review]:
> 
> Nice!
> 
> ::: modules/overrides/GObject.js
> @@ +31,3 @@
> +var properties = Symbol('GObject properties');
> +var signals = Symbol('GObject signals');
> +var requires = Symbol('GObject interface requires');
> 
> Nit: alphabetical order

OK, fixed. I'm half wondering if they should be ALL_CAPS, but this is more in line with Symbol.hasInstance and APIs like that. We can still change it in the .91 release if necessary, I think.

(In reply to Cosimo Cecchi from comment #20)
> Review of attachment 356893 [details] [review] [review]:
> 
> ::: modules/lang.js
> @@ +443,3 @@
>      .filter((p) => !(p in proto) || proto[p] === Interface.UNIMPLEMENTED);
>      if (unimplementedFns.length > 0)
> +        throw new Error(`The following members of ${this.constructor.name}`
> +
> 
> What I meant is that elsewhere you have this replacement:
> - this.__name__ -> this.constructor.name
> - this.prototype.__name__ -> this.prototype.constructor.name
> 
> However here you're changing this.prototype.__name__ ->
> this.constructor.name, where I'd expect this.prototype.constructor.name to
> be used instead.

Hmm, I think I need to take an extra look at this, anyway, because this.prototype.constructor doesn't actually make sense.

(In reply to Cosimo Cecchi from comment #26)
> Review of attachment 356899 [details] [review] [review]:
> 
> ::: installed-tests/js/testGtk.js
> @@ +44,3 @@
> +// Sadly, putting this in the body of the class will prevent calling
> +// get_template_child, since MyComplexGtkSubclass will be bound to the ES6
> +// class name without the GObject goodies in it
> 
> Not sure I fully understand this... Will it be a problem in practice for
> other GTK classes out there?

No, I don't believe so -- in practice they will use the Gtk.internalChildren symbol instead of calling get_template_child. Just seemed good to document it since it would have been supported before.

The more detailed explanation is that within a class body, the class name is bound locally to the ES6 class being defined. Since we have something like

const Foo = GObject.registerClass(class Foo {
   ...use(Foo)...
});

then "Foo" refers to the inner ES6 class expression, not the outer class object that is eventually bound to Foo in the rest of the program.

I'm sure that if we did this

const Foo = GObject.registerClass(class _FooInner {
    ...use(Foo)...
});

it would work correctly, though the class's name property would be set up incorrectly.

Created attachment 357206 [details] [review]
Gtk: Use GObject.registerClass() for Gtk.Widgets

This extends the GObject.registerClass() API to deal with the magic
properties from Gtk.WidgetClass: CssName, Template, Children, and
InternalChildren.

Created attachment 357207 [details] [review]
legacy class: Add name property to class object

Instead of the __name__ property on instances, add a name property to
class objects. The __name__ property continues to work, for backwards
compatibility in case any code used it (unlikely), but this is compatible
with ES6 classes.

Review of attachment 357206 [details] [review]:

OK

Review of attachment 357207 [details] [review]:

Looks good!

Attachment 357206 [details] pushed as 2ede057 - Gtk: Use GObject.registerClass() for Gtk.Widgets
Attachment 357207 [details] pushed as 039738d - legacy class: Add name property to class object

Created attachment 357269 [details] [review]
legacy class: Reinstate Lang.getMetaClass()

Some client code was using this, so we should keep it as public API. Add
a trivial test to make sure it doesn't break in the future.

Fixed up a straggler.

Review of attachment 357269 [details] [review]:

OK

Attachment 357269 [details] pushed as d9218eb - legacy class: Reinstate Lang.getMetaClass()