Closed Bug 812198 Opened 8 years ago Closed 8 years ago

Add support for preinstalled packaged apps

Categories

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

defect

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +

People

(Reporter: ochameau, Assigned: fabrice)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: QARegressExclude)

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → poirot.alex
blocking-basecamp: --- → ?
Whiteboard: DUPEME
blocking-basecamp: ? → +
Priority: -- → P1
Whiteboard: DUPEME
blocking-basecamp: + → ?
Priority: P1 → --
blocking-basecamp: ? → +
Whiteboard: [3rd-party-preloaded-apps]
Target Milestone: --- → B2G C2 (20nov-10dec)
Attached file Pull request 6749 (obsolete) —
Potential patch for gaia, but may depend on how Webapps.jsm is going to handle packaged apps.
Priority: -- → P1
No longer blocks: privileged-apps
Depends on: 817982
Hi Alex, what's your estimate for when this work will be finished?
(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.
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.
Depends on: sign-packaged-apps
No longer depends on: market-packaged-apps
Actually, let's just keep this simple and stick with the whiteboard.
No longer depends on: sign-packaged-apps
I added that link for a reason, please do not remove it, thanks.
(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.
No longer depends on: market-packaged-apps
bug 815411 is the bug you actually want
Whiteboard: [3rd-party-preloaded-apps]
Alex we can land this before the platform blocker right? This is not going to break anything I guess?
This can't land as is. I needed to make changes while working on the gecko part.
Attached patch patch (obsolete) — Splinter Review
Attachment #689330 - Flags: review?(poirot.alex)
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 ?
Attached patch patch v2Splinter Review
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.
Assignee: poirot.alex → fabrice
Attachment #687035 - Attachment is obsolete: true
Attachment #689330 - Attachment is obsolete: true
Attachment #689330 - Flags: review?(poirot.alex)
Attachment #689580 - Flags: review?(poirot.alex)
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.
Attachment #689580 - Flags: review?(poirot.alex) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: 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
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.
Yep, reverting c94c8a6a0927260e9825933383f73d8f21f5e497 makes my phone work again. Can we back this out, please?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry for the breakage. Pulling https://github.com/mozilla-b2g/gaia/pull/6906 would have fixed it though.
re-pushed with a fix for eng builds :
https://github.com/mozilla-b2g/gaia/commit/1e66fc60fa6f6dd14cee169b09a518f181e33ce2
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Keywords: verifyme
Whiteboard: QARegressExclude
No longer blocks: packaged-apps
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.