Closed
Bug 1011173
Opened 11 years ago
Closed 11 years ago
Get rid of background-noise-toolbar.png and it's references (in the devtools/ directory only)
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: ntim, Assigned: alexharris6)
References
Details
(Whiteboard: [good first bug][mentor=ntim][lang=css])
Attachments
(1 file, 7 obsolete files)
18.21 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 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•11 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)
Comment 3•11 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.
Sure, you can mentor it - I will do a final review before landing
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 4•11 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•11 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•11 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•11 years ago
|
||
Ok, great. Thanks Tim.
Reporter | ||
Comment 8•11 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•11 years ago
|
||
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•11 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•11 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?
Comment 12•11 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?
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•11 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•11 years ago
|
||
Note that bug 1009145 is now in mozilla-central, so you should have less trouble now.
Assignee | ||
Comment 15•11 years ago
|
||
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•11 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 17•11 years ago
|
||
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•11 years ago
|
||
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 19•11 years ago
|
||
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•11 years ago
|
||
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 21•11 years ago
|
||
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•11 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•11 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•11 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.
Comment 25•11 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?
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•11 years ago
|
||
Another try at fixing the merge issue.
Attachment #8425705 -
Attachment is obsolete: true
Attachment #8426367 -
Flags: review?(bgrinstead)
Updated•11 years ago
|
Attachment #8426367 -
Flags: review?(bgrinstead) → review+
Comment 27•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 29•11 years ago
|
||
Looks good to me, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b379b5e7e171
Reporter | ||
Comment 30•11 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•11 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)
Comment 32•11 years ago
|
||
Sorry ! :)
Comment 33•11 years ago
|
||
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•11 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 35•11 years ago
|
||
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•11 years ago
|
Keywords: checkin-needed
Comment 36•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=ntim][lang=css] → [good first bug][mentor=ntim][lang=css][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 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•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•