I started experimenting with the various Object.defineProperty related functions in Rhino and found a few issues that I am happy to correct, but would like to start a discussion first before I write a patch:
First of all, all the code from that patch is using different formatting rules than the rest of Rhino: Two white spaces instead of four for intention, no limit to line length, so it would be good to also correct this along the other things discussed here. Beyond these cosmetic changes, here a list of what I found and would suggest to change / improve: - There seem to be a row of issues with checkValidPropertyDefinition() not acting according to the specs. One I found is that if configurable is false for a property that was defined by a data descriptor, I can still convert it to a getter / setter once (and then probably back to a normal data property). But I cannot change the getter again after it was already defined as a getter. On the following page it is stated: "If the old descriptor had its configurable field as false, then no fields present in the new descriptor can be different from those in the old descriptor. Further, the type of the new descriptor must be the same as the type of the old one." So it seems this behaviour should be fixed. https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/defineProperty - The various methods checkValidPropertyDefinition(), isDataDescriptor(), isAccessorDescriptor(), isGenericDescriptor() repeatedly access properties on the passed descriptor ScriptableObject through ScriptableObject.hasProperty() / ScriptableObject.getProperty(), so traveling up the inheritance chain each time. None of this is cached anywhere, and methods like isAccessorDescriptor() etc. are used in many places, each time accessing these values again. I see the potential for a performance improvement by introducing a new class (e.g. PropertyDescriptor) that holds all the properties of the descriptors, including information about its state / type, and some logic to convert back and forth from ScriptableObjects and attributes flags. - GetterSlot#getPropertyDescriptor() erases properties again that were set by Slot#getPropertyDescriptor(). This does not seem optimal. If the code was refactured to return a PropertyDescriptor object instead which then can convert itself to a ScriptableObject, this could be solved elegantly. I am curious to hear your thoughts on this matter, and am happy to commit some time to this. Jürg _______________________________________________ dev-tech-js-engine-rhino mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-js-engine-rhino
