[webkit-reviews] review granted: [Bug 123394] [Cocoa] Make all API objects have Cocoa wrappers, and delegate refcounting to those wrappers : [Attachment 215482] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org <mitz at webkit.org>'s
request for review:
Bug 123394: [Cocoa] Make all API objects have Cocoa wrappers, and delegate
refcounting to those wrappers
https://bugs.webkit.org/show_bug.cgi?id=123394

Attachment 215482: Give APIObject an Objective-C wrapper and make wrappers
manage the lifetime of API objects
https://bugs.webkit.org/attachment.cgi?id=215482&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215482&action=review


> Source/WebKit2/Shared/APIObject.h:31
> +#if PLATFORM(MAC)

Maybe we should start phasing out PLATFORM(MAC) for these purposes and
replacing it with USE(COCOA). At first the two would be identical. Then, some
day in the future we could make PLATFORM(MAC) be false on iOS.

> Source/WebKit2/Shared/APIObject.h:38
> +#if !(PLATFORM(MAC) && WK_API_ENABLED)

For clarity, it might be worthwhile to define a derived constant for this
header:

    #define DELEGATE_REF_COUNTING_TO_COCOA (PLATFORM(MAC) && WK_API_ENABLED)

We could then #undef it at the bottom of the header.

> Source/WebKit2/Shared/APIObject.h:163
> +    void* wrapper() { return m_wrapper; }

We should consider using objc_object* here instead of void*, to avoid all the
(id) casts we otherwise need. With appropriate forward declaration so we can
compile it in plain C++. Or even NSObject * and use the OBJC_CLASS technique.

> Source/WebKit2/Shared/APIObject.h:177
> +    void* operator new(size_t) = delete;

Does this actually work and create a compile time error if you forget to
override operator new? I remember trying this a long time ago to force arena
allocation, and I failed.

> Source/WebKit2/Shared/Cocoa/APIObject.mm:51
> +    switch (type) {

Maybe need a comment about the different idioms needed for variable-sized vs.
fixed-sized object? Otherwise it’s mysterious why these are all different.

If there was no need for the two different idioms I would suggest factoring the
code differently so that we would just map the type to the wrapper class rather
than having separate code to allocate each type of wrapper.

> Source/WebKit2/Shared/Cocoa/WKNSArray.h:34
> +inline NSArray *wrapper(ImmutableArray& item) { return (NSArray
*)item.wrapper(); }

Unfortunate that the typecast is here in a non-member function far from the
guarantee that the wrapper has the appropriate class. It would be nice if it
was in ImmutableArray itself did the typecast, but I guess even that isn’t all
that close to the guarantee of the wrapper type. And I suppose that doing the
cast here has the benefit of keeping it out of the platform-independent header.


Also, would be nice to have an assertion that the cast is correct.

> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:34
> +    uint8_t _array[sizeof(ImmutableArray)];

Does this guarantee sufficient alignment? In WTF we use std::aligned_storage to
guarantee that. I also think that the field should be named _arrayStorage so
it’s clearer why a typecast is needed at each place that uses it.

> Source/WebKit2/Shared/Cocoa/WKNSURL.mm:41
> +    return (NSURL *)CFMakeCollectable(WKURLCopyCFURL(kCFAllocatorDefault,
toAPI(reinterpret_cast<WebURL*>(&self._apiObject))));

I don’t think we support GC with WebKit2, so maybe you could just omit
CFMakeCollectable entirely.

We should be using CFBridgingRelease rather than CFMakeCollectable in call
sites like these now. I was surprised to find some CFMakeCollectable calls
still in our code. But I guess CFBridgingRelease would get the retain/release
count wrong and you’d need to also call retain.

> Source/WebKit2/Shared/Cocoa/WKObject.mm:37
> +    BOOL _initializedTarget;
> +    NSObject *_target;

Any reason we can’t just use null for _target instead of a separate boolean? I
suppose there is.

Also, not 100% happy with “initialized target” as the field name. Maybe
_hasInitializedTarget?

> Source/WebKit2/Shared/Cocoa/WKObject.mm:136
> +    return nil;

Should this also be doing an “assert not reached” sort of thing? Or are there
some that have no target?

I think it’s a little confusing that our wrapper is called our “target”.