[Webkit-unassigned] [Bug 195225] Check whether to launch a default action instead of action sheet

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=195225

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #363875|review?                     |review+
              Flags|                            |

--- Comment #14 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 363875
  --> https://bugs.webkit.org/attachment.cgi?id=363875
Patch

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

Can we write tests for this code change? We seem to have existing ActionSheet tests for iOS in <https://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/ios/ActionSheetTests.mm?rev=242468>.

> Source/WebCore/PAL/ChangeLog:9
> +        Add more new SPI declarations

OK as-is. Missing period.

> Source/WebKit/ChangeLog:15
> +        (-[WKActionSheetAssistant _createSheetWithElementActions:defaultTitle:showLinkTitle:]):

In my opinion, I suggest you add a inline comment here to describe the purpose of this defaultTitle, maybe say something like this parameter is only used by Data Detectors, etc.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:286
> +    if (!_delegate)
> +        return;
> +
> +    if (![self synchronouslyRetrievePositionInformation])
> +        return;
> +
> +    if (!WebCore::DataDetection::canBePresentedByDataDetectors(_positionInformation->url))
> +        return;
> +
> +    NSURL *targetURL = _positionInformation->url;
> +    if (!targetURL)
> +        return;

Let's not duplicate. Extract method in -showDataDetectorsSheet and share code.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:288
> +    auto controller = [getDDDetectionControllerClass() sharedController];

auto*?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:335
> +    } else if (title)
> +        titleString = title;

This is ok as-is. I just can't help myself, but think maybe a better argument name than "title" could help someone better understand the purpose of this code, which you told me is just for Data Detectors, given that this function is being shared for both Data Detectors and other things.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:609
> +    if ([_delegate respondsToSelector:@selector(dataDetectionContextForActionSheetAssistant:)])
> +        context = [_delegate dataDetectionContextForActionSheetAssistant:self];
> +    if ([_delegate respondsToSelector:@selector(selectedTextForActionSheetAssistant:)])
> +        textAtSelection = [_delegate selectedTextForActionSheetAssistant:self];

Nice.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:630
> +        auto retainedSelf = retainPtr(self);
> +        _WKElementAction *elementAction = [_WKElementAction elementActionWithTitle:action.localizedName actionHandler:^(_WKActivatedElementInfo *actionInfo) {
> +            retainedSelf->_isPresentingDDUserInterface = action.hasUserInterface;
> +            [[getDDDetectionControllerClass() sharedController] performAction:action fromAlertController:retainedSelf->_interactionSheet.get() interactionDelegate:retainedSelf.get()];
> +        }];
> +        elementAction.dismissalHandler = ^BOOL {
> +            return !action.hasUserInterface;
> +        };

We shouldn't duplicate this. Extract into function or block or member function and share.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:632
> +        [self cleanupSheet];

Something about this line does not feel right because we may not have an interactionSheet. The code below bails out in this case and never calls -cleanupSheet. Calling -cleanupSheet seems to notify the delegate unconditionally, not sure if this could cause bad things.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:655
> +    NSString *title = [controller titleForURL:targetURL results:nil context:context];
> +    [self _createSheetWithElementActions:elementActions defaultTitle:title showLinkTitle:NO];

This is ok as-is. Maybe inlining would be just as readable. Clearly, name of variable does not add anything because  -titleForURL is self describing.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190307/e76fc793/attachment.html>