Summary: | JSC needs an API to allow custom objects to have aprivate GC-accessible properties | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ddkilzer, eric | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Oliver Hunt
2010-03-20 16:26:36 PDT
Created attachment 51238 [details]
Patch
Comment on attachment 51238 [details]
Patch
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
(In reply to comment #2) > (From update of attachment 51238 [details]) > 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 I'm not sure about adding references to the new API from actual API until the new API has gone through review. > > > 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? i'll make it return bool as setPrivate does > it looks like JSObjectGetPrivateProperty will return garbage when called on a > non-API object > I think it would make more sense to return undefined It returns NULL > Silent failure on delete seems ok I'll return bool to be consistent with SetPrivateProperty > > Testing: > > you could try harder in the test case to avoid the possibility that a copy of > aHeapRef remains on the stack It does get blown from the stack -- i verified the test failed before i added marking support :D > I think you should test calling these on non-API objects in the test case Done. Created attachment 51240 [details]
Patch
Comment on attachment 51240 [details]
Patch
Please add @result to the headerdoc for JSObjectSetPrivateProperty. Otherwise r=me.
Committed r56314: <http://trac.webkit.org/changeset/56314> This appears to have broken the windows build. (In reply to comment #7) > This appears to have broken the windows build. Blind attempt #1 to fix the Windows build (failed): <http://trac.webkit.org/changeset/56317> Blind attempt #2 (worked): <http://trac.webkit.org/changeset/56318> |