[webkit-reviews] review granted: [Bug 123109] [Cocoa] Back/forward list UI process API : [Attachment 214765] Add WKBackForwardList and WKBackForwardListItem

[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 123109: [Cocoa] Back/forward list UI process API
https://bugs.webkit.org/show_bug.cgi?id=123109

Attachment 214765: Add WKBackForwardList and WKBackForwardListItem
https://bugs.webkit.org/attachment.cgi?id=214765&action=review

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


> Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm:50
> +    if (!(self = [super init]))
> +	   return nil;

Should we “unique” these wrappers instead so that == works and we don’t have to
implement isEqual: and hash? Or is the locking needed for that too slow?

> Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm:69
> +    return (NSUInteger)&*_object;

Is &* really better syntax to use than get()? Should we get rid of get() maybe?


> Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm:88
> +    switch (object->type()) {

We could do this kind of thing inside [WKObject initWithAPIObject:] instead of
doing it like this.

> Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm:261
> +    if (WebBackForwardList* list = toImpl(self._pageRef)->backForwardList())

> +	   return [[[WKBackForwardList alloc] _initWithList:*list]
autorelease];
> +
> +    return nil;

Most other similar methods in this patch are using early return. I think that
would be better here too.

> Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm:46
> +    return  [[[WKBackForwardListItem alloc] _initWithItem:*item]
autorelease];

two spaces

> Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm:48
> +    return CFBridgingRelease(WKURLCopyCFURL(kCFAllocatorDefault,
adoptWK(toCopiedURLAPI(_item->url())).get()));

kCFAllocatorDefault has to be the most annoying long way to say “do the normal
thing” in all of Apple API.