1181602 - Display pin badge for Firefox Apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

Built-in Gaia apps currently show the default icon, they should show the app icon. I think we can get this from the AppWindow object if necessary.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Comment 2

[Alberto Pastor [:albertopq]]

Comment on attachment 8631620 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Ben, could you please review this? Thanks!

Comment 3

[Ben Francis [:benfrancis]]

Comment on attachment 8631620 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

I haven't fully reviewed this yet but I quickly tested it out. I noticed that for Facebook I see a tiny icon and for Twitter I just see the default icon, not sure what's going on there?

Comment 4

[Ben Francis [:benfrancis]]

Comment on attachment 8631620 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

This is great. r+ with some comments...

I don't think the logic for converting from Firefox App Manifest to W3C App Manifest icons format should live in AppWindow, it should probably live in IconsHelper (or possibly ManifestHelper or WebManifestHelper).

Also, it would be better to use a consistent format for a processed manifest.icons rather than trying to support both a string and Set.

We're not testing for absolute app:// URLs but I'm not sure if that's a problem in practice.

Comment 5

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/0585f7ec07d0ed12f5bb53e051d01d33b91288cb

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

reverted for causing bug 1183348.

master: https://github.com/mozilla-b2g/gaia/commit/a4278e41c08fa619edbd6537b6182d46dc475a4e

(In reply to Ben Francis [:benfrancis] from comment #4)
> Also, it would be better to use a consistent format for a processed
> manifest.icons rather than trying to support both a string and Set.

I agree with this in theory, but it seems weird that manifest.icons will mean something else in the system app then it will everywhere else. Perhaps the icons helper can generate a new processed icons field rather than overwrite manifest.icons. In any case, if we do overwrite, we will have to make sure we fix all the places in the system app that use manifest.icons (app_window, attention_window, and cards_helper it seems).

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

sam has a suggestion for fixing cards_helper:
https://bugzilla.mozilla.org/show_bug.cgi?id=1183199#c5

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: iconshelper_convertToWebManifestIcons_fix.patch

Alberto, bug 1183199 seems to come down to the _convertToWebManifestIcons call here that replaces app.manifest.icons. We should and will update Card to just use the new icons helpers, but I think the safest bet to get this re-landed without growing scope is just to avoid this side-effect.

Comment 9

[Kevin Grandon :kgrandon]

The backout here caused a unit test to perma-fail. Disabling for now, so please re-enable this test when re-lands.

https://github.com/mozilla-b2g/gaia/commit/7676b68b4d32ed13243eeb719188847121bd5611

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Comment 11

[Alberto Pastor [:albertopq]]

Comment on attachment 8633367 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Sorry for the inconvenience. Overriding the real manifest wasn't the intention.
Thanks for catching this!

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8633367 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Looks good to me.

Comment 13

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/32f1eba8fded1873561cde59dd87301c86ee646e

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1181602-icon-apps > mozilla-b2g:master