Prompts user before transferring to App Store

ios/chrome/browser/web/external_app_launcher.mm

Index: ios/chrome/browser/web/external_app_launcher.mm
diff --git a/ios/chrome/browser/web/external_app_launcher.mm b/ios/chrome/browser/web/external_app_launcher.mm
index 20b9664a179f3a04190345d615830351f03f521a..25522118172b309ab0e928fe49f58dd68f02d488 100644
--- a/ios/chrome/browser/web/external_app_launcher.mm
+++ b/ios/chrome/browser/web/external_app_launcher.mm
@@ -6,6 +6,7 @@
 
 #include "base/ios/ios_util.h"
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/sys_string_conversions.h"
 #include "components/strings/grit/components_strings.h"
@@ -31,12 +32,12 @@ NSSet<NSString*>* ITMSSchemes() {
   static NSSet<NSString*>* schemes;
   static dispatch_once_t once;
   dispatch_once(&once, ^{
-    schemes =
-        [NSSet setWithObjects:@"itms", @"itmss", @"itms-apps", @"itms-appss",
-                              // There's no evidence that itms-bookss is
-                              // actually supported, but over-inclusion
-                              // costs less than under-inclusion.
-                              @"itms-books", @"itms-bookss", nil];
+    schemes = [NSSet<NSString*>
+        setWithObjects:@"itms", @"itmss", @"itms-apps", @"itms-appss",
+                       // There's no evidence that itms-bookss is actually
+                       // supported, but over-inclusion costs less than
+                       // under-inclusion.
+                       @"itms-books", @"itms-bookss", nil];
   });
   return schemes;
 }
@@ -70,15 +71,19 @@ NSString* PromptActionString(NSString* scheme) {
   return @"";
 }
 
+// Keys for dictionary parameters passed into -openExternalAppWithPromptForURL:.
+NSString* const kAlertPrompt = @"prompt";
+NSString* const kAlertAction = @"action";
+NSString* const kAlertURL = @"url";
+
 }  // namespace
 
 @interface ExternalAppLauncher ()
 // Returns the Phone/FaceTime call argument from |URL|.
 + (NSString*)formatCallArgument:(NSURL*)URL;
