Closed Bug 1011624 Opened 11 years ago Closed 10 years ago

Move themes/*/devtools/inspector.css into themes/shared/devtools/inspector.css

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: bgrins, Assigned: alexharris6)

References

Details

(Whiteboard: [mentor=bgrins][lang=css])

Attachments

(1 file, 3 obsolete files)

Whiteboard: [mentor=bgrins][lang=css]
Ok so my workflow will be: 1. diff each of the themes' inspector.css files, copy any unique CSS to a single file 2. delete old files, except the one which I will rename/move to /shared 3. update theme jar files (paths, and stars if needed) 4. add any necessary %if conditions to new /shared file
(In reply to alexharris6 from comment #1) > Ok so my workflow will be: > > 1. diff each of the themes' inspector.css files, copy any unique CSS to a > single file > 2. delete old files, except the one which I will rename/move to /shared > 3. update theme jar files (paths, and stars if needed) > 4. add any necessary %if conditions to new /shared file Yeah, and as we've seen in some other patches sometimes the changes across OSes are not needed or unintentional (side effect of trying to keep 3 things in sync). But if it seems important go ahead and use the %if
Attached patch Patch for bug 1011624 (obsolete) (deleted) — Splinter Review
This patch will: - Move contents of various theme's inspector.css files into single shared file - Add %if conditions to CSS for various OS - Modify paths and add *s to jar files
Attachment #8424246 - Flags: review?(bgrinstead)
Assignee: nobody → alexharris6
Status: NEW → ASSIGNED
Comment on attachment 8424246 [details] [diff] [review] Patch for bug 1011624 Review of attachment 8424246 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/inspector.css @@ +38,5 @@ > max-width: 20px !important; > -moz-padding-end: 5px; > + %if defined(XP_WIN) > + -moz-padding-end: 6px; > + %endif I don't think this %if is needed. 6px and 5px don't make a pretty big difference. Especially when it comes to right padding.
Comment on attachment 8424246 [details] [diff] [review] Patch for bug 1011624 Review of attachment 8424246 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/inspector.css @@ +2,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +%if defined(XP_MACOSX) I don't think it matters either way, but I've seen %ifdef XP_MACOSX used more often in the css files. So let's go with that just to stay consistent (though this syntax seems good when checking for two different platforms). @@ +8,1 @@ > %include ../shared.inc I know these were in the original file, but I'm pretty sure that we don't need this %include or %filter line anymore @@ +38,5 @@ > max-width: 20px !important; > -moz-padding-end: 5px; > + %if defined(XP_WIN) > + -moz-padding-end: 6px; > + %endif Removing the condition is fine with me. I guess go with 5px to match the other 2 platforms.
Attachment #8424246 - Flags: review?(bgrinstead)
Attached patch Patch for bug 1011624 (obsolete) (deleted) — Splinter Review
This patch address the concerns mentioned: removes padding discrepancy, unnecessary filters, standardizes %if syntax
Attachment #8424246 - Attachment is obsolete: true
Attachment #8424951 - Flags: review?(bgrinstead)
Comment on attachment 8424951 [details] [diff] [review] Patch for bug 1011624 Review of attachment 8424951 [details] [diff] [review]: ----------------------------------------------------------------- Can you rebase this please? I'm getting rejects on the */jar.mn files when apply the patch on tip
Attachment #8424951 - Flags: review?(bgrinstead)
Attached patch Patch for bug 1011624 (obsolete) (deleted) — Splinter Review
This *should* be rebased now. Might've broken something else, but we will see :)
Attachment #8424951 - Attachment is obsolete: true
Attachment #8425097 - Flags: review?(bgrinstead)
(In reply to alexharris6 from comment #8) > Created attachment 8425097 [details] [diff] [review] > Patch for bug 1011624 > > This *should* be rebased now. Might've broken something else, but we will > see :) Yeah, it applied cleanly. I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=83d5e2bf765b
Comment on attachment 8425097 [details] [diff] [review] Patch for bug 1011624 Review of attachment 8425097 [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 (../shared.devtools/inspector.css) Should be shared/devtools/inspector.css
Attachment #8425097 - Flags: review?(bgrinstead)
Blocks: 1015627
Attached patch Updated patch (deleted) — Splinter Review
Just addressed Brian's comments, hope you don't mind. I need this fixed for bug 1015627. So here's what I changed : - fixed the typo - Added r=bgrins I still kept you commit info though.
Attachment #8425097 - Attachment is obsolete: true
Attachment #8429354 - Flags: review?(bgrinstead)
Attachment #8429354 - Attachment is patch: true
Comment on attachment 8429354 [details] [diff] [review] Updated patch Review of attachment 8429354 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=281197ebd7f3
Attachment #8429354 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
Thanks Tim. I guess I thought I had done this, but it mustve either been for a different bug with a similar situation, or I just never uploaded my fixed patch :)
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [mentor=bgrins][lang=css] → [mentor=bgrins][lang=css][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=css][fixed-in-fx-team] → [mentor=bgrins][lang=css]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: