Closed
Bug 1457786
Opened 6 years ago
Closed 6 years ago
Remove question-32.png
Categories
(Toolkit :: Themes, enhancement, P3)
Toolkit
Themes
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)
3.43 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
https://searchfox.org/mozilla-central/search?q=question-32.png&case=true®exp=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•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Assignee: nobody → 1991manish.kumar
Comment 1•6 years ago
|
||
'help.svg' doesn't exist here now. https://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/icons
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 2•6 years ago
|
||
It's under the shared/ folder: https://searchfox.org/mozilla-central/source/toolkit/themes/shared/icons
Flags: needinfo?(ntim.bugs)
Comment 3•6 years ago
|
||
So Should I add 'help.svg' in /toolkit/themes/osx/global/icons folder also?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 4•6 years 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•6 years ago
|
||
Sorry, I mean: chrome://global/skin/icons/help.svg
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Attachment #8985534 -
Flags: review?(dao+bmo)
Comment 7•6 years ago
|
||
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-
Comment 8•6 years ago
|
||
Please review
Attachment #8985534 -
Attachment is obsolete: true
Attachment #8990865 -
Flags: review?(dao+bmo)
Comment 9•6 years ago
|
||
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•6 years ago
|
Summary: Remove question-32.png and replace usage with help.svg → Remove question-32.png
Updated•6 years ago
|
Assignee: 1991manish.kumar → nobody
Reporter | ||
Updated•6 years ago
|
Mentor: ntim.bugs
Keywords: good-first-bug
Assignee | ||
Comment 10•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
Attachment #9006672 -
Flags: review+
Reporter | ||
Updated•6 years ago
|
Attachment #9006672 -
Flags: review+ → review?(dao+bmo)
Reporter | ||
Comment 15•6 years ago
|
||
You'll also need to remove the entry for question-32.png in toolkit/themes/osx/global/jar.mn
Assignee | ||
Updated•6 years ago
|
Attachment #9006672 -
Attachment is obsolete: true
Attachment #9006672 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 16•6 years ago
|
||
Attachment #9006692 -
Flags: review+
Assignee | ||
Comment 17•6 years 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•6 years 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•6 years ago
|
Attachment #9006692 -
Flags: review+
Assignee | ||
Comment 19•6 years ago
|
||
Patch file regenerated after histedit roll-ing commits together.
Flags: needinfo?(jason)
Attachment #9006940 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Reporter | ||
Updated•6 years ago
|
Attachment #9006940 -
Flags: review+ → review?(dao+bmo)
Comment 21•6 years ago
|
||
Comment on attachment 9006940 [details] [diff] [review] bug-1457786-2.patch Looks good, thanks!
Attachment #9006940 -
Flags: review?(dao+bmo) → review+
Updated•6 years ago
|
Attachment #8990865 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9006692 -
Attachment is obsolete: true
Comment 22•6 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d74c0e9d7add Remove question-32.png. r=dao
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d74c0e9d7add
Status: NEW → RESOLVED
Closed: 6 years 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.
Description
•