Expose the back/forward list via the UI process Cocoa API
Created attachment 214765 [details] Add WKBackForwardList and WKBackForwardListItem
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.
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.
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
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>.
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?
(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.