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

Issue 892013 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Feature

Blocking:
issue 889660



Sign in to add a comment

Option to bypass service worker check for hard-coded apps

Project Member Reported by mgiuca@chromium.org, Oct 4

Issue description

Chrome Version: 71
OS: Chrome OS

Continuing from  Issue 889660  (filing a new bug since this represents the chosen course of action, which would solve the above use case).

There seems to be a race condition when installing a PWA through PendingAppManager::Install; sometimes we can't detect that it's a PWA and then it can't be installed.

Since we know it's a PWA*, we shouldn't have to rely on the flaky PWA installability check. We should keep checking that everything else is there (manifest, icon) but skip the service worker check.

The simplest way to do this is probably to add a field to PendingAppManager::AppInfo like "force_pwa_install".

*This SHOULD NOT be used on sites that aren't PWAs. That would unfairly allow us to force install in PWA mode of an app that isn't actually a PWA. This should have an associated policy (documented on the force_pwa_install field) that the target app SHOULD pass the PWA installability checks, so that we do not unfairly advantage programmatically installed PWAs.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 10

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

commit 36a492c3fbd97636d3291bdb358c85a95abb4d41
Author: Alan Cutter <alancutter@chromium.org>
Date: Wed Oct 10 04:44:10 2018

Bypass service worker check for Android SMS PWA default app

This CL adds a way to skip checking for a service worker when
installing a PWA via internal APIs. This is to work around a
problem where the service worker hasn't had time to get
registered before installability check is made.

Bug:  892013 
Change-Id: Ifc5685bc9c766a1db200e84234c4112d7ef81197
Reviewed-on: https://chromium-review.googlesource.com/c/1267917
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598212}
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/36a492c3fbd97636d3291bdb358c85a95abb4d41/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10

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

commit d9d6437a33a99ee005180c100dc004771444cd65
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Wed Oct 10 08:48:10 2018

Revert "Bypass service worker check for Android SMS PWA default app"

This reverts commit 36a492c3fbd97636d3291bdb358c85a95abb4d41.

Reason for revert: the new tests are failing on Mac10.13
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/5689

[1612:775:1009/233543.457141:FATAL:extension_util.cc(340)] Check failed: base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing).

Original change's description:
> Bypass service worker check for Android SMS PWA default app
> 
> This CL adds a way to skip checking for a service worker when
> installing a PWA via internal APIs. This is to work around a
> problem where the service worker hasn't had time to get
> registered before installability check is made.
> 
> Bug:  892013 
> Change-Id: Ifc5685bc9c766a1db200e84234c4112d7ef81197
> Reviewed-on: https://chromium-review.googlesource.com/c/1267917
> Commit-Queue: Alan Cutter <alancutter@chromium.org>
> Reviewed-by: Jeremy Klein <jlklein@chromium.org>
> Reviewed-by: Matt Giuca <mgiuca@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#598212}

TBR=alancutter@chromium.org,mgiuca@chromium.org,jlklein@chromium.org

Change-Id: I9982defac895172916421e0e716046409edd52ac
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  892013 
Reviewed-on: https://chromium-review.googlesource.com/c/1273039
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598260}
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/d9d6437a33a99ee005180c100dc004771444cd65/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 11

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

commit 378aedf60294cb27ecc3f534562d69987a2bf99d
Author: Alan Cutter <alancutter@chromium.org>
Date: Thu Oct 11 00:45:00 2018

Reland "Bypass service worker check for Android SMS PWA default app"

Changes made in reland:
Added DesktopPWAWindowing flag to failing tests.

Original change's description:
> Revert "Bypass service worker check for Android SMS PWA default app"
>
> This reverts commit 36a492c3fbd97636d3291bdb358c85a95abb4d41.
>
> Reason for revert: the new tests are failing on Mac10.13
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/5689
>
> [1612:775:1009/233543.457141:FATAL:extension_util.cc(340)] Check failed: base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing).
>
> Original change's description:
> > Bypass service worker check for Android SMS PWA default app
> >
> > This CL adds a way to skip checking for a service worker when
> > installing a PWA via internal APIs. This is to work around a
> > problem where the service worker hasn't had time to get
> > registered before installability check is made.
> >
> > Bug:  892013 
> > Change-Id: Ifc5685bc9c766a1db200e84234c4112d7ef81197
> > Reviewed-on: https://chromium-review.googlesource.com/c/1267917
> > Commit-Queue: Alan Cutter <alancutter@chromium.org>
> > Reviewed-by: Jeremy Klein <jlklein@chromium.org>
> > Reviewed-by: Matt Giuca <mgiuca@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#598212}
>
> TBR=alancutter@chromium.org,mgiuca@chromium.org,jlklein@chromium.org
>
> Change-Id: I9982defac895172916421e0e716046409edd52ac
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  892013 
> Reviewed-on: https://chromium-review.googlesource.com/c/1273039
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#598260}

TBR=vasilii@chromium.org,alancutter@chromium.org,mgiuca@chromium.org,jlklein@chromium.org

Change-Id: I850161f8c95df0f8d735dda23199e344b80ffc43
Bug:  892013 
Reviewed-on: https://chromium-review.googlesource.com/c/1275065
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598599}
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/378aedf60294cb27ecc3f534562d69987a2bf99d/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment