[webkit-reviews] review denied: [Bug 36420] JSC needs an API to allow custom objects to have aprivate GC-accessible properties : [Attachment 51238] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 36420: JSC needs an API to allow custom objects to have aprivate
GC-accessible properties
https://bugs.webkit.org/show_bug.cgi?id=36420

Attachment 51238: Patch
https://bugs.webkit.org/attachment.cgi?id=51238&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Comments given on IRC:

Docs:

the headerdoc could maybe explain the purpose of this
i.e. that it's a way to hold on to JavaScript values in a way that is safe for
GC
I think probably in the set one would be the best place to explain the purpose
also maybe add a note to JSValueProtect
to make clear that if you want one JS object to keep another alive, you
shouldn't use it, you should use a private property
otherwise you may get uncollectable reference cycles


Error cases:

JSObjectSetPrivateProperty seems to fail silently if the argument is not a
callback object
maybe it should have a boolean return? or raise an exception?
it looks like JSObjectGetPrivateProperty will return garbage when called on a
non-API object
I think it would make more sense to return undefined
Silent failure on delete seems ok

Testing:

you could try harder in the test case to avoid the possibility that a copy of
aHeapRef remains on the stack
I think you should test calling these on non-API objects in the test case