Bug 146658 – Update default link action sheets for app links

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Joseph Pecoraro]

* SUMMARY
Update the default action sheets for links to include an "Open in “App Name"" action in addition to the usual "Open" action for links that have an associated app link.

[Joseph Pecoraro]

<rdar://problem/21221902>

[Joseph Pecoraro]

Created attachment 256249 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.h:40
> +    _WKElementActionTypeOpenInExternalApplication,

We could wrap this enum value in a HAVE(APP_LINKS) guard, kind of like the AddToReadingList value below. Let me know what you think.

[Enrica Casucci]

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

The patch looks good to me.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.h:40
>> +    _WKElementActionTypeOpenInExternalApplication,
> 
> We could wrap this enum value in a HAVE(APP_LINKS) guard, kind of like the AddToReadingList value below. Let me know what you think.

That sounds right.

(In reply to comment #4)
> Comment on attachment 256249 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256249&action=review
> 
> The patch looks good to me.
> 
> >> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.h:40
> >> +    _WKElementActionTypeOpenInExternalApplication,
> > 
> > We could wrap this enum value in a HAVE(APP_LINKS) guard, kind of like the AddToReadingList value below. Let me know what you think.
> 
> That sounds right.

We can’t use the HAVE macro in a framework header.

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/English.lproj/Localizable.strings:413
> +"Open in External Application ActionSheet Link" = "Open in â%@â";

This is not an appropriate key. The key should generally be the same as the value. If there’s ambiguity, the key should be the same as the value plus a disambiguating parenthetical, but in this case there’s no ambiguity.

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

>>>> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.h:40
>>>> +    _WKElementActionTypeOpenInExternalApplication,
>>> 
>>> We could wrap this enum value in a HAVE(APP_LINKS) guard, kind of like the AddToReadingList value below. Let me know what you think.
>> 
>> That sounds right.
> 
> We can’t use the HAVE macro in a framework header.

An availability annotation is required here.

> Source/WebKit2/UIProcess/ApplicationStateTracker.h:44
> +    bool hasEntitlement(NSString *entitlement) const;

Why is this a member function?

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:89
> +static _WKElementAction *openInExternalApplicationElementActionForURL(NSURL *url)
> +{
> +    if (!applicationHasCanGetAppLinkEntitlement())
> +        return nil;
> +
> +    __block _WKElementAction *action = nil;
> +
> +    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
> +    [LSAppLink getAppLinkWithURL:url completionHandler:^(LSAppLink *appLink, NSError *error) {
> +        if (appLink) {
> +            NSString *localizedAppName = [appLink.targetApplicationProxy localizedNameForContext:nil];
> +            NSString *title = [NSString stringWithFormat:WEB_UI_STRING_KEY("Open in â%@â", "Open in External Application ActionSheet Link", "Title for Open in External Application Link action button"), localizedAppName];
> +            action = [[_WKElementAction elementActionWithType:_WKElementActionTypeOpenInExternalApplication customTitle:title] retain];
> +        }
> +        dispatch_semaphore_signal(semaphore);
> +    }];
> +    dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
> +
> +    return [action autorelease];
> +}

Can we start doing this work at the touch down time so that by the time we need to show a sheet we may already have this information and not need to wait for it?

[Joseph Pecoraro]

Comment on attachment 256249 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/English.lproj/Localizable.strings:413
>> +"Open in External Application ActionSheet Link" = "Open in â%@â";
> 
> This is not an appropriate key. The key should generally be the same as the value. If there’s ambiguity, the key should be the same as the value plus a disambiguating parenthetical, but in this case there’s no ambiguity.

I'll update this. I was following the pattern of all the other ActionSheet links which had this style:

  /* Title for Copy Link or Image action button */
  "Copy ActionSheet Link" = "Copy";

  /* Title for Open Link action button */
  "Open ActionSheet Link" = "Open";

>> Source/WebKit2/UIProcess/ApplicationStateTracker.h:44
>> +    bool hasEntitlement(NSString *entitlement) const;
> 
> Why is this a member function?

Laziness. Is there a more appropriate place to put a general hasEntitlement function, or would just a static method on this class be fine?

[Joseph Pecoraro]

Created attachment 256653 [details]
[PATCH] Proposed Fix

Newer version.

[WebKit Commit Bot]

Attachment 256653 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:76:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:301:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:303:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:310:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:312:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.mm:95:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Wrong link! I meant to paste: r186718

http://trac.webkit.org/changeset/186718