Fix: long-tapping a WebAPK in recents shows Chrome.

Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity. Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make one of the WebAPK's activity be the root acitvity and lives in the
same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since this attribute
makes the WebApkActivity always be instanciated in a new task, not in the same task stack of
WebAPK's activity. Besides, we keep WebAPK's main activity unchanged, but we will launch another
activity, called ShellActivity on L+. The ShellActivity will remains in the recents.

We also increase the shell_version, which will result in updates of all existing installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug on L+. New version of WebAPKs will be fixed directly, existing installed
WebAPKs which will be fixed after updating to the new version.

BUG=700157

[Xi Han, 2017-03-22 20:30:41]

Description was changed from

==========
Long-tapping a WebAPK in recents shows Chrome issue

BUG=
==========

to

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

BUG=700157
==========

[Xi Han, 2017-03-22 20:51:54]

Description was changed from

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

BUG=700157
==========

to

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack. For WebApks, the root activity is WebApkActivity, which is a
Chrome's activity. Therefore, ChromeApplication's icon and title are shown
instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since
this attribute makes the WebApkActivity always be instanciated in a new task,
not in the same task stack of WebAPK's main activity. Besides, in WebAPK's main
activity, we don't call finish() and include it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and add proper flag when creating a launch Intent for
WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs, which will be fixed after updating to the new version.

BUG=700157
==========

[Xi Han, 2017-03-22 20:52:52]

Description was changed from

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack. For WebApks, the root activity is WebApkActivity, which is a
Chrome's activity. Therefore, ChromeApplication's icon and title are shown
instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since
this attribute makes the WebApkActivity always be instanciated in a new task,
not in the same task stack of WebAPK's main activity. Besides, in WebAPK's main
activity, we don't call finish() and include it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and add proper flag when creating a launch Intent for
WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs, which will be fixed after updating to the new version.

BUG=700157
==========

to

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity.
Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the
same task stack with WebApkActivity. Therefore, we have to
remove android:documentLaunchMode="InfoExisting" attribute from WebApkActivity,
since this
attribute makes the WebApkActivity always be instanciated in a new task, not in
the same task
stack of WebAPK's main activity. Besides, in WebAPK's main activity, we don't
call finish()
and include it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs,
which will be fixed after updating to the new version.

BUG=700157
==========

[Xi Han, 2017-03-22 20:54:07]

Description was changed from

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity.
Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the
same task stack with WebApkActivity. Therefore, we have to
remove android:documentLaunchMode="InfoExisting" attribute from WebApkActivity,
since this
attribute makes the WebApkActivity always be instanciated in a new task, not in
the same task
stack of WebAPK's main activity. Besides, in WebAPK's main activity, we don't
call finish()
and include it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs,
which will be fixed after updating to the new version.

BUG=700157
==========

to

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity.
Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the
same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since
this attribute
makes the WebApkActivity always be instanciated in a new task, not in the same
task stack of
WebAPK's main activity. Besides, in WebAPK's main activity, we don't call
finish() and include
it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs,
which will be fixed after updating to the new version.

BUG=700157
==========

[Xi Han, 2017-03-22 21:30:50]

Patchset #4 (id:60001) has been deleted

[Xi Han, 2017-03-22 21:30:56]

Patchset #3 (id:40001) has been deleted

[Xi Han, 2017-03-22 21:31:03]

Patchset #1 (id:1) has been deleted

[gone, 2017-03-22 22:20:07]

dfalcantara@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2017-03-22 22:20:07]

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (left):

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:567:
android:documentLaunchMode="intoExisting"
Removing this flag means you can end up with multiple WebApkActivities with the
same data.  Do you really want that?

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:47:
private static final int SHELL_APK_VERSION_BEFROE_REMOVE_DOCUMENT_LAUNCH_MODE =
1;
BEFORE

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:176:
} else {
You've changed this logic here so that non WebApks do either the first half of
the statement or the second half, depending on whether you're doing this from a
notification.  Is that intentional?

[gone, 2017-03-22 22:23:28]

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:176:
} else {
On 2017/03/22 22:20:07, dfalcantara (load balance plz) wrote:
> You've changed this logic here so that non WebApks do either the first half of
> the statement or the second half, depending on whether you're doing this from
a
> notification.  Is that intentional?

Well, I can see how this would stay the same logically.  The other version was
weirdly written so that it would override the flags from the previous call
instead of using an else clause.

[Xi Han, 2017-03-22 22:39:33]

Patchset #3 (id:100001) has been deleted

[Xi Han, 2017-03-23 15:24:17]

Hi Dan, PTAL, thanks!

[Yaron, 2017-03-23 15:29:53]

yfriedman@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2017-03-23 15:29:54]

