1148442 - Add support for the W3C icons property to <gaia-grid>

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Components, defect)

Comment 1

[Ben Francis [:benfrancis]]

A more general fix.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] benfrancis:1148442 > mozilla-b2g:master

Comment 3

[Ben Francis [:benfrancis]]

Comment on attachment 8584763 [details] [review]
[gaia] benfrancis:1148442 > mozilla-b2g:master

Kevin, I'm implementing this as a helper in Gaia for the short term until we figure out where we're going with Gecko but I think we can move most of this to Gecko in future.

The WebManifestHelper and associated tests are just being moved from the bookmarks app to /shared and I'm re-using that helper in GaiaGrid to parse icons from W3C web app manifests when detected.

Let me know what you think, thanks!

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8584763 [details] [review]
[gaia] benfrancis:1148442 > mozilla-b2g:master

I left some comments and there are a few test failures that need to be addressed.

My bigger question is about the work necessary to move this into gecko, as it looks like that's something we want to do? Are there bugs filed yet and where are they?

It just seems to me as if we're adding engineering debt to gaia with no clear path out of it. I would prefer to wait if we are going to handle this from gecko unless there is a big benefit to land this. What is the story with android and web manifests?

Comment 5

[Ben Francis [:benfrancis]]

Thanks Kevin,

I've addressed your review comments and rebased.

Fabrice advised implementing this in Gaia rather than Gecko while the Web Manifest implementation in Gecko gets smoothed out.

The plan is roughly:
* Marcos is working on a W3C web app manifest implementation in Gecko which fetches and sanitises a manifest in bug 1083410, but this is currently undergoing a re-factor and is not hooked up to the apps API and is therefore not currently exposed to Gaia in any way. This is the long term solution but is separate to the short term solution with rudimentary Gecko support in the Apps API like short_name.
* In future we can probably move the icon parsing logic to Gecko in bug 1087978 but a) This will need hooking up to Marcos' Manifest obtainer and b) it will only work for apps which are already installed. In order to get rid of the Gaia WebManifestHelper entirely we'll need an API to let content request the icon for an app which isn't installed yet.
* Depending on the outcome of bug 1091786 we may need a new more privileged app installation API which installs an app via a document URL rather than a manifest URL and bypasses the content type check, but that's still under discussion.
* Desktop and Android implementations will presumably hook straight into Marcos' manifest obtainer, they don't need APIs exposed to content to do everything.

In summary, once this patch lands we'll have completed the 2.2 Web Manifest specification with an initial implementation split across Gecko and Gaia which works as a proof of concept. We have plans for a cleaner implementation in Gecko in future but there are still lots of things to figure out, which are tied into broader architectural changes regarding the 3.0 apps architecture. I also hope we can introduce UX changes for 3.0 because the current UI kind of sucks!

I hope that helps explain the situation.

Comment 6

[Ben Francis [:benfrancis]]

Comment on attachment 8584763 [details] [review]
[gaia] benfrancis:1148442 > mozilla-b2g:master

Treeherder looks green and shiny!

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8584763 [details] [review]
[gaia] benfrancis:1148442 > mozilla-b2g:master

Looks good to me. Thanks!

Comment 8

[Ben Francis [:benfrancis]]

Thanks Kevin!

Comment 9

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 10

[Kevin Grandon :kgrandon]

It looks like this landed manually? https://github.com/mozilla-b2g/gaia/commit/fb7414fa6f5dbb898adc5bd2bbd9fb75df0d0054