Check whether to launch a default action instead of action sheet

• Component: WebKit2
• Status: RESOLVED FIXED

[Jennifer Moore]

<rdar://problem/47715544>

[Jennifer Moore]

Created attachment 363375 [details]
Patch

[Wenson Hsieh]

Comment on attachment 363375 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:639
> +        DDAction *action = [controller defaultActionForURL:targetURL results:nil context:context];

It looks like this method will need to be guarded as well.

[Daniel Bates]

Comment on attachment 363375 [details]
Patch

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

This patch doesn’t look correct. It contains computations that are not used and that is just weird.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:285
> +    NSURL *targetURL = _positionInformation->url;

auto?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:292
> +    RetainPtr<NSMutableDictionary> extendedContext;

Too far from its use. Please move.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:297
> +        textAtSelection = [delegate selectedTextForActionSheetAssistant:self];

We don’t even use textAtSelection. Just assign to it. Why have it then?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:299
> +        extendedContext = adoptNS([@{ getkDataDetectorsLeadingText() : _positionInformation->textBefore, getkDataDetectorsTrailingText() : _positionInformation->textAfter } mutableCopy]);

auto extendedContent = ...
(If you take my suggestion and remove line 292)

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:302
> +        context = extendedContext.get();

What is going on here? |context| is not used after this point. So, why are we doing all this work to compute it?

[Jennifer Moore]

Created attachment 363511 [details]
Patch

[Jennifer Moore]

Created attachment 363648 [details]
Patch

[Jennifer Moore]

Created attachment 363650 [details]
Patch

[EWS Watchlist]

Comment on attachment 363650 [details]
Patch

Attachment 363650 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11380890

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 363662 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Wenson Hsieh]

Comment on attachment 363662 [details]
Archive of layout-test-results from ews112 for mac-highsierra

This macOS test failure looks unrelated.

[Daniel Bates]

Comment on attachment 363650 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:275
> +    auto delegate = _delegate.get();

.get() is not needed. This local is not needed as its only used in this line. Just inline right hand side.

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

auto?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:624
> +        DDAction *action = [controller defaultActionForURL:targetURL results:nil context:context];

auto?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:625
> +        RetainPtr<WKActionSheetAssistant> retainedSelf = self;

