Preload appcache into webapp's profile during installation

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P1
normal
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

({dev-doc-complete})

Trunk
Firefox 16
dev-doc-complete
Dependency tree / graph

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
This is the bug that uses the APIs that Honza and Olli created in bug 753990 and bug 756131 to create a webapp profile and populate its appcache during webapp installation.

I've got the main piece ready for review, but still need to hook up it to the UI that triggers the app installation.


One thing that I decided in the current patch is to create the app profile in advance only if we're going to populate it with appcache (that is, avoid profile creation during installation for apps without appcache support). But I can be convinced otherwise if someone thinks that the best is to always create it.
(Assignee)

Comment 1

5 years ago
Created attachment 629060 [details] [diff] [review]
Patch

Honza, can you review this patch which is mostly my usage of your API and the manifest discovery using XHR?

I have 2 questions:

- By listening to the STATE_FINISHED update will I also catch error situations that stops the process (say, one file could not be found?). Basically I want to guarantee that I am observing to something that will tell me when the process finishes, either if it succeeded or gave up/timed out.

- On the other bug I asked you about redirects. Here I'm using the req.responseXML.documentURI as the final URI instead of the original url that I passed as a parameter. Does this look like correct? Hopefully this will cover it, but I don't even know if XHR follows redirects.


And one observation:

- This page https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#Using_XMLHttpRequest_from_JavaScript_modules_.2F_XPCOM.C2.A0components
says that to use XHR from JS modules I should use the XHR object from the hidden window. The problem is that this doesn't work :( It simply fails for me on Windows, I think because the hidden window on Win is different than on Mac  (resource:// vs. chrome://)
So I'm using Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1");
The fail case that that page mentions is so rare and non catastrophic here for me to care..
Attachment #629060 - Flags: review?(honzab.moz)
Felipe,

I'm working on the b2g version in bug 702369. I don't think we want to do the discovery dance, but rather add a property in the manifest itself. cc-ing Jonas to get his opinion.
I definitely don't think we should be shy about adding things to the manifest at this point. It's still very early in the process of figuring out what manifests will look like.

So I definitely think that we should put a link to the appcache manifest there. Having to download the main page to get the manifest doesn't seem helpful to neither authors nor us.
(Assignee)

Comment 4

5 years ago
Thanks for the info. I kinda like the discovery because I find it a bit odd for us to spec a different method to make appcache work for webapps instead of using the existing behavor,and appcache apps wouldn't be pre-cached out of the box unless the developer specify that in the manifest.

Having said that, maybe that's something necessary for paid apps, or at least something that app developers might want? I don't know. Maybe it'd make sense to have both? explicit declaration and auto-discovery

In any case, if b2g won't have discovery I think we shouldn't have it on desktop either to not lead developers to test it on desktop only and see it worked and then not add the entry to the manifest.
It's not at all certain that developers will want to use the appcache when loaded as a website rather than as a webapp. Especially since we for appcache using websites put up a prompt which developers hate.
(Assignee)

Comment 6

5 years ago
Ok, so should I remove discovery from this patch?

Fabrice: in the b2g bug has it been decided what is the field name for the cache manifest in the app manifest?

And what about the top-level URL which is often omitted from the appcache manifest but should also be cached, per the spec?

Comment 7

5 years ago
I do prefer the automatic discovery. No need to copy manifest url to several places,
but appcache just works.
Comment on attachment 629060 [details] [diff] [review]
Patch

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

For me looks good, but I'm not the best reviewer for this file.  Maybe let Olli take a look too.

Giving only f+.