I don't know this stuff well enough to know whether this could cause other
issues

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:548: android:launchMode="singleTask"
Hmm, I worry that us deviating from this may lead to unexpected bugs. That said,
there is a different interaction model for WebApks. That said, I see that
CustomTabs also uses singleTask.

[Xi Han, 2017-03-23 21:07:42]

hanxi@chromium.org changed reviewers:
+ tedchoc@chromium.org, yusufo@chromium.org

[Xi Han, 2017-03-23 21:07:42]

+yusufo@ and tedchoc@:
Do you have any idea why the WebApkActivity{i} behaviors differently from
SeparateTaskCustomTabActivity{i}?

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:548: android:launchMode="singleTask"
On 2017/03/23 15:29:53, Yaron (limited availability) wrote:
> Hmm, I worry that us deviating from this may lead to unexpected bugs. That
said,
> there is a different interaction model for WebApks. That said, I see that
> CustomTabs also uses singleTask.

I don't know why singleTask is just doesn't work for WebApkActivity. When they
are started without new_task or new_document flags, they are supposed to on top
of the WebappLauncherActivity and in the same task. But this isn't true for
WebApkActivity, Android always creates a new task stack for it.

[Ted C, 2017-03-23 23:16:08]

What do you mean SeparateTaskCustomTabActivity behaves differently from
WebApkActivity?

SeparateTaskCustomTabActivity is only NEW_TASK so I assume it always shows
Chrome when you long tap on it.

Are you sure you're not seeing vanilla CustomTabActivity that happens if you sit
on top of another activity?

If you turn on Elderberry in chrome:flags (pre L), and do adb shell am start -a
android.intent.action.VIEW -d "https://www.yahoo.com" you are using
SeparateTaskCustomTabActivity.

I don't see how it could say anything other than Chrome at that point TBH.

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:578: android:launchMode="singleTop"
The assigner pretty strongly assumes a 1:1 relationship, and this breaks that in
all types of ways.  This has to remain singleTask if you want the existing logic
to work.

[Xi Han, 2017-03-24 16:05:24]

On 2017/03/23 23:16:08, Ted C wrote:
> What do you mean SeparateTaskCustomTabActivity behaves differently from
> WebApkActivity?
When I use gmail app or search app on KK, they will open a CustomTabActivity
when clicking a link. I use "adb shell dumpsys activity" and I can see the
CustomTabActivity is in the same task stack with gmail or search acitivity:
 TaskRecord{42b4d008 #167 A=com.google.android.gm U=0 sz=2}
        Run #4: ActivityRecord{4254a268 u0
com.android.chrome/org.chromium.chrome.browser.customtabs.CustomTabActivity
t167}
        Run #3: ActivityRecord{42a52b20 u0
com.google.android.gm/.ConversationListActivityGmail t167}

However, if I launch a WebAPK, the WebApkActivity1 is always in its own task
stack:
TaskRecord{42b65c48 #165
I=com.google.android.apps.chrome/org.chromium.chrome.browser.webapps.WebApkActivity1
U=0 sz=1}
        Run #2: ActivityRecord{42569088 u0
com.google.android.apps.chrome/org.chromium.chrome.browser.webapps.WebApkActivity1
t165}
      TaskRecord{4282cd90 #164 A=org.chromium.webapk U=0 sz=2}
        Run #1: ActivityRecord{423565e0 u0
com.google.android.apps.chrome/org.chromium.chrome.browser.webapps.WebappLauncherActivity
t164}
        Run #0: ActivityRecord{422c9800 u0
org.chromium.webapk/.shell_apk.MainActivity t164}

> SeparateTaskCustomTabActivity is only NEW_TASK so I assume it always shows
> Chrome when you long tap on it.
Which activity is used on pre-L? For WebAPK, we have WebApkActivity{i} in
Chrome's AndroidManifest. Which one is for custom tab? I thought it is
SeparateTaskCustomTabActivity{i}, but maybe I am wrong.


> Are you sure you're not seeing vanilla CustomTabActivity that happens if you
sit
> on top of another activity?
From the above dumpsys activity results, I guess what I see is the regular
CustomTabActivity, which is standard mode.

 
> If you turn on Elderberry in chrome:flags (pre L), and do adb shell am start
-a
> android.intent.action.VIEW -d "https://www.yahoo.com" you are using
> SeparateTaskCustomTabActivity.
> 
> I don't see how it could say anything other than Chrome at that point TBH.
Yes, it is in its own task. Will it be the same case if the Intent is sent from
another app, like gmail? I guess yes, since the launch mode is singleTask.
 
>
https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
> File chrome/android/java/AndroidManifest.xml (right):
> 
>
https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
> chrome/android/java/AndroidManifest.xml:578: android:launchMode="singleTop"
> The assigner pretty strongly assumes a 1:1 relationship, and this breaks that
in
> all types of ways.  This has to remain singleTask if you want the existing
logic
> to work.

[Xi Han, 2017-03-24 16:05:36]

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/140001/chrome/android/java/An...
chrome/android/java/AndroidManifest.xml:578: android:launchMode="singleTop"
On 2017/03/23 23:16:07, Ted C wrote:
> The assigner pretty strongly assumes a 1:1 relationship, and this breaks that
in
> all types of ways.  This has to remain singleTask if you want the existing
logic
> to work.

I think only a WebAPK will launch a WebApkAcitivity instance, and each WebAPK
will have its own WebApkAcitvity since the meta-date in the Intent to start the
activity is unique for each WebAPK. Beside, we have a launcher class
WebappLauncherActivity to launch the WebApkActivityXXX, the activity launching
order is:
 WebAPK's main activity -> WebappLauncherActivity -> WebApkActivityXXX (has a
suffix number on KK)

Do you think it will really break by changing to singleTop ?

[Xi Han, 2017-03-28 17:53:37]

Patchset #6 (id:170001) has been deleted

[Xi Han, 2017-03-28 17:53:56]

Patchset #5 (id:150001) has been deleted

[Xi Han, 2017-03-28 18:00:46]

Description was changed from

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity.
Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make the WebAPK's main activity be the root acitvity and
lives in the
same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since
this attribute
makes the WebApkActivity always be instanciated in a new task, not in the same
task stack of
WebAPK's main activity. Besides, in WebAPK's main activity, we don't call
finish() and include
it in recents. 

We also increase the shell_version, which will result in updates of all existing
installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug for the new version WebAPKs, but not the existing installed
WebAPKs,
which will be fixed after updating to the new version.

BUG=700157
==========

to

==========
Fix: long-tapping a WebAPK in recents shows Chrome.

The icon shown in the recents is the application icon of the root activity in
the task stack.
For WebApks, the root activity is WebApkActivity, which is a Chrome's activity.
Therefore,
ChromeApplication's icon and title are shown instead.

To fix this, we need to make one of the WebAPK's activity be the root acitvity
and lives in the
same task stack with WebApkActivity. Therefore, we have to remove
android:documentLaunchMode="InfoExisting" attribute from WebApkActivity, since
this attribute
makes the WebApkActivity always be instanciated in a new task, not in the same
task stack of
WebAPK's activity. Besides, we keep WebAPK's main activity unchanged, but we
will launch another
activity, called ShellActivity on L+. The ShellActivity will remains in the
recents.

We also increase the shell_version, which will result in updates of all existing
installed
WebAPKs after WebAPK Server synced with the shell_apk changes.

To not break existing WebAPKs, we check the shell APK version in
WebappLauncherActivity and
add proper flag when creating a launch Intent for WebAPKs.

This CL fix the bug on L+. New version of WebAPKs will be fixed directly,
existing installed
WebAPKs which will be fixed after updating to the new version.

BUG=700157
==========

[Xi Han, 2017-03-28 18:21:48]

I have update my CL and description. PTAL, thanks!

[Yaron, 2017-03-29 14:07:34]

