GNOME Bugzilla – Bug 692025
Replace native modules with JS equivalents
Last modified: 2013-01-20 15:24:38 UTC
Easier to understand, maintain and document.
Created attachment 233776 [details] [review] Convert many native modules to JS modules Less C code and more JS + introspection means more maintenability and fewer bugs.
Created attachment 233777 [details] [review] Rework Lang module to be ES5 compliant Use ES5 features to provide the same functionality without a native module, and stop relying on SpiderMonkey details for accessor properties.
Created attachment 233778 [details] [review] ByteArray: don't define the ByteArray class in the global object Otherwise, a ByteArray property appears, which prevents the byteArray test from setting it with the module object. It was not failing before because the byte array module was not initialized.
Created attachment 233779 [details] [review] Rework Lang module to be ES5 compliant Use ES5 features to provide the same functionality without a native module, and stop relying on SpiderMonkey details for accessor properties. Argh, I was hoping to avoid the rebase on Jasper's lookupGetter patch. Btw, there is bug there, wrt prototype handling in copyProperties.
Review of attachment 233776 [details] [review]: I'm fine with this.
Review of attachment 233778 [details] [review]: OK.
Review of attachment 233779 [details] [review]: ::: modules/lang.js @@ +40,3 @@ function _copyProperty(source, dest, property) { + let descriptor = getPropertyDescriptor(source, property); + Object.defineProperty(dest, property, descriptor); Make this fix a separate patch, please.
Created attachment 233793 [details] [review] Lang: fix copyProperties regression We need to recurse through the prototype while fetching property descriptors.
Created attachment 233794 [details] [review] Rework Lang module to be ES5 compliant Use ES5 features to provide the same functionality without a native module, and stop relying on SpiderMonkey details for accessor properties.
Review of attachment 233793 [details] [review]: ::: modules/lang.js @@ +35,3 @@ + if (obj.hasOwnProperty(property)) + return Object.getOwnPropertyDescriptor(obj, property); + return getPropertyDescriptor(Object.getPrototypeOf(obj), property); We need to guard against the prototype eventually becoming null.
Review of attachment 233794 [details] [review]: One comment nit, otherwise ACN. ::: modules/lang.js @@ +93,3 @@ } + // ECMAScript 5 (but only if not passing any bindArguments, // Use ES5 Function.prototype.bind (but only...
Created attachment 233795 [details] [review] Lang: fix copyProperties regression We need to recurse through the prototype while fetching property descriptors.
Review of attachment 233795 [details] [review]: Almost. ::: modules/lang.js @@ +42,3 @@ function _copyProperty(source, dest, property) { + let descriptor = getPropertyDescriptor(source, property); + if (descriptor != null) If the user passes in a bad property, we should throw an error, not silently do nothing, I think.
(In reply to comment #13) > Review of attachment 233795 [details] [review]: > > Almost. > > ::: modules/lang.js > @@ +42,3 @@ > function _copyProperty(source, dest, property) { > + let descriptor = getPropertyDescriptor(source, property); > + if (descriptor != null) > > If the user passes in a bad property, we should throw an error, not silently do > nothing, I think. But _copyProperty is not public API, is it is an implementation helper used by copyProperties*()
Ah, then we should never get a null prototype. Nevermind.
Review of attachment 233793 [details] [review]: Let's go with this one without the guard, then.
Attachment 233776 [details] pushed as b5e467d - Convert many native modules to JS modules Attachment 233794 [details] pushed as c39baaa - Rework Lang module to be ES5 compliant Attachment 233795 [details] pushed as 391aec6 - Lang: fix copyProperties regression
Attachment 233778 [details] pushed as 03a2147 - ByteArray: don't define the ByteArray class in the global object
This broke the mainloop test. Please revert. (and please make test your patches from now on)
(In reply to comment #19) > This broke the mainloop test. Please revert. > > (and please make test your patches from now on) It was make tested. I just forgot it depends on glib bug 692034.
Be happy, the glib bug is fixed.