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

Issue 876173 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 864904
issue 876577



Sign in to add a comment

desktop-pwas: Set source when installing through PendingAppManager

Project Member Reported by ortuno@chromium.org, Aug 21

Issue description

PendingAppManager is used for installing Default Apps and Policy Apps. In both of these cases, we need to set a special flag to indicate the source of app installed.

 
Labels: M-70
Labels: -Pri-3 Pri-2
> to indicate the app of app installed

Do you mean source of app installed?

Why do we need this? Does it help with  Issue 876182 ?
Blocking: 876577
Labels: -Pri-2 Pri-1
From discussion, the practical consequence of this is that it blocks the user from uninstalling a policy-installed app.

Setting to Pri=1, because otherwise if the user uninstalls a policy-installed app, it will come back again upon restart, which could be surprising.

Also note that  Issue 876577  won't actually be an issue *until* we set the source, and then we'll have to fix it.
Description: Show this description
Be aware that there could be more than one special flag to set.

There's Manifest::Location in extensions/common/manifest.h, e.g. INTERNAL vs EXTERNAL_POLICY.

There's Extension::InitFromValueFlags in extensions/common/extension.h, e.g. WAS_INSTALLED_BY_DEFAULT and WAS_INSTALLED_BY_OEM. They might not be needed for policy-installed apps, but will probably be needed for default-installed apps.
Yup. All of that is handled by BookmarkAppHelper::set_is_policy_installed_app() and BookmarkAppHelper::set_is_default_app()[1]. We just need to actually call these.

[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/bookmark_app_helper.h?sq=package:chromium&dr&g=0&l=93
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 23

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

commit 158420d6de59aed9754df28d305c3a20856ad621
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Aug 23 12:04:05 2018

desktop-pwas: Move ownership of AppInfo to the InstallationTask

AppInfo holds information about how the app should be installed e.g.
window vs. tab, is default app, is policy-installed, should create
shortcuts, etc. The installation task needs this information, so we
could either pass it as arguments, i.e. one argument per property in
AppInfo, or we could pass the AppInfo object directly. PendingAppManager
has little use for AppInfo, so this CL implements the latter and passes
the whole AppInfo object to BookmarkAppInstallationTask.

Bug:  876173 
Change-Id: I3d3c2e28ca2740b4149bae13263e798eaafa7eea
Reviewed-on: https://chromium-review.googlesource.com/1184649
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585455}
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/bookmark_app_installation_task.h
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.cc
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h
[modify] https://crrev.com/158420d6de59aed9754df28d305c3a20856ad621/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 24

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

commit b4839fbde6974e55ae95f32bcfd49c339d2071eb
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Aug 24 10:36:57 2018

desktop-pwas: Plumb AppInfo through TestBookmarkAppInstallationTask

Changes TestBookmarkAppInstallationTask in
pending_bookmark_app_manager_unittest.cc to notify the test that
Install is called. Then the test saves a copy of the used AppInfo to
compare to the app info originally passed to PendingAppManager.

This helps make sure AppInfo is correctly plumbed from PendingAppManager
up until BookmarkAppInstallationTask which will use it to install the
app.

Bug:  876173 
Change-Id: Iac87fc6e941f1da239717b306f1be44b18704320
Reviewed-on: https://chromium-review.googlesource.com/1186213
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585785}
[modify] https://crrev.com/b4839fbde6974e55ae95f32bcfd49c339d2071eb/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/b4839fbde6974e55ae95f32bcfd49c339d2071eb/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/b4839fbde6974e55ae95f32bcfd49c339d2071eb/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 28

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

commit 948cae415a34f5b07b46ba21b35f4b89414ba2c5
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Tue Aug 28 00:42:28 2018

desktop-pwas: Set the 'default app' installation flag

Adds a new bool to AppInfo; when set, the InstallationTask marks the
installed app as a "default app", which means that the app will behave
like other default installed Bookmark Apps.

TBR=khorimoto@chromium.org

