Devlin: What do you think about changing the Manifest::Location we currently use to install these apps from INTERNAL to EXTERNAL_* (EXTERNAL_PREF_DOWNLOAD seems like the most appropriate one).

Then if the user uninstalls the app, the extensions system will save it in the State extension pref, which we do check through IsExternalExtensionUninstalled before trying to install the app.

Another option would be to add our own pref similar to State where we (WebApps code) keep track of which of our apps have been uninstalled and check that before trying to uninstall a new app.

Any preferences?

Hmm... I'm not entirely opposed to either approach.  I'd probably lean towards introducing a new web apps preference, to isolate the logic, and avoid introducing other "external extension"-specific behavior.  I'm also curious - why don't we have this problem with default installed apps?  They are also installed using Location::INTERNAL, [1] and don't leave any trace once uninstalled.  But they are not subsequently re-installed...

[1] https://chromium.googlesource.com/chromium/src/+/1c87a710cff78be4ebd3d10fad1ed80bc747a6ba/chrome/browser/extensions/external_provider_impl.cc#746

Huh... good question...

Nigel do you know why this is not a problem for other default installed apps?

Are default extension based apps installed at every login? I had thought that they were only tried to be installed once (i.e. for a new profile).

Heh, benwells@, this was the thing I was telling you, about an hour ago, that I was trying to figure out why some bit of code was *not* getting called...

They (default extension apps) are installed only once, on first login, not on every login. After that, there's a "default_apps_install_state" pref stored so that they're not installed again.

See the default_apps::Provider::ShouldInstallInProfile function at [1] and also default_apps::Provider::VisitRegisteredExtension a bit further down.

[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/default_apps.cc?type=cs&q=default_apps::Provider::ShouldInstallInProfile&sq=package:chromium&g=0&l=59

I don't think that's the ideal behavior. It means that we can't add new default installed apps to existing users as we upgrade Chrome (or Chrome OS).

FWIW, default_apps::Provider::ShouldInstallInProfile also has this comment:

// The old default apps were provided as external extensions and were
// installed everytime Chrome was run. Thus, changing the list of default
// apps affected all users. Migrate old default apps to new mechanism where
// they are installed only once as INTERNAL.
// TODO(grv) : remove after Q1-2013.

As for what to do, I'd lean towards changing INTERNAL to EXTERNAL_PREF_DOWNLOAD, although that does conflict with the recent comment update I made in https://chromium-review.googlesource.com/c/chromium/src/+/1170448/2/extensions/common/manifest.h after a conversation with Devlin.

Ah, that explains it.  Thanks for the investigation, nigeltao@!

(I'm still fine either way between using EXTERNAL_PREF_DOWNLOAD or having a separate web_applications-managed pref)

Changing to EXTERNAL_PREF_DOWNLOAD should be fairly easy.

Do we want EXTERNAL_PREF or EXTERNAL_PREF_DOWNLOAD for default-installed web apps? For policy-installed, it looks like, in BookmarkAppHelper::OnBubbleCompleted, we're using EXTERNAL_POLICY, not EXTERNAL_POLICY_DOWNLOAD.

Having BookmarkAppHelper::OnBubbleCompleted set EXTERNAL_POLICY was added a couple of weeks ago in https://chromium-review.googlesource.com/1170152 "desktop-pwas: Allow clients to set the location and extra install flags"

I don't really have strong feelings for or against the FOO vs FOO_DOWNLOAD flavors, other than we should probably be consistent across default-installed and policy-installed.

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

commit 50b58dbaf9c217a0fe5c9435b1393eb7b10bb42b
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Wed Sep 05 01:35:24 2018

desktop-pwas: Change Manifest::Location for default apps and policy apps

Sets Manifest::EXTERNAL_PREF_DOWNLOAD as the Manifest::Location for
default apps. This way, when a user uninstall a default app, we'll
save this user decision in a pref and avoid installing the app in the
future.

Also changes the location for policy apps to
MANIFEST::EXTERNAL_POLICY_DOWNLOAD, to be consistent with default
apps' location.

Bug:  878663 
Change-Id: I3930feb146f2b7e7c92052d983f23d18b3f5ab95
Reviewed-on: https://chromium-review.googlesource.com/1198715
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588732}
[modify] https://crrev.com/50b58dbaf9c217a0fe5c9435b1393eb7b10bb42b/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/50b58dbaf9c217a0fe5c9435b1393eb7b10bb42b/chrome/browser/extensions/bookmark_app_helper_unittest.cc
[modify] https://crrev.com/50b58dbaf9c217a0fe5c9435b1393eb7b10bb42b/chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager_unittest.cc
[modify] https://crrev.com/50b58dbaf9c217a0fe5c9435b1393eb7b10bb42b/chrome/browser/web_applications/extensions/web_app_extension_ids_map.cc

This should be fixed now.

khorimoto/jklein: do you need this to be merged to 70?

The Android SMS app isn't a default app, so I don't think this applies to us.

We mark the Android SMS app as "default installed" so that has all the same characteristics as other default installed apps e.g. not syncing.

Regardless, I think this might apply depending on how you install the app. Do you try install it once after finishing setup or do you try to install it on startup every time?

If the former, then you're right, this doesn't apply to Android SMS, if the latter then this would apply to it.

Right now, we just try to install it once after finishing setup, so we're probably alright here. If that changes in M70 for some reason, I'll be sure to keep this in mind :-).

We talked offline about this. Sydney will make a change to allow the PWA check to be forced successful. I had a quick look and it might just need [1] to be updated to also take into account something we plumb through in AppInfo.

[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/bookmark_app_helper.cc?rcl=814df605b56ec2f70ed21ebe90f42f027811ed44&l=392