Closed Bug 1075716 Opened 10 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: benfrancis, Assigned: benfrancis)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 3 obsolete files)

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
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.
Assignee: nobody → bfrancis
Status: NEW → ASSIGNED
Attachment #8506095 - Flags: feedback?(fabrice)
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
Attachment #8506095 - Flags: feedback?(fabrice) → feedback+
Blocks: 1003876
Thanks Fabrice, here's a revised patch for review.
Attachment #8506095 - Attachment is obsolete: true
Attachment #8506993 - Flags: review?(fabrice)
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.
Attachment #8506993 - Flags: review?(fabrice)
Added mochitest. I've never written one before so no idea if some of that code was unnecessary/I missed things.
Attachment #8506993 - Attachment is obsolete: true
Attachment #8508196 - Flags: review?(fabrice)
Whiteboard: [systemsfe]
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.
Attachment #8508196 - Flags: review?(fabrice) → feedback+
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.
Attachment #8508196 - Attachment is obsolete: true
Attachment #8509465 - Flags: review?(fabrice)
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!
Attachment #8509465 - Flags: review?(fabrice) → review+
Comment on attachment 8509465 [details] [diff] [review]
Patch to add support for Web Manifest MIME type

Could someone check this in for me?
Attachment #8509465 - Flags: checkin?
Keywords: checkin-needed
Target Milestone: --- → 2.1 S7 (24Oct)
Hi, could you provide a try run for this? Thanks!
Flags: needinfo?(bfrancis)
Keywords: checkin-needed
Attachment #8509465 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/e19ef031a3fd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
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?
Flags: needinfo?(bfrancis) → needinfo?(fabrice)
(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.
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.
Flags: needinfo?(fabrice)
(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.
OK cool, I filed bug 1091786
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: