Created attachment 58759 [details] Fix v1

Comment on attachment 58759 [details] Fix v1 This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed. Additionally you get weird behaviour if have: proto = {prop: "foo"} o = Object.create(proto); and do: JSObjectSetProperty(o, "prop", ..., ...); you will delete "prop" from proto, and then add "prop" to o. Clearly this is less than ideal. :-( --Oliver

(In reply to comment #2) > (From update of attachment 58759 [details]) > This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed. > > Additionally you get weird behaviour if have: > > proto = {prop: "foo"} > o = Object.create(proto); > > and do: > JSObjectSetProperty(o, "prop", ..., ...); > > you will delete "prop" from proto, and then add "prop" to o. Clearly this is less than ideal. The hasProperty() call seems weird. Just because the object has some property in the prototype chain, the attributes you specify will be ignored when you create a property of the same name on your object? Would calling hasOwnProperty() be sufficient? Better, by calling getOwnPropertyDescriptor() one can get the attributes, and only delete the property if they differ. Oh, looks like JSObject::defineOwnProperty() already has such logic. ;) Still seems like a work-around, though. How about fixing JSObject::putWithAttributes() so it takes care of updating the attributes of an existing entry in case the property already exists (just like it takes care of updating the value)? Then unconditionally call putWithAttributes() from JSObjectSetProperty() when attributes != 0. (Which means there's no way to remove all attributes without recreating the property; oh well.)

Sigh - the problem here is that it's not really clear what JSObjectSetProperty should be doing. Does it correspond to the spec's [[Put]] functionality? [[DefineOwnProperty]]? - or a mix of the two? Given the interface we have, if the property already exists on the object being put to, it probably does make sense to make this call defineOwnProperty so the attributes are updated accordingly.

I don't think we're going to change this interface right now. The API behaviour is if ([[HasProperty]]) [[Put]] else [[DefineOwnProperty]] It might be nice to separate put & defineOwnProperty traps in the API, but JSObjectSetProperty's behaviour isn't going to change now.