Do not launch external apps while page is scrolling

Do not launch external apps while page is scrolling

This is a long outstanding bug of launching a Google app while page
is scrolling. The root cause is that a URL is loaded as the page
scrolls down. Since the user touched the screen to initiate the scroll,
the "link clicked" logic took that scroll touch as a link click and
the URL load is then treated as if the user has clicked on the link.
This affects links such as maps.google.com and youtube.com.

Refer to http://crbug/704417 regarding the following types of 
navigations. This CL simplifies the logic, at the expense of a change
in behavior:
[1] MV tiles - No change in behavior
[2] Bookmarks - Used to launch native iOS app, now opens web site in browser.
[3] Recently closed - Used to launch native iOS app, now opens web site in browser.

BUG=663583, 700709
TEST=Test loading of Google web sites (maps, drive, etc). Check redirects. Embedded YT videos and Maps.

Review-Url: https://codereview.chromium.org/2764033005
Cr-Commit-Position: refs/heads/master@{#459072}
Committed: https://chromium.googlesource.com/chromium/src/+/fd4f2da01569ad80206f09edd2b585e186d06ed8

[pkl (ping after 24h if needed), 2017-03-22 06:36:04]

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

[pkl (ping after 24h if needed), 2017-03-22 06:36:05]

The bug(s) is (are) currently marked as RBS for M58.
This is the smallest fix I can find for this.

I think this whole area is messy and deserves some cleanup / simplification
(from first principles), but an attempt to do that would never be safe enough to
be cherry-pickable.

PTAL.

[Eugene But (OOO till 7-30), 2017-03-22 15:11:30]

You change does not make |shouldAllowLoadWithNavigationAction:| worse :) lgtm

[rohitrao (ping after 24h), 2017-03-22 20:57:56]

It looks like there is a second change in this CL that isn't captured in the CL
description: we'll no longer launch external apps for clicks on NTP tiles.  Why
was that change necessary?

Are there any negative side effects to no longer treating scrolling as a user
interaction?

[pkl (ping after 24h if needed), 2017-03-23 05:27:49]

On 2017/03/22 20:57:56, rohitrao wrote:
> It looks like there is a second change in this CL that isn't captured in the
CL
> description: we'll no longer launch external apps for clicks on NTP tiles. 
Why
> was that change necessary?
> 
> Are there any negative side effects to no longer treating scrolling as a user
> interaction?

You are absolutely correct that there's a change in behavior. I've filed a new
crbug/704417 to track this behavior. I will update this CL description with
details.

[pkl (ping after 24h if needed), 2017-03-23 05:29:34]

Description was changed from

==========
Do not launch external apps while page is scrolling

This is a long outstanding bug of launching a Google app while page
is scrolling. The root cause is that a URL is loaded as the page
scrolls down. Since the user touched the screen to initiate the scroll,
the "link clicked" logic took that scroll touch as a link click and
the URL load is then treated as if the user has clicked on the link.
This affects links such as maps.google.com and youtube.com.

BUG=663583,700709
TEST=Test loading of Google web sites (maps, drive, etc). Check redirects.
Embedded YT videos and Maps.
==========

to

==========
Do not launch external apps while page is scrolling

This is a long outstanding bug of launching a Google app while page
is scrolling. The root cause is that a URL is loaded as the page
scrolls down. Since the user touched the screen to initiate the scroll,
the "link clicked" logic took that scroll touch as a link click and
the URL load is then treated as if the user has clicked on the link.
This affects links such as maps.google.com and youtube.com.

Refer to http://crbug/704417 regarding the following types of 
navigations. This CL simplifies the logic, at the expense of a change
in behavior:
[1] MV tiles - No change in behavior
[2] Bookmarks - Used to launch native iOS app, now opens web site in browser.
[3] Recently closed - Used to launch native iOS app, now opens web site in
browser.

BUG=663583,700709
TEST=Test loading of Google web sites (maps, drive, etc). Check redirects.
Embedded YT videos and Maps.
==========

[pkl (ping after 24h if needed), 2017-03-23 05:35:54]

Please take another look. There's no change in code, just updated the comment
with a crbug reference and this CL's description.

Regarding whether there are side effects of not treating scrolling as a user
interaction, I think it may actually be an improvement. It leans towards false
negatives (i.e. not launching when we should have) instead of false positives.
False positives are more jarring for the user.

[rohitrao (ping after 24h), 2017-03-23 11:58:56]

Code lgtm assuming we're happy with the behavior changes.

[pkl (ping after 24h if needed), 2017-03-23 13:25:09]

On 2017/03/23 11:58:56, rohitrao wrote:
> Code lgtm assuming we're happy with the behavior changes.

Please see http://crbug/704417 comment 2 for reasoning. Based on that, this CL
fixed the more serious issue but introduced a regression bug in Recent Tabs
where native app should be launch but it is not.

I'm going to land this as-is since we are fixing the false positives that are
more more likely to affect users.

The native app launching logic needs to be rewritten for the WKWebView world.
The current code is mostly based on UIWebView's lack of information.

[pkl (ping after 24h if needed), 2017-03-23 13:25:15]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-23 13:25:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2764033005/#ps20001
(title: "updated comment")

[commit-bot: I haz the power, 2017-03-23 13:25:45]

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-03-23 14:08:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490275515867100,
"parent_rev": "c99783c636b1169cb670c2a042b107c7371c4221", "commit_rev":
"fd4f2da01569ad80206f09edd2b585e186d06ed8"}

[commit-bot: I haz the power, 2017-03-23 14:08:48]

Description was changed from

==========
Do not launch external apps while page is scrolling

This is a long outstanding bug of launching a Google app while page
is scrolling. The root cause is that a URL is loaded as the page
scrolls down. Since the user touched the screen to initiate the scroll,
the "link clicked" logic took that scroll touch as a link click and
the URL load is then treated as if the user has clicked on the link.
This affects links such as maps.google.com and youtube.com.

Refer to http://crbug/704417 regarding the following types of 
navigations. This CL simplifies the logic, at the expense of a change
in behavior:
[1] MV tiles - No change in behavior
[2] Bookmarks - Used to launch native iOS app, now opens web site in browser.
[3] Recently closed - Used to launch native iOS app, now opens web site in
browser.

BUG=663583,700709
TEST=Test loading of Google web sites (maps, drive, etc). Check redirects.
Embedded YT videos and Maps.
==========

to

==========
Do not launch external apps while page is scrolling

This is a long outstanding bug of launching a Google app while page
is scrolling. The root cause is that a URL is loaded as the page
scrolls down. Since the user touched the screen to initiate the scroll,
the "link clicked" logic took that scroll touch as a link click and
the URL load is then treated as if the user has clicked on the link.
This affects links such as maps.google.com and youtube.com.

Refer to http://crbug/704417 regarding the following types of 
navigations. This CL simplifies the logic, at the expense of a change
in behavior:
[1] MV tiles - No change in behavior
[2] Bookmarks - Used to launch native iOS app, now opens web site in browser.
[3] Recently closed - Used to launch native iOS app, now opens web site in
browser.

BUG=663583,700709
TEST=Test loading of Google web sites (maps, drive, etc). Check redirects.
Embedded YT videos and Maps.

Review-Url: https://codereview.chromium.org/2764033005
Cr-Commit-Position: refs/heads/master@{#459072}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd4f2da01569ad80206f09edd2b5...
==========

[commit-bot: I haz the power, 2017-03-23 14:08:50]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/fd4f2da01569ad80206f09edd2b5...