871435 - Allow for a customized appcache_path to be used for preinstalled hosted apps using appcache

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Per discussion on bug 870178, we're apparently placing the requirement right now that the appcache_path has to be 'manifest.appcache' - nothing else. This is problem for the preinstalled apps, given that different apps have different appcache_paths. We need to allow the appcache_path to be customized for preinstalled hosted apps.

Note - this is the underlying gecko work required to fix bug 870178, bug 869361, bug 866720.

Comment 1

[Jason Smith [:jsmith]]

Blocks three tef+ blockers.

Comment 2

[[:fabrice] Fabrice Desré]

Hm, yes. We hardcode the appcache manifest url here: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/OfflineCacheInstaller.jsm#211

We should read it from manifest.webapp instead.

Comment 3

[Jason Smith [:jsmith]]

I'm assuming you didn't mean to unassign ferjm

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch

Needs some testing, but is pretty simple.

Comment 5

[[e-mail]]

A related issue is how to 'store/parse URL resources(resource start from http:// or https://) that within or without the same domain' to cache/ folder.

Is it possible or should we avoid it for preinstalled webapp?

Comment 6

[[e-mail]]

for reference,

The accuweather link is http://m.accuweather.com/mozilla.webapp

appcache path is http://m.accuweather.com//awx.appcache

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Fred Lin [:gasolin] from comment #5)
> A related issue is how to 'store/parse URL resources(resource start from
> http:// or https://) that within or without the same domain' to cache/
> folder.
> 
> Is it possible or should we avoid it for preinstalled webapp?

It's not possible currently, no. Feel free to add support if you really need that.

Comment 8

[[e-mail]]

There might be some other preinstalled app use URL resources in the future.
But currenly the influence is accuweather will not get default small icon while showing offline page, and it only occurred when app is never used with network connection.

So I'd prefer not to prefetch URL resources(resource start from http:// or https://) now since its a bit tricky: we have to come up with a new rule to store URL resources in cache/ then parse it in OfflineCacheInstaller.


Should we solve fetching URL resources here, or fire another track issue that not block the tef?

needinfo? Jason, how do you think?

Comment 9

[Jason Smith [:jsmith]]

The critical use case is making sure that we can get the offline cache resources preloaded as part of a preinstalled app. If we fail on first run due to not having cached resources available, then that might be okay for 1.01 given the time constraint. However, if post first run we fail due to not having cached resources available, then that's a blocker.

Given that, we could implement a simpler solution for 1.01 and work towards a longer term solution in a different bug.

Let me get Fabrice's opinion here though.

Comment 10

[[:fabrice] Fabrice Desré]

I think we're good there. The "online" version of the appcache manifest will be different from the preloaded one, and this will trigger a re-refetch of all the resources from this manifest, including then the missing absolute uris.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: patch v2

So, this patch fixes several issues with our appcache preloading:

- the path is not always app.basePath since we move some apps out of /system/b2g
- origins have no trailing / so we must keep it in relative urls.
- we were not adding urls from FALLBACK sections.

With all these changes, and the update to accuweather from bug 870178, I get accuweather preloaded properly, and serving the fallback content when we are offline.

Comment 12

[Jason Smith [:jsmith]]

We should also verify this works with Notes and Wikipedia as well.

Comment 13

[[e-mail]]

FYR,

Wikipedia
https://bits.wikimedia.org/WikipediaMobileFirefoxOS/manifest.webapp

Notes
http://everythingme.github.io/ffos-notes/manifest.webapp

Just confirmed with yuren, since Notes app will be changed to packaged app, it might not relevant.

Comment 14

[Jason Smith [:jsmith]]

Right. Notes is moving to packaged so long as nothing surprising shows up in review.

Comment 15

[[e-mail]]

We've setup a offline appcache branch for customization. If it works with your patch, we'll merge them back to customization master


copy yuren's comment #33 from bug 870178 :

Fabrice, you can use those for testing gecko part.

latam: https://github.com/telefonicaid/firefoxos-gaia-latam/commit/fe88245f395e265387b373cbf12c624258aa4d66

Spain: https://github.com/telefonicaid/firefoxos-gaia-spain/commit/05a845b14f6786d2d39cfc872ddcabaa005b5a3a

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 749609 [details] [diff] [review]
patch v2

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

> - origins have no trailing / so we must keep it in relative urls.

I wasn't able to see the code that ensure that.
Are we relying on this helper in content side?
  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#72

Otherwise the patch looks good.

::: dom/apps/src/Webapps.jsm
@@ +323,5 @@
>          file.copyTo(destDir, aFile);
>        });
>  
>      app.installState = "installed";
> +    app.cachePath = app.basePath;

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Created attachment 749609 [details] [diff] [review]
> patch v2
> 
> So, this patch fixes several issues with our appcache preloading:
> 
> - the path is not always app.basePath since we move some apps out of
> /system/b2g

It's not very clear how you address this, as cachePath is always set to basePath?

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #16)
> Comment on attachment 749609 [details] [diff] [review]
> patch v2
> 
> Review of attachment 749609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > - origins have no trailing / so we must keep it in relative urls.
> 
> I wasn't able to see the code that ensure that.
> Are we relying on this helper in content side?
>   http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#72

Yes, in part. Origins are only scheme://host[:port] in general, no path at all. We've been tracking hand written bad origins on the gaia side to match that also.

> It's not very clear how you address this, as cachePath is always set to
> basePath?

The cachePath is set to basePath before we eventually move the app out of /system/b2g to /data/local and update basePath accordingly. Since we only support that on gonk I could also have hardcoded cachePath to /system/b2g but that did look even worse to me.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: patch v3

When trying to get wikipedia working I found some other issues, so here's an updated patch. The main change is that I use the origin to resolve urls instead of relying on some heuristics and string concatenation.

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 750032 [details] [diff] [review]
patch v3

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

Still looks good, thanks!

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/c9377ee6a251

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e81955b939ac
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e7e7d9dab5da