Bug 123109 – [Cocoa] Back/forward list UI process API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

Expose the back/forward list via the UI process Cocoa API

Created attachment 214765 [details]
Add WKBackForwardList and WKBackForwardListItem

[WebKit Commit Bot]

Attachment 214765 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/UIProcess/API/C/WKBackForwardList.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardList.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItem.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItem.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItemRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItemRef.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.h', u'Source/WebKit2/UIProcess/API/C/WebKit2_C.h', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.h', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:43:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/C/WebKit2_C.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 13 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 214765 [details]
Add WKBackForwardList and WKBackForwardListItem

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.

[EFL EWS Bot]

Comment on attachment 214765 [details]
Add WKBackForwardList and WKBackForwardListItem

Attachment 214765 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/8908074

Comment on attachment 214765 [details]
Add WKBackForwardList and WKBackForwardListItem

View in context: https://bugs.webkit.org/attachment.cgi?id=214765&action=review

Thanks for the review. I am going to address some of your comments (see below) and fix other builds and post a new patch.

>> Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm:50
>> +        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?

I don’t think the non-uniqueness is going to turn into a problem. We can consider how to address it if it does.

>> 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?

Not sure why I wrote it this way. I will change to get().

>> 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.

By then we’d have already allocated a WKObject which we’d need to deallocate if we wanted to return an instance of a different class.

>> Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm:261
>> +    return nil;
> 
> Most other similar methods in this patch are using early return. I think that would be better here too.

I will change that.

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

Fixed.

Created attachment 214773 [details]
Add WKBackForwardList and WKBackForwardListItem

[WebKit Commit Bot]

Attachment 214773 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/UIProcess/API/C/WKBackForwardList.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardList.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItem.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItem.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItemRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListItemRef.h', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.h', u'Source/WebKit2/UIProcess/API/C/WebKit2_C.h', u'Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list.cpp', u'Source/WebKit2/UIProcess/API/efl/ewk_back_forward_list_item.cpp', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.h', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:43:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/C/WebKit2_C.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/CMakeLists.txt:356:  Alphabetical sorting problem. "UIProcess/API/C/WKBackForwardListItemRef.cpp" should be before "UIProcess/API/C/WKBackForwardListRef.cpp".  [list/order] [5]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.h:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/C/WKBackForwardListRef.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 14 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #7)
> Attachment 214773 [details] did not pass style-queue:
[…]
 > Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.h:37:  Extra space before ( in function call  [whitespace/parens] [4]
[…]
 > If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 123117.

Committed <http://trac.webkit.org/r157748>.

[Darin Adler]

Comment on attachment 214765 [details]
Add WKBackForwardList and WKBackForwardListItem

View in context: https://bugs.webkit.org/attachment.cgi?id=214765&action=review

>>> 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.
> 
> By then we’d have already allocated a WKObject which we’d need to deallocate if we wanted to return an instance of a different class.

Agreed. That is a common pattern, though, used by, for example, Foundation class clusters. I guess if we don’t have to do the extra allocation then it’s nice not to.

Should we have some assertions in WKObject initWithAPIObject: to make sure we never blow it?

(In reply to comment #10)
> (From update of attachment 214765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214765&action=review
> 
> >>> 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.
> > 
> > By then we’d have already allocated a WKObject which we’d need to deallocate if we wanted to return an instance of a different class.
> 
> Agreed. That is a common pattern, though, used by, for example, Foundation class clusters. I guess if we don’t have to do the extra allocation then it’s nice not to.

A common pattern in Foundation class clusters is to override +alloc.

> Should we have some assertions in WKObject initWithAPIObject: to make sure we never blow it?

Sorry, I don’t understand. Can you give me an example of what we could assert?

[Darin Adler]

(In reply to comment #11)
> A common pattern in Foundation class clusters is to override +alloc.

OK. My mistake. I thought there were init methods that deallocated self and allocated a replacement. I thought that was the reason for the "self = [super init]" idiom.

> > Should we have some assertions in WKObject initWithAPIObject: to make sure we never blow it?
> 
> Sorry, I don’t understand. Can you give me an example of what we could assert?

Something like this, perhaps?

ASSERT((_object->type() == APIObject::TypeBackForwardListItem) == [self isKindOfClass:[WKBackForwardListItem class]]);

Not really sure.

(In reply to comment #12)
> (In reply to comment #11)
> > A common pattern in Foundation class clusters is to override +alloc.
> 
> OK. My mistake. I thought there were init methods that deallocated self and allocated a replacement. I thought that was the reason for the "self = [super init]" idiom.
> 
> > > Should we have some assertions in WKObject initWithAPIObject: to make sure we never blow it?
> > 
> > Sorry, I don’t understand. Can you give me an example of what we could assert?
> 
> Something like this, perhaps?
> 
> ASSERT((_object->type() == APIObject::TypeBackForwardListItem) == [self isKindOfClass:[WKBackForwardListItem class]]);
> 
> Not really sure.

Darin explained to me in person that the motivation is to ensure that generic wrappers aren’t created when a specific wrapper should be created. I noted that the only code that could make this mistake would have to be in WKNSObjectExtras.m. We can either use better naming or remove -initWithAPIObject: if we want to prevent such mistakes, if we think people could make them.