Last Comment Bug 505587 - Implement ES5 Object.getOwnPropertyDescriptor
: Implement ES5 Object.getOwnPropertyDescriptor
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 516351
  Show dependency treegraph
Reported: 2009-07-21 16:30 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2010-08-18 10:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (10.61 KB, patch)
2009-08-28 17:43 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mrbkap: review+
Details | Diff | Splinter Review
A decent set of tests for a start (6.13 KB, patch)
2009-09-04 12:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-07-21 16:30:16 PDT
Needed for verifying correctness of Object.defineProperty...
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-28 17:43:26 PDT
Created attachment 397398 [details] [diff] [review]

Still needs tests, will start formally writing them shortly...
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-04 12:27:55 PDT
Created attachment 398735 [details] [diff] [review]
A decent set of tests for a start

I also ran the patch against the ES5 tests at <>, 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 3 User image Blake Kaplan (:mrbkap) 2009-09-09 15:05:23 PDT
Comment on attachment 397398 [details] [diff] [review]

>+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.
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-09 16:40:31 PDT

with the requested changes, also with the attached tests in the new ecma_5/Object/ directory.
Comment 5 User image Blake Kaplan (:mrbkap) 2009-09-16 17:32:15 PDT
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-28 13:12:58 PDT to update tests to use reportCompare once
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2010-01-11 11:36:11 PST
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:

Note You need to log in before you can comment on or make changes to this bug.