Bug 208779 – Adopt Context Menus for Data Detectors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Adopt Context Menus for Data Detectors

[Megan Gardner]

Created attachment 392914 [details]
Patch

[Tim Horton]

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

[Megan Gardner]

Created attachment 392918 [details]
Patch

[Tim Horton]

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

[Megan Gardner]

<rdar://problem/59358079>

[Megan Gardner]

Created attachment 392935 [details]
Patch

[Megan Gardner]

Created attachment 392937 [details]
Patch

[Tim Horton]

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

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 392938 [details]
Patch

[Megan Gardner]

Created attachment 392939 [details]
Patch

[Megan Gardner]

Created attachment 392940 [details]
Patch

[Megan Gardner]

Created attachment 392941 [details]
Patch

[Megan Gardner]

Created attachment 392944 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392944 [details]
Patch for landing

Clearing flags on attachment: 392944

Committed r258102: <https://trac.webkit.org/changeset/258102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.