Closed
Bug 772363
Opened 12 years ago
Closed 12 years ago
Implement installation API for packaged apps
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla16
People
(Reporter: sicking, Assigned: fabrice)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: [qa!])
Attachments
(1 file)
(deleted),
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
Implement a separate mozApps.install function which rather than provide a URL to a manifest, provides a URL to a packaged app, which contains manifest as well as other app resources.
Assignee | ||
Comment 1•12 years ago
|
||
I'm moving here Part 1 of bug 769350.
> @@ +412,5 @@
> > + let ostream = FileUtils.openSafeFileOutputStream(zipFile);
> > + NetUtil.asyncCopy(aInput, ostream, function (aResult) {
> > + if (!Components.isSuccessCode(aResult)) {
> > + // We failed to save the zip.
> > + cleanup("NETWORK_ERROR");
>
> This is not really a NETWORK_ERROR
Yep, I changed it to DOWNLOAD_ERROR.
> @@ +436,5 @@
> > + .createInstance(Ci.nsIZipReader);
> > + try {
> > + zipReader.open(zipFile);
> > + if (!zipReader.hasEntry("manifest.webapp")) {
> > + throw "No manifest.webapp found.";
>
> Don't you want to call cleanup("INVALID_MANIFEST") here instead?
No, I'm throwing there so the cleanup is called in the catch() and we still close the reader properly in the finally block.
Assignee: nobody → fabrice
Attachment #640816 -
Flags: review?(21)
Updated•12 years ago
|
Component: General → DOM: Mozilla Extensions
Product: Web Apps → Core
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 2•12 years ago
|
||
What is the risk in using the same mozApps.install() function, and have it take either a manifest URL or a package URL? In other words, what is the rationale for a separate installPackage() function?
Attachment #640816 -
Flags: review?(21) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
Shouldn't we address API concerns in comment 2 before landing this? Has this gone through an API review yet on dev.webapps? I know I saw discussion on dev.webapps about packaged apps, but not sure if I explicitly saw the API part of the discussion (although I could have missed it while reading it).
Comment 5•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
>
> Shouldn't we address API concerns in comment 2 before landing this? Has this
> gone through an API review yet on dev.webapps? I know I saw discussion on
> dev.webapps about packaged apps, but not sure if I explicitly saw the API
> part of the discussion (although I could have missed it while reading it).
Also to add to this - We should also address how this affects each platform (desktop, android, and b2g) and know if there are any other pieces that need to be implemented on each side as followups.
Comment 6•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/484d2214d81a
>
> Shouldn't we address API concerns in comment 2 before landing this? Has this
> gone through an API review yet on dev.webapps?
It's often useful to discuss API changes/additions on dev.webapps, but there is no requirement that we do so, and discussions in that forum would not constitute an API review in any case. API reviews are the responsibility of the folks in charge of standardizing the APIs (namely: Jonas and Anant), and it's up to them to determine whether, how, and when such reviews are conducted.
Assignee | ||
Comment 7•12 years ago
|
||
I pushed a followup to fix a test failure :
https://hg.mozilla.org/integration/mozilla-inbound/rev/633f88dda03b
Comment 8•12 years ago
|
||
I think we can just discuss the API change in this bug, since Jonas opened it.
I'm not against adding an installPackage() function, I'm only trying to determine the reasoning behind it as it seems like the existing install() function would be adequate. If Jonas believes
there's a good reason, that's great, it would be nice to know what it is.
Fabrice, have you tested your change on Desktop/Android or does this only work on B2G for now?
Reporter | ||
Comment 9•12 years ago
|
||
If we have them as the same function we have to detect somehow what the URI is pointing at. I can think of the following solutions:
* Use the mime type of the response.
* Use the extension of the file-part of the URL.
* Detect if the response body looks like a JSON file, or like a zip file (or
whatever other package format we end up standardizing on)
Using mime types suck. Developers hate having to configure their server to send correct mime types, and many times they can't or don't know how to.
Using extensions is always iffy since in theory file names shouldn't mean anything. And it might make it harder to serve the file from some servers which would want to use URLs like: http://myserver.com/manifestServer.php?id=42231
Looking at the response body always feels scary. It might limit the type of packaging formats we can support in the future if some of them doesn't have a prefixed which can be uniquely distinguishable from whatever a JSON file can start with.
In general, given that the behavior of the two functions are so different it seems worth it to me to be explicit about what the function does.
The only downside that I can see of having two functions is if a store keeps a database of all apps and stores a download-URL for each app. With the two separate functions they would have to store the URL differently for packaged apps, and apps that are hosted by the developer.
However I suspect stores will store those differently anyway since it's hosting the packaged apps and likely will be managing them very differently.
Comment 10•12 years ago
|
||
Thanks for the explanation, Jonas!
We do however already require the manifest to be served with a Content-Type of "application/x-web-app-manifest+json". Is it your intention that we should get rid of that requirement at some point?
The original purpose was mostly for security - it is a way to prove that the developer had control over the server that served a particular app. This is important in a one-app-per-origin world, since in shared hosting environments (geocities, or university sites like uni.edu/~user) we don't want subdirectories to represent an app on behalf of the top-level domain.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Anant Narayanan [:anant] from comment #10)
> Thanks for the explanation, Jonas!
>
> We do however already require the manifest to be served with a Content-Type
> of "application/x-web-app-manifest+json". Is it your intention that we
> should get rid of that requirement at some point?
Note that this is not implemented currently - and I'm reluctant to make that a MUST, for the reasons Jonas explained.
> The original purpose was mostly for security - it is a way to prove that the
> developer had control over the server that served a particular app. This is
> important in a one-app-per-origin world, since in shared hosting
> environments (geocities, or university sites like uni.edu/~user) we don't
> want subdirectories to represent an app on behalf of the top-level domain.
Here also, things should change. We'd like to allow multiple apps per origin, each app being identified by its manifest url instead of its origin.
Comment 12•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #11)
> > We do however already require the manifest to be served with a Content-Type
> > of "application/x-web-app-manifest+json". Is it your intention that we
> > should get rid of that requirement at some point?
>
> Note that this is not implemented currently - and I'm reluctant to make that
> a MUST, for the reasons Jonas explained.
This is already implemented in the Marketplace at-least, if not in Gecko.
> > The original purpose was mostly for security - it is a way to prove that the
> > developer had control over the server that served a particular app. This is
> > important in a one-app-per-origin world, since in shared hosting
> > environments (geocities, or university sites like uni.edu/~user) we don't
> > want subdirectories to represent an app on behalf of the top-level domain.
>
> Here also, things should change. We'd like to allow multiple apps per
> origin, each app being identified by its manifest url instead of its origin.
Sure, we should be working towards lifting that restriction. We should still be cautious though. For instance, if we only use the manifest URL to identify an app, I would be able to create a manifest for mozilla.github.com/awesomeapp at anantn.github.com/foobar.
In essence, it allows anyone to create an app on behalf of someone else, and increases the burden on the reviewers in our Marketplace to ensure that whoever uploaded the manifest does indeed own the app itself (which in itself can be hard to verify). No doubt, this is critically important for paid apps.
Comment 13•12 years ago
|
||
(In reply to Anant Narayanan [:anant] from comment #12)
> Sure, we should be working towards lifting that restriction. We should still
> be cautious though. For instance, if we only use the manifest URL to
> identify an app, I would be able to create a manifest for
> mozilla.github.com/awesomeapp at anantn.github.com/foobar.
Correction, it won't work on github because of the subdomain (which makes it a different origin), but does on shared hosting environments like example.org/~user1 and example.org/~user2, or places like geocities, where user accounts are subfolders rather than subdomains.
It is also dangerous on places like Dropbox, Google Docs, etc. where user-generated content has publicly available URLs, which will allow any user to be able create an "app" for the top-level domain and potentially charge for it.
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/484d2214d81a
https://hg.mozilla.org/mozilla-central/rev/633f88dda03b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment 15•12 years ago
|
||
Reference to the bug where we are discussing safely being able to relax the MIME type requirement: bug 712045
Comment 16•12 years ago
|
||
This sounds like this is testable, but I need some pointers on how to understand how to test this. Questions:
- Could you provide a description of the API function at a high-level (I know I've seen discussion on this, just want to make sure I understand it correctly)
- Do we have any automated tests for this? If not, can we get a bug on file to get automated test support for this?
- Could you provide an example of an app demonstrating the use of the API function?
Whiteboard: [qa+]
Updated•12 years ago
|
QA Contact: jsmith
Comment 17•12 years ago
|
||
Could someone address my questions in comment 16? I'd really like to help make sure this works across each major apps platform piece (desktop, android, and b2g).
Updated•12 years ago
|
Blocks: sign-packaged-apps
Updated•12 years ago
|
QA Contact: jsmith
Whiteboard: [qa+] → [qa-]
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM: Apps
Updated•12 years ago
|
QA Contact: jsmith
Whiteboard: [qa-] → [qa+]
Comment 18•12 years ago
|
||
Verified on 8/6/2012 build using Fabrice's test app here - http://people.mozilla.org/~fdesre/openwebapps/test.html that the implementation is there at a proof of concept level. More testing and followups will be filed as needed.
Important Notes:
- The implementation for packaged apps only appears to work on Firefox OS. Desktop will fail to install the packaged app (errors out). Android will install the app, but with the incorrect icon, and also fail to launch the app.
- There's more testing to be done here. I'll look into this.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Updated•12 years ago
|
Blocks: Apps-Dev-Doc-Needed
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•