Adopt Context Menus for Data Detectors
Created attachment 392914 [details] Patch
Comment on attachment 392914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392914&action=review > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.h:65 > +- (CGPoint)contextMenuDisplayLocation; Cocoa tradition is to pass the object that's delegating behavior to the delegate methods. You even realized why, earlier (so that you can have one object be the delegate of multiple instances of the same class). See all of the others above. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:738 > +- (_UIContextMenuStyle *)_contextMenuInteraction:(UIContextMenuInteraction *)interaction styleForMenuWithConfiguration:(UIContextMenuConfiguration *)configuration Missing a newline
Created attachment 392918 [details] Patch
Comment on attachment 392918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392918&action=review > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:691 > + [self ensureContextMenuInteraction]; Seems bad that we make and install it, and then don't present it, which means we won't remove it. I think this needs to be JUST before the -present > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:694 > + if ([delegate respondsToSelector:@selector(contextMenuDisplayLocation)]) { > + [_dataDetectorContextMenuInteraction _presentMenuAtLocation:[delegate contextMenuDisplayLocation]]; You forgot to change the name here
<rdar://problem/59358079>
Created attachment 392935 [details] Patch
Created attachment 392937 [details] Patch
Comment on attachment 392937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392937&action=review > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.h:65 > +- (CGPoint)contextMenuDisplayLocationForActionSheetAssistant:(WKActionSheetAssistant *)assistant; Maybe "presentation" or "origin" instead of "display", but I don't really care. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:719 > +- (UIContextMenuConfiguration *)contextMenuInteraction:(UIContextMenuInteraction *)interaction configurationForMenuAtLocation:(CGPoint)location USE(UICONTEXTMENU)? > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:747 > + if ([delegate respondsToSelector:@selector(contextMenuDisplayLocationForActionSheetAssistant:)]) > + center = [delegate contextMenuDisplayLocationForActionSheetAssistant:self]; > + RetainPtr<UIPreviewParameters> unusedPreviewParameters = adoptNS([[UIPreviewParameters alloc] init]); > + RetainPtr<UIPreviewTarget> previewTarget = adoptNS([[UIPreviewTarget alloc] initWithContainer:_view.getAutoreleased() center:center]); > + RetainPtr<UITargetedPreview> preview = adoptNS([[UITargetedPreview alloc] initWithView:_view.getAutoreleased() parameters:unusedPreviewParameters.get() target:previewTarget.get()]); How ugly is the hint? We probably want to share some of this with the better code in WKContentViewInteraction, but that can be in a follow-up
Comment on attachment 392937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392937&action=review > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:645 > + > +- (void)ensureContextMenuInteraction Generally, -ensure* methods should return the object they’re ensuring (in this case, the _dataDetectorContextMenuInteraction). I think that if you added a return _dataDetectorContextMenuInteraction.get(); at the end here, it would allow you to then do this below: auto delegate = _delegate.get(); if ([delegate respondsToSelector:@selector(contextMenuDisplayLocationForActionSheetAssistant:)]) { [[self ensureContextMenuInteraction] _presentMenuAtLocation:[delegate contextMenuDisplayLocationForActionSheetAssistant:self]]; return; } I believe this would also address Tim’s earlier comment in https://bugs.webkit.org/show_bug.cgi?id=208779#c4.
Created attachment 392938 [details] Patch
Created attachment 392939 [details] Patch
Created attachment 392940 [details] Patch
Created attachment 392941 [details] Patch
Created attachment 392944 [details] Patch for landing
Comment on attachment 392944 [details] Patch for landing Clearing flags on attachment: 392944 Committed r258102: <https://trac.webkit.org/changeset/258102>
All reviewed patches have been landed. Closing bug.