It looks like the existing code does attempt to check whether the app is actually installed or not (@ app_banner_manager.cc:537) but if the preceeding call to  AppBannerSettingsHelper::ShouldShowBanner is less strict, so code will be ALREADY_INSTALLED even if the app has been uninstalled.

Hmm, on android, IsWebAppInstalled only considered WebAPKs:

  // Returns true if a WebAPK is installed or is being installed.
  // Does not check whether a non-WebAPK web app is installed: this is detected
  // by the content settings check in AppBannerSettingsHelper::ShouldShowBanner
  // (due to the lack of an API to detect what is and isn't on the Android
  // homescreen). This method will still detect the presence of a WebAPK even if
  // Chrome's data is cleared.

@dominickn: is there really no way to know if non-WebAPKs are currently installed on android? On desktop this is implemented by inspecting the enabled extensions in the ExtensionRegistry. I guess on android we have no extension support, so am I correct in guessing that non-WebAPKs are simply shortcuts that are added to the homescreen in a fire-and-forget manner?

c#2: you're mostly correct. :)

On Android, add to home screen doesn't create an extension. It just drops a shortcut on the launcher. That shortcut contains an intent which when fired opens up Chrome and gives it a URL to visit. Pre-O, we have no way of detecting non-WebAPK shortcuts after we've added them to the homescreen since there's no launcher API for doing so. That means the user could have removed a shortcut and we'd be none the wiser.

For O+, there is a new homescreen shortcut API which does allows us to to query what shortcuts exist (though it doesn't work with shortcuts created pre-O and kept during the upgrade). We haven't yet done the plumbing that lets us compare what is installed to what we think is installed (this is probably a few weeks worth of Java side work) since O-only work has not been a priority.

We have a heuristic in place (AppBannerSettingsHelper::WasLaunchedRecently) which is used as a proxy for installed state: we consider a shortcut to still exist if it was launched from home screen in the last ten days. This isn't particularly strict though, and it's only used for things like deep-linking or metadata cleanup that don't need to be as reliable as installation (if you haven't launched in a while but the shortcut is still there we really don't want to offer you install again).

https://developer.android.com/reference/android/content/pm/ShortcutManager.html#getPinnedShortcuts() is the O+ API. Plumbing that in would require:

 - adding Java side code to call that and pass the list to C++
 - comparing the C++ side list to the list from Java
 - also pruning the Java-side WebappRegistry/WebappDataStorage for uninstalled things

Ideally we'd do all of this checking on the Java side so there's not a duplication of data. But we don't have Java on non-Android platforms so it needs to stay in C++ for desktop PWAs (and vice-versa, we need to keep the Java-side WebappRegistry/DataStorage because that needs to be accessed before the C++ library is loaded on startup).

AFAICT, WasLaunchedRecently() is only called from tests. ShouldShowBanner() does not actually exclude banner events that happened outside of the kRecentLastLaunchInDays window. Which means that we never show the banner again if you've ever installed the app (I'm presuming that the content settings never expire via some other mechanism).

Just to recap:

1. For desktop, we can authoritatively determine whether an app is currently installed via extensions::BookmarkAppHelper::BookmarkOrHostedAppInstalled.

2. For Oreo+ we can authoritatively determine whether an app is currently installed via getPinnedShortcuts, except that apps which were installed pre-Oreo and survived the upgrade will be missed (false negatives).

3. Regardless of platform, we currently treat all apps that have been installed at some point as if they are installed.


I think we should address this separately for desktop vs android:

For desktop: stop relying on AppBannerSettingsHelper heuristics and just use BookmarkAppHelper::BookmarkOrHostedAppInstalled.  This can be done immediately AFAICT.


For android:
  pre-Oreo: should we be using kRecentLastLaunchInDays?
  Oreo+: 
    * if we switch to relying on getPinnedShortcuts() and offer to install an app that was installed pre-oreo, what is the impact if the user chooses to install it again? Would we end up with two shortcuts, or just one?  Would the shortcut thenceforth be returned by getPinnedShortcuts()?
    * what is the prioritization of the required work?

I think the android part deserves a separate bug.

WasLaunchedRecently has an Android equivalent (wasUsedRecently) that behaves exactly the same, though for required-before-native-library-loaded reasons, it's implemented separately in Java. I actually removed the last C++ client of WasLaunchedRecently() this week, so perhaps that method can be removed.

Re desktop plan: BookmarkAppHelper::BookmarkOrHostedAppInstalled is actually already used (see AppBannerManagerDesktop::IsWebAppInstalled). But removing the heuristic here SGTM

Re Android pre-O: I don't think we should use kRecentLastLaunchDays - as mentioned in c#3, if you haven't launched in a while but the shortcut is still there we probably don't want to offer an install again. I think the best we can do here is watch legacy PWA vs WebAPK install rate, and at some point when we feeling that WebAPKs have enough saturation (and we've removed the legacy PWA fallback path), remove the heuristic and rely purely on the WebAPK installation check (which is authoritative)

Re Android post-O: yes, you would get 2 shortcuts (an old one and a new one; the new one would be badged with a Chrome icon and the old one would be unbadged) for the app. The new one would be returned by getPinnedShortcuts(), but we'd never ever know about the other one. I think the plan here should be the same as the pre-O plan.

Re prioritisation: I'd prioritise the Android O work pretty low since O remains a tiny fraction of our user base. Additionally, WebAPKs obsoleted the need to do it somewhat because we can tell if they're installed without a problem. Splitting to a different bug SGTM

Thanks for investigating here - hope my thoughts are clear. :)

I agree with everything you've said, but have one clarification:  AFAICT, IsWebAppInstalled() is never called if the app has previously been installed, because AppBannerSettingsHelper::ShouldShowBanner() returns ALREADY_INSTALLED at app_banner_manager:533. This means that it is only called when we already know that the app has never been installed, and therefore has no effect.  That's why I want to remove the heuristic for desktop.

I've started working on a CL and will create separate android bug.

Ah, thanks for the clarification. That all makes sense, thanks for looking at this. :)

Android bug is crbug.com/786268.

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

commit 65d0e0f4d84abab407f942e81b3937821075ec0d
Author: Michael McGreevy <mcgreevy@chromium.org>
Date: Thu Nov 23 05:18:03 2017

Don't use a heuristic when checking app install status on desktop.

On desktop, we can tell precisely whether an app is installed. Therefore, using
"has the app ever been installed?" as a heuristic is unnecessary and results in
false positives. By removing this heuristic, we now correctly send the
beforeinstallprompt event on pages whose app has been uninstalled.

Bug:  786186 
Change-Id: I86482ad4a79f7707ffd35dcdb1f14a0fa7365e29
Reviewed-on: https://chromium-review.googlesource.com/778679
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Michael McGreevy <mcgreevy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518855}
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/android/instantapps/instant_apps_settings.cc
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager.h
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager_android.cc
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager_android.h
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager_desktop.cc
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_manager_desktop.h
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_settings_helper.h
[modify] https://crrev.com/65d0e0f4d84abab407f942e81b3937821075ec0d/chrome/browser/banners/app_banner_settings_helper_unittest.cc