Bug 195225 - Check whether to launch a default action instead of action sheet
Summary: Check whether to launch a default action instead of action sheet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-01 13:49 PST by Jennifer Moore
Modified: 2019-03-12 15:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.48 KB, patch)
2019-03-01 14:29 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2019-03-04 08:15 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (9.91 KB, patch)
2019-03-05 09:46 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2019-03-05 10:01 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (2.39 MB, application/zip)
2019-03-05 11:42 PST, EWS Watchlist
no flags Details
Patch (9.96 KB, patch)
2019-03-07 08:16 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2019-03-07 15:49 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (13.43 KB, patch)
2019-03-07 15:59 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (12.73 KB, patch)
2019-03-07 16:30 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2019-03-08 13:47 PST, Jennifer Moore
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2019-03-11 08:40 PDT, Jennifer Moore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jennifer Moore 2019-03-01 13:49:49 PST
<rdar://problem/47715544>
Comment 1 Jennifer Moore 2019-03-01 14:29:47 PST
Created attachment 363375 [details]
Patch
Comment 2 Wenson Hsieh 2019-03-01 15:08:05 PST
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.
Comment 3 Daniel Bates 2019-03-02 15:51:55 PST
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?
Comment 4 Jennifer Moore 2019-03-04 08:15:55 PST
Created attachment 363511 [details]
Patch
Comment 5 Jennifer Moore 2019-03-05 09:46:56 PST
Created attachment 363648 [details]
Patch
Comment 6 Jennifer Moore 2019-03-05 10:01:51 PST
Created attachment 363650 [details]
Patch
Comment 7 EWS Watchlist 2019-03-05 11:42:15 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-03-05 11:42:17 PST Comment hidden (obsolete)
Comment 9 Wenson Hsieh 2019-03-05 12:20:56 PST
Comment on attachment 363662 [details]
Archive of layout-test-results from ews112 for mac-highsierra

This macOS test failure looks unrelated.
Comment 10 Daniel Bates 2019-03-05 21:09:42 PST
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.
Comment 11 Jennifer Moore 2019-03-07 08:16:30 PST
Created attachment 363875 [details]
Patch
Comment 12 Jennifer Moore 2019-03-07 11:05:12 PST
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?
Comment 13 Tim Horton 2019-03-07 11:19:12 PST
(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 :)
Comment 14 Daniel Bates 2019-03-07 12:05:00 PST
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.
Comment 15 Jennifer Moore 2019-03-07 15:49:43 PST
Created attachment 363942 [details]
Patch
Comment 16 Jennifer Moore 2019-03-07 15:51:02 PST
(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?
Comment 17 Tim Horton 2019-03-07 15:53:47 PST
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.
Comment 18 Jennifer Moore 2019-03-07 15:56:16 PST
Ah ha, thanks!
Comment 19 Daniel Bates 2019-03-07 15:57:49 PST
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
Comment 20 Jennifer Moore 2019-03-07 15:59:49 PST
Created attachment 363944 [details]
Patch
Comment 21 Jennifer Moore 2019-03-07 16:01:23 PST
(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
Comment 22 Daniel Bates 2019-03-07 16:03:05 PST
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()
Comment 23 Daniel Bates 2019-03-07 16:05:00 PST
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)
Comment 24 Daniel Bates 2019-03-07 16:06:43 PST
(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.
Comment 25 Jennifer Moore 2019-03-07 16:30:44 PST
Created attachment 363953 [details]
Patch
Comment 26 Jennifer Moore 2019-03-08 09:44:39 PST
Any final comments?

After discussing with Andy Estes, it looks like the win test failures are not related to this patch
Comment 27 Tim Horton 2019-03-08 10:21:01 PST
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.
Comment 28 Tim Horton 2019-03-08 10:36:40 PST
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.
Comment 29 Jennifer Moore 2019-03-08 13:47:52 PST
Created attachment 364059 [details]
Patch
Comment 30 Jennifer Moore 2019-03-11 08:40:13 PDT
Created attachment 364254 [details]
Patch
Comment 31 WebKit Commit Bot 2019-03-12 10:42:04 PDT
Comment on attachment 364254 [details]
Patch

Clearing flags on attachment: 364254

Committed r242801: <https://trac.webkit.org/changeset/242801>
Comment 32 WebKit Commit Bot 2019-03-12 10:42:07 PDT
All reviewed patches have been landed.  Closing bug.