I'm not familiar with this stuff enough to know whether the extra activity is
needed but if it is, this seems reasonable

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:170:
launchIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK
doesn't this change behaviour pre-L? We didn't use to specify this.

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File chrome/android/webapk/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/AndroidManifest.xml:35:
android:theme="@android:style/Theme.Translucent.NoTitleBar">
android:exported=false so others can't use it

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/LaunchWebApkHelper.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/LaunchWebApkHelper.java:45:
private Activity mActivity;
I don't love that this holds a strong reference to an activity because we could
accidentally cause activity to not get gc'd. This basically seems like a static
util class that you can pass activity references through function calls

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java:14:
public class ShellActivity extends Activity {
i think we need a better name for this. Perhaps something like
RecentsPlaceholderActivity

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java:18:
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
This should only be called on L+? Then why the conditional below?

[Xi Han, 2017-03-29 14:49:37]

Patchset #8 (id:250001) has been deleted

[Xi Han, 2017-03-29 15:16:44]

Hi Yaron, PTAL, thanks!

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:170:
launchIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK
On 2017/03/29 14:07:33, Yaron (limited availability) wrote:
> doesn't this change behaviour pre-L? We didn't use to specify this.

Rewrite the logic here to make the skd version check go first. I guess it is
because on pre-L, singleTask always creates a new task for WebApkActivity{i}, so
it doesn't matter whether we set the flags or not.

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File chrome/android/webapk/shell_apk/AndroidManifest.xml (right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/AndroidManifest.xml:35:
android:theme="@android:style/Theme.Translucent.NoTitleBar">
On 2017/03/29 14:07:33, Yaron (limited availability) wrote:
> android:exported=false so others can't use it

Done.

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/LaunchWebApkHelper.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/LaunchWebApkHelper.java:45:
private Activity mActivity;
On 2017/03/29 14:07:33, Yaron (limited availability) wrote:
> I don't love that this holds a strong reference to an activity because we
could
> accidentally cause activity to not get gc'd. This basically seems like a
static
> util class that you can pass activity references through function calls

Done.

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java:14:
public class ShellActivity extends Activity {
On 2017/03/29 14:07:33, Yaron (limited availability) wrote:
> i think we need a better name for this. Perhaps something like
> RecentsPlaceholderActivity

Done.

https://codereview.chromium.org/2762873003/diff/230001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShellActivity.java:18:
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
On 2017/03/29 14:07:33, Yaron (limited availability) wrote:
> This should only be called on L+? Then why the conditional below?

Removed. I though assert is used like DCHECK, isn't it?

[gone, 2017-03-29 18:48:41]

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:162:
> SHELL_APK_VERSION_BEFORE_REMOVE_DOCUMENT_LAUNCH_MODE) {
This conditional logic should be a method on the WebApkInfo class instead of
logic known by the WebappLauncherActivity.  Like
WebApkInfo#shouldLaunchNewTask() or something.

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:25:
// On L+, we launch {@link ShellActivity} before launching {@link
WebApkActivity}.
Does this comment or the commit message still make sense?  I don't see a
ShellActivity here.

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/RecentsPlaceholderActivity.java
(right):

https://codereview.chromium.org/2762873003/diff/270001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/RecentsPlaceholderActivity.java:25:
}
This thing should always finish, shouldn't it?  Otherwise a failed launch
results in an invisible activity that responds to Intent launches when it
shouldn't.

[pkotwicz, 2017-03-29 19:24:22]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-03-29 20:20:11]

I am still looking at this CL. This CL breaks opening the WebAPK at a custom URL
from a deep link.

I am still investigating. I don't have any good suggestions yet

[pkotwicz, 2017-03-30 03:13:05]

This CL makes the launch intent sent by the WebAPK too complex. I dread
documenting this intent so that non-Chrome browsers can implement WebAPK
functionality. Can we leave things as is?

In order to support deep links, you will need to pass Intent#getDataString() as
an extra to RecentsPlaceholderActivity.java You can't pass it as the data string
because that will not play nicely with the intent matching done by
Intent.FLAG_ACTIVITY_NEW_DOCUMENT
I don't have a better suggestion because I was not able to get deep links to
work despite trying. Based on my understanding of
Intent.FLAG_ACTIVITY_NEW_DOCUMENT it should not be necessary. However, based on
trying it out, it seems necessary

[Yaron, 2017-03-30 14:09:55]

On 2017/03/30 03:13:05, pkotwicz wrote:
> This CL makes the launch intent sent by the WebAPK too complex. I dread
> documenting this intent so that non-Chrome browsers can implement WebAPK
> functionality. Can we leave things as is?
> 
The status quo doesn't work on recent versions of Android. That doesn't mean we
need this solution but we definitely need to do something. Additionally, a lot
of this is implementation details of the WebApk. I agree that we want to have a
clear rule for what the browser needs to do. Currently it appears that they will
only need to unconditionally perform CLEAR_TOP because they won't have to worry
about legacy.

> In order to support deep links, you will need to pass Intent#getDataString()
as
> an extra to RecentsPlaceholderActivity.java You can't pass it as the data
string
> because that will not play nicely with the intent matching done by
> Intent.FLAG_ACTIVITY_NEW_DOCUMENT
> I don't have a better suggestion because I was not able to get deep links to
> work despite trying. Based on my understanding of
> Intent.FLAG_ACTIVITY_NEW_DOCUMENT it should not be necessary. However, based
on
> trying it out, it seems necessary