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

VERIFIED FIXED in mozilla16

Status

()

Core
DOM
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gal, Assigned: fabrice)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

6 years ago
Blocks: 673923
Depends on: 702360
This might be bug 674728, or this bug might need to use that mechanism.
(Reporter)

Updated

6 years ago
Depends on: 674728
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → fabrice
Blocks: 715782
Blocks: 728081
Depends on: 739868
I think we'll need bug 756717 to do this properly.
blocking-kilimanjaro: --- → ?
Depends on: 756717
Should this block bug 756620?
see also bug 753990
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.
Depends on: 744713, 744719
Summary: ensure that web app install caches them into app cache → ensure that web app install caches them into app cache on B2G
Duplicate of this bug: 751750

Updated

5 years ago
Blocks: 715784

Updated

5 years ago
No longer blocks: 715782
No longer depends on: 702360
Whiteboard: [b2g:blocking+]

Updated

5 years ago
blocking-basecamp: --- → +
blocking-kilimanjaro: ? → +
Whiteboard: [b2g:blocking+]
(Assignee)

Comment 8

5 years ago
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.
Attachment #576605 - Attachment is obsolete: true
Attachment #631144 - Flags: review?(jonas)
Attachment #631144 - Flags: review?(21)
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.
Attachment #631144 - Flags: review?(jonas) → review+
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?
(Assignee)

Comment 11

5 years ago
(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.
(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?
(Assignee)

Comment 13

5 years ago
(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 :(
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 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.
Attachment #631144 - Flags: review?(21) → review+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eded0507a6e
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
Depends on: 766382
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).
Attachment #631144 - Attachment is obsolete: true
(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.
Created attachment 634662 [details] [diff] [review]
nsIVariant -> jsval

Fabrice suggested the same thing on IRC; here's an updated patch that uses jsval.
Attachment #634655 - Attachment is obsolete: true
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.
(Assignee)

Comment 22

5 years ago
(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.
(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.
(Assignee)

Comment 24

5 years ago
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.
Attachment #634662 - Attachment is obsolete: true
Attachment #635376 - Flags: review?(felipc)
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.
Attachment #635376 - Flags: review?(felipc) → review+
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e6ff595e91
https://hg.mozilla.org/mozilla-central/rev/c5e6ff595e91
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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?
Whiteboard: [qa+]

Updated

5 years ago
Whiteboard: [qa+] → [qa+:jsmith]

Updated

5 years ago
QA Contact: jsmith
Whiteboard: [qa+:jsmith] → [qa+]
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.
Whiteboard: [qa+] → [qa verification failed]

Updated

5 years ago
Depends on: 777429

Updated

5 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [qa verification failed] → [qa!]
Depends on: 837622
No longer depends on: 837622
You need to log in before you can comment on or make changes to this bug.