833945 - provide packaged and hosted app stubs to dogfooders

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

A bunch of folks, including @jsmith, @rfant, and @sicking, are itching to test packaged and hosted app stubs for preloaded third-party apps, even though we don't yet have enough information to construct stubs for any of the actual apps.

So @sicking et. al. suggested we add test stubs to Gaia and push them to dogfooding devices.  Here's a patch that does that.

Note that the packaged stub experience is still a prototype, and I'm in conversation with @jcarpenter about it.  Also, it depends on the fix for bug 833531.

Fabrice: it isn't clear how to build these apps only for dogfooding devices, and I'd appreciate your advice about that.  I see some support for conditionally including apps when ifdef DOGFOOD, but it doesn't seem like that applies to apps in external-apps/.

Also note that the patch includes a binary application.zip file for the packaged stub.  That is required by the build scripts, but I included the source files as well, so you can see what the package contains.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: adds packaged and hosted stubs

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 705506 [details] [diff] [review]
patch v1: adds packaged and hosted stubs

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

If we want that only in dogfood builds, they should be in dogfood_apps and not in external-apps, but then we won't process them to be preloaded. A bit of hacking in build/webapp-manifests.js is needed to get this right.

::: external-apps/{47f70abc-3a9f-492b-af61-49832b24244a}/metadata.json
@@ +1,2 @@
> +{
> +  "origin": "app://{47f70abc-3a9f-492b-af61-49832b24244a}",

can you use a more descriptive ID? It's fine to have any unique name, so something like 'packaged-stub.gaiamobile.org' or whatever suits you.

::: external-apps/{47f70abc-3a9f-492b-af61-49832b24244a}/update.webapp
@@ +1,4 @@
> +{
> +  "name": "PackStubTest",
> +  "description": "a packaged app for testing stub updates",
> +  "launch_path": "/index.html",

There are no launch path in update manifests.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses feedback issues

(In reply to Fabrice Desré [:fabrice] from comment #2)
> If we want that only in dogfood builds, they should be in dogfood_apps and
> not in external-apps, but then we won't process them to be preloaded. A bit
> of hacking in build/webapp-manifests.js is needed to get this right.

It looks like the simplest and lowest-risk solution is to put them in a directory specific to apps that are both external and dogfood, then process that directory via the special external apps codepath only when building a profile for a dogfood device.

This patch implements that solution.


> can you use a more descriptive ID? It's fine to have any unique name, so
> something like 'packaged-stub.gaiamobile.org' or whatever suits you.

Yes, I renamed it to packstubtest.


> There are no launch path in update manifests.

For an external packaged app, there is no app manifest (manifest.webapp) on B2G firstrun, per <https://github.com/mozilla-b2g/gaia/blob/4973f952862d1ec6a18ce430201160dcb7a84d93/build/webapp-manifests.js#L112>.

So DOMApplicationRegistry uses the update manifest (update.webapp) as the app manifest, per <https://github.com/mozilla/mozilla-central/blob/54d68d7b8433c9b34a025d921e79e7a9830181e5/dom/apps/src/Webapps.jsm#L1878-L1880>.

Which means removing launch_path from that file would prevent the app from launching to the correct path (until the update process creates an app manifest in the process of updating the app).  Thus I have left it in.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 705632 [details] [diff] [review]
patch v2: addresses feedback issues

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

r=me with the launch path removed from the update.webapp manifest.

What happens is that when we are at first run or updating, we run installPreinstalledApp() at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#221
In this function, we unpack the application manifest from the zip and install it properly, at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#278

I bet you don't go in this code path because you don't restart from a fresh profile or after a real update. Also, this is only supported on devices.

::: external-dogfood-apps/packstubtest/update.webapp
@@ +1,4 @@
> +{
> +  "name": "PackStubTest",
> +  "description": "a packaged app for testing stub updates",
> +  "launch_path": "/index.html",

I still disagree about that line.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: removes launch_path from update manifest

(In reply to Fabrice Desré [:fabrice] from comment #4)
> What happens is that when we are at first run or updating, we run
> installPreinstalledApp() at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#221
> In this function, we unpack the application manifest from the zip and
> install it properly, at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#278
> 
> I bet you don't go in this code path because you don't restart from a fresh
> profile or after a real update. Also, this is only supported on devices.

Ah, I see.  Thanks for the explanation!  This might be the first packaged app stub issue I have experienced that is actually desktop-specific.

This patch removes launch_path from update.webapp.  Carrying forward review.

@fabrice: can you commit this patch?  I don't have Gaia commit privileges.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: formatted for commission

And here's a version of the patch formatted with `git format-patch`, to make it easier to apply via `git apply` (or import via `hg import`).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla-b2g/gaia/compare/b39f61ecf6e3...50c14856efe7

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 705923 [details] [diff] [review]
patch v4: formatted for commission

[Approval Request Comment]
A bunch of folks are itching to test packaged and hosted app stubs for preloaded third-party apps, even though we don't yet have enough information to construct stubs for any of the actual apps.

So this patch adds one test packaged stub and a test hosted stub to Gaia.  The stubs are for testing purposes, so they are only built for dogfood devices, and they won't show up on production builds.

Note that the UX for the packaged stub is still under review, and the current implementation is only a prototype.  So we expect it to change over time.  But it's still useful to test these stubs now, so we can verify they work before the window of opportunity for platform changes closes further.

Comment 9

[Jason Smith [:jsmith]]

Comment on attachment 705923 [details] [diff] [review]
patch v4: formatted for commission

I think you want approval-gaia-v1. This is a Gaia patch, not a gecko patch.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 705923 [details] [diff] [review]
patch v4: formatted for commission

Approving, please land to v1-train.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Thanks @akeybl!  I have requested the pull:

https://github.com/mozilla-b2g/gaia/pull/7794

@fabrice: can you merge it?

Note: I had to include the fix for bug 828843 in the pull request, because it turns out that my patch depends on it (it adds the *dogfood* MAKECMDGOAL that isolates a set of apps to dogfood builds).

cc @vingtetun and @ochameau, who worked on bug 828843, so they're aware of this merge.  That bug is marked blocking-b2g-tef+, so this merge should have happened anyway.

Comment 12

[[:fabrice] Fabrice Desré]

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