Unused chrome://browser/skin/sync-128.png file

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: kevin.kwong.chip, Mentored)

Tracking

({good-first-bug})

53 Branch
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
Bug 1296767 removed browser/base/content/sync/customize.css which contained the only referenced to chrome://browser/skin/sync-128.png at https://hg.mozilla.org/integration/autoland/rev/a30baab71ce2#l6.15
Mentor: eoger
Keywords: good-first-bug
Priority: -- → P3

Comment 1

2 years ago
Hey Florian,
 I would like to work on this bug. If anyone can guide and mentor me on how to do this.
This would be my first bug.
Reporter

Comment 2

2 years ago
(In reply to indraniltiwary from comment #1)
> Hey Florian,
>  I would like to work on this bug. If anyone can guide and mentor me on how
> to do this.
> This would be my first bug.

Hi. Mark indicated Edouard will be mentoring this bug, I'll let him guide you.
Flags: needinfo?(eoger)
Hi Indranil,

This bug is a fairly simple one, you just have to be thorough :)
Here's the work to do:

- There are 3 different sync-128.png images, one per platform (osx, linux, windows) located in browser/themes/[platform]/sync-128.png. Delete them.
- We reference these images in our build system using JAR Manifests (completely optional read - https://gecko.readthedocs.io/en/latest/build/buildsystem/jar-manifests.html), remove these references in browser/themes/[platform]/jar.mn

Once you're done, rebuild your browser and take it for a spin, just to make sure everything's OK (it should be, because these files are not used anywhere now).

Good luck!
Flags: needinfo?(eoger)
Assignee

Comment 4

2 years ago
Hi Edouard, i'd like to also try this bug out as my good first bug. If I built mozilla following these instructions found here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation
would that be the correct branch version to fix this bug on?
Flags: needinfo?(eoger)
Hi Kevin, I think you got beat by Indranil. Let's give him a week to work on this bug, if he doesn't the bug is all yours.
Flags: needinfo?(eoger)
Assignee

Comment 6

2 years ago
Posted patch proposed patch (obsolete) — Splinter Review
hey Edouard, I've made changes locally and produced a diff file. Please let me know if/how to take the patch further, thanks.
Flags: needinfo?(eoger)
Attachment #8838814 - Flags: review?(eoger)
Comment on attachment 8838814 [details] [diff] [review]
proposed patch

Review of attachment 8838814 [details] [diff] [review]:
-----------------------------------------------------------------

Hello Kevin,

Thank you for your patch!
Now that you have removed the references to these sync-128.png images, you have to remove the files themselves.
As a reminder, they are located in browser/themes/[platform]/sync-128.png.

Please amend your patch with theses changes and change the commit message to something like:
Bug 1335409 - Remove unused sync-128.png image. r=eoger
Attachment #8838814 - Flags: review?(eoger)
Assignee: nobody → kevin.kwong.chip
Status: NEW → ASSIGNED
Flags: needinfo?(eoger)
Assignee

Comment 8

2 years ago
Posted patch kevin-proposed-change.patch (obsolete) — Splinter Review
Thanks for getting back Edouard, I've made your requested changes and amended them to the previous patch file.
Attachment #8839160 - Flags: review?(eoger)
Comment on attachment 8839160 [details] [diff] [review]
kevin-proposed-change.patch

Review of attachment 8839160 [details] [diff] [review]:
-----------------------------------------------------------------

I think you attached the wrong patch, this one is for bug 1331166 :)
Attachment #8839160 - Flags: review?(eoger)
Assignee

Comment 10

2 years ago
really sorry about that, here it is.
Attachment #8839631 - Flags: review?(eoger)
Comment on attachment 8839631 [details] [diff] [review]
revised-change.patch

Review of attachment 8839631 [details] [diff] [review]:
-----------------------------------------------------------------

r+! Thank you Kevin. I'll ask for a sheriff to land your patch on your behalf.
Attachment #8839631 - Flags: review?(eoger) → review+
Attachment #8838814 - Attachment is obsolete: true
Attachment #8839160 - Attachment is obsolete: true

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec25fc08dcf
Remove unused sync-128.png image. r=eoger
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dec25fc08dcf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.