Prompts user before transferring to App Store

ios/chrome/browser/web/external_app_launcher.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/chrome/browser/web/external_app_launcher.h"
 
 #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"
 #include "ios/chrome/browser/experimental_flags.h"
 #import "ios/chrome/browser/open_url_util.h"
 #import "ios/chrome/browser/web/mailto_url_rewriter.h"
 #include "ios/chrome/grit/ios_strings.h"
 #import "net/base/mac/url_conversions.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
 namespace {
 
 typedef void (^AlertHandler)(UIAlertAction* action);
 
 // Returns a set of NSStrings that are URL schemes for iTunes Stores.
 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;
 }
 
 bool UrlHasAppStoreScheme(const GURL& gURL) {
   std::string scheme = gURL.scheme();
   return [ITMSSchemes() containsObject:base::SysUTF8ToNSString(scheme)];
 }
 
 // Logs an entry for |Tab.ExternalApplicationOpened|.  If the user decided to
@@ skipping 13 matching lines @@
 NSString* PromptActionString(NSString* scheme) {
   if ([scheme isEqualToString:@"facetime"])
     return l10n_util::GetNSString(IDS_IOS_FACETIME_BUTTON);
   else if ([scheme isEqualToString:@"tel"] ||
            [scheme isEqualToString:@"facetime-audio"])
     return l10n_util::GetNSString(IDS_IOS_PHONE_CALL_BUTTON);
   NOTREACHED();
   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
                               openHandler:(AlertHandler)openHandler
                             cancelHandler:(AlertHandler)cancelHandler;
 @end
 
 @implementation ExternalAppLauncher
 
 + (NSString*)formatCallArgument:(NSURL*)URL {
   NSCharacterSet* charSet =
       [NSCharacterSet characterSetWithCharactersInString:@"/"];
   NSString* scheme = [NSString stringWithFormat:@"%@:", [URL scheme]];
   NSString* URLString = [URL absoluteString];
   if ([URLString length] <= [scheme length])
     return URLString;
   NSString* prompt = [[[[URL absoluteString] substringFromIndex:[scheme length]]
       stringByTrimmingCharactersInSet:charSet] stringByRemovingPercentEncoding];
   // Returns original URL string if there's nothing interesting to display
   // other than the scheme itself.
   if (![prompt length])
     return URLString;
   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);
       }
       cancelHandler:^(UIAlertAction* action) {
         RecordExternalApplicationOpened(false);
       }];
 }
 
-- (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
                             cancelHandler:(AlertHandler)cancelHandler {
   UIAlertController* alertController =
       [UIAlertController alertControllerWithTitle:nil
                                           message:message
                                    preferredStyle:UIAlertControllerStyleAlert];
   UIAlertAction* openAction =
       [UIAlertAction actionWithTitle:openTitle
                                style:UIAlertActionStyleDefault
                              handler:openHandler];
   UIAlertAction* cancelAction =
       [UIAlertAction actionWithTitle:l10n_util::GetNSString(IDS_CANCEL)
                                style:UIAlertActionStyleCancel
                              handler:cancelHandler];
   [alertController addAction:cancelAction];
   [alertController addAction:openAction];
 
   [[[[UIApplication sharedApplication] keyWindow] rootViewController]
       presentViewController:alertController
                    animated:YES
                  completion:nil];
 }
 
 - (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;
   }
 
   // Replaces |URL| with a rewritten URL if it is of mailto: scheme.
   if (!experimental_flags::IsNativeAppLauncherEnabled() &&
       gURL.SchemeIs(url::kMailToScheme)) {
     MailtoURLRewriter* rewriter =
         [[MailtoURLRewriter alloc] initWithStandardHandlers];
     NSString* launchURL = [rewriter rewriteMailtoURL:gURL];
     if (launchURL) {
       URL = [NSURL URLWithString:launchURL];
     }
     UMA_HISTOGRAM_BOOLEAN("IOS.MailtoURLRewritten", launchURL != nil);
   }
 
   // If the following call returns YES, an external application is about to be
   // launched and Chrome will go into the background now.
   // TODO(crbug.com/622735): This call still needs to be updated.
   // It's heavily nested so some refactoring is needed.
   return [[UIApplication sharedApplication] openURL:URL];
 }
 
 @end