1436445 - Use correct icon for Firefox Snap

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[Mike Kaply [:mkaply]]

From Ubuntu:

I just noticed the icon in the desktop file is actually using the image installed from the deb.  This leaves firefox with a missing icon if you only have the snap installed.  Here's a patch that will use the icon included in your snap.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1436445 - Use icon included in the snap.

Review commit: https://reviewboard.mozilla.org/r/218494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218494/

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8949088 [details]
Bug 1436445 - Use icon included in the snap.

https://reviewboard.mozilla.org/r/218494/#review225134

We have control over firefox.desktop. I'd prefer to manually edit [1], instead of using a sed command. I'll put the patch up, to save some time. 

[1] https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/taskcluster/docker/firefox-snap/firefox.desktop#157

Comment 3

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1436445 - Use icon included in the snap

Review commit: https://reviewboard.mozilla.org/r/219414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219414/

Comment 4

[Johan Lorenzo [:jlorenzo]]

I see we're using the 128x128 icon, which is okay as a first fix. Do you know how can we let Gnome decide which icon to use (depending on the screen resolution), Ken?

Comment 5

[Olivier Tilloy]

AFAIK we cannot specify a generic icon name and let GNOME pick the right resolution, because the icon has to be embedded inside the snap, and the icon lookup algorithm (https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#icon_lookup) isn't snap-aware.

We used that 128×128 icon because that's the highest resolution available in the snap, but if the build had a 512×512 icon we could use that, and assume the shell can downscale properly where needed.

Comment 6

[Johan Lorenzo [:jlorenzo]]

Thanks for the background! We don't appear to have a 512*512 icon versioned in-tree. I just asked it in bug 1437478.

I see we have a 256*256 icon[1], but it's not included in the final Linux build. Bug 1437478 asks about it too.

[1] https://searchfox.org/mozilla-central/source/browser/branding/official/default256.png

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 8950154 [details]
Bug 1436445 - Use icon included in the snap

https://reviewboard.mozilla.org/r/219414/#review225222

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61d324b24f98
Use icon included in the snap r=mkaply

Comment 9

[Johan Lorenzo [:jlorenzo]]

Landed on beta at https://hg.mozilla.org/releases/mozilla-beta/rev/ac2e53fc9c8e43cf5e88d59eeba83bd865db75e5

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/61d324b24f98

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 8950154 [details]
Bug 1436445 - Use icon included in the snap

Approval Request Comment
[Feature/Bug causing the regression]: Needed for proper icon in our Snap
[User impact if declined]: incorrect icon
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only applies to our Snap file.
[String changes made/needed]:

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8950154 [details]
Bug 1436445 - Use icon included in the snap

Looks fine to uplift. This should land in time for beta 12.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Wait, this already landed on beta (Comment 9)?   Well, ok. 
Is there anything left to do here?

Comment 14

[Mike Kaply [:mkaply]]

I completely missed that this landed. Sorry about that.