740586 - launch of basic app in a chromeless window

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Jennifer Arguello :ticachica]

Flow - connected:
* User taps the icon on the home screen 
* App opens in a chromeless window
* User uses app
* For example: a chat app

Flow - offline:
* User taps the icon on the home screen
* App opens in a chromeless window
    ** If manifest says it  does not work offline then display appropriate message
    ** Otherwise, app works as designed
* For example: Text editor app

Comment 1

[Jason Smith [:jsmith]]

This needs more use cases describing chromeless requirements while the app is running. During creation of the apps quality checklist currently being built, example cases to consider:

- Platform integration (e.g. uploading a file)
- Plugins and Experimental Features (e.g. flash, webGL)
- Out of origin content and links

Comment 2

[Jennifer Arguello :ticachica]

This bug does not cover the offline case. That is in bug 743917.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP

This is a really rough WIP. After trying a few things and talking to a few people I think we're going to try and run webapps in their own process with their own profile (maybe eventually per-app profiles, but for now just one special webapp one).

This gets us some of the way there. Creates a special "chromeless" WebApp app (that currently displays nothing....), and pulls most of the mBrowserToolbar out of GeckoApp. On the way I realized I've taken some wrong tracks and some of those are still in here waiting to be cleaned up. Should probably be in a few separate patches, but wanted to put it up somewhere as a start.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Created attachment 627045 [details] [diff] [review]
> WIP
> 
> This is a really rough WIP. After trying a few things and talking to a few
> people I think we're going to try and run webapps in their own process with
> their own profile (maybe eventually per-app profiles, but for now just one
> special webapp one).
> 
> This gets us some of the way there. Creates a special "chromeless" WebApp
> app (that currently displays nothing....), and pulls most of the
> mBrowserToolbar out of GeckoApp. On the way I realized I've taken some wrong
> tracks and some of those are still in here waiting to be cleaned up. Should
> probably be in a few separate patches, but wanted to put it up somewhere as
> a start.

FYI - In building this, may good to get feedback from Myk periodically, so that we avoid creating any runtime bugs that happened on desktop in the android version too. I can help in this area as well. Just let me know what you need - I've got information on the style of bugs we've detected, pain points, etc.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: WIP 2

Just keeping this up to date for anyone following along. This fixes some issues with the webapp program not showing anything (bad commandline args being passed by me), except you can still only run one (i.e. you're either chromeless fennec, or normal fennec, but not both).

Some headache inducing digging tells me that we're actually opening the profile manager when you try to open the second app, and Gecko isn't very smart about switching windows right now, so it just shows nothing.

We could implement reading and writing profiles.ini to fix this and create the profile folder (correctly) in Java before we start Gecko.

Or we need to fix Gecko to not try to open the profile manager for Android, but just automagically create the profile.

OR, if we were really fancy, we'd write an Android profile manager as part of our sign-into-browser work and show that instead? but webapps shouldn't show that, so we'll still need a "don't show this but still make a profile" fallback.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I don't think we need a profile manager -- I think we want automatic profile creation if you start with a web app intent.

Putting it here so I don't forget -- I think to support multiple web apps, we'll really need multiple activities(so they can all have their separate processes).  We can just create WebAppActivity1 .. WebAppActivity20, and create some classes that inherit all the code.  That way we can support 20 separate web apps (and bump the number higher as we need to).

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 715307 has a patch to read and use the profile set in profiles.ini

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> bug 715307 has a patch to read and use the profile set in profiles.ini

We need that to support "sign into browser" too.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP 2.5, updated

Updated WIP 2 to m-c and made a few changes, including commenting out the code that passes args in GeckoThread.  -P webapps -no-remote isn't working until we can create the profile, so we just use the default profile until we fix the profile bits.  Also, not sure what, if anything, -no-remote would do on Android -- didn't look at the source.

This works with:

  adb shell am start -a android.activity.MAIN -n org.mozilla.fennec_vladimir/.WebApp --es args "-url http://www.mozilla.org "

mozilla.org shows up fullscreen with no chrome.

-However- the regular browser activity crashes with:

E/GeckoAppShell( 2650): android.view.InflateException: Binary XML file line #129: Error inflating class org.mozilla.gecko.AboutHomeSection
...
E/GeckoAppShell( 2650): Caused by: java.lang.NullPointerException
E/GeckoAppShell( 2650): 	at org.mozilla.gecko.AboutHomeSection.setTitle(AboutHomeSection.java:51)
E/GeckoAppShell( 2650): 	at org.mozilla.gecko.AboutHomeSection.<init>(AboutHomeSection.java:39)
E/GeckoAppShell( 2650): 	at org.mozilla.gecko.GeckoViewsFactory.onCreateView(GeckoViewsFactory.java:48)

haven't quite figured out why yet, but I'm guessing some initialization got moved around.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP3

More updates.  Has two WebApp activities, WEBAPP1 and WEBAPP2, and creates separate profiles for everything.  Needs the patch in bug 715307 applied first.

Launching via:

adb shell am start -a org.mozilla.gecko.WEBAPP1 -n org.mozilla.fennec_vladimir/.WebApp1

or 2 will launch the web app intent, while normal fennec startup will launch normal fennec.  browser/app1/app2 will each run in their own process with their own profile.  One big issue is that only one seems to be visible to the user at a time; that is, if you launch WebApp1, then you launch WebApp2, you only see WebApp2 in the recent activities list.  We can maybe fix this by just having multiple applications instead of just multiple activities.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP V3.1

This is basically the same patch with all the BrowserToolbar stuff yanked out and put into a separate patch (bug 753102).

Comment 12

[Wesley Johnston (:wesj)]

I have to run for today too. One bug I know about here. We need to set the right activity for the intents:

Second, since we're running this in a second process, we don't have access to our installed webapps manifest. So this stuff fails (I think):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5013

We have the url, so I'm not sure what using the manifest gains us....

Comment 13

[Wesley Johnston (:wesj)]

whoops. meant to add a link to this for the first problem:

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoAppShell.java#801

Easy to solve. We just need a different setClassName call for different shortcut types.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP 3.2

This depends on the patches in bug 715307 and 753102.

With this, you can demo the full cycle: fire up Fennec, go to the app marketplace, install an app and it'll create a shortcut; clicking that shortcut will launch a new instance of fennec with -webapp and the URL in chromeless mode.

Things that need to be done:

- This just hardcodes WebApp1.  We need code that'll parcel out the available WebApp# processes/intents based on app installs/uninstalls.
- We need to change the name of the running activity from "WebApp#" to the name of the web app.
- We need to always create a default profile; right now if you happen to wipe fennec's data, and then launch with a web app shortcut, you'll create a webapp# profile and it'll get marked as the default.  Launching the browser will then use that profile incorrectly.
- We need to add a tab switcher or at least a menu item that'll get you a tab or window list; if you open a new tab (long press and select, or something doing window.open), you can't get to it in any way.
- Get an answer to wesj's question from above -- do we need access to the manifest in the original profile if we have the URL?  It wasn't clear to me how this was supposed to work on the desktop, either.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: WIP 3.3

This now parcels out App0..9, and it's easy enough to bump this limit at compile time.  (We should probably ship with 50 or something; I don't think there's a significant cost.)  It also tracks uninstalls, but doesn't do enough cleanup; it needs to delete the old webapp# profile on uninstall.

Also, we need to do something about old shortcuts.  We can't delete them, but we should at least store an extra uuid cookie along with each -- if it doesn't match on launch, then just give a notice saying that the application was uninstalled, and maybe ask if the user would like to navigate to the URL in the regular browser (for potential reinstallation).  Otherwise we could have an old shortcut that could be used to launch an "uninstalled" app in the profile of a different more recently-installed one.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: WIP 3.4

Wanted to throw this up, but I think we should take this and review. There's tons of edge case issues to look into, but this gets the basic premise (chromeless apps) of this bug up and running.

Little more cleanup and I'll mark for review.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I know I'm overloading you mfinkle, but I think its time to start reviews here as well! This gets us basic support. Things from vlad's list we still probably need to look at?

- Removing the webapp-specific profile on app uninstall.

- Having the correct names/icons in the recent apps list. I think this is impossible...

- Window list + other menu things + ICS support? This leaves in "Quit" for the menu for now, but we probably need something better....

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: un-bitrotted

Updated to current m-c

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: un-bitrotted

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: un-bitrotted again

adds missing web_app.xml from previous unbitrots and original patch

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 631513 [details] [diff] [review]
un-bitrotted again

>diff --git a/mobile/android/base/WebAppAllocator.java b/mobile/android/base/WebAppAllocator.java

>+            if (!mPrefs.contains("App" + i)) {

nit: can we use "app" (lowercase) in this file for the sharepref key?

>+++ b/mobile/android/base/resources/layout/web_app.xml

We might need to add more web content support to this file, but we can do that as needed. Form autocomplete is a good start. Looks like Doorhangers are handled outside the XML.

What about a tabs list? Also a good followup bug/feature.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Will make the "app" change (and coordinate with wesj for landing this).  Definitely a bunch of followup bugs to file.

Tabs list will be a little tricky; I'd like to avoid the horrible 3-dot menu item showing up, especially since it ends up looking gross on my One X (see http://www.androidcentral.com/htc-one-x-shows-us-why-developers-need-lose-menu-button), but I don't know if we can shove a menu button/tab switch thing on top of the web app's primary UI.  Something to discuss in that followup bug :)

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated, one more time

Sorry mfinkle -- I accidentally folded a patch into this one along the way, so will need to do review again.  The bits in the previous patch are the same; this one just adds more robust web app install/uninstall, and does the right thing with:

1) creating a shortcut on install
2) removing shortcut on uninstall
3) killing process on uninstall, if any
4) deleting profile on uninstall

