Get rid of background-noise-toolbar.png and it's references (in the devtools/ directory only)

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ntim, Assigned: alexharris6)

Tracking

Trunk
Firefox 32
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ntim][lang=css])

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

5 years ago
No description provided.
Reporter

Comment 1

5 years ago
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
Whiteboard: [good first bug][mentor=ntim][lang=css]
Reporter

Comment 2

5 years ago
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.
Flags: needinfo?(bgrinstead)
Flags: needinfo?(alexharris6)
(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
Flags: needinfo?(bgrinstead)
Assignee

Comment 4

5 years ago
(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.
Flags: needinfo?(alexharris6)
Assignee

Comment 5

5 years ago
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.
Flags: needinfo?(ntim007)
Reporter

Comment 6

5 years ago
(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).
Assignee: nobody → alexharris6
Status: NEW → ASSIGNED
Flags: needinfo?(ntim007)
Assignee

Comment 7

5 years ago
Ok, great. Thanks Tim.
Reporter

Comment 8

5 years ago
Adding dependency to bug 1009145 as it'll cause patch conflicts if bug 1009145 lands first or vice versa.
Depends on: 1009145
Assignee

Comment 9

5 years ago
Posted patch Patch for bug 1011173 (obsolete) — Splinter Review
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.
Attachment #8424278 - Flags: review?(ntim007)
Reporter

Comment 10

5 years ago
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()
Attachment #8424278 - Flags: review?(ntim007) → feedback+
Assignee

Comment 11

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

Comment 13

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

Comment 14

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

Comment 15

5 years ago
Posted patch Patch for bug 1011173 (obsolete) — Splinter Review
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.
Attachment #8424278 - Attachment is obsolete: true
Attachment #8425083 - Flags: review?(ntim007)
Reporter

Comment 16

5 years ago
Comment on attachment 8425083 [details] [diff] [review]
Patch for bug 1011173

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

Everything seems to look good :)
Attachment #8425083 - Flags: review?(ntim007)
Attachment #8425083 - Flags: review?(bgrinstead)
Attachment #8425083 - Flags: review+
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 */
Attachment #8425083 - Flags: review?(bgrinstead) → review-
Assignee

Comment 18

5 years ago
Posted patch fixed patch (obsolete) — Splinter Review
Ok, this patch adds the missing Toolbar css comments, and should resolve the merge conflict.
Attachment #8425083 - Attachment is obsolete: true
Attachment #8425622 - Flags: review?(bgrinstead)
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
Attachment #8425622 - Flags: review?(bgrinstead)
Assignee

Comment 20

5 years ago
Posted patch Patch for bug 1011173 (obsolete) — Splinter Review
Ok, another attempt at the patch. Removed the accidental jar.mn changes. Hopefully this one will successfully merge with tip.
Attachment #8425622 - Attachment is obsolete: true
Attachment #8425705 - Flags: review?(bgrinstead)
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
Attachment #8425705 - Flags: review?(bgrinstead)
Reporter

Comment 22

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

Comment 23

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

Comment 24

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

Comment 26

5 years ago
Posted patch Patch for bug 1011173 (obsolete) — Splinter Review
Another try at fixing the merge issue.
Attachment #8425705 - Attachment is obsolete: true
Attachment #8426367 - Flags: review?(bgrinstead)
Attachment #8426367 - Flags: review?(bgrinstead) → review+
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
Attachment #8426367 - Flags: review+ → review-
Assignee

Comment 28

5 years ago
Posted patch Patch for bug 1011173 (obsolete) — Splinter Review
Adds * to inspector.css in osx jar file, in order to fix merge issue
Attachment #8426367 - Attachment is obsolete: true
Attachment #8426545 - Flags: review?(bgrinstead)
Reporter

Comment 30

5 years ago
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 :)
Attachment #8426545 - Flags: review?(bgrinstead) → review-
Assignee

Comment 31

5 years ago
For the millionth time. This one now removes the new reference to the png from widgets css.
Attachment #8426545 - Attachment is obsolete: true
Attachment #8427345 - Flags: review?(bgrinstead)
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
Attachment #8427345 - Flags: review?(bgrinstead) → review-
Assignee

Comment 34

5 years ago
Not sure what happened there, this one includes all three jar.mn files.
Attachment #8427345 - Attachment is obsolete: true
Attachment #8427434 - Flags: review?(bgrinstead)
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
Attachment #8427434 - Flags: review?(bgrinstead) → review+
Reporter

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/59e2d582157a
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=ntim][lang=css] → [good first bug][mentor=ntim][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/59e2d582157a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=ntim][lang=css][fixed-in-fx-team] → [good first bug][mentor=ntim][lang=css]
Target Milestone: --- → Firefox 32

Updated

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