702369 - ensure that web app install caches them into app cache on B2G

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This might be bug 674728, or this bug might need to use that mechanism.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: 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) No longer reading bugmail consistently]

I think we'll need bug 756717 to do this properly.

Comment 4

[Andrew Overholt [:overholt]]

Should this block bug 756620?

Comment 5

[Bill Walker [:bwalker] [@wfwalker]]

see also bug 753990

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 8

[[:fabrice] Fabrice Desré]

Attachment: 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) No longer reading bugmail consistently]

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)]

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é]

(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)]

(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é]

(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)]

On 11/06/2012 17:23, [e-mail] wrote:
> https://bugzilla.mozilla.org/show_bug.cgi?id=702369
>
> --- Comment #13 from Fabrice Desré [:fabrice] <[e-mail]> 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)]

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é]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eded0507a6e

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ca24cf29b02c for webapps tests timing out like in https://tbpl.mozilla.org/php/getParsedLog.php?id=12559457&tree=Mozilla-Inbound

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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]]

(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]]

Attachment: nsIVariant -> jsval

Fabrice suggested the same thing on IRC; here's an updated patch that uses jsval.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

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é]

(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 for replies!)]

(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é]

Attachment: 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 for replies!)]

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é]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e6ff595e91

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5e6ff595e91

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

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.