This bug is about removing [0] and it's references [1]. Note that some removals of the references should land soon (bug 1009145 and bug 973691) [0] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/background-noise-toolbar.png [1] : http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fthemes%2Fshared%2Fdevtools%2F&string=background-noise-toolbar.png

Alex, are you interested in this bug ? And Brian, can I mentor this bug ? I'm not a peer, but I did work on lots of theme bugs already.

(In reply to Tim Nguyen [:ntim] from comment #2) > Alex, are you interested in this bug ? > > And Brian, can I mentor this bug ? I'm not a peer, but I did work on lots of > theme bugs already. Sure, you can mentor it - I will do a final review before landing

(In reply to Tim Nguyen [:ntim] from comment #2) > Alex, are you interested in this bug ? > > And Brian, can I mentor this bug ? I'm not a peer, but I did work on lots of > theme bugs already. Hi Tim, yes I am interested in this. I'll review your initial comments and see what questions I have.

Am I correct in understanding that I should only be worrying about removing references to the .png from within /browser/themes/shared/devtools/ as the cross-reference search indicates? If so, I will need to remove all instances that show up in those search results, except for the one in /browser/themes/shared/devtools/debugger.inc.css, which is removed in bug 1009145. Bug 973691 removes it from /browser/devtools/sourceeditor/codemirror/mozilla.css, which is outside of the theme folders I am working in.

(In reply to alexharris6 from comment #5) > Am I correct in understanding that I should only be worrying about removing > references to the .png from within /browser/themes/shared/devtools/ as the > cross-reference search indicates? > > If so, I will need to remove all instances that show up in those search > results, except for the one in > /browser/themes/shared/devtools/debugger.inc.css, which is removed in bug > 1009145. > > Bug 973691 removes it from > /browser/devtools/sourceeditor/codemirror/mozilla.css, which is outside of > the theme folders I am working in. Yes, you'll also need to remove the references in the jar.mn files (browser/themes/(linux|osx|windows)/jar.mn).

Ok, great. Thanks Tim.

Adding dependency to bug 1009145 as it'll cause patch conflicts if bug 1009145 lands first or vice versa.

Here is my initial attempt at this patch. A few issues came up during the course of making the changes. I made my best guess as to proper course of action, but please correct me if I am wrong. 1. In instances where the background image was the only css property in that particular class, i removed the entire class reference. 2. I left alone all other properties in each class. A majority of these are text colors, which I am assuming should be the same despite the different background. 3. I removed the entire "background" property each time the image was included, although I am now thinking I should have only removed the url(...), and left the color. Perhaps changing the property to background-color. Ok, those are the only issues I can recall.

Comment on attachment 8424278 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8424278 [details] [diff] [review]: ----------------------------------------------------------------- This is going in the right direction. But it causes some regressions though. For the properties like : background: url(background-noise-toolbar.png), <COLOR>; I would change them into : background-color: <color>; Also, can you rebase your patch on top of bug 1009145 ? Otherwise, there will be patch conflicts. ::: browser/themes/shared/devtools/commandline.inc.css @@ -9,5 @@ > #developer-toolbar { > -moz-appearance: none; > padding: 0; > min-height: 32px; > - background-image: url(devtools/background-noise-toolbar.png), linear-gradient(#303840, #2d3640); For this property, you might just want to use : background-color: #343C45; Our goal is to flat-out the DevTools UI, so, we should get rid of the linear-gradient()

Hi Tim, Ok. I think I understand what changes need to be made to my changes. I deleted the old patch from my queue and am starting a new one. I did `hg pull` and `hg update` thinking that would include the changes from 1009145, but when i search the log with `hg log -r 1009145` it says it is unknown. Is there a way to rebase on top of a patch that hasn't been committed yet?

(In reply to alexharris6 from comment #11) > Hi Tim, > > Ok. I think I understand what changes need to be made to my changes. I > deleted the old patch from my queue and am starting a new one. I did `hg > pull` and `hg update` thinking that would include the changes from 1009145, > but when i search the log with `hg log -r 1009145` it says it is unknown. Is > there a way to rebase on top of a patch that hasn't been committed yet? In this case you would want to do `hg log -r 79d74ac58388`, where the the ID is coming from: https://bugzilla.mozilla.org/show_bug.cgi?id=1009145#c28

(In reply to alexharris6 from comment #11) > Hi Tim, > > Ok. I think I understand what changes need to be made to my changes. I > deleted the old patch from my queue and am starting a new one. I did `hg > pull` and `hg update` thinking that would include the changes from 1009145, > but when i search the log with `hg log -r 1009145` it says it is unknown. Is > there a way to rebase on top of a patch that hasn't been committed yet? You can also import patches as shown in this video : http://codefirefox.com/video/import-patch

Note that bug 1009145 is now in mozilla-central, so you should have less trouble now.

Here is another attempt at this patch. I fixed the CSS as discussed in the thread. The only thing I wasn't sure of was whether to use the `background` or `background-color` property for cases where the only was color was RGBA(). I went with `background-color`. I've seen it done both ways, and I think it renders the same either way. I also deleted the actual .png in this patch, which I think I omitted from the previous. Also, I believe I correctly rebased this, so it should merge correctly, but let me know if there are further issues.

Comment on attachment 8425083 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8425083 [details] [diff] [review]: ----------------------------------------------------------------- Everything seems to look good :)

Comment on attachment 8425083 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8425083 [details] [diff] [review]: ----------------------------------------------------------------- Please rebase this - I'm getting conflicts on the jar.mn files ::: browser/themes/shared/devtools/commandline.inc.css @@ +9,5 @@ > #developer-toolbar { > -moz-appearance: none; > padding: 0; > min-height: 32px; > + background-color: #343C45; Add a comment at the end of this line so we can keep track of the hard coded color: /* Toolbars */ ::: browser/themes/shared/devtools/splitview.css @@ +17,5 @@ > background-position: center center; > } > > .theme-dark .splitview-nav-container { > + background-color: #343c45; Add a comment at the end of this line so we can keep track of the hard coded color: /* Toolbars */

Ok, this patch adds the missing Toolbar css comments, and should resolve the merge conflict.

Comment on attachment 8425622 [details] [diff] [review] fixed patch Review of attachment 8425622 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/jar.mn @@ -396,5 @@ > skin/classic/browser/devtools/responsive-background.png (../shared/devtools/images/responsive-background.png) > skin/classic/browser/devtools/toggle-tools.png (../shared/devtools/images/toggle-tools.png) > skin/classic/browser/devtools/dock-bottom@2x.png (../shared/devtools/images/dock-bottom@2x.png) > skin/classic/browser/devtools/dock-side@2x.png (../shared/devtools/images/dock-side@2x.png) > -* skin/classic/browser/devtools/inspector.css (devtools/inspector.css) This line looks like it leaked over from bug 1011624 in all the jar.mn files

Ok, another attempt at the patch. Removed the accidental jar.mn changes. Hopefully this one will successfully merge with tip.

Comment on attachment 8425705 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8425705 [details] [diff] [review]: ----------------------------------------------------------------- There are rejects on the jar.mn files - can you pull latest changes and resubmit the patch? Also please add r=bgrins in the commit message

Alex, you might want to check here : http://hg.mozilla.org/integration/fx-team/log/tip/browser/themes/windows/jar.mn for possible conflicting bugs. Some are not on mozilla-central, but you can either wait, or manually import the patch from the bug.

Tim, I assume those are patches that are waiting to be checked in to mozilla central from fx-team? Is there an easy way to determine which might be conflicting, or does it just involve combing through them?

(In reply to alexharris6 from comment #23) > Tim, I assume those are patches that are waiting to be checked in to mozilla > central from fx-team? Is there an easy way to determine which might be > conflicting, or does it just involve combing through them? Yes, they are first checked-in to fx-team or mozilla-inbound, and then they are pushed to mozilla-central. Well, there's no real easy way, you'll need to check the latest patches landing, and see which patch has changes at similar places, and import that patch.

(In reply to alexharris6 from comment #23) > Tim, I assume those are patches that are waiting to be checked in to mozilla > central from fx-team? Is there an easy way to determine which might be > conflicting, or does it just involve combing through them? I don't think you should generally have too many issues with this. It is mainly an issue on files that change a lot, like the jar.mn files, or if the bug depends on another that just got resolved. I think for this bug it would be safe to just pull the latest changes and qref the patch.

Another try at fixing the merge issue.

Comment on attachment 8426367 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8426367 [details] [diff] [review]: ----------------------------------------------------------------- Actually, there is still an inspector.css change in one of the jar.mn files ::: browser/themes/osx/jar.mn @@ -396,5 @@ > skin/classic/browser/devtools/responsive-background.png (../shared/devtools/images/responsive-background.png) > skin/classic/browser/devtools/toggle-tools.png (../shared/devtools/images/toggle-tools.png) > skin/classic/browser/devtools/dock-bottom@2x.png (../shared/devtools/images/dock-bottom@2x.png) > skin/classic/browser/devtools/dock-side@2x.png (../shared/devtools/images/dock-side@2x.png) > -* skin/classic/browser/devtools/inspector.css (devtools/inspector.css) This line should be unchanged

Adds * to inspector.css in osx jar file, in order to fix merge issue

Looks good to me, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b379b5e7e171

Comment on attachment 8426545 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8426545 [details] [diff] [review]: ----------------------------------------------------------------- Bug 993014 just added a reference to the file :/ Can you get rid of it too ? Thanks :)

For the millionth time. This one now removes the new reference to the png from widgets css.

Sorry ! :)

Comment on attachment 8427345 [details] [diff] [review] Remove background-noise-toolbar.png and its references. Review of attachment 8427345 [details] [diff] [review]: ----------------------------------------------------------------- Now it applies cleanly ... BUT ... it is missing the osx and linux jar.mn files

Not sure what happened there, this one includes all three jar.mn files.

Comment on attachment 8427434 [details] [diff] [review] Patch for bug 1011173 Review of attachment 8427434 [details] [diff] [review]: ----------------------------------------------------------------- Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ea9b66cc18b5