Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: Aug 17
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 709889



Sign in to add a comment
PWA opens _blank links in PWA whern an external app (e.g. YouTube) is more suitable.
Project Member Reported by piotrs@chromium.org, Jul 9 Back to list
Reported in  crbug.com/709889  by nekr.fabula@gmail.com:

I tested this a bit further and it turns out that with this features added Chrome stopped opening external links other apps. For example twitter/youtube/google maps links previously would be opened in its native app  if the one is installed, but now everything is being opened in CCT.

This is clearly a regression to me.

(now sure how other browsers behave here, e.g. Safari, but iOS home screen webapps are so poor and bad so I'm sure if that is even a good example)

Also one more note (might be useful to know too):

Previously, only when one specify rel="noopener" on a link, it was opening the link Chrome. Example:

<a href="https://youtube.com/channel/..." target="_blank">Youtube</a> -- this would open native app. 

<a href="https://youtube.com/channel/..." target="_blank" rel="noopener">Youtube</a> -- this would NOT open a native app, but rather open the link in Chrome.

With CCT feature it always opens in CCT. I can file a separate bug for rel="noopener" if that makes sense. That might not be a bug though, I'm not sure, but I haven't found any specs/docs about such specific behavior.

 
Blocking: 709889
Piotr, thank you for filing the bug! Having a separate bug to track this work makes what is being fixed clearer (and will likely make my future self happier)
Project Member Comment 3 by bugdroid1@chromium.org, Jul 12
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e

commit 727468b41b2fe48bf73983ab0c3aa2632b47d89e
Author: piotrs <piotrs@chromium.org>
Date: Wed Jul 12 04:06:43 2017

Fixes the redirect to external apps when navigating from PWA.

This is done with avoiding explicit chrome package name (using
component name instead) and using
EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent.

With this patch the redirect works, however it takes a long time for CCT to
consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect
in ChromeTabbedActivity is almost instantaneous. Result is that
for _blank links CCT shows up briefly before redirecting to an external app.
In a follow up it should be investigated why it is the case and if it cannot
be improved, we might need to bypass CCT in such case, which would diverge
from existing navigation paths for _blank links in Clank.

BUG= 740402 

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

[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java
[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java_sources.gni
[delete] https://crrev.com/28132e66abebde370e168fff9ff2fa157c979d55/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java
[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
[modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java

Status: Fixed
This should work now.
Project Member Comment 5 by bugdroid1@chromium.org, Jul 12
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f

commit a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f
Author: aberent <aberent@chromium.org>
Date: Wed Jul 12 10:03:45 2017

Revert of Fixes redirects to external apps when navigating from PWA. (patchset #9 id:280001 of https://codereview.chromium.org/2969143002/ )

Reason for revert:
Test failures from new test WebappNavigationTest#testNewTabLinkToExternalApp on hromium.android/Marshmallow 64 bit Tester.

BUG=741568

Original issue's description:
> Fixes the redirect to external apps when navigating from PWA.
>
> This is done with avoiding explicit chrome package name (using
> component name instead) and using
> EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent.
>
> With this patch the redirect works, however it takes a long time for CCT to
> consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect
> in ChromeTabbedActivity is almost instantaneous. Result is that
> for _blank links CCT shows up briefly before redirecting to an external app.
> In a follow up it should be investigated why it is the case and if it cannot
> be improved, we might need to bypass CCT in such case, which would diverge
> from existing navigation paths for _blank links in Clank.
>
> BUG= 740402 
>
> Review-Url: https://codereview.chromium.org/2969143002
> Cr-Commit-Position: refs/heads/master@{#485831}
> Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e

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

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

[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java
[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java_sources.gni
[add] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java
[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
[modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java

Project Member Comment 6 by bugdroid1@chromium.org, Jul 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f

commit 4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f
Author: piotrs <piotrs@chromium.org>
Date: Thu Jul 13 05:16:27 2017

Fixes the redirect to external apps when navigating from PWA.

This is done with avoiding explicit chrome package name (using
component name instead) and using
EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent.

With this patch the redirect works, however it takes a long time for CCT to
consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect
in ChromeTabbedActivity is almost instantaneous. Result is that
for _blank links CCT shows up briefly before redirecting to an external app.
In a follow up it should be investigated why it is the case and if it cannot
be improved, we might need to bypass CCT in such case, which would diverge
from existing navigation paths for _blank links in Clank.

BUG= 740402 

Review-Url: https://codereview.chromium.org/2969143002
Cr-Original-Commit-Position: refs/heads/master@{#485831}
Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e
Review-Url: https://codereview.chromium.org/2969143002
Cr-Commit-Position: refs/heads/master@{#486275}

[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java
[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java_sources.gni
[delete] https://crrev.com/184a75bcf8dda55ceef7f6df67154ce822c7cc5d/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java
[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
[modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java

Project Member Comment 7 by bugdroid1@chromium.org, Jul 13
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7

commit b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7
Author: aberent <aberent@chromium.org>
Date: Thu Jul 13 10:18:13 2017

Revert of Fixes redirects to external apps when navigating from PWA. (patchset #10 id:300001 of https://codereview.chromium.org/2969143002/ )

Reason for revert:
Still failing on Marshmallow 64 bit Tester. See, for example https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%2064%20bit%20Tester/builds/13798

BUG=741568

Original issue's description:
> Fixes the redirect to external apps when navigating from PWA.
>
> This is done with avoiding explicit chrome package name (using
> component name instead) and using
> EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent.
>
> With this patch the redirect works, however it takes a long time for CCT to
> consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect
> in ChromeTabbedActivity is almost instantaneous. Result is that
> for _blank links CCT shows up briefly before redirecting to an external app.
> In a follow up it should be investigated why it is the case and if it cannot
> be improved, we might need to bypass CCT in such case, which would diverge
> from existing navigation paths for _blank links in Clank.
>
> BUG= 740402 
>
> Review-Url: https://codereview.chromium.org/2969143002
> Cr-Original-Commit-Position: refs/heads/master@{#485831}
> Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e
> Review-Url: https://codereview.chromium.org/2969143002
> Cr-Commit-Position: refs/heads/master@{#486275}
> Committed: https://chromium.googlesource.com/chromium/src/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f

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

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

[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java
[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java_sources.gni
[add] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java
[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
[modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java

Labels: -Pri-3 M-61 Pri-1
Status: Assigned
Looks like re-landign this will fix issue 742449 too. Would appreciate it if you can get this in for 61
Definitely will do everything to get this in and will test WebAPK -> CCT -> WebAPK case explicitly.
Project Member Comment 10 by bugdroid1@chromium.org, Jul 20
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/471df3926c09fda0af15d9157eb29bd326b0f9fc

commit 471df3926c09fda0af15d9157eb29bd326b0f9fc
Author: piotrs <piotrs@chromium.org>
Date: Thu Jul 20 13:41:20 2017

Fixes the redirect to external apps when navigating from PWA.

This is done with avoiding explicit chrome package name (using
component name instead) and using
EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent.

With this patch the redirect works, however it takes a long time for CCT to
consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect
in ChromeTabbedActivity is almost instantaneous. Result is that
for _blank links CCT shows up briefly before redirecting to an external app.
In a follow up it should be investigated why it is the case and if it cannot
be improved, we might need to bypass CCT in such case, which would diverge
from existing navigation paths for _blank links in Clank.

BUG= 740402 

Review-Url: https://codereview.chromium.org/2969143002
Cr-Original-Original-Commit-Position: refs/heads/master@{#485831}
Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e
Review-Url: https://codereview.chromium.org/2969143002
Cr-Original-Commit-Position: refs/heads/master@{#486275}
Committed: https://chromium.googlesource.com/chromium/src/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f
Review-Url: https://codereview.chromium.org/2969143002
Cr-Commit-Position: refs/heads/master@{#488228}

[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java
[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java_sources.gni
[delete] https://crrev.com/9219f3033f0f56ec5477336b11915e7e2e9f46f2/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java
[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
[modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java

Status: Fixed
Hopefully the patch with stick this time and it will be fixed for good.
Labels: Merge-Request-61
I would like to merge commit 471df3926c09fda0af15d9157eb29bd326b0f9fc into m61, thanks!
Project Member Comment 13 by sheriffbot@chromium.org, Jul 24
Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Started
Reason for Merge request:
Usage of CCT in standalone PWAs has only been added in m61, this final patch fixes the regression where we don't favor native specialized handlers (e.g. native YouTube or Google Maps apps). It would be a shame to bring this to users, though arguably it's not very severe issue. Up for your consideration.
Cc: amineer@chromium.org
Hi Alex, it's been almost a week since I proposed the merge. It seems to me I followed the right procedure, but let me know if there's something I've missed.
Labels: -Merge-Review-61 Merge-Approved-61
Merge approved for M61 branch 3163.  Please merge before end of next week, preferably by Monday so that we can start getting coverage on this as it's a bit larger than I'd like take later in the release cycle.
Project Member Comment 18 by sheriffbot@chromium.org, Aug 8
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Approved-61
I must have made a mistake. Branch point is #488528, which is after the commit in question. I've also checked Chrome Beta and YT links do get open in a native app.

Taking off the merge labels.
Status: Fixed
Sign in to add a comment