Last Comment Bug 702369 - ensure that web app install caches them into app cache on B2G
: ensure that web app install caches them into app cache on B2G
Status: VERIFIED FIXED
[qa!]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: [:fabrice] Fabrice Desré
: Jason Smith [:jsmith]
Mentors:
: 751750 (view as bug list)
Depends on: 674728 739868 744713 744719 756717 766382 777429
Blocks: webapi b2g-product-phone b2g-app-updates
  Show dependency treegraph
 
Reported: 2011-11-14 11:32 PST by Andreas Gal :gal
Modified: 2013-02-04 03:37 PST (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
wip patch (1.89 KB, patch)
2011-11-23 13:26 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
patch (13.84 KB, patch)
2012-06-07 14:35 PDT, [:fabrice] Fabrice Desré
jonas: review+
21: review+
Details | Diff | Review
unrotted + fixes for test failures (14.45 KB, patch)
2012-06-19 16:38 PDT, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Review
nsIVariant -> jsval (14.42 KB, patch)
2012-06-19 17:00 PDT, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Review
Patch with support for desktop profiles (15.30 KB, patch)
2012-06-21 10:59 PDT, [:fabrice] Fabrice Desré
felipc: review+
Details | Diff | Review

Description Andreas Gal :gal 2011-11-14 11:32:10 PST

    
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 12:28:28 PST
This might be bug 674728, or this bug might need to use that mechanism.
Comment 2 [:fabrice] Fabrice Desré 2011-11-23 13:26:06 PST
Created attachment 576605 [details] [diff] [review]
wip patch

If we have an appcache_path property in the manifest, use it to updated the offline cache when installing an application.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-18 20:42:29 PDT
I think we'll need bug 756717 to do this properly.
Comment 4 Andrew Overholt [:overholt] 2012-05-23 13:08:18 PDT
Should this block bug 756620?
Comment 5 Bill Walker [:bwalker] [@wfwalker] 2012-05-23 13:12:29 PDT
see also bug 753990
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-24 15:56:01 PDT
This is basically the b2g equivalent of bug 756620 as I understand it, so they don't block each other. Likewise, bug 753990 is related to desktop rather than b2g.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-24 23:08:23 PDT
*** Bug 751750 has been marked as a duplicate of this bug. ***
Comment 8 [:fabrice] Fabrice Desré 2012-06-07 14:35:23 PDT
Created attachment 631144 [details] [diff] [review]
patch

Jonas, can you look at the IDL changes?

Vivien,
I also added in this patch a bunch of fixes that we'll need soon (the __exposedProps__). I'm testing with the "Install app with appcache" button at http://people.mozilla.com/~fdesre/openwebapps/test.html

One situation that is not managed yet is to resume a download at startup if we previously shutdown while downloading. I'll file a follow up for that.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-08 15:35:55 PDT
Comment on attachment 631144 [details] [diff] [review]
patch

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

Hmm.. are you implementing nsIDOMEventTarget in JS? That shouldn't be possible and definitely runs the risk of crashes. Ask Smaug for more details.

However that's not new in this patch as it seems, so r=me with the caveat that we need to fix that part.
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-11 03:35:28 PDT
Comment on attachment 631144 [details] [diff] [review]
patch

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

I would like to understand the possible 'status' of the application before r+'ing this patch.

::: dom/apps/src/Webapps.js
@@ +63,5 @@
>      let app = msg.app;
>      switch (aMessage.name) {
>        case "Webapps:Install:Return:OK":
> +        Services.DOMRequest.fireSuccess(req, createApplicationObject(this._window, app.origin, app.manifest, app.manifestURL, app.receipts,
> +                                                                     app.installOrigin, app.installTime));

Not related to this bug directly but it will probably make sense to pass 'app' directly as an argument to createApplicationObject.

@@ +218,5 @@
> +    this.manifestURL = aManifestURL;
> +    this.receipts = aReceipts;
> +    this.installOrigin = aInstallOrigin;
> +    this.installTime = aInstallTime;
> +    this.status = "unknown";

"unknow" is not in the .idl file. Should it be 'installed' instead of 'unknown'?

@@ +219,5 @@
> +    this.receipts = aReceipts;
> +    this.installOrigin = aInstallOrigin;
> +    this.installTime = aInstallTime;
> +    this.status = "unknown";
> +    this.progress = NaN;

Why not 0.0?

@@ +377,5 @@
>          }
>          break;
>        case "Webapps:Uninstall:Return:OK":
>          if (this._onuninstall) {
> +          let event = new WebappsApplicationEvent(createApplicationObject(this._window, msg.origin, null, null, null, null, 0));

Not directly related to this bug again, but is it the main reason why createApplicationObject does not expect an 'app' as parameter? If yes I wonder what are the reason to pass 'null' instead of the real values? If at some point there is multiple apps per origin it will be hard to differentiate them.

::: dom/apps/src/Webapps.jsm
@@ +465,5 @@
> +
> +    switch (aState) {
> +      case Ci.nsIOfflineCacheUpdateObserver.STATE_ERROR:
> +        aUpdate.removeObserver(this);
> +        setStatus("installed");

Really? Do we need to introduce an 'error' status instead?
Comment 11 [:fabrice] Fabrice Desré 2012-06-11 07:50:52 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #10)

> I would like to understand the possible 'status' of the application before
> r+'ing this patch.

This is documented in the IDL:
/*
 * The application status :
 * "installed" : The app is in the registry, but we have no offline cache.
 * "downlading" : We are downloading the offline cache.
 * "cached" : We are done with the offline cache download.
 */

> ::: dom/apps/src/Webapps.js
> @@ +63,5 @@
> >      let app = msg.app;
> >      switch (aMessage.name) {
> >        case "Webapps:Install:Return:OK":
> > +        Services.DOMRequest.fireSuccess(req, createApplicationObject(this._window, app.origin, app.manifest, app.manifestURL, app.receipts,
> > +                                                                     app.installOrigin, app.installTime));
> 
> Not related to this bug directly but it will probably make sense to pass
> 'app' directly as an argument to createApplicationObject.

yes, I'll do that in a follow up.

> @@ +218,5 @@
> > +    this.manifestURL = aManifestURL;
> > +    this.receipts = aReceipts;
> > +    this.installOrigin = aInstallOrigin;
> > +    this.installTime = aInstallTime;
> > +    this.status = "unknown";
> 
> "unknow" is not in the .idl file. Should it be 'installed' instead of
> 'unknown'?

good catch!

> @@ +219,5 @@
> > +    this.receipts = aReceipts;
> > +    this.installOrigin = aInstallOrigin;
> > +    this.installTime = aInstallTime;
> > +    this.status = "unknown";
> > +    this.progress = NaN;
> 
> Why not 0.0?

Because 0.0 is a valid progress indication, while what we want to express here is that even if we are downloading, we have no idea what the download progress is (we need bug 744713 for that).

> @@ +377,5 @@
> >          }
> >          break;
> >        case "Webapps:Uninstall:Return:OK":
> >          if (this._onuninstall) {
> > +          let event = new WebappsApplicationEvent(createApplicationObject(this._window, msg.origin, null, null, null, null, 0));
> 
> Not directly related to this bug again, but is it the main reason why
> createApplicationObject does not expect an 'app' as parameter? If yes I
> wonder what are the reason to pass 'null' instead of the real values? If at
> some point there is multiple apps per origin it will be hard to
> differentiate them.

When we will move to using the manifest URL to identify apps we will need to make a bunch of changes (notably changing the signature of getSelf() to take the manifest url as a parameter). In the case of uninstall, we don't have access to anything else than the app "identifier", so we null everything else.

> ::: dom/apps/src/Webapps.jsm
> @@ +465,5 @@
> > +
> > +    switch (aState) {
> > +      case Ci.nsIOfflineCacheUpdateObserver.STATE_ERROR:
> > +        aUpdate.removeObserver(this);
> > +        setStatus("installed");
> 
> Really? Do we need to introduce an 'error' status instead?

This is an appcache error. The application itself is still installed in the registry and usable. For sure we can add a dedicated status in this case, but I'm not convinced yet that it's needed.
Comment 12 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-11 08:18:28 PDT
(In reply to Fabrice Desré [:fabrice] from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) from comment #10)
> 
> > ::: dom/apps/src/Webapps.jsm
> > @@ +465,5 @@
> > > +
> > > +    switch (aState) {
> > > +      case Ci.nsIOfflineCacheUpdateObserver.STATE_ERROR:
> > > +        aUpdate.removeObserver(this);
> > > +        setStatus("installed");
> > 
> > Really? Do we need to introduce an 'error' status instead?
> 
> This is an appcache error. The application itself is still installed in the
> registry and usable. For sure we can add a dedicated status in this case,
> but I'm not convinced yet that it's needed.

If I develop an application I would find it handy to know that there is an error at install time. 
Also as a user I would like to know if something is wrong when I install an app so I could retry (if that was because I have lost my connection temporarily) or I will likely just remove the app (because I don't trust broken apps!)

But that can be done in a followup.

Last thing is I've not seen code to remove the cache from the application cache when the application is uninstalled?
Comment 13 [:fabrice] Fabrice Desré 2012-06-11 08:23:35 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #12)

> If I develop an application I would find it handy to know that there is an
> error at install time. 
> Also as a user I would like to know if something is wrong when I install an
> app so I could retry (if that was because I have lost my connection
> temporarily) or I will likely just remove the app (because I don't trust
> broken apps!)
> 
> But that can be done in a followup.

It's easy enough to do right here. We just need to define the status name. What about "cache-error" ?

> Last thing is I've not seen code to remove the cache from the application
> cache when the application is uninstalled?

No, because I didn't find anything in the offline cache API to do so :(
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-11 08:31:05 PDT
On 11/06/2012 17:23, bugzilla-daemon@mozilla.org wrote:
> https://bugzilla.mozilla.org/show_bug.cgi?id=702369
>
> --- Comment #13 from Fabrice Desré [:fabrice] <fabrice@mozilla.com> 2012-06-11 08:23:35 PDT ---
> (In reply to Vivien Nicolas (:vingtetun) from comment #12)
>
>> If I develop an application I would find it handy to know that there is an
>> error at install time. 
>> Also as a user I would like to know if something is wrong when I install an
>> app so I could retry (if that was because I have lost my connection
>> temporarily) or I will likely just remove the app (because I don't trust
>> broken apps!)
>>
>> But that can be done in a followup.
>
> It's easy enough to do right here. We just need to define the status name. What
> about "cache-error" ?

uncached? But I'm fine with whatever you suggest.

>
>
>> Last thing is I've not seen code to remove the cache from the application
>> cache when the application is uninstalled?
>
> No, because I didn't find anything in the offline cache API to do so :(
>

Could that help?
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIApplicationCache.idl#151
Comment 15 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-11 09:23:32 PDT
Comment on attachment 631144 [details] [diff] [review]
patch

Per IRC the uninstall portion could be fixed in a followup since this is already how behave installed applications from Gaia.
Comment 16 [:fabrice] Fabrice Desré 2012-06-11 11:43:09 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eded0507a6e
Comment 18 Myk Melez [:myk] [@mykmelez] 2012-06-19 16:38:32 PDT
Created attachment 634655 [details] [diff] [review]
unrotted + fixes for test failures

While investigating blocker bug 766382, I unrotted the fix for this one, and I fixed two issues with the XPIDL specification of mozIDOMApplication that were exposed by mochitest failures:

1. `receipts` is an nsIArray but would need to be an nsIVariant in order to be exposed to content as a JavaScript Array.

2. `installTime` is an `unsigned long` but would need to be an `unsigned long long` to be large enough to hold all values returned from Date.now()/Date.getTime().

This patch includes those two changes along with a trivial typo fix ("downlading" -> "downloading" in a comment).

Not sure if these changes are trivial enough to carry forward review or who should review it if not (as Jonas is away at the moment).
Comment 19 Philipp von Weitershausen [:philikon] 2012-06-19 16:47:30 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #18)
> 1. `receipts` is an nsIArray but would need to be an nsIVariant in order to
> be exposed to content as a JavaScript Array.

I think we just use `jsval` in these cases.
Comment 20 Myk Melez [:myk] [@mykmelez] 2012-06-19 17:00:35 PDT
Created attachment 634662 [details] [diff] [review]
nsIVariant -> jsval

Fabrice suggested the same thing on IRC; here's an updated patch that uses jsval.
Comment 21 :Felipe Gomes (needinfo me!) 2012-06-19 17:37:28 PDT
Comment on attachment 634662 [details] [diff] [review]
nsIVariant -> jsval

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

::: dom/apps/src/Webapps.jsm
@@ +210,5 @@
> +                            .getService(Ci.nsIOfflineCacheUpdateService);
> +      let docURI = Services.io.newURI(manifest.fullLaunchPath(), null, null);
> +      let cacheUpdate = updateService.scheduleUpdate(appcacheURI, docURI, null);
> +      cacheUpdate.addObserver(new AppcacheObserver(appObject), false);
> +    }

This patch is not B2G specific and will do wrong things on Desktop, for example appcache should be populated on a different profile on Desktop (i.e. updateService.scheduleCustomProfileUpdate needs to be called instead of .scheduleUpdate).  Another thing is that the cross-process messages on Desktop aren't needed so we shouldn't set the listeners for them.

On desktop, this functionality will be part of the installer, and I thought it would be the same in b2g, instead of living in Webapps.jsm which seems weird to me. We can keep it here fine but we need a way to let desktop do it differently.
Comment 22 [:fabrice] Fabrice Desré 2012-06-19 17:43:20 PDT
(In reply to Felipe Gomes (:felipe) from comment #21)

> This patch is not B2G specific and will do wrong things on Desktop, for
> example appcache should be populated on a different profile on Desktop (i.e.
> updateService.scheduleCustomProfileUpdate needs to be called instead of
> .scheduleUpdate).  Another thing is that the cross-process messages on
> Desktop aren't needed so we shouldn't set the listeners for them.

Why are the listeners and progress messages not needed on desktop?
 
> On desktop, this functionality will be part of the installer, and I thought
> it would be the same in b2g, instead of living in Webapps.jsm which seems
> weird to me. We can keep it here fine but we need a way to let desktop do it
> differently.

Well, on b2g, Webapps.jsm is the installer...
Not sure also what we need on Android, so ccing vlad and wesj.
Comment 23 :Felipe Gomes (needinfo me!) 2012-06-19 17:52:40 PDT
(In reply to Fabrice Desré [:fabrice] from comment #22)
> Why are the listeners and progress messages not needed on desktop?

The progress notifications are still necessary, we just don't need to replicate them through the cpmm as I can use the observer directly.

I think the easiest is if I can set up my own observer in WebappsInstaller.jsm without this observer in Webapps.jsm being activated.  Ideally I'd like to reuse the one in this patch (AppcacheObserver) but juggling the differences of the platforms together will probably be messier than having two separate ones.

> 
> Well, on b2g, Webapps.jsm is the installer...

Alright, I see. We should just be mindful that desktop also calls into these functions to "install" the app in the webapp registry, but the native installation happens elsewhere and we should not do b2g installation code unconditionally here.
Comment 24 [:fabrice] Fabrice Desré 2012-06-21 10:59:08 PDT
Created attachment 635376 [details] [diff] [review]
Patch with support for desktop profiles

Per discussion with felipe, this patch adds support to save the offline cache in the webRT directory, as well as letting the desktop installer hook up it's own update observer.
Comment 25 :Felipe Gomes (needinfo me!) 2012-06-21 11:17:16 PDT
Comment on attachment 635376 [details] [diff] [review]
Patch with support for desktop profiles

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

Thanks for the extra changes Fabrice. I reviewed the additions to confirmInstall that we talked about. I believe everything else was already reviewed.
Comment 26 [:fabrice] Fabrice Desré 2012-06-21 11:31:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e6ff595e91
Comment 27 Ed Morley [:emorley] 2012-06-22 03:46:09 PDT
https://hg.mozilla.org/mozilla-central/rev/c5e6ff595e91
Comment 28 :Ms2ger 2012-06-22 04:00:55 PDT
Comment on attachment 635376 [details] [diff] [review]
Patch with support for desktop profiles

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

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +26,5 @@
> +
> +  /*
> +   * The application status :
> +   * "installed"   : The app is in the registry, but we have no offline cache.
> +   * "downlading"  : We are downloading the offline cache.

-lading?
Comment 29 Jason Smith [:jsmith] 2012-07-25 11:02:05 PDT
Busted. If I try to install fabrice's app here - http://people.mozilla.com/~fdesre/openwebapps/test.html, no prompt appears on the latest B2G build. No app is installed either. I'll file a followup bug for this.

Note You need to log in before you can comment on or make changes to this bug.