824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Bug 791039 introduced support for appcache prepopulation for 3rd party preinstalled apps. Apps in `external-apps` gaia folder can ship a `cache` folder containing an `appcache.manifest` that will be read by gecko on first run and updates in order to prepopulate the appcache with files contained in `cache` folder.
But that patch was only adding a limited support of appcache manifest, it only care about cache entries but ignored namespace and fallback ones.

Such entry support was requested in bug 815814.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

Comment 2

[Alexandre Poirot [:ochameau]]

http://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp#938

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 696026 [details] [diff] [review]
Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

This code is based on existing c++ implementation available here:
http://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp#938

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 696026 [details] [diff] [review]
Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

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

::: dom/apps/src/OfflineCacheInstaller.jsm
@@ +129,5 @@
>        let urls = [];
> +      let namespaces = [];
> +      let fallbacks = [];
> +
> +      let section = 'CACHE';

nit: section -> currentSection

@@ +191,1 @@
>          }

I would have like a processCacheLine, processFallbackLine, processNetworkLine or something that makes the method smaller.

@@ +208,5 @@
>          storeCache(applicationCache, url, file, itemType);
>        });
> +
> +      let array = Cc['@mozilla.org/array;1'].createInstance(Ci.nsIArray);
> +      array.QueryInterface(Ci.nsIMutableArray);

Components.Contructor for the array maybe to stay in sinc with the rest of the code.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 696531 [details] [diff] [review]
Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

You opened my eyes, I didn't realized how long what that method alone in this file :o So I made some more substantial modifications and split in reasonable sized functions.

I hope you will like them!

I tested this patch again with potch patch against marketplace-dev.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 696531 [details] [diff] [review]
Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps

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

r+ with nits.

::: dom/apps/src/OfflineCacheInstaller.jsm
@@ +108,5 @@
> +  } else if (line.substr(0, 4) == 'http') {
> +    urls.push(line);
> +  } else {
> +    throw new Error('Only accept absolute path and http/https URLs');
> +

nit: extra line break

@@ +113,5 @@
> +  }
> +}
> +
> +function parseFallbackLine(app, namespaces, fallbacks, line) {
> +  let split = line.split(/[ \t]+/);

let [namespace, fallback] = line.split(/[ \t]+/); ? ;)

Comment 8

[JP Rosevear [:jpr]]

Can we get the nits addressed and land this?

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps r=vingtetun

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 697394 [details] [diff] [review]
Bug 824949 - Add support for FALLBACK appcache entries for pre-installed 3rd party apps r=vingtetun

Addressed nits.
Tested again with potch's version of marketplace-dev using fallback entry.
  https://github.com/potch/gaia/commit/f5960def1ed01924a39f2fd8e4f65d49ffd3d529
  The only modification needed is to add `/offline/home` in CACHE entries.

Comment 11

[Honza Bambas (:mayhemer)]

Do you know there is a support in Gecko to download offline cache to a selected profile folder for you?

http://hg.mozilla.org/mozilla-central/annotate/6955309291ee/uriloader/prefetch/nsIOfflineCacheUpdate.idl#l230

It is not very wise to reimplement the manifest parser and updater again, when there is a full offline cache update and download code in Gecko already.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Do you know there is a support in Gecko to download offline cache to a
> selected profile folder for you?
> 
> http://hg.mozilla.org/mozilla-central/annotate/6955309291ee/uriloader/
> prefetch/nsIOfflineCacheUpdate.idl#l230
> 
> It is not very wise to reimplement the manifest parser and updater again,
> when there is a full offline cache update and download code in Gecko already.

As far as I can tell there isn't any method close to what we are doing.
Here at the time we feed the offline cache DB, we do not have network connection. It is done on first run and then on updates. We are filling the offline cache with files coming from the file system.
We could have hacked something in c++, but I don't think it will be that easier.
The parser implemention doesn't seems to be hookable.
Note that we do not do any update. We just prepropulate offline cache in order to be able to load web apps without requiring to have data connection, then all existing c++ code do the rest.

Comment 13

[Honza Bambas (:mayhemer)]

Ah, it's that code.  Then yes, you unfortunately don't have another option.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d28aa6c376

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/217b18bf3b5a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dde166b9f09e