Bug 146658 - Update default link action sheets for app links
Summary: Update default link action sheets for app links
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-06 15:56 PDT by Joseph Pecoraro
Modified: 2015-07-11 16:00 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (11.48 KB, patch)
2015-07-06 16:11 PDT, Joseph Pecoraro
enrica: review+
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (21.89 KB, patch)
2015-07-11 03:33 PDT, Joseph Pecoraro
mitz: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-07-06 15:56:56 PDT
* 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.
Comment 1 Joseph Pecoraro 2015-07-06 15:57:19 PDT
<rdar://problem/21221902>
Comment 2 Joseph Pecoraro 2015-07-06 16:11:22 PDT
Created attachment 256249 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2015-07-06 16:12:30 PDT
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.
Comment 4 Enrica Casucci 2015-07-06 18:23:44 PDT
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.
Comment 5 mitz 2015-07-06 18:26:53 PDT
(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 6 mitz 2015-07-06 18:29:50 PDT
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 7 mitz 2015-07-06 18:31:54 PDT
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 8 mitz 2015-07-06 18:35:54 PDT
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?
Comment 9 Joseph Pecoraro 2015-07-06 23:21:17 PDT
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?
Comment 10 Joseph Pecoraro 2015-07-11 03:33:39 PDT
Created attachment 256653 [details]
[PATCH] Proposed Fix

Newer version.
Comment 11 WebKit Commit Bot 2015-07-11 03:36:37 PDT
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.
Comment 12 Joseph Pecoraro 2015-07-11 16:00:19 PDT
https://bugs.webkit.org/show_bug.cgi?id=146658
Comment 13 Joseph Pecoraro 2015-07-11 16:00:44 PDT
Wrong link! I meant to paste: r186718

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