Closed Bug 1335409 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Sync, defect, P3)

53 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

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

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

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
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.
(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)
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)
Attached 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)
Attached 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)
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
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
https://hg.mozilla.org/mozilla-central/rev/dec25fc08dcf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.