Closed Bug 1181602 Opened 8 years ago Closed 8 years ago

Display pin badge for Firefox Apps

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benfrancis, Assigned: apastor)

References

Details

(Whiteboard: [systemsfe])

Attachments

(4 files)

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.
Assignee: nobody → apastor
Comment on attachment 8631620 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Ben, could you please review this? Thanks!
Attachment #8631620 - Flags: review?(bfrancis)
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 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.
Attachment #8631620 - Flags: review?(bfrancis) → review+
master: https://github.com/mozilla-b2g/gaia/commit/0585f7ec07d0ed12f5bb53e051d01d33b91288cb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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 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!
Attachment #8633367 - Flags: review?(sfoster)
Comment on attachment 8633367 [details] [review]
[gaia] albertopq:1181602-icon-apps > mozilla-b2g:master

Looks good to me.
Attachment #8633367 - Flags: review?(sfoster) → review+
master: https://github.com/mozilla-b2g/gaia/commit/32f1eba8fded1873561cde59dd87301c86ee646e
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.