Closed Bug 1436445 Opened 2 years ago Closed 2 years ago

Use correct icon for Firefox Snap

Categories

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

defect
Not set

Tracking

(firefox59 fixed, firefox60 fixed)

RESOLVED FIXED
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: snappy
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
Attachment #8949088 - Flags: review?(jlorenzo)
Attachment #8949088 - Attachment is obsolete: true
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?
Flags: needinfo?(ken.vandine)
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.
Depends on: 1437478
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 on attachment 8950154 [details]
Bug 1436445 - Use icon included in the snap

https://reviewboard.mozilla.org/r/219414/#review225222
Attachment #8950154 - Flags: review?(mozilla) → review+
Pushed by jlorenzo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61d324b24f98
Use icon included in the snap r=mkaply
https://hg.mozilla.org/mozilla-central/rev/61d324b24f98
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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]:
Attachment #8950154 - Flags: approval-mozilla-beta?
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.
Attachment #8950154 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Wait, this already landed on beta (Comment 9)?   Well, ok. 
Is there anything left to do here?
Flags: needinfo?(mozilla)
Flags: needinfo?(jlorenzo)
I completely missed that this landed. Sorry about that.
Flags: needinfo?(mozilla)
Flags: needinfo?(ken.vandine)
Flags: needinfo?(jlorenzo)
You need to log in before you can comment on or make changes to this bug.