Closed
Bug 871435
Opened 11 years ago
Closed 11 years ago
Allow for a customized appcache_path to be used for preinstalled hosted apps using appcache
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(blocking-b2g:tef+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
People
(Reporter: jsmith, Assigned: fabrice)
References
Details
(Whiteboard: [apps watch list][target:05/17][qa-])
Attachments
(1 file, 2 obsolete files)
8.03 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Blocks three tef+ blockers.
Updated•11 years ago
|
blocking-b2g: tef? → tef+
Updated•11 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee: ferjmoreno → nobody
Reporter | ||
Comment 3•11 years ago
|
||
I'm assuming you didn't mean to unassign ferjm
Assignee: nobody → ferjmoreno
Updated•11 years ago
|
Assignee: ferjmoreno → fabrice
Assignee | ||
Comment 4•11 years ago
|
||
Needs some testing, but is pretty simple.
Updated•11 years ago
|
Whiteboard: [apps watch list]
Comment 5•11 years ago
|
||
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?
Flags: needinfo?(fabrice)
Comment 6•11 years ago
|
||
for reference, The accuweather link is http://m.accuweather.com/mozilla.webapp appcache path is http://m.accuweather.com//awx.appcache
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Flags: needinfo?(fabrice)
Comment 8•11 years ago
|
||
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?
Flags: needinfo?(jsmith)
Reporter | ||
Comment 9•11 years ago
|
||
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.
Flags: needinfo?(jsmith) → needinfo?(fabrice)
Assignee | ||
Comment 10•11 years ago
|
||
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.
Flags: needinfo?(fabrice)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [apps watch list] → [apps watch list][target:05/17]
Assignee | ||
Comment 11•11 years ago
|
||
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.
Attachment #749050 -
Attachment is obsolete: true
Attachment #749609 -
Flags: review?(poirot.alex)
Reporter | ||
Comment 12•11 years ago
|
||
We should also verify this works with Notes and Wikipedia as well.
Comment 13•11 years ago
|
||
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.
Reporter | ||
Comment 14•11 years ago
|
||
Right. Notes is moving to packaged so long as nothing surprising shows up in review.
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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?
Attachment #749609 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
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.
Attachment #749609 -
Attachment is obsolete: true
Attachment #750032 -
Flags: review?(poirot.alex)
Comment 19•11 years ago
|
||
Comment on attachment 750032 [details] [diff] [review] patch v3 Review of attachment 750032 [details] [diff] [review]: ----------------------------------------------------------------- Still looks good, thanks!
Attachment #750032 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/c9377ee6a251
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c9377ee6a251
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Updated•11 years ago
|
Whiteboard: [apps watch list][target:05/17] → [apps watch list][target:05/17][qa-]
Updated•11 years ago
|
Whiteboard: [apps watch list][target:05/17][qa-] → [status: needs uplift][apps watch list][target:05/17][qa-]
Updated•11 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → affected
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e81955b939ac https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e7e7d9dab5da
status-b2g18-v1.0.0:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → wontfix
status-firefox24:
--- → fixed
Whiteboard: [status: needs uplift][apps watch list][target:05/17][qa-] → [apps watch list][target:05/17][qa-]
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•