812198 - Add support for preinstalled packaged apps

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

When I worked on bug 791039 which introduced support for 3rd party apps being preinstalled, I haven't added support for packaged app.

That's something we would like to implement if we want to land an `application.zip` with its `update.manifest` in gaia `external-apps` folder and see it preinstalled in the phone.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 6749

Potential patch for gaia, but may depend on how Webapps.jsm is going to handle packaged apps.

Comment 2

[Dietrich Ayala (:dietrich)]

Hi Alex, what's your estimate for when this work will be finished?

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Dietrich Ayala (:dietrich) from comment #2)
> Hi Alex, what's your estimate for when this work will be finished?

I plan to finish the gecko part and minor changes to the gaia PR tomorrow.

Comment 4

[Jason Smith [:jsmith]]

This isn't per say related to the marketplace tracking bug. I've already added the whiteboard that appears to be getting general usage for tracking of preloaded apps - [3rd-party-preloaded-apps].

Installing of the packaged apps preloaded for testing isn't blocked on anything for marketplace. The only piece that could play a factor is the dependency on signing for the finalized builds. I'll link to that bug.

Comment 5

[Jason Smith [:jsmith]]

Actually, let's just keep this simple and stick with the whiteboard.

Comment 6

[Dietrich Ayala (:dietrich)]

I added that link for a reason, please do not remove it, thanks.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Dietrich Ayala (:dietrich) from comment #6)
> I added that link for a reason, please do not remove it, thanks.

Then what's the reason? We clarified Friday that implementing this shouldn't really have a direct dependency on a marketplace implementation of packaged apps. And the whiteboard was to be used for tracking temporarily. If we want to use a tracking bug, I'd morph the preinstalled-apps bug to do this. But I don't want that packaged-apps marketplace tracking bug getting convoluted with the tracking for preinstalled-apps.

I don't want tracking duplication for a tracking process that already exists and works.

Comment 8

[Jason Smith [:jsmith]]

bug 815411 is the bug you actually want

Comment 9

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

Alex we can land this before the platform blocker right? This is not going to break anything I guess?

Comment 10

[[:fabrice] Fabrice Desré]

This can't land as is. I needed to make changes while working on the gecko part.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 689330 [details] [diff] [review]
patch

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

Looks good as well, but per bug 817982 comment 6, I wasn't able to test it properly.
I'll continue tomorrow.

::: Makefile
@@ +223,5 @@
>  webapp-manifests: install-xulrunner-sdk
>  	@mkdir -p profile/webapps
>  	@$(call run-js-command, webapp-manifests)
> +#	@cat profile/webapps/webapps.json
> +	@echo "Done"

Note that this modification conflicts with master. Someone had same idea and commented the `cat`.

::: build/webapp-manifests.js
@@ -82,1 @@
>  

It would be helpfull to add a test against `webapp.metaData`, which is mandatory for external apps. Otherwise we receive a weird exception during make if we forget creating a metadata.json file.

@@ +79,5 @@
> +  let origin;
> +  let installOrigin;
> +  let manifestURL;
> +
> +  let removable = false;

Shouldn't they be removable ?

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Addressing comments.

removable is false by default, but set to true for packaged apps. I let it false for hosted apps since we don't move them out of /system/b2g in gecko.

Comment 14

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

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

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

r+ with nits.

::: Makefile
@@ +224,4 @@
>  	@mkdir -p profile/webapps
>  	@$(call run-js-command, webapp-manifests)
>  	@#cat profile/webapps/webapps.json
> +	@echo "Done"

No need for this line

::: build/utils.js
@@ +95,2 @@
>    let content = getFileContent(file);
>    return JSON.parse(content);

The indentation seems wrong.

::: build/webapp-manifests.js
@@ +95,5 @@
> +    if (!updateManifest.exists()) {
> +      throw new Error('External packaged webapp `' + webapp.domain + '  is ' +
> +                      'missing an `update.webapp` file. This JSON file ' +
> +                      'contains a `package_path` attribute specifying where ' +
> +                      ' download the application zip package from the origin' +

nit extra whitespace at the beginning of the line

@@ +108,5 @@
> +  } else {
> +    webapp.manifestFile.copyTo(webappTargetDir, 'manifest.webapp');
> +    origin = webapp.metaData.origin;
> +    installOrigin = webapp.metaData.origin;
> +    manifestURL = webapp.metaData.origin + 'manifest.webapp';

Maybe this code should be below. Otherwise the 'otherwise' in the other text seems misplaced.

Comment 15

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/c94c8a6a0927260e9825933383f73d8f21f5e497

Comment 16

[Jason Smith [:jsmith]]

Notes from Fabrice's email thread:


In bugs 812198 and 817982 we are landing support for preloaded packaged
apps.

The way it works is that for each you want to preload, you need to :
1) create a gaia/external-apps/APP_ID directory
2) In this directory, copy the application.zip and update.manifest files.
3) In this directory, create a metadata.json with the following structure:
{
  "origin": "http://ctr.com",
  "installOrigin": "https://marketplace.firefox.com",
  "manifestURL": "http://marketplace.firefox.com/ctr/update.manifest"
}

Then gaia's build system and gecko DOM Registry startup take care of
setting everything up.

	Fabrice

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Doesn't this bug require bug 817982 to land? My phone stays completely black and shows errors like stated in bug 817982 comment 9. I'll try to back this out and see if that fixes the problem for me.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Yep, reverting c94c8a6a0927260e9825933383f73d8f21f5e497 makes my phone work again. Can we back this out, please?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Talked to Vivien and he agreed on the back out:

https://github.com/mozilla-b2g/gaia/commit/4a52b386ef3383f820d8a02269282dd193df65c5

Comment 20

[[:fabrice] Fabrice Desré]

Sorry for the breakage. Pulling https://github.com/mozilla-b2g/gaia/pull/6906 would have fixed it though.

Comment 21

[[:fabrice] Fabrice Desré]

re-pushed with a fix for eng builds :
https://github.com/mozilla-b2g/gaia/commit/1e66fc60fa6f6dd14cee169b09a518f181e33ce2

Comment 22

[Jason Smith [:jsmith]]

Marking as verified given that we have proven that we were successful in preinstalling the maps app and the stubs. I've confirmed I was able to add a preinstalled app. Sanity level it's okay, but there's more testing that will be done as I go.