launch of basic app in a chromeless window

VERIFIED FIXED in Firefox 16

Status

()

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: jarguello, Assigned: wesj)

Tracking

unspecified
Firefox 16
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, firefox16 verified)

Details

(Whiteboard: [Phase2][k9o:p1:fx15?])

Attachments

(2 attachments, 12 obsolete attachments)

28.19 KB, patch
mfinkle
: review+
wesj
: review+
Details | Diff | Splinter Review
41.40 KB, patch
mfinkle
: review+
wesj
: review+
Details | Diff | Splinter Review
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
Depends on: 741624
Depends on: 741626
Component: General → Web Apps
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
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [WebRT:Phase2] → [Phase2]
QA Contact: general → web-apps
This bug does not cover the offline case. That is in bug 743917.
Target Milestone: --- → Firefox 15
blocking-kilimanjaro: --- → +
Whiteboard: [Phase2] → [Phase2][k9o:p1:fx15?]
Blocks: k9o-webrt
Assignee: nobody → wjohnston
Posted patch WIP (obsolete) — Splinter Review
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.
(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.
Posted patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #627045 - Attachment is obsolete: true
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).
bug 715307 has a patch to read and use the profile set in profiles.ini
(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.
Posted patch WIP 2.5, updated (obsolete) — Splinter Review
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.
Posted patch WIP3 (obsolete) — Splinter Review
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.
Attachment #627416 - Attachment is obsolete: true
Attachment #627781 - Attachment is obsolete: true
Posted patch WIP V3.1 (obsolete) — Splinter Review
This is basically the same patch with all the BrowserToolbar stuff yanked out and put into a separate patch (bug 753102).
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....
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.
Posted patch WIP 3.2 (obsolete) — Splinter Review
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.
Attachment #628010 - Attachment is obsolete: true
Attachment #628136 - Attachment is obsolete: true
Posted patch WIP 3.3 (obsolete) — Splinter Review
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.
Attachment #628698 - Attachment is obsolete: true
Posted patch WIP 3.4 (obsolete) — Splinter Review
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.
Attachment #628828 - Attachment is obsolete: true
Posted patch Patch v1 (obsolete) — Splinter Review
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....
Attachment #630281 - Attachment is obsolete: true
Attachment #630674 - Flags: review?(mark.finkle)
adds missing web_app.xml from previous unbitrots and original patch
Attachment #630674 - Attachment is obsolete: true
Attachment #631445 - Attachment is obsolete: true
Attachment #631453 - Attachment is obsolete: true
Attachment #630674 - Flags: review?(mark.finkle)
Attachment #631513 - Flags: review?(wjohnston)
Attachment #631513 - Flags: review?(mark.finkle)
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.
Attachment #631513 - Flags: review?(mark.finkle) → review+
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 :)
Posted patch updated, one more time (obsolete) — Splinter Review
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.
Attachment #633221 - Flags: review?(wjohnston)
Attachment #633221 - Flags: review?(mark.finkle)
Posted patch updated againSplinter Review
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.
Attachment #633221 - Attachment is obsolete: true
Attachment #633221 - Flags: review?(wjohnston)
Attachment #633221 - Flags: review?(mark.finkle)
Attachment #633621 - Flags: review?(wjohnston)
Attachment #633621 - Flags: review?(mark.finkle)
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
Attachment #631513 - Flags: review?(wjohnston) → review+
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?
Attachment #633621 - Flags: review?(wjohnston) → review+
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
Attachment #633621 - Flags: review?(mark.finkle) → review+
(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?
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?
(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.
Flags: in-moztrap?(aaron.train)
https://hg.mozilla.org/mozilla-central/rev/0b76534e9b26
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 766094
Depends on: 766098
Whiteboard: [Phase2][k9o:p1:fx15?] → [Phase2][k9o:p1:fx15?][qa+:AaronMT]
Duplicate of this bug: 741626
No longer depends on: 741624
Verified Fixed on Mozilla-Central (06.20)
Status: RESOLVED → VERIFIED
Whiteboard: [Phase2][k9o:p1:fx15?][qa+:AaronMT] → [Phase2][k9o:p1:fx15?]
Flags: in-moztrap?(aaron.train)
You need to log in before you can comment on or make changes to this bug.