We'll use the category from install_data (bug 759906).

install_data? Don't see that mentioned here: https://developer.mozilla.org/en/Apps/Apps_JavaScript_API/navigator.mozApps.install Are the docs out of date here?

(In reply to Jason Smith [:jsmith] from comment #1) > install_data? Don't see that mentioned here: > > https://developer.mozilla.org/en/Apps/Apps_JavaScript_API/navigator.mozApps. > install > > Are the docs out of date here? This doc is correct right now for Nightly. But per anant, michaelhanson and fabrice on #openwebapps 6/1/12, install_data will be added back in. Not sure if there's a bug on it yet.

Marco Castelluccio, nice work! I assume you mean the *Categories=* line, per http://standards.freedesktop.org/menu-spec/menu-spec-1.0.html#category-registry without a Categories= line, web apps show up in the "Lost & Found" submenu of the "Kickoff" start menu on the KDE Plasma Desktop (but it's great they show up at all); I mentioned this in https://wiki.mozilla.org/Marketplace/Mozillian_Preview

(In reply to skierpage from comment #3) > without a Categories= line, web apps show up in the "Lost & Found" submenu > of the "Kickoff" start menu on the KDE Plasma Desktop (but it's great they > show up at all); And on GNOME 2, they appear in the Other menu below Office. It might be nice to include a default category and prepend install_data before ‘Network’ which would put it into the Internet menu.

The web apps should behave exactly like native applications. If a developer writes, for example, an eBook reader, he doesn't want his application to appear in the Internet menu.

(In reply to Marco Castelluccio from comment #5) > The web apps should behave exactly like native applications. If a developer > writes, for example, an eBook reader, he doesn't want his application to > appear in the Internet menu. If the eBook reader is strictly offline, then I agree.

I'm requesting only feedback as I can't build and test the patch because of a problem with Angle.

I did my best to translate the Marketplace categories to freedesktop categories: http://standards.freedesktop.org/menu-spec/latest/apa.html

Comment on attachment 641805 [details] [diff] [review] Add support for the categories parameter Sorry, this isn't obsolete :)

(In reply to Marco Castelluccio from comment #8) > Created attachment 641812 [details] [diff] [review] > Use the categories parameter on Linux > > I did my best to translate the Marketplace categories to freedesktop > categories: http://standards.freedesktop.org/menu-spec/latest/apa.html May I suggest adding 'Network;' to "social"?

Comment on attachment 641812 [details] [diff] [review] Use the categories parameter on Linux Review of attachment 641812 [details] [diff] [review]: ----------------------------------------------------------------- Can you add two comments in this functions for easier reference: one above the function (javadoc style) that points to the FreeDesktop's list of categories, and another inside the function just saying that a trailing ; is needed. (to avoid someone accidentally removing it in the future) ::: browser/modules/WebappsInstaller.jsm @@ +776,5 @@ > }, > > + _translateCategories: function() { > + let translations = { > + "books-reference": "Education;Literature", you can indent this block just one indent further from the declaration, like: let translations = { "books-ref": "...", "business": "...", };

Comment on attachment 641805 [details] [diff] [review] Add support for the categories parameter This looks good to me. I'm leaving the feedback req for Ian for him to take a look as well. Fabrice would need to review it. Fabrice, if you are too busy to review this let me know and I'll see if Jst can look at it, or I can do a thorough review if you take a simple look over the approach. fwiw: this patch is adding support for another field in the install parameters called categories, so that mozApps.install can be called as: mozApps.install("foo.manifest", {receipt: xxx, categories: ["Office", "Chat"]});

I've just fixed a nit and tested the patch.

I've addressed your review comments. (In reply to Yann Brelière from comment #10) > May I suggest adding 'Network;' to "social"? Well, the social category is probably the worst translation I did :) However, I don't know if a social application should always be a network application. Getting feedback from some Linux package maintainers and from some Marketplace folks could be extremely useful.

Flagging dev-doc-needed. We should update the API spec to indicate how we handle categories.

Comment on attachment 642148 [details] [diff] [review] Add support for the categories parameter Review of attachment 642148 [details] [diff] [review]: ----------------------------------------------------------------- r- not for code issues, but because I need to be convinced that we need to expose that to web content on the Application object. ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl @@ +14,5 @@ > { > readonly attribute jsval manifest; > readonly attribute DOMString manifestURL; > readonly attribute jsval receipts; /* an array of strings */ > + readonly attribute jsval categories; /* an array of strings */ Do we really want to expose that to web content? @@ +92,5 @@ > * > * @param manifestUrl : the URL of the webapps manifest. > * @param parameters : A structure with optional information. > * { receipts: ... } will be used to specify the payment receipts for this installation. > + * { categories: ... } will be used to specify the categories of the webapp. nit: { receipts: .... categories: ... } @@ +119,5 @@ > * > * @param packageUrl : the URL of the webapps manifest. > * @param parameters : A structure with optional information. > * { receipts: ... } will be used to specify the payment receipts for this installation. > + * { categories: ... } will be used to specify the categories of the webapp. ditto

(In reply to Fabrice Desré [:fabrice] from comment #16) > Comment on attachment 642148 [details] [diff] [review] > Add support for the categories parameter > > Review of attachment 642148 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- not for code issues, but because I need to be convinced that we need to > expose that to web content on the Application object. For example a web dashboard could show the installed applications grouped by category. Or search the installed applications that are in a particular category.

I do agree with Fabrice that we should refrain from adding explicit getters for fields that have arbitrary meanings. The intention for install_data is precisely to store information like this, but we shouldn't be creating an index of allowed properties. The application/dashboard can already retrieve the install_data via getSelf() - atleast when bug 760339 is fixed - so an additional property on the application object is not required. Marco, do you feel like tackling bug 760339? :)

I will hasten to add that the "receipts" property in install_data /should/ be treated specially, so a fix for bug 760339 will retain the AppObject.receipts property, but we should not add special properties for anything else (you can always get to it via AppObject.installData.categories, for example).

(In reply to Anant Narayanan [:anant] from comment #18) > The application/dashboard can already retrieve the install_data via > getSelf() - atleast when bug 760339 is fixed - so an additional property on > the application object is not required. Great, I didn't know that bug :) > Marco, do you feel like tackling bug 760339? :) I'll work on it, but I won't be at home for nearly a week.

I've just added a check to add the "Categories" field to the desktop entry file only when at least a category is passed. Even if unrelated, I've removed an unused variable.

Comment on attachment 649489 [details] [diff] [review] Use the categories parameter on Linux Review of attachment 649489 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/webapps/WebappsInstaller.jsm @@ +829,5 @@ > + > + let categories = this._translateCategories(); > + if (categories.length > 0) > + writer.setString("Desktop Entry", "Categories", categories); > + just use "if (categories)"

Comment on attachment 649485 [details] [diff] [review] Add support for the categories parameter Review of attachment 649485 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +522,5 @@ > .createInstance(Ci.nsIZipReader); > try { > zipReader.open(zipFile); > if (!zipReader.hasEntry("manifest.webapp")) { > throw "No manifest.webapp found."; You also need to update _cloneAppObject

Typo fix, carrying forward r+.

Updated to the current tree, carrying forward r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6452db09473 https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b948b3565a

https://hg.mozilla.org/mozilla-central/rev/c6452db09473 https://hg.mozilla.org/mozilla-central/rev/b8b948b3565a