[webkit-reviews] review granted: [Bug 195225] Check whether to launch a default action instead of action sheet : [Attachment 363875] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jennifer Moore
<Jennifer.moore at apple.com>'s request for review:
Bug 195225: Check whether to launch a default action instead of action sheet
https://bugs.webkit.org/show_bug.cgi?id=195225

Attachment 363875: Patch

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




--- 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/ActionShee
tTests.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->u
rl))
> +	   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.