Closed Bug 769821 Opened 9 years ago Closed 9 years ago

Download app cache specified in appcache_path of manifest at app install time

Categories

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

ARM
Android
defect

Tracking

(blocking-kilimanjaro:?, firefox16 verified, firefox17 verified)

VERIFIED FIXED
Firefox 17
blocking-kilimanjaro ?
Tracking Status
firefox16 --- verified
firefox17 --- verified

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(1 file)

If an app has an app cache, we should download and install those files at install time if possible.
Summary: Download app cache at app install time → Download app cache specified in appcache_path of manifest at app install time
k9o nomination for the same reason the other bugs got nominated and a +.
blocking-kilimanjaro: --- → ?
See Also: → 760365
(In reply to Jason Smith [:jsmith] from comment #1)
> k9o nomination for the same reason the other bugs got nominated and a +.

See bug 760365 for more information on the desktop equivalent version of the bug.
QA Contact: aaron.train
Priority: -- → P1
Keywords: uiwanted
From felipe, instructions on using this. We need to pass the install path to confirmInstall:

http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#110

I think we can just move our Webapps:Install message to before confirmInstall and have it return to us the correctly profile path.
Attached patch Patch v1Splinter Review
This does our work finishing the install in Java before we call confirmInstall. That lets us pass the profile path from Java back to Gecko, and then on to the WebApps backend. I'm still using the webapps-sync-install notification to show a notification to the user that the webapp was installed, but the shortcut icon will be created early on. (It also fixes our character encoding problems from bug 766604, but only because it cheats).

Hoping fabrice can make sure I did the cache bit right. mbrubeck for the fennec bits.

We could pass in a listener to be notified when the cache is downloaded. Then we could break the java install flow in two. First set up the profile in Java. Send it back to Gecko. Once Gecko has downloaded the cache, set up the desktop shortcuts and show a notification. I want to avoid the extra code complexity for now. Worst case downloading the cache takes awhile and the user starts the app without waiting (I think?)
Assignee: nobody → wjohnston
Attachment #645538 - Flags: review?(mbrubeck)
Attachment #645538 - Flags: review?(fabrice)
Comment on attachment 645538 [details] [diff] [review]
Patch v1

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

r=me for the browser.js part.
Attachment #645538 - Flags: review?(fabrice) → review+
OS: All → Android
Hardware: All → ARM
Status: NEW → ASSIGNED
Removing uiwanted - I don't think there's any special UI needed here, or am I mistaken?
Keywords: uiwanted
Comment on attachment 645538 [details] [diff] [review]
Patch v1

Matt's out (and I think forgot about this). Mark, can you review?
Attachment #645538 - Flags: review?(mbrubeck) → review?(mark.finkle)
Comment on attachment 645538 [details] [diff] [review]
Patch v1


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

>+    public String getResponse() {
>+        Log.i(LOGTAG, "Return " + mCurrentResponse);
>+        String res = mCurrentResponse;
>+        mCurrentResponse = "";
>+        return res;
>+    }

First time I have seen this. I think getResponse is too generic of a method name. Can we open a new bug to change this to getEventResponse ?

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>@@ -5953,32 +5953,18 @@ var WebappsUI = {
>           this.openURL(manifest.fullLaunchPath(), data.origin);
>         }).bind(this));
>         break;
>       case "webapps-sync-install":
>         // Wait until we know the app install worked, then make a homescreen shortcut

This comment is wrong now, right?

>   doInstall: function doInstall(aData) {

>+    if (Services.prompt.confirm(null, Strings.browser.GetStringFromName("webapps.installTitle"), name)) {
>+      // Add a homescreen shortcut -- we can't use createShortcut, since we need to pass
>+      // a unique ID for Android webapp allocation
>+      this.makeBase64Icon(this.getBiggestIcon(manifest.icons, Services.io.newURI(aData.app.origin, null, null)),
>+        function(icon) {
>+          var profilePath = sendMessageToJava({
>+            gecko: {
>+              type: "WebApps:Install",
>+              name: manifest.name,
>+              launchPath: manifest.fullLaunchPath(),
>+              iconURL: icon,
>+              uniqueURI: aData.app.origin
>+            }
>+          });
>+  
>+          // if java returned a profile path to us, try to use it to pre-populate the app cache
>+          var file = null;
>+          if (profilePath) {
>+            var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>+            file.initWithPath(profilePath);
>+          }
>+          DOMApplicationRegistry.confirmInstall(aData, false, file);

There is no downside to making the icon before we confirm the install?
Attachment #645538 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4991ba3af6

> There is no downside to making the icon before we confirm the install?

There is some risk. confirmInstall copies the (already downloaded and installed) manifest into the profile. If that fails I think about:apps might have some issues, but in general the app should still work. TBH, since the apps don't have access to the manifest and I don't anticipate people using about:apps as a launcher, there isn't a lot for the platform to do for our installs. Most of what it is doing is support for stores on the web and payments stuff.
Comment on attachment 645538 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New stuff
User impact if declined: Loading apps can take longer
Testing completed (on m-c, etc.): landed on mc 8/6
Risk to taking this patch (and alternatives if risky): Low risk. Webapp on mobile only. 
String or UUID changes made by this patch: None.
Attachment #645538 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0b4991ba3af6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Comment on attachment 645538 [details] [diff] [review]
Patch v1

low risk, mobile only, approving.
Attachment #645538 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Using Fabrice's test app at people.mozilla.com/~fdesre/openwebapps/test.html to install a web app utilizing app_cache, this definitely doesn't work for me. I did the following STR:

1. Installed the app with appcache (foobar)
2. Killed my data connection
3. Started up foobar

Result - I got a server not found error.

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Are you sure you let enough time to download the app cache?
This works fine for me on mozilla-16 and mozilla-17.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
(In reply to Fabrice Desré [:fabrice] from comment #15)
> Are you sure you let enough time to download the app cache?

Probably not. It worked for me the second time I tested it.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.