fix #934 Implement ES2017 Object.getOwnPropertyDescriptors #1193
Conversation
| @@ -507,6 +513,30 @@ public Object execIdCall( | |||
| Scriptable desc = obj.getOwnPropertyDescriptor(cx, nameArg); | |||
| return desc == null ? Undefined.instance : desc; | |||
| } | |||
| case ConstructorId_getOwnPropertyDescriptors: | |||
There was a problem hiding this comment.
EcmaScript spec says the getOwnPropertyDescriptors impl should use the CreateDataPropertyOrThrow Abstract Object operation
We don't have such method yet in AbstractEcmaObjectOperations yet. Would it make sense to refactor things (assuming that the logic of that Abstract Operation is already covered elsewhere) into the appropriate method in AbstractEcmaObjectOperations?
There was a problem hiding this comment.
Perhaps CreateDataPropertyOrThrow looks like the same behavior as defineOwnProperty(cx, id, desc, checkValid = true).
rhino/src/org/mozilla/javascript/ScriptableObject.java
Lines 1569 to 1570 in c275c35
I could use defineOwnProperty here, but I couldn't think of a test case. And, I think we will need FromPropertyDescriptor too. If any test case is violated by this PR, I will be fixed it.
| case ConstructorId_getOwnPropertyDescriptors: | ||
| { | ||
| Object arg = args.length < 1 ? Undefined.instance : args[0]; | ||
| // TODO(norris): There's a deeper issue here if |
There was a problem hiding this comment.
A super minor thing but I don't think that we should add a new TODO comment when copying code, especially one that I bet is 10+ years old and assigned to someone who hasn't worked on this project for a very long time ;-)
There was a problem hiding this comment.
ref. 2b80a8a I removed. I will fixup in a few days.
|
I got tripped up by that TODO comment and I think you should just take it out -- see above. It's minor but if you can please address that then I think we should merge this. Thanks! |
|
This looks good -- thanks! |
Closes #934
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptors