Hi, I wrote most of Object.defineProperty, so I guess I can answer to some of these points.
On Apr 19, 8:00 pm, Jürg Lehni <[email protected]> wrote: > 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. > Yes, feel free to correct that. I guess I was using the wrong editor setting when I wrote that code. > 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... > Perhaps I'm misunderstanding you, but I can't replicate the behaviour you describe with: "if configurable is false for a property that was defined by a data descriptor, I can still convert it to a getter / setter once " I get: [rhino(master)]$ java -jar build/rhino1_7R3pre/js.jar -version 180 Rhino 1.7 release 3 PRERELEASE 2010 01 06 js> var obj = {a:1} js> Object.defineProperty(obj, "b", {configurable:false, enumerable:true, value:5}) [object Object] js> Object.defineProperty(obj, "b", {configurable:false, enumerable:true, get: function(){print("gotcha"); return 6}}) js: "<stdin>", line 4: uncaught JavaScript runtime exception: TypeError: Cannot change "b" from a data property to an accessor property because configurable is false. at <stdin>:4 js> Could you write a failing test for this behaviour? > - 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 agree that the PropertyDescriptor class sounds like a better solution. However when I initially started working on defineProperty, the feedback was that the this abstraction was not a good idea, so best to confirm with Norris et al about that one. See https://bugzilla.mozilla.org/show_bug.cgi?id=489329#c3 > I am curious to hear your thoughts on this matter, and am happy to commit > some time to this. > > Jürg Cheers, Raphael _______________________________________________ dev-tech-js-engine-rhino mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-js-engine-rhino
