Closed Bug 802683 Opened 8 years ago Closed 8 years ago

mozApp.download() packaged app issues

Categories

(Core Graveyard :: DOM: Apps, defect, P1)

defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: etienne, Assigned: fabrice)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 6 obsolete files)

I have a packaged app with |downloadAvailable| true, and calling |.download()| isn't doing much.

Leads:
- We send a |Webapps:Download| message while the ppmm listens for |Webapps::Download|
- |startDownload| is doing a |this.getAppByManifestURL(manifestURL)| while the parameter name is |aManifestURL|

But fixing those isn't enough...

PS: The gaia branch where the basic app update workflow is implemented is here https://github.com/etiennesegonzac/gaia/tree/app-updates
blocking-basecamp: --- → ?
Just uncompress at the http server root, I'm pointer fakeapp.com at 127.0.0.1.

The installation is done by opening the browser app, going to fakeapp.com and clicking Install.

The manifest in the zip is different from the one at the root, so the app always has a download available.
Fabrice says he'll take this and that it's a blocker.
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Priority: -- → P1
Attached patch wip (obsolete) — Splinter Review
Etienne, can you try this patch? Here's what I got with it:

XXX FIXME : Got a mozContentEvent: webapps-install-granted
-*-*- Webapps.jsm : {"name":"Fake app","installOrigin":"http://fakeapp.com","origin":"http://fakeapp.com","receipts":[],"installTime":1350522676422,"manifestURL":"http://fakeapp.com/manifest.webapp","appStatus":1,"removable":true,"localId":1001,"progress":0,"installState":"pending","downloadAvailable":true,"downloading":true,"readyToApplyDownload":false,"downloadSize":0,"lastUpdateCheck":1350522676422,"etag":"\"3006f0-150-4cc42a828da00\"","basePath":"/home/fabrice/dev/gaia/profile/webapps"}
-*-*- Webapps.jsm : About to download http://fakeapp.com/application.zip
-*-*- Webapps.jsm : onProgress: 5601/5601
[Child 10598] WARNING: NS_ENSURE_TRUE(IsChromeProcess()) failed: file /home/fabrice/dev/inbound/content/base/src/nsFrameMessageManager.cpp, line 687
-*-*- Webapps.jsm : About to fire Webapps:PackageEvent
JavaScript error: app://homescreen.gaiamobile.org/js/grid.js, line 644: Applications.getManifest(...) is undefined

Everything looked good in the webapps.json registry, and indeed when relaunching b2g I had the new app listed and loadable.
Attached patch Wip with fixed typo? (obsolete) — Splinter Review
I can't find anywhere where we listen to |Webapps::Download| and the switch case refers to |Webapps:Download|.

So sending a |Webapps:Download|.
With this patch I get a:

-*-*- Webapps.jsm : startDownload: No update manifest found at /home/segonzac/Desktop/gaia/profile/webapps/{4ac2a55a-129b-4622-a79f-76f5c0b4dcdc}/update.webapp http://fakeapp.com/manifest.webapp

But it's unclear to me where the update.webapp should come from.

Some more details to clarify:
- No issue at app install
- even if the manifest in the zip is the same that outside, |downloadAvailable| is always true after the first |checkForUpdate()|
- calling |download()| at this point will trigger the "no update manifest error"
Attached patch wip v2 (obsolete) — Splinter Review
Another wip for you to try, with also a bunch of new debug statements.

Also, you need to change your test site to use mozApp.installPackage() instead of install() !

Here's a small gaia fix to let this work now that the first manifest is app.updateManifest and not app.manifest for packaged apps:

diff --git a/apps/system/js/permission_manager.js b/apps/system/js/permission_manager.js
index 1ee6ed1..07f8617 100644
--- a/apps/system/js/permission_manager.js
+++ b/apps/system/js/permission_manager.js
@@ -77,8 +77,9 @@ var PermissionManager = (function() {
       return;
     }
 
-    var name = app.manifest.name;
-    var locales = app.manifest.locales;
+    var manifest = app.manifest ? app.manifest : app.updateManifest;
+    var name = manifest.name;
+    var locales = manifest.locales;
     var lang = navigator.language;
     if (locales && locales[lang] && locales[lang].name)
       name = locales[lang].name;
Attachment #672627 - Attachment is obsolete: true
Attachment #672772 - Attachment is obsolete: true
Attached patch WIP v3 (obsolete) — Splinter Review
Some events names fixes, a pinch of app retrieval change, a zest of typos corrections.

And...
IT'S UPDATING \o/
Attachment #672906 - Attachment is obsolete: true
Blocks: 803790
No longer blocks: 803790
Downloading and updating still work fine, but new issue (trying to group them here).

Every time checkForUpdate is called, downloadAvailable is set to true, even if the app just got updated and the manifest are identical.

Can this just be an apache config error causing etag issues?
Summary: mozApp.download() doesn't do anything (packaged app) → mozApp.download() packaged app issues
Attached patch wip 4 (obsolete) — Splinter Review
Etienne, we were not setting the If-None-Match header properly, so we were seeing a new package each time. Can you test again?
Attachment #673228 - Attachment is obsolete: true
Some progress...

* After the initial installation I won't get any downloadAvailable until I change the manifest on the server.

* _But_, after an update I get into the same state than before where checkForUpdate always put |downloadAvailable| to true
* The last version of your patch still calls |this.ApplyDownload| instead of |this.applyDownload|
Attached patch wip v5 (obsolete) — Splinter Review
Persisting the etag properly on update.
Attachment #674360 - Attachment is obsolete: true
Blocks: 799888
Attachment #674668 - Attachment is patch: true
Attached patch patchSplinter Review
Very similar to v5 - I removed a duplicate |let id =...| in startDownload()
Attachment #674668 - Attachment is obsolete: true
Attachment #674765 - Flags: review?(21)
Comment on attachment 674765 [details] [diff] [review]
patch

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

r+ with the debug method turned off by default.

::: dom/apps/src/Webapps.jsm
@@ +19,5 @@
>  Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
>  Cu.import("resource://gre/modules/OfflineCacheInstaller.jsm");
>  
>  function debug(aMsg) {
> +  dump("-*-*- Webapps.jsm : " + aMsg + "\n");

You forgot to comment that back. :)

@@ +848,5 @@
>        aMm.sendAsyncMessage("Webapps:CheckForUpdate:Return:KO", aData);
>        return;
>      }
>  
> +    let installOrigin = app.installOrigin;

Is is used?
Attachment #674765 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/3e4d63da1bce
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
Whiteboard: [qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.