-// Ask user for confirmation before dialing Phone or FaceTime destinations.
-- (void)openPromptForURL:(NSURL*)URL;
-// Ask user for confirmation before moving to external app.
-- (void)openExternalAppWithPromptForURL:(NSURL*)URL;
+// Asks user for confirmation before moving to external app. |params| is a
+// dictionary keyed with values of kAlert* above.
+- (void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params;

[Eugene But (OOO till 7-30), 2017/05/12 22:53:51]

Do you use NSDictionary for passing the arguments, because of |performSelector:|
API limitation? Would it be better to use GCD's dispatch_async instead?

[pkl (ping after 24h if needed), 2017/05/12 23:16:11]

As the first pass, I'm just adding the alert prompts under the same structure.

I suspect the delay may no longer be necessary given that the old comments
mentioned UIWebView. I have a follow up CL that just removes the delay
completely and test if it works or not. If the delay is still required, I'll
switch to use dispatch_async() then.

[Eugene But (OOO till 7-30), 2017/05/13 00:27:52]

SG. In case if using GCD will not be possible, please rename the method to
conform to Cocoa Coding Guidelines: "Make the word before the argument describe
the argument".

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co...

e.g. openExternalAppWithPromptWithParams:

[pkl (ping after 24h if needed), 2017/05/17 22:23:02]

Renamed now.

 // Presents a configured alert controller on the root view controller.
 - (void)presentAlertControllerWithMessage:(NSString*)message
                                 openTitle:(NSString*)openTitle
@@ -104,12 +109,13 @@ NSString* PromptActionString(NSString* scheme) {
   return prompt;
 }
 
-- (void)openExternalAppWithPromptForURL:(NSURL*)URL {
-  NSString* message = l10n_util::GetNSString(IDS_IOS_OPEN_IN_ANOTHER_APP);
-  NSString* openTitle =
-      l10n_util::GetNSString(IDS_IOS_APP_LAUNCHER_OPEN_APP_BUTTON_LABEL);
-  [self presentAlertControllerWithMessage:message
-      openTitle:openTitle
+- (void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params {
+  NSURL* URL = base::mac::ObjCCastStrict<NSURL>(params[kAlertURL]);
+  NSString* prompt = base::mac::ObjCCastStrict<NSString>(params[kAlertPrompt]);
+  NSString* actionTitle =
+      base::mac::ObjCCastStrict<NSString>(params[kAlertAction]);
+  [self presentAlertControllerWithMessage:prompt
+      openTitle:actionTitle
       openHandler:^(UIAlertAction* action) {
         RecordExternalApplicationOpened(true);
         OpenUrlWithCompletionHandler(URL, nil);
@@ -119,15 +125,6 @@ NSString* PromptActionString(NSString* scheme) {
       }];
 }
 
-- (void)openPromptForURL:(NSURL*)URL {
-  [self presentAlertControllerWithMessage:[[self class] formatCallArgument:URL]
-                                openTitle:PromptActionString(URL.scheme)
-                              openHandler:^(UIAlertAction* action) {
-                                OpenUrlWithCompletionHandler(URL, nil);
-                              }
-                            cancelHandler:nil];
-}
-
 - (void)presentAlertControllerWithMessage:(NSString*)message
                                 openTitle:(NSString*)openTitle
                               openHandler:(AlertHandler)openHandler
@@ -156,32 +153,45 @@ NSString* PromptActionString(NSString* scheme) {
 - (BOOL)openURL:(const GURL&)gURL linkClicked:(BOOL)linkClicked {
   if (!gURL.is_valid() || !gURL.has_scheme())
     return NO;
-  NSURL* URL = net::NSURLWithGURL(gURL);
-
-  // iOS 10.3 introduced new prompts when facetime: and facetime-audio:
-  // URL schemes are opened. It is no longer necessary for Chrome to present
-  // another prompt before the system-provided prompt.
-  if (!base::ios::IsRunningOnOrLater(10, 3, 0) && UrlHasPhoneCallScheme(gURL)) {
-    // Showing an alert view immediately has a side-effect where focus is
-    // taken from the UIWebView so quickly that mouseup events are lost and
-    // buttons get 'stuck' in the on position. The solution is to defer
-    // showing the view.
-    [self performSelector:@selector(openPromptForURL:)
-               withObject:URL
-               afterDelay:0.0];
-    return YES;
-  }
 
   // Don't open external application if chrome is not active.
   if ([[UIApplication sharedApplication] applicationState] !=
       UIApplicationStateActive)
     return NO;
 
-  // Prompt user to open itunes when opening it is not a result of a link
-  // click.
-  if (!linkClicked && UrlHasAppStoreScheme(gURL)) {
+  NSURL* URL = net::NSURLWithGURL(gURL);
+  NSMutableDictionary<NSString*, id>* params = [NSMutableDictionary dictionary];
+  if (base::ios::IsRunningOnOrLater(10, 3, 0)) {
+    if (UrlHasAppStoreScheme(gURL)) {
+      params[kAlertPrompt] =
+          l10n_util::GetNSString(IDS_IOS_OPEN_IN_ANOTHER_APP);
+      params[kAlertAction] =
+          l10n_util::GetNSString(IDS_IOS_APP_LAUNCHER_OPEN_APP_BUTTON_LABEL);
+    }
+  } else {

[rohitrao (ping after 24h), 2017/05/15 12:07:48]

My understanding of this block is that:
1) On 10.3 and later, iOS prompts the user before launching facetime, so we
suppress our prompt to avoid double-prompting.  On earlier OSes, we want to show
the prompt because we think that's better behavior.

2) On 10.3 and later, Safari prompts the user before launching the appstore, and
we do the same.  On earlier OSes, we don't prompt, again to match Safari's
behavior.

If my summary is correct, then we have two very different behaviors and
rationales mixed in this one block of code.  Is there a way to rework the code
or the comment to make it easier to understand what's going on?  Maybe even just
breaking the comment up into two pieces would help make it clearer that we're
handling two different classes of behavior.

[pkl (ping after 24h if needed), 2017/05/17 22:23:02]

Thank you! I've broken up the two if-statements so they are no longer linked.
I've also rewritten the comments so they specifically addressed the pre-10.3
behavior.

+    // iOS 10.3 prompts user for permission when facetime: and facetime-audio:
+    // URL schemes are opened. For pre-10.3 users, Chrome needs to present an
+    // alert for phone calls. Prior iOS 10.3, Safari does not prompt when user
+    // taps on an App Store links, so Chrome wouldn't prompt either.

[rohitrao (ping after 24h), 2017/05/15 12:07:48]

Feature request: should we consider always prompting for appstore launches?  Do
we think that prompting is better for the user?

[pkl (ping after 24h if needed), 2017/05/17 22:23:02]

I don't think it would matter, but I'd rather not change the behavior for
pre-10.3 users. 

+    // This section can be deleted once iOS 10.3 support is dropped.
+    if (UrlHasPhoneCallScheme(gURL)) {
+      params[kAlertPrompt] = [[self class] formatCallArgument:URL];
+      params[kAlertAction] = PromptActionString([URL scheme]);
+    } else if (!linkClicked && UrlHasAppStoreScheme(gURL)) {
+      params[kAlertPrompt] =
+          l10n_util::GetNSString(IDS_IOS_OPEN_IN_ANOTHER_APP);
+      params[kAlertAction] =
+          l10n_util::GetNSString(IDS_IOS_APP_LAUNCHER_OPEN_APP_BUTTON_LABEL);
+    }
+  }
+  if ([params count] > 0) {
+    params[kAlertURL] = URL;
+    // Note: Showing an alert view immediately has a side-effect where focus is
+    // taken from the UIWebView so quickly that mouseup events are lost and
+    // buttons get 'stuck' in the on position. The solution is to defer
+    // showing the view using -performSelector:withObject:afterDelay:.
     [self performSelector:@selector(openExternalAppWithPromptForURL:)
-               withObject:URL
+               withObject:params
                afterDelay:0.0];
     return YES;
   }