Closed Bug 1457786 Opened 6 years ago Closed 6 years ago

Remove question-32.png

Categories

(Toolkit :: Themes, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ntim, Assigned: jason, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 4 obsolete files)

https://searchfox.org/mozilla-central/search?q=question-32.png&case=true&regexp=false&path=

The file and its reference in jar.mn needs to be removed.

The reference in aboutPrivateBrowsing.js needs to be replaced with help.svg
Priority: -- → P3
Assignee: nobody → 1991manish.kumar
'help.svg' doesn't exist here now.

https://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/icons
Flags: needinfo?(ntim.bugs)
It's under the shared/ folder: https://searchfox.org/mozilla-central/source/toolkit/themes/shared/icons
Flags: needinfo?(ntim.bugs)
So Should I add 'help.svg' in /toolkit/themes/osx/global/icons folder also?
Flags: needinfo?(ntim.bugs)
(In reply to Manish Kumar [:manishkk] from comment #3)
> So Should I add 'help.svg' in /toolkit/themes/osx/global/icons folder also?

No, you don't need to add any file, you can just refer to "chrome://global/skin/help.svg".
Flags: needinfo?(ntim.bugs)
Sorry, I mean: chrome://global/skin/icons/help.svg
Attached patch Patch_Bug1457786 (obsolete) — Splinter Review
Please review
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Attachment #8985534 - Flags: review?(dao+bmo)
Comment on attachment 8985534 [details] [diff] [review]
Patch_Bug1457786

So, question-32.png doesn't exist on Windows and Linux, so this was already broken there and nobody cared. Let's just remove FAVICON_QUESTION and the related code.
Attachment #8985534 - Flags: review?(dao+bmo) → review-
Attached patch Patch_BugV21457786 (obsolete) — Splinter Review
Please review
Attachment #8985534 - Attachment is obsolete: true
Attachment #8990865 - Flags: review?(dao+bmo)
Comment on attachment 8990865 [details] [diff] [review]
Patch_BugV21457786

Still need to actually remove question-32.png. I believe you can also remove the "favicon" id from the link element in aboutPrivateBrowsing.xhtml.
Attachment #8990865 - Flags: review?(dao+bmo) → review-
Summary: Remove question-32.png and replace usage with help.svg → Remove question-32.png
Assignee: 1991manish.kumar → nobody
Mentor: ntim.bugs
Keywords: good-first-bug
May I take on this bug? I'm looking to get started as a contributor and this looks pretty simple and good practice for committing a bug fix. Thanks!
(In reply to Jason Chapin from comment #10)
> May I take on this bug? I'm looking to get started as a contributor and this
> looks pretty simple and good practice for committing a bug fix. Thanks!

Hi Jason, I've assigned the bug to you :)

If you haven't set up a build yet, I'd recommend looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1486630#c3

Once you've done with the setup, you'll need to edit Manish's patch to also cover the changes from comment 9.

Please let me know if you have any questions.
Assignee: nobody → jason
Thanks Tim! Can you provide some advice on how to edit the earlier patch from Manish? I'm made my changes (edited one file, deleted one image file) not sure how to proceed.
Flags: needinfo?(ntim.bugs)
(In reply to Jason Chapin from comment #12)
> Thanks Tim! Can you provide some advice on how to edit the earlier patch
> from Manish? I'm made my changes (edited one file, deleted one image file)
> not sure how to proceed.

You can also recreate the patch, no need to reuse Manish's :) For that, you can commit your changes by doing `hg commit -m "Bug 1457786 - Remove question-32.png".

Once you've done that you should be able to export your revision as a patch file using `hg export`, or using Phabricator.
Flags: needinfo?(ntim.bugs)
Attached patch bug-1457786.patch (obsolete) — Splinter Review
Attachment #9006672 - Flags: review+
Attachment #9006672 - Flags: review+ → review?(dao+bmo)
You'll also need to remove the entry for question-32.png in toolkit/themes/osx/global/jar.mn
Attachment #9006672 - Attachment is obsolete: true
Attachment #9006672 - Flags: review?(dao+bmo)
Attached patch bug-1457786.patch (obsolete) — Splinter Review
Attachment #9006692 - Flags: review+
Thanks for noticing that other reference to question-32.png. I hope I did this right, made a new commit for the changed file, then did another export containing both commit infos. I set the previously attempted patch as obsolete and created a new one.
(In reply to Jason Chapin from comment #17)
> Thanks for noticing that other reference to question-32.png. I hope I did
> this right, made a new commit for the changed file, then did another export
> containing both commit infos. I set the previously attempted patch as
> obsolete and created a new one.

Can you combine both commits by using `hg histedit` then using the roll command ?
Flags: needinfo?(jason)
Attachment #9006692 - Flags: review+
Patch file regenerated after histedit roll-ing commits together.
Flags: needinfo?(jason)
Attachment #9006940 - Flags: review+
Flags: needinfo?(ntim.bugs)
Attachment #9006940 - Flags: review+ → review?(dao+bmo)
Thanks, looks good to me.
Flags: needinfo?(ntim.bugs)
Comment on attachment 9006940 [details] [diff] [review]
bug-1457786-2.patch

Looks good, thanks!
Attachment #9006940 - Flags: review?(dao+bmo) → review+
Attachment #8990865 - Attachment is obsolete: true
Attachment #9006692 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d74c0e9d7add
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: