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

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: bgrins, Assigned: johannes)

Tracking

Trunk
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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).
Assignee

Comment 1

5 years ago
Posted patch moveimages.patch (obsolete) — Splinter Review
Attachment #8415989 - Flags: review?(bgrinstead)
Reporter

Updated

5 years ago
Assignee: nobody → johannes
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=bgrins]
Reporter

Comment 2

5 years ago
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)
Assignee

Comment 3

5 years ago
Posted 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)
Reporter

Comment 4

5 years ago
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+
Assignee

Comment 5

5 years ago
Posted 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)
Reporter

Comment 6

5 years ago
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)
Assignee

Comment 7

5 years ago
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)
Reporter

Comment 8

5 years ago
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+
Reporter

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/479dbf1077ad
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.