-
Notifications
You must be signed in to change notification settings - Fork 846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
protected void defineOwnProperty( | |
Context cx, Object id, ScriptableObject desc, boolean checkValid) { |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
b4da1ca
to
2b80a8a
Compare
2b80a8a
to
923a9af
Compare
This looks good -- thanks! |
Closes #934
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptors