New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 814102 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

[desktop-pwas] Remove link capturing by default

Project Member Reported by mgiuca@chromium.org, Feb 21 2018

Issue description

Chrome Version: 64
OS: Chrome

What steps will reproduce the problem?
(1) Install a PWA (e.g., mobile.twitter.com).
(2) Click this link: https://mobile.twitter.com

What is the expected result?
The link opens in a normal browser tab.

What happens instead?
It opens a Twitter app window.

Rationale: We have decided to not automatically grab the link and put it into an app window. The "last straw" was a question of whether to spawn a new app window if one already existed. Doing so would cause potentially many duplicate app windows. Not doing so would result in data loss if the existing window was re-navigated. Keeping a single app window may be the best user experience, but it should be up to the app. So we will wait until we have an API to let the developer control the linking experience: https://github.com/w3c/ServiceWorker/issues/1028 / https://mgiuca.github.io/sw-launch/
 

Comment 1 by ortuno@chromium.org, Feb 26 2018

Current plan:

Two Navigation Throttles:
  1. BookmarkAppNavigationThrottle
  2. BookmarkAppExperimentalNavigationThrottle

*BookmarkAppNavigationThrottle*
 - Brand new Navigation Throttle
 - In charge of handling out of scope navigations inside app windows.
 - Enabled by the DesktopPWAWindowing feature flag
 - Tested by a subset of tests in bookmark_app_navigation_throttle_browsertests.cc

*BookmarkAppExperimentalNavigationThrottle*
 - Formerly BookmarkAppNavigationThrottle.
 - Handles all in-scope and out-of-scope navigations inside tabs and app windows.
 - Enabled by new DesktopPWALinkCapturing feature flag
 - Tested by all tests in bookmark_app_navigation_throttle_browsertests.cc

There is just some minor overlap in functionality between BookmarkAppNavigationThrottle and BookmarkAppExperimentalNavigationThrottle, so we will initially re-implement the necessary functionality in BookmarkAppNavigationThrottle and remove the overlap later.

Comment 2 by ortuno@chromium.org, Feb 26 2018

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 28 2018

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

commit 40c8e796f939d69f73d7b2dd62b5d73c2012baef
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Wed Feb 28 04:18:59 2018

desktop-pwas: Introduce experimental link capturing flag

Currently, link capturing is enabled by the EnableDesktopPWAWindowing
feature, but in the future, it will be disabled unless you also have
this flag turned on.

Bug:  814102 
Change-Id: Ia13f48d38d963d8a2b93b327cb9866213390cd19
Reviewed-on: https://chromium-review.googlesource.com/936524
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539707}
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/chrome/browser/about_flags.cc
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/chrome/common/chrome_features.cc
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/chrome/common/chrome_features.h
[modify] https://crrev.com/40c8e796f939d69f73d7b2dd62b5d73c2012baef/tools/metrics/histograms/enums.xml

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 2 2018

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

commit ba5d9afb197d8e0ec8f843f1e3d79935b8372b48
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Mar 02 03:19:46 2018

desktop-pwas: Specify that throttle and tests are for experimental feature

Renames BookmarkAppNavigationThrottle to
BookmarkAppExperimentalNavigationThrottle.

Renames BookmarkAppNavigationThrottle*BrowserTests to
BookmarkAppNavigationThrottleExperimental*BrowserTests since the
majority of these tests are only valid when Link Capturing is
enabled.

First patch in a series of patches to disable link capturing by
default.

[1] Specify that throttle and tests are for experimental feature
    (this patch)
[2] Extract tests that should run when link capturing is off
    (https://crrev.com/c/939883)
[3] Handle out-of-scope navigations and stop link capturing by
    default (https://crrev.com/c/942597)

Bug:  814102 
Change-Id: I9ec3923e3ee66f93617feb2f68f210fe58bc553b
Reviewed-on: https://chromium-review.googlesource.com/940824
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540433}
[modify] https://crrev.com/ba5d9afb197d8e0ec8f843f1e3d79935b8372b48/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/ba5d9afb197d8e0ec8f843f1e3d79935b8372b48/chrome/browser/extensions/BUILD.gn
[rename] https://crrev.com/ba5d9afb197d8e0ec8f843f1e3d79935b8372b48/chrome/browser/extensions/bookmark_app_experimental_navigation_throttle.cc
[rename] https://crrev.com/ba5d9afb197d8e0ec8f843f1e3d79935b8372b48/chrome/browser/extensions/bookmark_app_experimental_navigation_throttle.h
[modify] https://crrev.com/ba5d9afb197d8e0ec8f843f1e3d79935b8372b48/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 2 2018

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

commit e3aa04a14dabe830f7a3c9420260a3e0d867fd23
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Mar 02 05:31:51 2018

desktop-pwas: Extract tests that should run when link capturing is off

This patch extracts a set of tests that should be run when
kDesktopPWAsLinkCapturing is enabled and when it's disabled.

The flag has currently no effect, so running these test with
and without the flag turned on has no effect.

Second patch in a series of patches to disable link capturing by
default.

[1] Specify that throttle and tests are for experimental feature
    (https://crrev.com/ba5d9af)
[2] Extract tests that should run when link capturing is off
    (this patch)
[3] Handle out-of-scope navigations and stop link capturing by
    default (https://crrev.com/c/942597)

Bug:  814102 
Change-Id: Ieff98836e425b1df6c70dc89b5faafdff8aa465b
Reviewed-on: https://chromium-review.googlesource.com/939883
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540454}
[modify] https://crrev.com/e3aa04a14dabe830f7a3c9420260a3e0d867fd23/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 8 2018

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

commit 79bd8f4485f2fa545d777f250e66acdb40e70e9f
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Mar 08 02:14:09 2018

desktop-pwas: Handle out-of-scope navigations

Introduces BookmarkAppNavigationThrottle to handle out-of-scope
navigations in app windows and puts
BookmarkAppExperimentalNavigationThrottle behind
kDesktopPWAsLinkCapturing.

Changes kDesktopPWAsLinkCapturing to be on by default, so there should
be no change in behavior for users. A follow up patch will change
kDesktopPWAsLinkCapturing to be off by default.

BookmarkAppNavigationThrottle and
BookmarkAppExperimentalNavigationThrottle share some
functionality that has been extracted into
bookmark_app_navigation_throttle_utils.h

[1] Specify that throttle and tests are for experimental feature
    (https://crrev.com/c/940824)
[2] Extract tests that should run when link capturing is off
    (https://crrev.com/c/939883)
[3] Handle out-of-scope navigations and stop link capturing by
    default (this patch)
[4] Disable link capturing by default (https://crrev.com/c/950484)

Bug:  814102 

Change-Id: I7d394899d85922fce9e51831bf8705e31d8a53bf
Reviewed-on: https://chromium-review.googlesource.com/942597
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541692}
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/BUILD.gn
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_experimental_navigation_throttle.cc
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_experimental_navigation_throttle.h
[add] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_navigation_throttle.cc
[add] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_navigation_throttle.h
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc
[add] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_navigation_throttle_utils.cc
[add] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/browser/extensions/bookmark_app_navigation_throttle_utils.h
[modify] https://crrev.com/79bd8f4485f2fa545d777f250e66acdb40e70e9f/chrome/common/chrome_features.cc

Labels: -Pri-3 Merge-Request-66 OS-Linux OS-Mac OS-Windows Pri-2
Status: Fixed (was: Started)
Requesting merge to 66. This is low risk since the features we are changing are behind a flag.
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 8 2018

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

commit 57f0c7b7b1feae5ad94fd5bb8849c6588077151a
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Mar 08 03:36:18 2018

desktop-pwas: Disable link capturing by default

After this patch, Desktop PWAs should no longer capture links without
users enable the kDesktopPWAsLinkCapturing flag.

Final patch to disable link capturing by default:

[1] Specify that throttle and tests are for experimental feature
    (https://crrev.com/ba5d9af)
[2] Extract tests that should run when link capturing is off
    (https://crrev.com/e3aa04a)
[3] Handle out-of-scope navigations and stop link capturing by
    default (https://crrev.com/c/942597)
[4] Disable link capturing by default (This patch)

Bug:  814102 
Change-Id: I0d03c93c2594c3da7d5139496502244e299c8e84
Reviewed-on: https://chromium-review.googlesource.com/950484
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541711}
[modify] https://crrev.com/57f0c7b7b1feae5ad94fd5bb8849c6588077151a/chrome/common/chrome_features.cc

Labels: -Merge-Request-66
Removing merge request since we don't think its critical to have this disabled for a milestone.

Sign in to add a comment