[Custom Tabs] Navigate to other apps if the first url is a redirect

[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667, 616825
Committed: https://crrev.com/d3665e0bb00964faf5f1f2a497225879f3da5e97
Cr-Commit-Position: refs/heads/master@{#400029}

[Ian Wen, 2016-06-10 01:55:40]

ianwen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Ian Wen, 2016-06-10 01:55:40]

Hi Maria, could you PTAL?

[Maria, 2016-06-10 18:38:15]

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:201:
// http://crbug/613667: For custom tabs, even if the intent is heading to
Chrome, still
crbug/ -> crbug.com/


Hmm, why do we want to allow external navigations without gestures for
CustomTabs?

Also, this changes behaviour for non-custom tabs to allow external intents
during redirects even if they weren't triggered by a gesture. I think this
breaks the behaviour we want which prevents pages auto-intenting to apps.

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java:87:
mInitialIntent.putExtra(CustomTabsIntent.EXTRA_SESSION, "");
I think this inappropriately exposes part of the logic of how we determine
something is a custom tabs intent.

I would suggest that instead we add a new field mIsCustomTabsIntent and set it
here by calling ChromeLauncherActivity.isCustomTabIntent(mInitialIntent,
mContext) immediately.

We'll just need to reset the value whenever mInitialIntent is reset.

[Ian Wen, 2016-06-10 21:02:09]

PTAL. All addressed.

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:201:
// http://crbug/613667: For custom tabs, even if the intent is heading to
Chrome, still
On 2016/06/10 18:38:15, Maria wrote:
> crbug/ -> crbug.com/
> 
> 
> Hmm, why do we want to allow external navigations without gestures for
> CustomTabs?
> 
> Also, this changes behaviour for non-custom tabs to allow external intents
> during redirects even if they weren't triggered by a gesture. I think this
> breaks the behaviour we want which prevents pages auto-intenting to apps.

Done. I put custom tabs as a different if check so that they don't mingle
together.

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java:87:
mInitialIntent.putExtra(CustomTabsIntent.EXTRA_SESSION, "");
On 2016/06/10 18:38:15, Maria wrote:
> I think this inappropriately exposes part of the logic of how we determine
> something is a custom tabs intent.
> 
> I would suggest that instead we add a new field mIsCustomTabsIntent and set it
> here by calling ChromeLauncherActivity.isCustomTabIntent(mInitialIntent,
> mContext) immediately.
> 
> We'll just need to reset the value whenever mInitialIntent is reset.

Very good idea. Done.

[Maria, 2016-06-10 21:14:53]

https://codereview.chromium.org/2057933002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:205:
// url, unless the first url is a redirect.
I think you might want to check that you are not causing this to prevent
external navigations after a navigation has occurred in a custom tab. I am not
sure the intent would get updated for that.

It would also be nice to add a new unit test to ExternalNavigationHandlerTest
for this case.

https://codereview.chromium.org/2057933002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java
(right):

https://codereview.chromium.org/2057933002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java:83:
mIsCustomTabIntent = true;
nit: can also be written as
mIsCustomTabIntent = ChromeLauncherActivity.isCustomTabIntent(intent, mContext);

[Ian Wen, 2016-06-13 18:31:20]

PTAL

https://chromiumcodereview.appspot.com/2057933002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://chromiumcodereview.appspot.com/2057933002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:205:
// url, unless the first url is a redirect.
On 2016/06/10 21:14:53, Maria wrote:
> I think you might want to check that you are not causing this to prevent
> external navigations after a navigation has occurred in a custom tab. I am not
> sure the intent would get updated for that.
> 
> It would also be nice to add a new unit test to ExternalNavigationHandlerTest
> for this case.

Added a test for custom tabs. Done.

https://chromiumcodereview.appspot.com/2057933002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java
(right):

https://chromiumcodereview.appspot.com/2057933002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/TabRedirectHandler.java:83:
mIsCustomTabIntent = true;
On 2016/06/10 21:14:53, Maria wrote:
> nit: can also be written as
> mIsCustomTabIntent = ChromeLauncherActivity.isCustomTabIntent(intent,
mContext);

Done.

[Ian Wen, 2016-06-13 18:31:28]

The CQ bit was checked by ianwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-13 18:32:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2057933002/20001

[Ian Wen, 2016-06-13 18:33:04]

The CQ bit was unchecked by ianwen@chromium.org

[Ian Wen, 2016-06-13 18:34:08]

ianwen@chromium.org changed reviewers:
+ isherman@chromium.org

[Ian Wen, 2016-06-13 18:34:09]

isherman@chromium.org: Please review changes in FieldTrial.

[Ian Wen, 2016-06-13 18:34:43]

Description was changed from

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.

BUG=613667,616825
==========

to

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667,616825
==========

[Maria, 2016-06-13 18:39:57]

lgtm

[Ilya Sherman, 2016-06-13 21:56:47]

field trial testing config change lgtm

[Ian Wen, 2016-06-13 21:57:09]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2016-06-13 21:57:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2057933002/40001

[commit-bot: I haz the power, 2016-06-13 22:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 22:00:51]

Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ian Wen, 2016-06-15 21:26:04]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-06-15 21:26:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2057933002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-15 21:26:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2057933002/60001

[commit-bot: I haz the power, 2016-06-15 22:31:08]

Description was changed from

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667,616825
==========

to

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667,616825
==========

[commit-bot: I haz the power, 2016-06-15 22:31:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-15 22:33:14]

Description was changed from

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667,616825
==========

to

==========
[Custom Tabs] Navigate to other apps if the first url is a redirect

This CL fixes two bugs.
1. Eliminate the possibility that a custom tab navigates away to an
external app while the activity is still hanging there.
2. Make external navigation possible if the first url redirects to
another one. This is because the client apps mostly lack the ability to
tell if an app can handle a shortened url. And Chrome should help them
doing so.
3. Remove an unused fieldtrial.

BUG=613667,616825

Committed: https://crrev.com/d3665e0bb00964faf5f1f2a497225879f3da5e97
Cr-Commit-Position: refs/heads/master@{#400029}
==========

[commit-bot: I haz the power, 2016-06-15 22:33:15]

Patchset 4 (id:??) landed as
https://crrev.com/d3665e0bb00964faf5f1f2a497225879f3da5e97
Cr-Commit-Position: refs/heads/master@{#400029}