[webkit-reviews] review granted: [Bug 18205] DOMNode objects are garbage collected although there are strong references : [Attachment 25982] Fix for the resurrection bug of a varity of wrapper objects

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kai Brüning <kai at granus.net>'s
request for review:
Bug 18205: DOMNode objects are garbage collected although there are strong
references
https://bugs.webkit.org/show_bug.cgi?id=18205

Attachment 25982: Fix for the resurrection bug of a varity of wrapper objects
https://bugs.webkit.org/attachment.cgi?id=25982&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looking good.

> +2008-12-12  Kai  <set EMAIL_ADDRESS environment variable>

This needs your full name and email address.

> +	   WARNING: NO TEST CASES ADDED OR CHANGED

You should remove this warning and replace it with the explanation of why
there's no regression test.

The change log should also mention the URL of the bug.

> +#ifdef BUILDING_ON_TIGER
> +// Declarations needed by createWrapperCache()
> +enum {
> +    NSPointerFunctionsOpaquePersonality  = (1 << 8),
> +    NSPointerFunctionsIntegerPersonality = (5 << 8)
> +};
> +typedef NSUInteger NSPointerFunctionsOptions;
> +#endif

Will these constants actually work on Tiger? Can they be defined in terms of
something in a header that will be present on Tiger?

I think we should just have two functions, one for object keys and one for
integer keys. Inside the file they can both call a common function.

Or if you really think that one function is best, we should just use a boolean
(false for objects, true for integers) or our own enum for the argument to our
createWrapperCache function; if would be good to keep this relatively messy
thing out of the header.

The patch otherwise looks great. I think I'm going to say review+ and land this
myself with the fixes above.