Remove question-32.png

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: ntim, Assigned: jason, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
mozilla64
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 months ago
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

Updated

11 months ago
Priority: -- → P3
Assignee: nobody → 1991manish.kumar
(Reporter)

Comment 2

10 months ago
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)
(Reporter)

Comment 4

10 months ago
(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)
(Reporter)

Comment 5

10 months ago
Sorry, I mean: chrome://global/skin/icons/help.svg
Posted patch Patch_Bug1457786 (obsolete) — Splinter Review
Please review
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

9 months ago
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-
Posted 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-
(Reporter)

Updated

9 months ago
Summary: Remove question-32.png and replace usage with help.svg → Remove question-32.png
Assignee: 1991manish.kumar → nobody
(Reporter)

Updated

7 months ago
Mentor: ntim.bugs
Keywords: good-first-bug
(Assignee)

Comment 10

7 months ago
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!
(Reporter)

Comment 11

7 months ago
(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
(Assignee)

Comment 12

7 months ago
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)
(Reporter)

Comment 13

7 months ago
(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)
(Assignee)

Comment 14

7 months ago
Posted patch bug-1457786.patch (obsolete) — Splinter Review
Attachment #9006672 - Flags: review+
(Reporter)

Updated

7 months ago
Attachment #9006672 - Flags: review+ → review?(dao+bmo)
(Reporter)

Comment 15

7 months ago
You'll also need to remove the entry for question-32.png in toolkit/themes/osx/global/jar.mn
(Assignee)

Updated

7 months ago
Attachment #9006672 - Attachment is obsolete: true
Attachment #9006672 - Flags: review?(dao+bmo)
(Assignee)

Comment 16

7 months ago
Posted patch bug-1457786.patch (obsolete) — Splinter Review
Attachment #9006692 - Flags: review+
(Assignee)

Comment 17

7 months ago
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.
(Reporter)

Comment 18

7 months ago
(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)
(Reporter)

Updated

7 months ago
Attachment #9006692 - Flags: review+
(Assignee)

Comment 19

7 months ago
Patch file regenerated after histedit roll-ing commits together.
Flags: needinfo?(jason)
Attachment #9006940 - Flags: review+
(Assignee)

Updated

7 months ago
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

7 months ago
Attachment #9006940 - Flags: review+ → review?(dao+bmo)
(Reporter)

Comment 20

7 months ago
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

Comment 23

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d74c0e9d7add
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.