We should also specify the WebAPK package name in WebApkInstaller#installWebApkAsync(). I think that ServiceTabLauncher will do something weird if a user has never launched a WebAPK (And the Identity service returns a null runtime host)

Some of the weirdness in this bug is due to WebappDataStorage#updateFromShortcutIntent() storing the wrong action. (We want to storage the action for WebappLauncherActivity, not WebappActivity/WebApkActivity)

Assigning to Xi since she has touched ServiceTabLauncher recently

Hmm - WebappDataStorage should only be cleared if you clear chrome's data - not the WebApk's. 

I guess I don't fully understand the bug you're describing - what's broken?

The bug is more of a "hunch of steps to produce incorrect behavior" rather than something that I have tested extensively. I think that things go badly when ServiceTabLauncher attempts to launch a Webapp using a WebappDataStorage which belongs to a WebAPK.

Yesterday, I was getting an intent picker with really weird options when tapping on a notification that I generated via tests.peter.sh/notification-generator/ None of the options on the intent picker included a browser. The intent picker was getting shown because WebappDataStorage#createWebappLaunchIntent() was returning an intent with action android.intent.action.VIEW instead of com.google.android.apps.chrome.webapps.WebappManager.ACTION_START_WEBAPP I don't know how to get Chrome into this situation but I have a hunch that the steps in comment #1 will do the trick.

I think that the steps that I outlined in the bug description results in launching a URL in webapp mode when:
- the URL belongs to a WebAPK not a webapp
- the WebAPK is backed by another browser

This morning Xi pointed out that my steps "only work for comment signed WebAPKs"

Yes, I have repro it locally with an unbound WebApk. Haven't had a chance to look into it though.

Oh gotcha. Sorry, the bug description didn't make that very clear but that does sound pretty bad. 

And we may want to pull this onto m61 once we understand it better. Let's see waht comes of investigation

I realize that the repro is with my last patch of the Identity service, which query the runtime host of WebAPK in ServiceTabLauncher. If there is a WebAPK cached in the webappDataStorage but not backs by the Chrome, the ServiceTabLauncher will try to launch it in WebappActivity. It seems the WebAPK's data in webappDataStorage isn't able to create a correct Intent to launch a WebappActivity. Maybe the ServiceTabLauncher should just open a tab in such case (not try to open a WebappActivity)? It also reminds me that perhaps we need query the Identity service when doing regular clean up tasks in WebappRegistry, and remove the entries of WebAPKs that no long backs by Chrome. Thoughts? 

agreed that it should open tab in that case.
cleanup makes sense too

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

commit 524465a1372efcd98ec44c4ee1fb8cda38e227f4
Author: Xi Han <hanxi@google.com>
Date: Wed Aug 09 18:12:14 2017

Skip WebAPKs when querying WebappDataStorage for existing legacy Webapps for a URL.

In CL (https://chromium-review.googlesource.com/c/583438), a check of whether
the browser backs the found WebAPK is added in the ServiceTabLauncher when
service worker launches tab. As a result, as long as there is an WebAPK can
handle the URL, ServiceTabLauncher will create an intent to launch a
WebappActivity when Chrome doesn't back the WebAPK.

WebappDataStorage has an incorrect WebappDataStorage#KEY_ACTION for WebAPKs.
WebappDataStorage#KEY_ACTION is populated via
WebappDataStorage#updateFromShortcutIntent(). In the case of webapps,
WebappDataStorage#updateFromShortcutIntent() is initially called with the
WebappLauncherActivity launch intent from ShortcutHelper#addWebapp().
WebappDataStorage#updateFromShortcutIntent() is called with the intent to launch
WebappActivity / WebApkActivity when a WebAPK / webapp is launched. The action for
launching WebappLauncherActivity and for launching WebappActivity are different
(WebappLauncherActivity#ACTION_START_WEBAPP and Intent#ACTION_VIEW respectively). When
using a WebAPK's WebappDataStorage to launch a webapp, Android shows a weird intent
picker because it cannot figure out which app to launch for Intent#ACTION_VIEW
(WebappDataStorage#updateFromShortcutIntent() does not set the target package name and
does not set the data URI)

This CL makes two improvements:
1) Since launching a webapp with an intent generated from WebappDataStorage for a
WebAPK is bad , we skip WebAPKs in WebappDataStorage#getWebappDataStorageForUrl().
The query of Webapps will return the storage of a legacy Webapp if exists.
2) The CL sets the target package name in
WebappDataStorage#getWebappDataStorageForUrl().

Bug:  749084 ,  750320 
Change-Id: I7349fdea9412d9419ed07946ffd42d04d21f2e58
Reviewed-on: https://chromium-review.googlesource.com/590582
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493056}
[modify] https://crrev.com/524465a1372efcd98ec44c4ee1fb8cda38e227f4/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
[modify] https://crrev.com/524465a1372efcd98ec44c4ee1fb8cda38e227f4/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
[modify] https://crrev.com/524465a1372efcd98ec44c4ee1fb8cda38e227f4/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
[modify] https://crrev.com/524465a1372efcd98ec44c4ee1fb8cda38e227f4/chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java