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)
DevTools
General
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)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
See https://bugzilla.mozilla.org/show_bug.cgi?id=1010959#c0 for general guidelines
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=bgrins][lang=css]
Assignee | ||
Comment 1•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
(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
Assignee | ||
Comment 3•11 years ago
|
||
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)
Updated•11 years ago
|
Assignee: nobody → alexharris6
Status: NEW → ASSIGNED
Comment 4•11 years ago
|
||
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.
Reporter | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
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)
Reporter | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Reporter | ||
Comment 9•11 years ago
|
||
(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
Reporter | ||
Comment 10•11 years ago
|
||
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)
Comment 11•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8429354 -
Attachment is patch: true
Reporter | ||
Comment 12•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•10 years ago
|
||
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 :)
Comment 14•10 years ago
|
||
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [mentor=bgrins][lang=css] → [mentor=bgrins][lang=css][fixed-in-fx-team]
Comment 15•10 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•