Bug:  876173 
Change-Id: I102b85178862500ac5ba233a483192819cfb4e5c
Reviewed-on: https://chromium-review.googlesource.com/1188732
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586525}
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/bookmark_apps/external_web_apps.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/bookmark_apps/external_web_apps_unittest.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc
[modify] https://crrev.com/948cae415a34f5b07b46ba21b35f4b89414ba2c5/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 28

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

commit aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655
Author: Trent Apted <tapted@chromium.org>
Date: Tue Aug 28 00:58:51 2018

Revert "desktop-pwas: Set the 'default app' installation flag"

This reverts commit 948cae415a34f5b07b46ba21b35f4b89414ba2c5.

Reason for revert: breaks compile on https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win%20x64%20Builder/54466

errors like

In file included from ../..\chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h:8:
 error: no matching constructor for initialization of 'web_app::PendingAppManager::AppInfo'


Original change's description:
> desktop-pwas: Set the 'default app' installation flag
> 
> Adds a new bool to AppInfo; when set, the InstallationTask marks the
> installed app as a "default app", which means that the app will behave
> like other default installed Bookmark Apps.
> 
> TBR=khorimoto@chromium.org
> 
> Bug:  876173 
> Change-Id: I102b85178862500ac5ba233a483192819cfb4e5c
> Reviewed-on: https://chromium-review.googlesource.com/1188732
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#586525}

TBR=khorimoto@chromium.org,ortuno@chromium.org,dominickn@chromium.org

Change-Id: I9b79c4ff827eda4f9c58952863cf99cbb1c66724
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  876173 
Reviewed-on: https://chromium-review.googlesource.com/1192483
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586528}
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/bookmark_apps/external_web_apps.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/bookmark_apps/external_web_apps_unittest.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc
[modify] https://crrev.com/aa2eb9e6dff12d3fe1a56b4eb3b8234ea0006655/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 28

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

commit 4ed8265a8d3be3d0a6f4aa5be726819aadd08bad
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Tue Aug 28 05:02:20 2018

Reland "desktop-pwas: Set the 'default app' installation flag"

This is a reland of 948cae415a34f5b07b46ba21b35f4b89414ba2c5

The original CL conflicted with a CL that landed recently.

Original change's description:
> desktop-pwas: Set the 'default app' installation flag
>
> Adds a new bool to AppInfo; when set, the InstallationTask marks the
> installed app as a "default app", which means that the app will behave
> like other default installed Bookmark Apps.
>
> TBR=khorimoto@chromium.org
>
> Bug:  876173 
> Change-Id: I102b85178862500ac5ba233a483192819cfb4e5c
> Reviewed-on: https://chromium-review.googlesource.com/1188732
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#586525}

TBR=khorimoto@chromium.org, dominickn@chromium.org

Bug:  876173 
Change-Id: Id6ae9cc9ae9db7840807bc1ec38f0cf301711af9
Reviewed-on: https://chromium-review.googlesource.com/1192563
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586584}
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/chromeos/multidevice_setup/android_sms_app_helper_delegate_impl_unittest.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/bookmark_apps/external_web_apps.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/bookmark_apps/external_web_apps_unittest.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc
[modify] https://crrev.com/4ed8265a8d3be3d0a6f4aa5be726819aadd08bad/chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

Labels: -Pri-1 Pri-2
We now set the "default app" flag for default apps. So lowering to p2
Project Member

Comment 14 by bugdroid1@chromium.org, Aug 30

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

commit f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Aug 30 15:41:58 2018

desktop-pwas: Add an option to create policy-installed apps

Replaces the "default app" boolean with an enum. If the enum has a
kFromPolicy value, the InstallationTask marks the installed app as
"policy installed" which means that the app will behave like other
policy installed apps e.g. won't sync, user won't be able to uninstall
them, etc.

Bug:  876173 
Change-Id: I37821523387cdccd4150ac3c600c94dd9c510050
Reviewed-on: https://chromium-review.googlesource.com/1195181
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587594}
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/extensions/bookmark_app_helper.h
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.cc
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/components/pending_app_manager.cc
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/components/pending_app_manager.h
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc
[modify] https://crrev.com/f67f7127cb1aa1e7f5351de295c8f4dcd2fc6e8c/chrome/browser/web_applications/extensions/bookmark_app_installation_task_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment