1347605 - Web Apps opened from homescreen should be singletons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Dale Harvey (:daleharvey)]

With pref("manifest.install.enabled", true)

Add a pwa, say pokedex.org to the homescreen, click on the bookmark created, it opens in its own activity, go to the homescreen and click on it again and you now have 2 windows, the second click should have returned to the already opened instance

Comment 1

[Dale Harvey (:daleharvey)]

was hoping launchMode=singleTask may be a simple fix for this but not looking like it it

Comment 2

[Dale Harvey (:daleharvey)]

So 
        <activity android:name="org.mozilla.gecko.LauncherActivity"
-            android:excludeFromRecents="true" />
+            android:launchMode="singleInstance" />

Means all homescreen bookmarks open as the same activity (ie it works for 1 bookmark, but not 2+), We may want to avoid LauncherActivity and launch WebAppActivity directly to avoid breaking any existing workflows, but still need some way for android to see that the different activity data (url's) are mean different activitys, looks like taskAffinity is close but not quite right, Sebastian do you know what to look at here? Cheers

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Dale Harvey (:daleharvey) from comment #2)
>         <activity android:name="org.mozilla.gecko.LauncherActivity"
> -            android:excludeFromRecents="true" />
> +            android:launchMode="singleInstance" />

This alone shouldn't be a problem. It just means that there should only be a single LauncherActivity instance. But there can be multiple instances of whatever activity to launch from here. For example I can have multiple chrome custom tabs open at the same time right now just fine.

> Means all homescreen bookmarks open as the same activity (ie it works for 1
> bookmark, but not 2+), We may want to avoid LauncherActivity and launch
> WebAppActivity directly

Yeah, this could be a good idea regardless of the problem here. Just try if this introduces other problems (e.g. I wonder if you need to prevent the same app opening in multiple activities).

> but still need some way for android to see that the different activity data (url's)
> are mean different activitys, looks like taskAffinity is close but not quite
> right, Sebastian do you know what to look at here? Cheers

Good question. I can't think of something in Android that would make this simple. However we should have solved this problem in the past when we supported Firefox OS web apps. This is the patch that removed the code, maybe it's interesting code for you:
https://hg.mozilla.org/mozilla-central/rev/93156962855d

I'll keep the NI and will respond once I find something.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Seems like for the old web apps we had 100 manifest entries and used some dispatching code to pick one for the current app: https://hg.mozilla.org/mozilla-central/rev/93156962855d

Comment 5

[Dale Harvey (:daleharvey)]

Hah yeh I jad just figured out that from the code when you commented, I think James Hugman had tried to explain this to me before and just got it. 

That doesnt seem ideal for us, not even certain we can since we arent getting full apk's, trying to figure out chromes (non apk) implementation now @ https://cs.chromium.org/chromium/src/chrome/browser/android/webapps/add_to_homescreen_manager.cc?dr=CSs

Comment 6

[Dale Harvey (:daleharvey)]

Chrome do the same thing @ https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?l=342

So as far as I can tell, this is how it has to work on android. Not the cleanest in the world but hey there is existing code to get it working :)

Not sure if N is the limit of of installable apps or concurrent running apps, if its installable 10 (chromes limit) seems awfully low, if its concurrent then 100 (our old limit) seems a bit high, but will experiment

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> (In reply to Dale Harvey (:daleharvey) from comment #2)
> >         <activity android:name="org.mozilla.gecko.LauncherActivity"
> > -            android:excludeFromRecents="true" />
> > +            android:launchMode="singleInstance" />
> 
> This alone shouldn't be a problem. It just means that there should only be a
> single LauncherActivity instance. But there can be multiple instances of
> whatever activity to launch from here. For example I can have multiple
> chrome custom tabs open at the same time right now just fine.

I just saw that you call filterFlags(intent) in LauncherActivity. This will remove the FLAG_ACTIVITY_NEW_TASK flag too. I guess in your case you always want to explicitly set that - you want it's own independent entry for ever ypp launch. You still need to avoid duplicated but adding this flag should at least allow you to have multiple instance.

Comment 8

[Dale Harvey (:daleharvey)]

Just a quick update, got this working based on old code + chrome and yeh I needed FLAG_ACTIVITY_NEW_TASK, thanks :)

The "slots" defined in the manifest can technically only be required for running apps, I was wondering if they were hardcoded into the launch activity but they are just routed within LaunchActivity so thats fine. However need to figure out how to best route manifest <-> index, how do we make sure each separate activity gets a unique index given we will have a fixed number of indexes and what happens when we run out?

One possibility seems like a rolling (FIFO) index of N that we store in the Java launcher, if our manifest is already in the index is gets its number back, if not the oldest manifest to be given an index is replaced and we make sure to CLEAR_TASK when we launch (just in case we are running more than N apps concurrently)

Sounds like it would work to me, but will check old chrome / firefox implementations

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: Ensure standalone pwa's are singletons

This is based on both our old webapp code and chromes current webapp implementation, the WebAppIndexer is based on chromes ActivityAssigner (https://github.com/crosswalk-project/chromium-crosswalk/blob/master/chrome/android/java/src/org/chromium/chrome/browser/webapps/ActivityAssigner.java) 

As per the comments we have a round robin of 10 predefined activity slots which we give to launched standalone mode webapps, this allows them to have the same singleTask semantics as native android apps

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8850909 [details] [diff] [review]
Ensure standalone pwa's are singletons

Review of attachment 8850909 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/WebAppManifestFragment.xml.frag.in
@@ +1,3 @@
> +        <activity android:name="org.mozilla.gecko.webapps.WebApps$WebApp@APPNUM@"
> +                  android:label="WebApp"
> +                  android:configChanges="keyboard|keyboardHidden|mcc|mnc|orientation|screenSize"

This should mirror the configChanges line from BrowserApp. We want to handle all config changes ourselves and there have been some new ones.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
@@ +93,5 @@
>      private void dispatchWebAppIntent() {
> +        final Context context = getApplicationContext();
> +        final Intent intent = new Intent(getIntent());
> +        final String manifestPath = getIntent().getStringExtra(WebAppActivity.MANIFEST_PATH);
> +        final int index = WebAppIndexer.getInstance().getIndexForManifest(manifestPath, context);

LauncherActivity is an implementation of context. You can just use "this" and do not need to call getApplicationContext().

(The only difference here is that if you want to keep a static reference then use the application context and not an activity because they come and go. However if this would be the case then you could call getApplicationContext() inside WebAppIndexer. But it doesn't seem like you keep a reference around.)

@@ +95,5 @@
> +        final Intent intent = new Intent(getIntent());
> +        final String manifestPath = getIntent().getStringExtra(WebAppActivity.MANIFEST_PATH);
> +        final int index = WebAppIndexer.getInstance().getIndexForManifest(manifestPath, context);
> +        intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
> +        intent.setClassName(context, WebAppIndexer.WEBAPP_CLASS + index);

Do we need to implement onNewIntent() in WebAppActivity in case it's already active from a previous launch of a different app? GeckoApp might handle opening the new URL but we might need to close the previous tab the activity was displaying?

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppIndexer.java
@@ +34,5 @@
> +    private static final int INVALID_INDEX = -1;
> +
> +    private ArrayList<ActivityEntry> mActivityList = new ArrayList<ActivityEntry>();
> +
> +    static class ActivityEntry {

nit: private?

@@ +36,5 @@
> +    private ArrayList<ActivityEntry> mActivityList = new ArrayList<ActivityEntry>();
> +
> +    static class ActivityEntry {
> +        public final int mIndex;
> +        public final String mManifest;

For the sake of simplicity I'm okay to omit getters for final fields. But we should drop the m-Prefix.

@@ +44,5 @@
> +        }
> +    }
> +
> +    private WebAppIndexer() { }
> +    private static WebAppIndexer _instance = new WebAppIndexer();

nit: we do not use an underscore prefix for static references. This could be final too, right? In this case it would be INSTANCE.

@@ +82,5 @@
> +    }
> +
> +    private void markActivityUsed(int index, String manifest, Context context) {
> +        int elementIndex = findActivityElement(index);
> +        ActivityEntry updatedEntry = new ActivityEntry(index, manifest);

nit: final, here and in some other places.. :)

@@ +99,5 @@
> +    }
> +
> +    // Store the list of assigned indexes in sharedPrefs because the LauncherActivity
> +    // is likely to be killed and restarted between webapp launches
> +    private void storeActivityList(Context context) {

store and load look like they need synchronization, but then again they are only called on the UI thread - so there shouldn't be concurrency, right? In this case let's add a @UiThread annotation to make this explicit.

@@ +101,5 @@
> +    // Store the list of assigned indexes in sharedPrefs because the LauncherActivity
> +    // is likely to be killed and restarted between webapp launches
> +    private void storeActivityList(Context context) {
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(context);
> +        SharedPreferences.Editor editor = prefs.edit();

nit: final

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebApps.java
@@ +1,1 @@
> +package org.mozilla.gecko.webapps;

nit: license header

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: Ensure standalone pwa's are singletons

Thanks for the review, addressed all your comments

After testing you were right, onNewIntent needed implemented to handle the case of us reusing open activities so if you could take a quick look at that code its the only part that changed aside from your comments. Its working well in testing for me but I would like to make sure. (will also send this to try)

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8852531 [details] [diff] [review]
Ensure standalone pwa's are singletons

Cancelling, I missed a case here

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Ensure standalone pwa's are singletons

Ok caught it, need to make sure the chrome / theme etc also updated

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8852563 [details] [diff] [review]
Ensure standalone pwa's are singletons

Review of attachment 8852563 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2284,5 @@
>  
>          Restrictions.update(this);
>      }
>  
> +    protected void restoreLastSelectedTab() {

Wait, doesn't this need to be called from inside GeckoApp now? You moved the code but didn't add a call to it inside GeckoApp.

Comment 15

[Dale Harvey (:daleharvey)]

heh yeh good catch, silly mistake

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7abe37fae43f
Ensure standalone pwa's are singletons

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7abe37fae43f

Comment 18

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 55.0a2 (2017-03-30). 
Devices: LG G4 (Android 5.1) & Huawei Honor 8 (Android 6.0)