Closed
Bug 769821
Opened 13 years ago
Closed 12 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)
Tracking
(blocking-kilimanjaro:?, firefox16 verified, firefox17 verified)
People
(Reporter: wesj, Assigned: wesj)
References
Details
Attachments
(1 file)
8.29 KB,
patch
|
fabrice
:
review+
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
If an app has an app cache, we should download and install those files at install time if possible.
Updated•13 years ago
|
Summary: Download app cache at app install time → Download app cache specified in appcache_path of manifest at app install time
Comment 1•13 years ago
|
||
k9o nomination for the same reason the other bugs got nominated and a +.
blocking-kilimanjaro: --- → ?
Comment 2•13 years ago
|
||
(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.
Updated•13 years ago
|
QA Contact: aaron.train
Updated•13 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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+
Updated•13 years ago
|
OS: All → Android
Hardware: All → ARM
Updated•13 years ago
|
Comment 6•13 years ago
|
||
Removing uiwanted - I don't think there's any special UI needed here, or am I mistaken?
Keywords: uiwanted
Assignee | ||
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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+
Assignee | ||
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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?
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•13 years ago
|
status-firefox17:
affected → ---
Comment 12•13 years ago
|
||
Comment on attachment 645538 [details] [diff] [review]
Patch v1
low risk, mobile only, approving.
Attachment #645538 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 13•13 years ago
|
||
Comment 14•12 years ago
|
||
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 → ---
Updated•12 years ago
|
Comment 15•12 years ago
|
||
Are you sure you let enough time to download the app cache?
Comment 16•12 years ago
|
||
This works fine for me on mozilla-16 and mozilla-17.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
status-firefox17:
--- → verified
Resolution: --- → FIXED
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Comment 17•12 years ago
|
||
(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.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•