Closed Bug 1004111 Opened 6 years ago Closed 6 years ago

Move remaining themes/*/devtools images into themes/shared/devtools/images

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: bgrins, Assigned: johannes)

References

Details

(Whiteboard: [good first bug][mentor=bgrins])

Attachments

(1 file, 3 obsolete files)

We may as well move the remaining icons all at once.

If we use `hg rename browser/themes/linux/devtools/whatever.png browser/themes/shared/devtools/images/whatever.png` it will keep history on them.

Then we can remove the icons from the remaining folders, and update browser/themes/*/jar.mn to replace (devtools/whatever.png) with (../shared/devtools/images/whatever.png).
Attached patch moveimages.patch (obsolete) — Splinter Review
Attachment #8415989 - Flags: review?(bgrinstead)
Assignee: nobody → johannes
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=bgrins]
Comment on attachment 8415989 [details] [diff] [review]
moveimages.patch

Review of attachment 8415989 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/jar.mn
@@ +306,5 @@
>    skin/classic/browser/devtools/filters.svg                 (../shared/devtools/filters.svg)
>    skin/classic/browser/devtools/controls.png                (../shared/devtools/images/controls.png)
>    skin/classic/browser/devtools/controls@2x.png             (../shared/devtools/images/controls@2x.png)
>  * skin/classic/browser/devtools/widgets.css                 (devtools/widgets.css)
> +  skin/classic/browser/devtools/commandline-icon.png        (../shared/devtools/commandline-icon.png)

Shouldn't all of the references in these files be to ../shared/devtools/images/* instead of ../shared/devtools/*?
Attachment #8415989 - Flags: review?(bgrinstead)
Attached patch moveimagesV2.patch (obsolete) — Splinter Review
Of course! I just noticed and was building in the wrong directory before.
Attachment #8415989 - Attachment is obsolete: true
Attachment #8416015 - Flags: review?(bgrinstead)
Comment on attachment 8416015 [details] [diff] [review]
moveimagesV2.patch

Review of attachment 8416015 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  Looks like this will get rid of 44 files and ~200K from the tree.  Let's wait for a green try push before marking checkin-needed just to make sure that it builds on each OS: https://tbpl.mozilla.org/?tree=Try&rev=af1a6b5acaa8.
Attachment #8416015 - Flags: review?(bgrinstead) → review+
Attached patch moveimagesV3.patch (obsolete) — Splinter Review
There were some errors in the tests. This should be fixed now. https://tbpl.mozilla.org/?tree=Try&rev=9ed4afe936fd
Attachment #8416015 - Attachment is obsolete: true
Attachment #8416383 - Flags: review?(bgrinstead)
Comment on attachment 8416383 [details] [diff] [review]
moveimagesV3.patch

Review of attachment 8416383 [details] [diff] [review]:
-----------------------------------------------------------------

There is something odd about the way this patch is formatted - linux/jar.mn and windows/jar.mn seem to show up twice (the second changes appear to be the ones that were made to fix the original try push).  It looks like it works, but it makes it harder to review and figure out what is going on.  Assuming you are using Mercurial Queues and have the item on your queue you should be able `hg qref` then upload the patch .hg/patches/my-item where my-item is the name of the item on the queue.
Attachment #8416383 - Flags: review?(bgrinstead)
That is indeed quite strange. I have no idea how this got into the patch. I tried again and this time is looks like beeing ok. This is the exact version I pushed to the try servers before.
Attachment #8416383 - Attachment is obsolete: true
Attachment #8416501 - Flags: review?(bgrinstead)
Comment on attachment 8416501 [details] [diff] [review]
moveimagesV4.patch

Review of attachment 8416501 [details] [diff] [review]:
-----------------------------------------------------------------

Went ahead and did a fresh push with the latest patch just to be sure, and it is all green: https://tbpl.mozilla.org/?tree=Try&rev=d7822e5cee52.  Going to mark checkin-needed
Attachment #8416501 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/479dbf1077ad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.