1075716 - Support app manifests served with the application/manifest+json MIME type

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Ben Francis [:benfrancis]]

The latest Editor's Draft of the W3C "Manifest for web application" specification proposes the "application/manifest+json" MIME type for manifests [1].

Currently, trying to install an app with a manifest of this MIME type will throw an INVALID_MANIFEST_CONTENT_TYPE error.

This bug is to track supporting the installation of apps using manifests with that MIME type.

1. http://w3c.github.io/manifest/#obtaining-a-manifest

Comment 1

[Ben Francis [:benfrancis]]

Attachment: Patch to add support for Web Manifest MIME type

Fabrice, this adds support for the Web Manifest MIME type to window.navigator.mozApps.install(). What do you think? (Be gentle, this may be my first ever Gecko patch...)

Is there a good place to add a test for this?

I've manually tested as follows:
* Navigate to http://people.mozilla.org/~bfrancis/app_installer/
* Click "Install Webian Tasks" which calls mozApps.install() on a manifest served with the application/manifest+json MIME type from another origin

On mozilla-central this will show a content type error. With this patch applied it will install the app.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8506095 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

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

You can also add the new mime type to http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-helper.js#444

::: dom/apps/AppsUtils.jsm
@@ +470,5 @@
>      let netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
>      let contentType = netutil.parseContentType(aContentType, charset, hadCharset);
>      if (aInstallOrigin != aWebappOrigin &&
> +        !(contentType == "application/x-web-app-manifest+json" ||
> +        contentType == "application/manifest+json")) {

nit: align both contentType

Comment 3

[Ben Francis [:benfrancis]]

Attachment: Patch to add support for Web Manifest MIME type

Thanks Fabrice, here's a revised patch for review.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8506993 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

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

That would be good to add a test. See https://mxr.mozilla.org/mozilla-central/search?string=web-app-manifest&find=%2Fdom%2Fapps%2Ftests%2F&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central for places where we set the mime type in tests.

Comment 5

[Ben Francis [:benfrancis]]

Attachment: Patch to add support for Web Manifest MIME type

Added mochitest. I've never written one before so no idea if some of that code was unnecessary/I missed things.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8508196 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

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

::: dom/apps/tests/test_web_app_install.html
@@ +51,5 @@
> +  * Install a web app from a manifest with application/manifest+json MIME type.
> +  */
> +function runTest() {
> +  SpecialPowers.setAllAppsLaunchable(true);
> +  

nit: trailing whitespace

@@ +53,5 @@
> +function runTest() {
> +  SpecialPowers.setAllAppsLaunchable(true);
> +  
> +  var manifestURL = "http://test/tests/dom/apps/tests/file_manifest.json";
> +  

here too

@@ +56,5 @@
> +  var manifestURL = "http://test/tests/dom/apps/tests/file_manifest.json";
> +  
> +  SpecialPowers.autoConfirmAppInstall(continueTest);
> +  yield undefined;
> +  

here too

@@ +63,5 @@
> +  request.onsuccess = continueTest;
> +  yield undefined;
> +  var initialAppsCount = request.result.length;
> +  info("Starting with " + initialAppsCount + " apps installed.");
> +  

here too

@@ +68,5 @@
> +  var request = navigator.mozApps.install(manifestURL, { });
> +  request.onerror = cbError;
> +  request.onsuccess = continueTest;
> +  yield undefined;
> +  

here too

@@ +72,5 @@
> +  
> +  var app = request.result;
> +  ok(app, "App is non-null");
> +  is(app.manifestURL, manifestURL, "App manifest url is correct.");
> +  

here too. Which editor are you using? :P

@@ +77,5 @@
> +  request = navigator.mozApps.mgmt.getAll();
> +  request.onerror = cbError;
> +  request.onsuccess = continueTest;
> +  yield undefined;
> +  is(request.result.length, initialAppsCount+1, "Correct number of apps.");

so, we actually want to end up with the same set of apps with started with, to prevent issues with other tests running afterward from the same profile.
That means that you need to uninstall your app, and here check that you get back initialAppsCount.

Comment 7

[Ben Francis [:benfrancis]]

Attachment: Patch to add support for Web Manifest MIME type

Oops, it seems I have come to rely too heavily on the Gaia linter :)

Thanks for the review Fabrice, I've addressed your comments and attached a new patch.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8509465 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

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

Thanks Ben!

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 8509465 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

Could someone check this in for me?

Comment 10

[Carsten Book [:Tomcat]]

Hi, could you provide a try run for this? Thanks!

Comment 11

[[:fabrice] Fabrice Desré]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=05c0f300cbb1

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/e19ef031a3fd

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Marcos Caceres [:marcosc]]

Just confirming, not having a content type or having that standard JSON content type should also work fine, right? The content type in the manifest spec is just to be polite (as with all MIME types): it's not a requirement that web manifests be served with it - or that the UA demand it.

Comment 15

[Ben Francis [:benfrancis]]

Thanks for pushing the patch Fabrice.

Hmm yes, I see it says the manifest is just "recommended" to use the MIME type.

As I understand it currently the mozApps.install() method in Gecko requires that the MIME type be set if the manifest is from a different origin than the one calling the API. I guess this is because the mozApps API is designed to allow an app to install itself, installation from another origin requires a slightly higher bar to be met.

For the use case of Gaia's system browser installing an app from a web manifest, these origins will always be different and so the MIME type will always be required.

Fabrice, I think at one point you said you wouldn't be opposed to dropping this requirement in the mozApps API? I wonder how we want to handle this in the long term. Perhaps in a future where Web Manifests eventually completely take over from Open Web App Manifests, the install() method would move to the more privileged Apps Management API?

Comment 16

[Marcos Caceres [:marcosc]]

(In reply to Ben Francis [:benfrancis] from comment #15)
> Thanks for pushing the patch Fabrice.
> 
> Hmm yes, I see it says the manifest is just "recommended" to use the MIME
> type.

Correct. It's a "should", not a "must". 
 
> As I understand it currently the mozApps.install() method in Gecko requires
> that the MIME type be set if the manifest is from a different origin than
> the one calling the API. I guess this is because the mozApps API is designed
> to allow an app to install itself, installation from another origin requires
> a slightly higher bar to be met.
> 
> For the use case of Gaia's system browser installing an app from a web
> manifest, these origins will always be different and so the MIME type will
> always be required.
> 
> Fabrice, I think at one point you said you wouldn't be opposed to dropping
> this requirement in the mozApps API?

I agree. It's a good idea to keep it for the mozApps API or else it would conflict. 

> I wonder how we want to handle this in
> the long term. Perhaps in a future where Web Manifests eventually completely
> take over from Open Web App Manifests, the install() method would move to
> the more privileged Apps Management API?

Having the MIME for the proprietary type makes sense for legacy reasons. It's a clean way to distinguish between the standardized one and the proprietary format.

Comment 17

[[:fabrice] Fabrice Desré]

I never understood why we needed these mime type constraints. They are just annoying for developers, and don't give any guarantee on the content we get anyway. If you install the app from its own origin, nothing will tell you if it's an OWA manifest or a Web Manifest.

Comment 18

[Marcos Caceres [:marcosc]]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> I never understood why we needed these mime type constraints. They are just
> annoying for developers, and don't give any guarantee on the content we get
> anyway. If you install the app from its own origin, nothing will tell you if
> it's an OWA manifest or a Web Manifest.

Exactly :D This is why it's optional in the W3C spec. The point in just to follow the "be conservative in what you send, be liberal in what you receive" principle of the interwebs.

Comment 19

[Ben Francis [:benfrancis]]

OK cool, I filed bug 1091786