Need JS_AlreadyHasOwnProperty (UCProperty, Element)

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: bz, Assigned: Brian Crowder)

Tracking

Trunk
mozilla1.9beta2
x86
Linux
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Created attachment 285953 [details] [diff] [review]
WIP, handing off to crowder

Updated

11 years ago
Summary: Need JS_HasOwnProperty → Need JS_AlreadyHasOwnProperty (UCProperty, Element)
(Assignee)

Updated

11 years ago
Assignee: general → crowder
Can we start allowing JSAPI functions to take jsvals instead of creating *{Property,Element} functions?
(In reply to comment #2)
> Can we start allowing JSAPI functions to take jsvals instead of creating
> *{Property,Element} functions?

That would hurt XBL because it keeps a const PRUnichar *mName around to pass. See the other bug.

I think we're not ready to force that divergence into the JS API. The cost is pretty small (someone measure), carry the torch a bit longer.

/be

(Assignee)

Comment 4

11 years ago
Created attachment 286046 [details] [diff] [review]
v2, adds header declarations and finishes !OBJ_IS_NATIVE case

bz: does this fit your use-case?  Can you apply it and see if it does what you require?
Attachment #285953 - Attachment is obsolete: true
(Assignee)

Comment 5

11 years ago
Created attachment 286047 [details] [diff] [review]
cleanup patch-file

Just tidying up the patchfile a bit.
Attachment #286046 - Attachment is obsolete: true
Comment on attachment 286047 [details] [diff] [review]
cleanup patch-file

>Index: jsapi.c
>+static JSBool
>+AlreadyHasOwnPropertyHelper(JSContext *cx, JSObject *obj, jsid id,
>+                            JSBool *foundp)
...
>+        ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);

This should just be:

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

>+        *foundp = (prop != NULL);

...or you could be using an uninitialized prop here. Also, I think this could just be:

*foundp = obj == obj2;

Also, you need:

if (prop)
    OBJ_DROP_PROPERTY(cx, obj2, prop);
Assuming that XPCWrappedNatives are OBJ_IS_NATIVE, that looks like it'll do what I want.
(Assignee)

Comment 8

11 years ago
Created attachment 286049 [details] [diff] [review]
v2.1, addressing mrbkap's feedback
Attachment #286047 - Attachment is obsolete: true
Comment on attachment 286049 [details] [diff] [review]
v2.1, addressing mrbkap's feedback

Nit: order of prototypes in jsapi.h does not match corresponding definition order in jsapi.c. I put the JS_AlreadyHas* before the JS_Has* jazz in jsapi.c, FWIW.

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

Nit: house style overparenthesizes bitwise, equality, and relational expressions on the RHS of assignment expressions.

>+

Nit: no need for a blank line here, really.

>+        if (prop)
>+            OBJ_DROP_PROPERTY(cx, obj2, prop);
>+        return JS_TRUE;
>+    }
>+
>+    scope = OBJ_SCOPE(obj);
>+    JS_LOCK_SCOPE(cx, scope);

Not sure who suggested this change from my WIP patch, but you need JS_LOCK_OBJ(cx, obj); scope = OBJ_SCOPE(obj) in that order -- see jslock.c:js_LockObj.

/be
Attachment #286049 - Flags: review-
(Assignee)

Comment 10

11 years ago
Created attachment 286178 [details] [diff] [review]
v2.2, addressing brendan's feedback

> >+    scope = OBJ_SCOPE(obj);
> >+    JS_LOCK_SCOPE(cx, scope);
> 
> Not sure who suggested this change from my WIP patch, but you need
> JS_LOCK_OBJ(cx, obj); scope = OBJ_SCOPE(obj) in that order -- see
> jslock.c:js_LockObj.

Original WIP patch did:

+    JS_LOCK_SCOPE(cx, scope); /* uninited scope */
+    scope = OBJ_SCOPE(obj);

So the suggestion was my own.  The old AlreadyHasOwnProperty() routine (any concern about the naming here, for the new functions?  Especially the *Helper function that doesn't, in fact, help this old routine?) in jsapi.c did this:

    JS_LOCK_OBJ(cx, obj);
    scope = OBJ_SCOPE(obj);
    sprop = SCOPE_GET_PROPERTY(scope, ATOM_TO_JSID(atom));
    JS_UNLOCK_SCOPE(cx, scope);

Is that correct?  It looks unbalanced to me, but I will assume it is correct and do similar in the attached revision.
Attachment #286049 - Attachment is obsolete: true
Attachment #286178 - Flags: review?(brendan)
(In reply to comment #10)
> Is that correct?  It looks unbalanced to me, but I will assume it is correct
> and do similar in the attached revision.

JS_UNLOCK_OBJ calls js_UnlockObj, which is a thin wrapper around js_UnlockScope(cx, OBJ_SCOPE(obj)).
Comment on attachment 286178 [details] [diff] [review]
v2.2, addressing brendan's feedback

Thanks, r/a=me (after M9). sorry about the WIP glitch with SCOPE instead of OBJ on the front side. Yeah, as we discussed on IRC, SM uses the lower layer JS_UNLOCK_SCOPE to unlock when there's a scope = OBJ_SCOPE(obj) following a JS_LOCK_OBJ, just to avoid reloading scope.

/be
Attachment #286178 - Flags: review?(brendan)
Attachment #286178 - Flags: review+
Attachment #286178 - Flags: approval1.9+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.364; previous revision: 3.363
done
Checking in js/src/jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.164; previous revision: 3.163
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.