Closed
Bug 1208121
Opened 9 years ago
Closed 9 years ago
Remove some unused images in the browser theme
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: ntim, Assigned: chowdhuryshaif, Mentored)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
4.27 KB,
patch
|
ntim
:
review+
|
Details | Diff | Splinter Review |
browser/themes/shared/magnifier.png browser/themes/shared/magnifier@2x.png browser/themes/windows/tabview/tabview-inverted.png
Reporter | ||
Updated•9 years ago
|
Summary: Remove some unused images in the browser themes → Remove some unused images in the browser theme
Reporter | ||
Comment 1•9 years ago
|
||
To fix this bug, you need to remove the files mentioned above, and remove their reference in browser/themes/shared/jar.inc.mn and browser/themes/windows/jar.mn.
Mentor: ntim.bugs
Whiteboard: [good first bug]
So, I am working on this bug. could someone please assign me??
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → chowdhuryshaif
Status: NEW → ASSIGNED
Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: [Describe test coverage new/current, TreeHerder]: [Risks and why]: [String/UUID change made/needed]:
Attachment #8670930 -
Flags: review?(ntim.bugs)
Comment 4•9 years ago
|
||
Comment on attachment 8670930 [details] [diff] [review] bug1208121.diff Hi Shaif, the patch that you attached is blank. You might want to join #introduction on the Mozilla IRC server to get help in creating the patch. (Or maybe you just need to run `hg qrefresh` if you're using Mercurial queues)
Flags: needinfo?(chowdhuryshaif)
Attachment #8670930 -
Flags: review?(ntim.bugs)
Updated•9 years ago
|
Attachment #8670930 -
Attachment is obsolete: true
Had attached diff file by mistake last time. Please review the updated patch.
Flags: needinfo?(chowdhuryshaif)
Attachment #8671997 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8671997 [details] [diff] [review] removed unused images from theme Review of attachment 8671997 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch ! I've added a few comments. Also, can you add a commit message and commit info for this patch ? See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions ::: browser/themes/shared/jar.inc.mn @@ +65,5 @@ > skin/classic/browser/identity-mixed-active-loaded.svg (../shared/identity-block/identity-mixed-active-loaded.svg) > skin/classic/browser/info.svg (../shared/info.svg) > skin/classic/browser/tracking-protection-16.svg (../shared/identity-block/tracking-protection-16.svg) > skin/classic/browser/tracking-protection-disabled-16.svg (../shared/identity-block/tracking-protection-disabled-16.svg) > + nit: Please remove this blank line. ::: browser/themes/windows/jar.mn @@ +251,5 @@ > skin/classic/browser/tabview/grain.png (tabview/grain.png) > skin/classic/browser/tabview/search.png (tabview/search.png) > skin/classic/browser/tabview/stack-expander.png (tabview/stack-expander.png) > skin/classic/browser/tabview/tabview.png (tabview/tabview.png) > + skin/classic/browser/tabview/tabview.css (tabview/tabview.css) Please restore the indentation on this line to 2 spaces (as it was before).
Attachment #8671997 -
Flags: review?(ntim.bugs) → feedback+
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8671997 [details] [diff] [review] removed unused images from theme Review of attachment 8671997 [details] [diff] [review]: ----------------------------------------------------------------- Also, it seems you only removed the references to the files, and forgot to remove the files themselves from the source code.
Comment 8•9 years ago
|
||
Tim, you can handle the review for this patch. I am fine with this patch getting r=ntim when Tim believes the patch is sufficient.
Attachment #8671997 -
Attachment is obsolete: true
Attachment #8672206 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8672206 [details] [diff] [review] bug1208121.diff Review of attachment 8672206 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me ! Can you make sure your patch has commit info as mentioned in comment #6 ? It's needed for your patch to be checked in. If you have trouble adding commit info to your patch, feel free to ask in #introduction on IRC. (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8) > Tim, you can handle the review for this patch. I am fine with this patch > getting r=ntim when Tim believes the patch is sufficient. Thanks !
Attachment #8672206 -
Flags: review?(ntim.bugs) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Added commit messege..
Attachment #8672206 -
Attachment is obsolete: true
Attachment #8672265 -
Flags: review?(ntim.bugs)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8672265 [details] [diff] [review] bug1208121.diff Review of attachment 8672265 [details] [diff] [review]: ----------------------------------------------------------------- The commit message should normally be formatted like this as the doc mentions: Bug 1208121 - Remove some unused images in the browser theme. r=ntim Also, this patch still misses commit user info (so we can identify who made the patch), there should be a line between `# HG changeset patch` and `# Parent (...)` that looks like this: # Your Name <email@address.com>
Attachment #8672265 -
Flags: review?(ntim.bugs) → review+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8672265 -
Attachment is obsolete: true
Attachment #8672286 -
Flags: review?(ntim.bugs)
Reporter | ||
Updated•9 years ago
|
Attachment #8672286 -
Flags: review?(ntim.bugs) → review+
Reporter | ||
Comment 14•9 years ago
|
||
Thanks for the patch ! I'm marking this for checkin, once the patch lands in mozilla-central, it'll appear on next day's Nightly. Let me know if you'd like another bug to work on.
Keywords: checkin-needed
Reporter | ||
Comment 15•9 years ago
|
||
Sheriffs, please see comment #8 for my permission to review this patch.
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3eea09a56dc9
Keywords: checkin-needed
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3eea09a56dc9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in
before you can comment on or make changes to this bug.
Description
•