Last Comment Bug 760365 - Preload appcache into webapp's profile during installation
: Preload appcache into webapp's profile during installation
Status: VERIFIED FIXED
[qa!]
: dev-doc-complete
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: All All
: P1 normal
: Firefox 16
Assigned To: :Felipe Gomes (needinfo me!)
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 756620 775703
  Show dependency treegraph
 
Reported: 2012-05-31 22:19 PDT by :Felipe Gomes (needinfo me!)
Modified: 2016-02-04 15:00 PST (History)
15 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (6.12 KB, patch)
2012-05-31 22:32 PDT, :Felipe Gomes (needinfo me!)
honzab.moz: feedback+
Details | Diff | Splinter Review
Patch v2 (5.64 KB, patch)
2012-06-21 19:30 PDT, :Felipe Gomes (needinfo me!)
mcastelluccio: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-05-31 22:19:45 PDT
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.
Comment 1 :Felipe Gomes (needinfo me!) 2012-05-31 22:32:59 PDT
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..
Comment 2 [:fabrice] Fabrice Desré 2012-05-31 23:54:21 PDT
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.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-01 00:17:50 PDT
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.
Comment 4 :Felipe Gomes (needinfo me!) 2012-06-01 12:48:47 PDT
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.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-01 19:12:54 PDT
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.
Comment 6 :Felipe Gomes (needinfo me!) 2012-06-01 23:38:42 PDT
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 Olli Pettay [:smaug] 2012-06-02 02:42:00 PDT
I do prefer the automatic discovery. No need to copy manifest url to several places,
but appcache just works.
Comment 8 Honza Bambas (:mayhemer) 2012-06-03 06:37:20 PDT
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
Comment 9 Jason Smith [:jsmith] 2012-06-03 17:53:36 PDT
Nominating for k9o using the same rationale as in bug 756620.
Comment 10 [:fabrice] Fabrice Desré 2012-06-04 14:41:34 PDT
(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).
Comment 11 Jason Smith [:jsmith] 2012-06-20 17:29:27 PDT
Random question - should we file an equilvalent bug for this on fennec native webrt?
Comment 12 :Felipe Gomes (needinfo me!) 2012-06-21 19:30:13 PDT
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.
Comment 13 Marco Castelluccio [:marco] 2012-06-22 11:32:43 PDT
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.
Comment 14 Jason Smith [:jsmith] 2012-06-22 12:24:30 PDT
Will this implementation work on Linux?
Comment 15 :Felipe Gomes (needinfo me!) 2012-06-22 12:37:34 PDT
Yeah, I'll include bug 764634 together and it will make it work on Linux
Comment 16 :Felipe Gomes (needinfo me!) 2012-06-22 15:20:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9662c4822cb
Comment 17 Jason Smith [:jsmith] 2012-06-22 15:24:52 PDT
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...
Comment 18 :Felipe Gomes (needinfo me!) 2012-06-22 15:28:08 PDT
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
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:44:39 PDT
https://hg.mozilla.org/mozilla-central/rev/e9662c4822cb
Comment 20 (not reading bugmail) Nick Desaulniers [:\n] 2012-06-25 09:06:53 PDT
The documentation should be updated: https://developer.mozilla.org/en/Apps/Manifest
Comment 21 (not reading bugmail) Nick Desaulniers [:\n] 2012-06-25 13:47:33 PDT
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
Comment 22 Jason Smith [:jsmith] 2012-06-25 13:49:58 PDT
(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?
Comment 23 (not reading bugmail) Nick Desaulniers [:\n] 2012-06-25 14:42:32 PDT
For the record, MDN was updated.  https://developer.mozilla.org/en/Apps/Manifest
Comment 24 (not reading bugmail) Nick Desaulniers [:\n] 2012-06-25 14:43:34 PDT
and the new validator ticket can be found: https://bugzilla.mozilla.org/show_bug.cgi?id=768205
Comment 25 Jason Smith [:jsmith] 2012-06-25 18:37:20 PDT
(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.
Comment 26 Jason Smith [:jsmith] 2012-06-26 16:32:57 PDT
Using Fabrice's test case, this works on Windows.
Comment 27 Jason Smith [:jsmith] 2012-06-26 16:44:45 PDT
(In reply to Jason Smith [:jsmith] from comment #26)
> Using Fabrice's test case, this works on Windows.

Also works on Mac.
Comment 28 :Felipe Gomes (needinfo me!) 2012-06-26 16:48:41 PDT
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
Comment 29 Jason Smith [:jsmith] 2012-06-26 16:55:33 PDT
(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.

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