Prompts user before transferring to App Store

Prompts user before transferring to App Store

Uses more aggressive prompting like iOS 10.3. Prompts user even if
user tapped on a link. In earlier versions of iOS, prompts are presented
only if user did not tap on a link.

BUG=707991
TEST=test links on browsingtest.appspot.com/external_url.html

Review-Url: https://codereview.chromium.org/2878813003
Cr-Commit-Position: refs/heads/master@{#472597}
Committed: https://chromium.googlesource.com/chromium/src/+/825edf8e5dadff7bce102fdf37d5be5128490c45

[pkl (ping after 24h if needed), 2017-05-12 22:41:39]

pkl@chromium.org changed reviewers:
+ eugenebut@chromium.org, rohitrao@chromium.org

[pkl (ping after 24h if needed), 2017-05-12 22:41:40]

PTAL. The doc in the crbug contains additional background of this change in
prompting behavior in 10.3+

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

Is it possible to write tests for your changes?

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:86: -
(void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params;
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]

Regarding adding tests, the performSelector delay makes it a bit tricky. I would
rather try to remove that delay before adding tests.

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:86: -
(void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params;
On 2017/05/12 22:53:51, Eugene But wrote:
> Do you use NSDictionary for passing the arguments, because of
|performSelector:|
> API limitation? Would it be better to use GCD's dispatch_async instead?

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]

lgtm

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:86: -
(void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params;
On 2017/05/12 23:16:11, pkl -pls.ping.after.24h. wrote:
> On 2017/05/12 22:53:51, Eugene But wrote:
> > Do you use NSDictionary for passing the arguments, because of
> |performSelector:|
> > API limitation? Would it be better to use GCD's dispatch_async instead?
> 
> 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.
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:

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

lgtm

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:171: } else {
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.

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:175: // taps on an App Store
links, so Chrome wouldn't prompt either.
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]

Thank you! Updated comments and renamed method to sync with changed
functionality.

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
File ios/chrome/browser/web/external_app_launcher.mm (right):

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:86: -
(void)openExternalAppWithPromptForURL:(NSDictionary<NSString*, id>*)params;
On 2017/05/13 00:27:52, Eugene But wrote:
> On 2017/05/12 23:16:11, pkl -pls.ping.after.24h. wrote:
> > On 2017/05/12 22:53:51, Eugene But wrote:
> > > Do you use NSDictionary for passing the arguments, because of
> > |performSelector:|
> > > API limitation? Would it be better to use GCD's dispatch_async instead?
> > 
> > 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.
> 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:

Renamed now.

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:171: } else {
On 2017/05/15 12:07:48, rohitrao (ping after 24h) wrote:
> 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.

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.

https://codereview.chromium.org/2878813003/diff/40001/ios/chrome/browser/web/...
ios/chrome/browser/web/external_app_launcher.mm:175: // taps on an App Store
links, so Chrome wouldn't prompt either.
On 2017/05/15 12:07:48, rohitrao (ping after 24h) wrote:
> Feature request: should we consider always prompting for appstore launches? 
Do
> we think that prompting is better for the user?

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

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

The CQ bit was checked by pkl@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2878813003/#ps60001
(title: "clarified comments and renamed method.")

[commit-bot: I haz the power, 2017-05-17 22:24:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 23:16:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495059788559640,
"parent_rev": "701cc20866b6f07e3f3f693d0a64bd93e5219c48", "commit_rev":
"825edf8e5dadff7bce102fdf37d5be5128490c45"}

[commit-bot: I haz the power, 2017-05-17 23:16:32]

Description was changed from

==========
Prompts user before transferring to App Store

Uses more aggressive prompting like iOS 10.3. Prompts user even if
user tapped on a link. In earlier versions of iOS, prompts are presented
only if user did not tap on a link.

BUG=707991
TEST=test links on browsingtest.appspot.com/external_url.html
==========

to

==========
Prompts user before transferring to App Store

Uses more aggressive prompting like iOS 10.3. Prompts user even if
user tapped on a link. In earlier versions of iOS, prompts are presented
only if user did not tap on a link.

BUG=707991
TEST=test links on browsingtest.appspot.com/external_url.html

Review-Url: https://codereview.chromium.org/2878813003
Cr-Commit-Position: refs/heads/master@{#472597}
Committed:
https://chromium.googlesource.com/chromium/src/+/825edf8e5dadff7bce102fdf37d5...
==========

[commit-bot: I haz the power, 2017-05-17 23:16:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/825edf8e5dadff7bce102fdf37d5...