::: browser/modules/WebappsInstaller.jsm
@@ +764,5 @@
> +function preloadAppCache(aShell) {
> +  discoverAppCacheManifestURI(aShell.launchURI, function WA_AppCacheDiscovery(result) {
> +    if (result.error) {
> +      if (result.error != "App cache manifest not available") {
> +        Cu.reportError(result.error);

Rather add result.dontLog ?  And actually, this should be IMO logged as a warning maybe or at least an info.

@@ +785,5 @@
> +      updateStateChanged: function WA_AppCacheUpdate(update, state) {
> +        if (state == Ci.nsIOfflineCacheUpdateObserver.STATE_FINISHED) {
> +          aShell._finishedAppCacheLoad(true);
> +        }
> +      },

What about case of an error or noupdate when the app is already cached?

- STATE_ERROR means we cannot cache
- STATE_NOUPDATE means the app is already there and you can treat it as STATE_FINISHED
Attachment #629060 - Flags: review?(honzab.moz) → feedback+
Nominating for k9o using the same rationale as in bug 756620.
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → +
(In reply to Felipe Gomes (:felipe) from comment #6)
> Ok, so should I remove discovery from this patch?
> 
> Fabrice: in the b2g bug has it been decided what is the field name for the
> cache manifest in the app manifest?

I'm using appcache_path

> And what about the top-level URL which is often omitted from the appcache
> manifest but should also be cached, per the spec?

I think it is cached automagically by the platform (testing with a cache manifest that has a single entry, I see it's downloading 2 resources).
Priority: -- → P1
Target Milestone: --- → Firefox 16
Random question - should we file an equilvalent bug for this on fennec native webrt?
(Assignee)

Comment 12

5 years ago
Created attachment 635581 [details] [diff] [review]
Patch v2

So the actual call to preload the appcache in the target profile was included in confirmInstall in bug 702369 and doesn't need to happen here anymore.  This patch now creates the app profile and does some refactoring to pass the correct information down to confirmInstall.

With bug 702369 the webpage can receive notifications about the appcache download progress. I'll split up any extra UI work displayed by the browser to a separate bug.
Attachment #629060 - Attachment is obsolete: true
Attachment #635581 - Flags: review?(mar.castelluccio)
Comment on attachment 635581 [details] [diff] [review]
Patch v2

Nit: denyInstall is a function with a single argument

And we should file a bug about creating the profile on installation also when we don't preload the appcache.
Attachment #635581 - Flags: review?(mar.castelluccio) → review+
Will this implementation work on Linux?
(Assignee)

Comment 15

5 years ago
Yeah, I'll include bug 764634 together and it will make it work on Linux
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9662c4822cb
So the way I would verify this fix would be something like the following below, right?

1. Install an app that uses app cache to support offline mode (lanyrd mobile)
2. Disconnect the internet connection
3. Launch Lanyrd Mobile

Goal - Verify that lanyrd mobile launched with app correctly using any app cache resources stored.

There might be a better way to verify this, but that's first way that comes to mind...
Whiteboard: [qa+]
(Assignee)

Comment 18

5 years ago
Correct. But the webapp has to have added the "appcache_path" entry to its app manifest for this to work, which I think Lanyrd Mobile doesn't do yet.
Fabrice has one to test here: http://people.mozilla.com/~fdesre/openwebapps/test.html
https://hg.mozilla.org/mozilla-central/rev/e9662c4822cb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
The documentation should be updated: https://developer.mozilla.org/en/Apps/Manifest
Also, adding the appcache_path attribute to the manifest.webapp fails the dev marketplace manifest validator.  Not sure if you guys can view this but: https://marketplace-dev.allizom.org/en-US/developers/upload/22886f29b5794dec961f7c8df20e8c97
(In reply to Nick Desaulniers from comment #20)
> The documentation should be updated:
> https://developer.mozilla.org/en/Apps/Manifest

Where? I don't see it added (or I could be blind, which is definitely possible).

(In reply to Nick Desaulniers from comment #21)
> Also, adding the appcache_path attribute to the manifest.webapp fails the
> dev marketplace manifest validator.  Not sure if you guys can view this but:
> https://marketplace-dev.allizom.org/en-US/developers/upload/
> 22886f29b5794dec961f7c8df20e8c97

Not surprised. Could you file a bug on marketplace's validator about this and cc myself, Krupa Raj, and Wil Clouser?
Keywords: dev-doc-needed
For the record, MDN was updated.  https://developer.mozilla.org/en/Apps/Manifest
and the new validator ticket can be found: https://bugzilla.mozilla.org/show_bug.cgi?id=768205
(In reply to Nick Desaulniers from comment #23)
> For the record, MDN was updated. 
> https://developer.mozilla.org/en/Apps/Manifest

Thanks. Flagging as such.
Keywords: dev-doc-needed → dev-doc-complete
Using Fabrice's test case, this works on Windows.
(In reply to Jason Smith [:jsmith] from comment #26)
> Using Fabrice's test case, this works on Windows.

Also works on Mac.
(Assignee)

Comment 28

5 years ago
Ah, ftr, Marco found a problem in the Linux part that will make it not work on Linux yet, but he has fixed it in the patch for bug 764172 which I'll land on inbound later today
(In reply to Felipe Gomes (:felipe) from comment #28)
> Ah, ftr, Marco found a problem in the Linux part that will make it not work
> on Linux yet, but he has fixed it in the patch for bug 764172 which I'll
> land on inbound later today

Gotcha. Marking as verified then.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Blocks: 775703
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.