Status

()

P1
normal
RESOLVED WONTFIX
11 years ago
8 years ago

People

(Reporter: sayrer, Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

604 bytes, application/x-javascript
Details
v2
5.58 KB, patch
mrbkap
: review-
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Lots of JS libraries have this.
getting this on the 1.9.1 triage queue.
Flags: blocking1.9.1?
Priority: -- → P1
(Reporter)

Updated

11 years ago
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Hardware: PC → All
Version: unspecified → Trunk

Comment 2

10 years ago
Created attachment 329817 [details]
Pure-JavaScript Object.extend

This is a pure-JavaScript version of Object.extend that I'm proposing. It includes support for getters and setters and does a simple loop over the object properties to bring them on to the base object.

Comment 3

10 years ago
jresig: a couple thoughts on your Pure-JavaScript version:

* As mentioned on IRC, I expected a hasOwnProperty check in here somewhere.
* In the jQuery implementation, you check to see if the obj being copied from is of type "object" or "function", and I think this restriction is sane.

Comment 4

10 years ago
jsresig:  Also, should I be overwriting previously set values on orig, or not?

iow, Object.extend({a:3}, {a:5}) does what?

My inclination is -not- to overwrite them.

Comment 5

10 years ago
> * As mentioned on IRC, I expected a hasOwnProperty check in here somewhere.

The version that I passed to you on IRC has one:
  http://ejohn.org/files/object-extend.js

I'm working to try and include more test cases.

> * In the jQuery implementation, you check to see if the obj being copied from
> is of type "object" or "function", and I think this restriction is sane.

Yeah, that's probably a good thing to test for - I agree. I'll add that to my working copy.

> My inclination is -not- to overwrite them.

All of the other implementations override by default. The only one that provides an option to toggle that behavior is Yahoo UI's (as an extra argument).

Comment 6

10 years ago
Created attachment 330306 [details] [diff] [review]
v1

Here's a stab at an implementation of this for spidermonkey.  I haven't even begun to try it against John's test-suite yet.  That's next.

Comment 7

10 years ago
Okay, my version seems to pass all 85 of the samples included in your suite, John.

Comment 8

10 years ago
Comment on attachment 330306 [details] [diff] [review]
v1

I'll ask for Brendan's review once you've looked it over.
Attachment #330306 - Flags: review?(mrbkap)
Comment on attachment 330306 [details] [diff] [review]
v1

>+++ mozilla/js/src/jsobj.cpp	2008-07-18 15:02:02.000000000 -0700
>+    JS_SET_RVAL(cx, vp, JS_ARGV(cx, vp)[0]);

It seems worth it to move the JS_ARGV computation into a local jsval *argv.

>+    orig = js_ValueToNonNullObject(cx, JS_ARGV(cx, vp)[0]);
>+    if (!orig) {
>+        return JS_TRUE;
>+    }

Nit: overbraced if statement.

Non-nit: js_ValueToNonNullObject sets an exception if it fails. You need to either JS_ClearPendingException or return JS_FALSE here. The latter makes more sense to me.

>+    for (i = 1; i < argc; i++) {
>+        jsval iter_state;
>+
>+        obj = js_ValueToNonNullObject(cx, JS_ARGV(cx, vp)[i]);
>+        if (!obj)
>+            continue;

Need to throw here or clear the pending exception.

>+            if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
>+                return JS_FALSE;

Need to ensure that you call into the enumerator with JSENUMERATE_DESTROY here and under js_DefineNativeProperty.

>+
>+            if (obj != obj2) {   /* Don't copy inherited props */
>+                OBJ_DROP_PROPERTY(cx, obj2, prop);
>+                continue;
>+            }

The enumeration API is only supposed to return direct properties of the given object, so the above check shouldn't be necessary.

crowder and I talked on IRC about using the JS_Enumerate API versus the OBJ_ENUMERATE API directly. I'll let brendan make the final call -- crowder argued that he didn't need to enumerate the properties twice.
Attachment #330306 - Flags: review?(mrbkap) → review-

Comment 10

10 years ago
Created attachment 332430 [details] [diff] [review]
v2

I think we don't want to throw for primitive "orig" (see jresig's original JS implementation), so I've reworked that a bit.  I'll propagate an exception if js_ValueToNonNullObject throws for non-null objects, though.  This version does the same as I loop through the arguments, as well.

Did a DESTROY under the GET/SET pair at the bottom of the loop, also, where I also return JS_FALSE.
Attachment #330306 - Attachment is obsolete: true
Attachment #332430 - Flags: review?(mrbkap)
Comment on attachment 332430 [details] [diff] [review]
v2

Looks OK to me. The API around primitive values seems wonky to me, but I'll let jresig and the rest of y'all fight it out.
Attachment #332430 - Flags: review?(mrbkap) → review+
Comment on attachment 332430 [details] [diff] [review]
v2

After talking with brendan, we decided that the caller of the OBJ_ENUMERATE api is responsible for rooting the iter_state passed down. This is done "for free" in the for-in case, but anybody else needs to use a tvr or something.
Attachment #332430 - Flags: review+ → review-

Comment 13

10 years ago
Mass unowning JS bugs...  I'm not likely to be able to move these forward.
Assignee: crowder → general

Updated

10 years ago
Assignee: general → jwalden+bmo
I think we've moved to doing this through the ECMAScript mailing list and working group these days, so I don't think we're doing this unless ECMA picks it up first.
Assignee: jwalden+bmo → general
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.