Closed Bug 1208121 Opened 4 years ago Closed 4 years ago

Remove some unused images in the browser theme

Categories

(Firefox :: Theme, defect)

defect
Not set

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)

browser/themes/shared/magnifier.png
browser/themes/shared/magnifier@2x.png
browser/themes/windows/tabview/tabview-inverted.png
Summary: Remove some unused images in the browser themes → Remove some unused images in the browser theme
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??
Assignee: nobody → chowdhuryshaif
Status: NEW → ASSIGNED
Attached patch bug1208121.diff (obsolete) — Splinter Review
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 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)
Attachment #8670930 - Attachment is obsolete: true
Attached patch removed unused images from theme (obsolete) — Splinter Review
Had attached diff file by mistake last time.
Please review the updated patch.
Flags: needinfo?(chowdhuryshaif)
Attachment #8671997 - Flags: review?(ntim.bugs)
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+
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.
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.
Attached patch bug1208121.diff (obsolete) — Splinter Review
Attachment #8671997 - Attachment is obsolete: true
Attachment #8672206 - Flags: review?(ntim.bugs)
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+
Attached patch bug1208121.diff (obsolete) — Splinter Review
Added commit messege..
Attachment #8672206 - Attachment is obsolete: true
Attachment #8672265 - Flags: review?(ntim.bugs)
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+
Attached patch bug1208121.diffSplinter Review
Attachment #8672265 - Attachment is obsolete: true
Attachment #8672286 - Flags: review?(ntim.bugs)
Attachment #8672286 - Flags: review?(ntim.bugs) → review+
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
Sheriffs, please see comment #8 for my permission to review this patch.
https://hg.mozilla.org/mozilla-central/rev/3eea09a56dc9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.