New issue
Advanced search Search tips

Issue 653254 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Redirect to app doesn't work in Herb

Project Member Reported by mariakho...@chromium.org, Oct 5 2016

Issue description

TabRedirectHandler checks whether the intent has an explicit packageName set on the intent in order to decide if the intent should stay in Chrome.

However, for Herb, we always add the package name ourselves in ChromeLauncherActivity, which means the redirect would never result in an external navigation.

 
Labels: -Pri-3 M-54 Merge-Request-54 Pri-1
This is a pretty annoying thing for redirects to Chrome when herb is enabled, so I think we should pull this back to 54.

Comment 3 by dimu@chromium.org, Oct 6 2016

Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M54, manual review required.
Should we be adding some tests here as well?  I'm a little wary given that we shipped an intent handling regression in M53 that required an emergency respin, can we confirm the TE folks will be executing test cases that'll cover this workflow?

We can likely merge once we confirm details above.
Labels: -Merge-Review-54 Merge-Approved-54
Proactively approving based on an offline conversation with tedchoc@.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 11 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1005c8597f728173c822bdc1d8a01609f8cbd02c

commit 1005c8597f728173c822bdc1d8a01609f8cbd02c
Author: Ted Choc <tedchoc@google.com>
Date: Tue Oct 11 19:59:29 2016

Fix tab redirect to apps in Herb mode.

BUG= 653254 