it requires the profiles.ini patch in bug 715307.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated again

Updated again -- some bitrot fixes and a missed qrefresh from last time.  Should be final, barring review comments; any other work will happen on top of this.

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 631513 [details] [diff] [review]
un-bitrotted again

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

I'm good with this too (I wrote some of it!).

::: mobile/android/base/App.java.in
@@ +18,2 @@
>      public String getPackageName() {
> +      return "@ANDROID_PACKAGE_NAME@";

tabs

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 633621 [details] [diff] [review]
updated again

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

This looks good to me (same thing but with a much better handling of install/uninstall). And its getting complex. I'd be happy landing this on nightly and moving forward with follow ups in other bugs.

::: mobile/android/base/GeckoProfile.java
@@ +349,5 @@
> +
> +    // returns File referencing the profile's directory, or null if it failed
> +    private File createProfileIfMissing(String profileName) throws IOException {
> +        return null;
> +    }

What is this for?

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 633621 [details] [diff] [review]
updated again


>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("WebApps:Open")) {
>+                String url = message.getString("uri");
>+                String origin = message.getString("origin");
>+                Intent intent = GeckoAppShell.getWebAppIntent(url, origin);

Missing the "forInstall" value here in getWebAppIntent. Is it optional?

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>+    public static Intent getWebAppIntent(String aURI, String aUniqueURI, boolean forInstall) {
>+        int index;
>+
>+        if (forInstall)
>+            index = WebAppAllocator.getInstance(GeckoApp.mAppContext).findAndAllocateIndex(aUniqueURI);
>+        else
>+            index = WebAppAllocator.getInstance(GeckoApp.mAppContext).getIndexForApp(aUniqueURI);

If "WebAppAllocator.getInstance(GeckoApp.mAppContext)." wasn't such a mouthful, I'd suggest making that specific call in the callee and just passing the "index" into this function. I'm not a fan of boolean "mode" params though.


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      case "webapps-sync-uninstall":
>+        sendMessageToJava({
>+          gecko: {
>+            type: "WebApps:Uninstall",
>+            uniqueURI: data.origin
>+          }
>+        });

I guess we still need to find a way to move the "Shortcut:Remove" to this notification too. Please file a followup for that in case we aren't already tracking it

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Mark Finkle (:mfinkle) from comment #27)
> Comment on attachment 633621 [details] [diff] [review]
> updated again
> 
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+            } else if (event.equals("WebApps:Open")) {
> >+                String url = message.getString("uri");
> >+                String origin = message.getString("origin");
> >+                Intent intent = GeckoAppShell.getWebAppIntent(url, origin);
> 
> Missing the "forInstall" value here in getWebAppIntent. Is it optional?

Erm, no, it's not; I'm confused as to how it compiled and ran, but I promise it did.  I may have done some stupid edit thing, but it should be 'false' there.  I'll fix.

> >diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
> 
> >+    public static Intent getWebAppIntent(String aURI, String aUniqueURI, boolean forInstall) {
> >+        int index;
> >+
> >+        if (forInstall)
> >+            index = WebAppAllocator.getInstance(GeckoApp.mAppContext).findAndAllocateIndex(aUniqueURI);
> >+        else
> >+            index = WebAppAllocator.getInstance(GeckoApp.mAppContext).getIndexForApp(aUniqueURI);
> 
> If "WebAppAllocator.getInstance(GeckoApp.mAppContext)." wasn't such a
> mouthful, I'd suggest making that specific call in the callee and just
> passing the "index" into this function. I'm not a fan of boolean "mode"
> params though.

Technically, the forInstall param isn't needed -- originally I didn't have it.  The danger I realized was that if somehow we got into a weird situation where we attempted to open or create a shortcut for a web app that wasn't actually installed/tracked, we'd end up allocating an index for it when we shouldn't.  This way it ensures that we don't.

If you'd like this to be cleaner though, I can make WebAppAllocator.getInstance(GeckoApp.mAppContext) to be a member variable in GeckoAppShell, so it can be mWebAppAllocator.findAndAllocate/getIndex in the caller?

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+      case "webapps-sync-uninstall":
> >+        sendMessageToJava({
> >+          gecko: {
> >+            type: "WebApps:Uninstall",
> >+            uniqueURI: data.origin
> >+          }
> >+        });
> 
> I guess we still need to find a way to move the "Shortcut:Remove" to this
> notification too. Please file a followup for that in case we aren't already
> tracking it

I actually explicitly didn't handle shortcut remove here, though I could have; I kept them separate because they seem like separate things to me.  You could potentially have a user action that just removes shortcuts without uninstalling, right?

Comment 29

[Jason Smith [:jsmith]]

Just wondering - I was thinking of organizing a test day (Desktop/Android Web Apps with Marketplace Test Day) that includes testing this feature next Friday (6/22) - does that sound alright? Or should we wait a week (6/29) to give this feature more time to bake in mozilla central to knock out critical bugs quickly before community members try this?

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #28)

> > If "WebAppAllocator.getInstance(GeckoApp.mAppContext)." wasn't such a
> > mouthful, I'd suggest making that specific call in the callee and just
> > passing the "index" into this function. I'm not a fan of boolean "mode"
> > params though.
> 
> Technically, the forInstall param isn't needed -- originally I didn't have
> it.  The danger I realized was that if somehow we got into a weird situation
> where we attempted to open or create a shortcut for a web app that wasn't
> actually installed/tracked, we'd end up allocating an index for it when we
> shouldn't.  This way it ensures that we don't.
> 
> If you'd like this to be cleaner though, I can make
> WebAppAllocator.getInstance(GeckoApp.mAppContext) to be a member variable in
> GeckoAppShell, so it can be mWebAppAllocator.findAndAllocate/getIndex in the
> caller?
> 

> > I guess we still need to find a way to move the "Shortcut:Remove" to this
> > notification too. Please file a followup for that in case we aren't already
> > tracking it
> 
> I actually explicitly didn't handle shortcut remove here, though I could
> have; I kept them separate because they seem like separate things to me. 
> You could potentially have a user action that just removes shortcuts without
> uninstalling, right?

Let's not let either of these suggestions block you from landing. We can refactor as needed after landing.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Pushed to inbound -- https://hg.mozilla.org/integration/mozilla-inbound/rev/0b76534e9b26

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b76534e9b26

Comment 34

[Aaron Train [:aaronmt]]

Verified Fixed on Mozilla-Central (06.20)