Implement ES5 Object.getOwnPropertyDescriptor

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

Needed for verifying correctness of Object.defineProperty...
Created attachment 397398 [details] [diff] [review]
Patch

Still needs tests, will start formally writing them shortly...
Attachment #397398 - Flags: review?(mrbkap)
Created attachment 398735 [details] [diff] [review]
A decent set of tests for a start

I also ran the patch against the ES5 tests at <http://es5conform.codeplex.com/>, and things seem to work perfectly fine.

This is currently limited to what's exposed by ES5 itself; bug 430133 and Object.defineProperty will allow access to the real corner cases that can't be examined in the current specified APIs.
Comment on attachment 397398 [details] [diff] [review]
Patch

>+static JSBool
>+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                             JSMSG_NOT_NONNULL_OBJECT,
>+                             js_getter_str);
>+        return JS_FALSE;
>+    }
>+    JSObject *obj = JSVAL_TO_OBJECT(vp[2]);

Nit: blank line after the }.

>+        JSScopeProperty *sprop = reinterpret_cast<JSScopeProperty *>(prop);
>+        if (!desc->defineProperty(cx, ATOM_TO_JSID(atomState.getAtom),
>+                                  ((attrs & JSPROP_GETTER) && OBJ_IS_NATIVE(obj))
>+                                  ? js_CastAsObjectJSVal(sprop->getter)
>+                                  : JSVAL_VOID,
>+                                  JS_PropertyStub, JS_PropertyStub,
>+                                  JSPROP_ENUMERATE, NULL) ||
>+            !desc->defineProperty(cx, ATOM_TO_JSID(atomState.setAtom),
>+                                  ((attrs & JSPROP_SETTER) && OBJ_IS_NATIVE(obj))
>+                                  ? js_CastAsObjectJSVal(sprop->setter)
>+                                  : JSVAL_VOID,
>+                                  JS_PropertyStub, JS_PropertyStub,
>+                                  JSPROP_ENUMERATE, NULL)) {

I'd prefer to move the OBJ_IS_NATIVE checks (and the cast to JSScopeProperty) into a common place.
Attachment #397398 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/b7ee9263cf96

with the requested changes, also with the attached tests in the new ecma_5/Object/ directory.
Whiteboard: fixed-in-tracemonkey
Depends on: 516351
http://hg.mozilla.org/mozilla-central/rev/b7ee9263cf96
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/tracemonkey/rev/87fcc6cb16af to update tests to use reportCompare once
I wrote up a little bit of documentation, but it needs to be made more visible than simply exposing it deep within the JS reference:

https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Object/getOwnPropertyDescriptor
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.