auto retainedSelf = retainPtr(self);

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:626
> +        _WKElementAction *elementAction = [_WKElementAction elementActionWithTitle:[action localizedName] actionHandler:^(_WKActivatedElementInfo *actionInfo) {

Dot notation please whenever possible in objective C.

auto?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:627
> +            retainedSelf.get()->_isPresentingDDUserInterface = action.hasUserInterface;

.get() not necessary

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:628
> +            [[getDDDetectionControllerClass() sharedController] performAction:action fromAlertController:retainedSelf.get()->_interactionSheet.get() interactionDelegate:retainedSelf.get()];

Ditto for second argument.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:631
> +            return (BOOL)!action.hasUserInterface;

Cast is unnecessary. Or if it is necessary then move return type to before block start to remove the cast.

[Jennifer Moore]

Created attachment 363875 [details]
Patch

[Jennifer Moore]

Just want to mention about the style comments, which is that most of the changes here were copied from other places in the code. Should I be making the same fixes (auto, no get, etc.) everywhere else as well?

[Tim Horton]

(In reply to Jennifer Moore from comment #12)
> Just want to mention about the style comments, which is that most of the
> changes here were copied from other places in the code. Should I be making
> the same fixes (auto, no get, etc.) everywhere else as well?

No, only in code that you're changing otherwise.

But you do need to get EWS green :)

[Daniel Bates]

Comment on attachment 363875 [details]
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.

[Jennifer Moore]

Created attachment 363942 [details]
Patch

[Jennifer Moore]

(In reply to Tim Horton from comment #13)
> (In reply to Jennifer Moore from comment #12)
> > Just want to mention about the style comments, which is that most of the
> > changes here were copied from other places in the code. Should I be making
> > the same fixes (auto, no get, etc.) everywhere else as well?
> 
> No, only in code that you're changing otherwise.
> 
> But you do need to get EWS green :)

Thanks, what does that mean exactly?

[Tim Horton]

It means that so, far, no version of your patch has built on the iOS build bots. See the red bubble next to the attachment? You can click there to see what the build errors are.

[Jennifer Moore]

Ah ha, thanks!

[Daniel Bates]

Comment on attachment 363942 [details]
Patch

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

The refactoring is bad in my opinion and makes things worse. I think the right refactoring is everything except the target URL computation. Haven’t thought it through but I would rather take the duplication then this refactor

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:-564
> -    auto delegate = _delegate.get();

This looks like a bad refactor. Should check delegate in this function too.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:604
> +        || !WebCore::DataDetection::canBePresentedByDataDetectors(targetURL)) return;

Formatting doesn’t look right. Grep code for examples or read style guide

[Jennifer Moore]

Created attachment 363944 [details]
Patch

[Jennifer Moore]

(In reply to Daniel Bates from comment #19)
> Comment on attachment 363942 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363942&action=review
> 
> The refactoring is bad in my opinion and makes things worse. I think the
> right refactoring is everything except the target URL computation. Haven’t
> thought it through but I would rather take the duplication then this refactor
> 
> > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:-564
> > -    auto delegate = _delegate.get();
> 
> This looks like a bad refactor. Should check delegate in this function too.
> 
> > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:604
> > +        || !WebCore::DataDetection::canBePresentedByDataDetectors(targetURL)) return;
> 
> Formatting doesn’t look right. Grep code for examples or read style guide

This formatting was required by the upload script

[Daniel Bates]

Comment on attachment 363944 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:184
> +- (_WKElementAction *)_elementActionForDDAction:(DDAction *)action

Nice. Good refactor.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:187
> +    _WKElementAction *elementAction = [_WKElementAction elementActionWithTitle:action.localizedName actionHandler:^(_WKActivatedElementInfo *actionInfo) {

Auto? And right-hand-side should be wrapped in adoptNS()

[Daniel Bates]

Comment on attachment 363944 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:187
>> +    _WKElementAction *elementAction = [_WKElementAction elementActionWithTitle:action.localizedName actionHandler:^(_WKActivatedElementInfo *actionInfo) {
> 
> Auto? And right-hand-side should be wrapped in adoptNS()

Eh, no need for adoptNS(). should use auto* (not auto)

[Daniel Bates]

(In reply to Jennifer Moore from comment #21)
> (In reply to Daniel Bates from comment #19)
> > Comment on attachment 363942 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=363942&action=review
> > 
> > The refactoring is bad in my opinion and makes things worse. I think the
> > right refactoring is everything except the target URL computation. Haven’t
> > thought it through but I would rather take the duplication then this refactor
> > 
> > > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:-564
> > > -    auto delegate = _delegate.get();
> > 
> > This looks like a bad refactor. Should check delegate in this function too.
> > 
> > > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:604
> > > +        || !WebCore::DataDetection::canBePresentedByDataDetectors(targetURL)) return;
> > 
> > Formatting doesn’t look right. Grep code for examples or read style guide
> 
> This formatting was required by the upload script

Don't know what upload script you are referring to, maybe webkit-patch (which calls check-webkit-style) <--- though no way check-webkit-style would suggest putting return on same line. Well, it must have based on your facts. That's a terrible bug. Anyway, the answer is this code is wrongly formatter. Read code style guide.

[Jennifer Moore]

Created attachment 363953 [details]
Patch

[Jennifer Moore]

Any final comments?

After discussing with Andy Estes, it looks like the win test failures are not related to this patch

[Tim Horton]

Comment on attachment 363953 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1220
> +        [_actionSheetAssistant interactionDidStart];

I apologize for not looking more carefully at this patch earlier.

-interactionDidStart is called on every touchBegin, and calls -synchronouslyRetrievePositionInformation, which is incredibly expensive and blocks the main thread. We can't do that.

[Tim Horton]

Comment on attachment 363953 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:291
> +    if (![self synchronouslyRetrievePositionInformation])

I think if you use WKContentViewInteraction's doAfterPositionInformationUpdate, you should be good.

[Jennifer Moore]

Created attachment 364059 [details]
Patch

[Jennifer Moore]

Created attachment 364254 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364254 [details]
Patch

Clearing flags on attachment: 364254

Committed r242801: <https://trac.webkit.org/changeset/242801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Additional build fixes:
https://trac.webkit.org/changeset/242805/webkit
https://trac.webkit.org/changeset/242806/webkit
https://trac.webkit.org/changeset/242824/webkit