Review-Url: https://codereview.chromium.org/2398833002
Cr-Commit-Position: refs/heads/master@{#423355}

Support native app redirects to herb.

If in Herb mode, use the same intent handling logic as Chrome,
which more aggressively intents out to external applications
than a normal CCT.

BUG= 653254 

Review URL: https://codereview.chromium.org/2411903003 .

Review-Url: https://codereview.chromium.org/2412613002
Cr-Original-Commit-Position: refs/heads/master@{#424483}
Cr-Commit-Position: refs/branch-heads/2840@{#718}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Status: Fixed (was: Assigned)
Merged to 54.  Will still work on tests, but want to mark this as fixed for verification availability.

For testing, have elderberry enabled and send a link that includes a redirect to a site that has a native app available (e.g. maps).

Here's a short link:
https://goo.gl/Tk4IcV

That will redirect to:
https://maps.google.com/maps?q=1600+amphitheatre+parkway&um=1&ie=UTF-8&sa=X&ved=0ahUKEwiB07yJnsfPAhUB-2MKHYfPCZcQ_AUIBygB
Issue 648444 has been merged into this issue.

Comment 11 by dgn@chromium.org, Oct 12 2016

Status: Assigned (was: Fixed)
#7 broke the M54 build on official.android:

../../chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:857: error: method updateHerbIntent in class ChromeLauncherActivity cannot be applied to given types;
                ChromeLauncherActivity.updateHerbIntent(ChromeTabbedActivity.this,
                                      ^
  required: Context,Intent
  found: ChromeTabbedActivity,Intent,Uri
  reason: actual and formal argument lists differ in length

https://uberchromegw.corp.google.com/i/official.android/builders/official-arm/builds/1263/steps/build/logs/stdio

Will revert.
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85174845b8d6adc96bafda12bea3f511e09c1942

commit 85174845b8d6adc96bafda12bea3f511e09c1942
Author: dgn <dgn@chromium.org>
Date: Wed Oct 12 10:10:02 2016

Revert of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2411903003/ )

Reason for revert:
Does not compile:

../../chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:857: error: method updateHerbIntent in class ChromeLauncherActivity cannot be applied to given types;
                ChromeLauncherActivity.updateHerbIntent(ChromeTabbedActivity.this,
                                      ^
  required: Context,Intent
  found: ChromeTabbedActivity,Intent,Uri
  reason: actual and formal argument lists differ in length

Original issue's description:
> Fix tab redirect to apps in Herb mode.
>
> BUG= 653254 
>
> Review-Url: https://codereview.chromium.org/2398833002
> Cr-Commit-Position: refs/heads/master@{#423355}
>
> Support native app redirects to herb.
>
> If in Herb mode, use the same intent handling logic as Chrome,
> which more aggressively intents out to external applications
> than a normal CCT.
>
> BUG= 653254 
>
> Review-Url: https://codereview.chromium.org/2412613002
> Cr-Commit-Position: refs/heads/master@{#424483}
> Committed: https://chromium.googlesource.com/chromium/src/+/1005c8597f728173c822bdc1d8a01609f8cbd02c

TBR=tedchoc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653254 

Review-Url: https://codereview.chromium.org/2419483002
Cr-Commit-Position: refs/branch-heads/2840@{#729}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Comment 13 by dgn@chromium.org, Oct 12 2016

The issue above had already been reported and addressed in issue 654973. Relanding.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01820f7efb29f3e6d0ba560b43d98c698e38fcb8

commit 01820f7efb29f3e6d0ba560b43d98c698e38fcb8
Author: dgn <dgn@chromium.org>
Date: Wed Oct 12 14:05:35 2016

Reland of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2419483002/ )

Reason for revert:
Reland.

Issue fixed by https://codereview.chromium.org/2408233003/

Original issue's description:
> Revert of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2411903003/ )
>
> Reason for revert:
> Does not compile:
>
> ../../chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:857: error: method updateHerbIntent in class ChromeLauncherActivity cannot be applied to given types;
>                 ChromeLauncherActivity.updateHerbIntent(ChromeTabbedActivity.this,
>                                       ^
>   required: Context,Intent
>   found: ChromeTabbedActivity,Intent,Uri
>   reason: actual and formal argument lists differ in length
>
> Original issue's description:
> > Fix tab redirect to apps in Herb mode.
> >
> > BUG= 653254 
> >
> > Review-Url: https://codereview.chromium.org/2398833002
> > Cr-Commit-Position: refs/heads/master@{#423355}
> >
> > Support native app redirects to herb.
> >
> > If in Herb mode, use the same intent handling logic as Chrome,
> > which more aggressively intents out to external applications
> > than a normal CCT.
> >
> > BUG= 653254 
> >
> > Review-Url: https://codereview.chromium.org/2412613002
> > Cr-Commit-Position: refs/heads/master@{#424483}
> > Committed: https://chromium.googlesource.com/chromium/src/+/1005c8597f728173c822bdc1d8a01609f8cbd02c
>
> TBR=tedchoc@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 653254 

TBR=tedchoc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653254 

Review-Url: https://codereview.chromium.org/2404163005
Cr-Commit-Position: refs/branch-heads/2840@{#730}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Verified on M54 - 54.0.2840.60 that the installed app is shown in the intent-picker the second time.
Status: Fixed (was: Assigned)
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1005c8597f728173c822bdc1d8a01609f8cbd02c

commit 1005c8597f728173c822bdc1d8a01609f8cbd02c
Author: Ted Choc <tedchoc@google.com>
Date: Tue Oct 11 19:59:29 2016

Fix tab redirect to apps in Herb mode.

BUG= 653254 

Review-Url: https://codereview.chromium.org/2398833002
Cr-Commit-Position: refs/heads/master@{#423355}

Support native app redirects to herb.

If in Herb mode, use the same intent handling logic as Chrome,
which more aggressively intents out to external applications
than a normal CCT.

BUG= 653254 

Review URL: https://codereview.chromium.org/2411903003 .

Review-Url: https://codereview.chromium.org/2412613002
Cr-Original-Commit-Position: refs/heads/master@{#424483}
Cr-Commit-Position: refs/branch-heads/2840@{#718}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/1005c8597f728173c822bdc1d8a01609f8cbd02c/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85174845b8d6adc96bafda12bea3f511e09c1942

commit 85174845b8d6adc96bafda12bea3f511e09c1942
Author: dgn <dgn@chromium.org>
Date: Wed Oct 12 10:10:02 2016

Revert of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2411903003/ )

Reason for revert:
Does not compile:

../../chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:857: error: method updateHerbIntent in class ChromeLauncherActivity cannot be applied to given types;
                ChromeLauncherActivity.updateHerbIntent(ChromeTabbedActivity.this,
                                      ^
  required: Context,Intent
  found: ChromeTabbedActivity,Intent,Uri
  reason: actual and formal argument lists differ in length

Original issue's description:
> Fix tab redirect to apps in Herb mode.
>
> BUG= 653254 
>
> Review-Url: https://codereview.chromium.org/2398833002
> Cr-Commit-Position: refs/heads/master@{#423355}
>
> Support native app redirects to herb.
>
> If in Herb mode, use the same intent handling logic as Chrome,
> which more aggressively intents out to external applications
> than a normal CCT.
>
> BUG= 653254 
>
> Review-Url: https://codereview.chromium.org/2412613002
> Cr-Commit-Position: refs/heads/master@{#424483}
> Committed: https://chromium.googlesource.com/chromium/src/+/1005c8597f728173c822bdc1d8a01609f8cbd02c

TBR=tedchoc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653254 

Review-Url: https://codereview.chromium.org/2419483002
Cr-Commit-Position: refs/branch-heads/2840@{#729}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/85174845b8d6adc96bafda12bea3f511e09c1942/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01820f7efb29f3e6d0ba560b43d98c698e38fcb8

commit 01820f7efb29f3e6d0ba560b43d98c698e38fcb8
Author: dgn <dgn@chromium.org>
Date: Wed Oct 12 14:05:35 2016

Reland of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2419483002/ )

Reason for revert:
Reland.

Issue fixed by https://codereview.chromium.org/2408233003/

Original issue's description:
> Revert of Fix tab redirect to apps in Herb mode. (patchset #1 id:1 of https://codereview.chromium.org/2411903003/ )
>
> Reason for revert:
> Does not compile:
>
> ../../chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:857: error: method updateHerbIntent in class ChromeLauncherActivity cannot be applied to given types;
>                 ChromeLauncherActivity.updateHerbIntent(ChromeTabbedActivity.this,
>                                       ^
>   required: Context,Intent
>   found: ChromeTabbedActivity,Intent,Uri
>   reason: actual and formal argument lists differ in length
>
> Original issue's description:
> > Fix tab redirect to apps in Herb mode.
> >
> > BUG= 653254 
> >
> > Review-Url: https://codereview.chromium.org/2398833002
> > Cr-Commit-Position: refs/heads/master@{#423355}
> >
> > Support native app redirects to herb.
> >
> > If in Herb mode, use the same intent handling logic as Chrome,
> > which more aggressively intents out to external applications
> > than a normal CCT.
> >
> > BUG= 653254 
> >
> > Review-Url: https://codereview.chromium.org/2412613002
> > Cr-Commit-Position: refs/heads/master@{#424483}
> > Committed: https://chromium.googlesource.com/chromium/src/+/1005c8597f728173c822bdc1d8a01609f8cbd02c
>
> TBR=tedchoc@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 653254 

TBR=tedchoc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 653254 

Review-Url: https://codereview.chromium.org/2404163005
Cr-Commit-Position: refs/branch-heads/2840@{#730}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java
[modify] https://crrev.com/01820f7efb29f3e6d0ba560b43d98c698e38fcb8/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Labels: Merge-Request-55
Status: Available (was: Fixed)
This also needs to be merged into M-55
Labels: M-55
Status: Started (was: Available)
Labels: -Merge-Request-55 Merge-Approved-55
Talked with amineer@ and since this was merged to 54, it is ok to merge to 55.
Issue 665095 has been merged into this issue.
Project Member

Comment 26 by bugdroid1@chromium.org, Nov 16 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d21d33fdf5de895ffe8e4989f9975a57d9687a2

commit 2d21d33fdf5de895ffe8e4989f9975a57d9687a2
Author: Ted Choc <tedchoc@google.com>
Date: Wed Nov 16 21:05:06 2016

Support native app redirects to herb.

If in Herb mode, use the same intent handling logic as Chrome,
which more aggressively intents out to external applications
than a normal CCT.

BUG= 653254 

Review URL: https://codereview.chromium.org/2505063003 .

Review-Url: https://codereview.chromium.org/2412613002
Cr-Original-Commit-Position: refs/heads/master@{#424483}
Cr-Commit-Position: refs/branch-heads/2883@{#591}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/2d21d33fdf5de895ffe8e4989f9975a57d9687a2/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/2d21d33fdf5de895ffe8e4989f9975a57d9687a2/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
[modify] https://crrev.com/2d21d33fdf5de895ffe8e4989f9975a57d9687a2/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java

Status: Fixed (was: Started)
Both fixes should be on 55 now

Sign in to add a comment