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

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1010959#c0 for general guidelines

Comment 1

[alexharris6]

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

Comment 2

[Brian Grinstead [:bgrins]]

(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

Comment 3

[alexharris6]

Attachment: Patch for bug 1011624

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

Comment 4

[Tim Nguyen :ntim]

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 5

[Brian Grinstead [:bgrins]]

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.

Comment 6

[alexharris6]

Attachment: Patch for bug 1011624

This patch address the concerns mentioned: removes padding discrepancy, unnecessary filters, standardizes %if syntax

Comment 7

[Brian Grinstead [:bgrins]]

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

Comment 8

[alexharris6]

Attachment: Patch for bug 1011624

This *should* be rebased now. Might've broken something else, but we will see :)

Comment 9

[Brian Grinstead [:bgrins]]

(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 10

[Brian Grinstead [:bgrins]]

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/[e-mail]          (../shared/devtools/images/[e-mail])
>    skin/classic/browser/devtools/[e-mail]          (../shared/devtools/images/[e-mail])
> +* skin/classic/browser/devtools/inspector.css               (../shared.devtools/inspector.css)

Should be shared/devtools/inspector.css

Comment 11

[Tim Nguyen :ntim]

Attachment: Updated patch

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.

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[alexharris6]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5f1c233ecf8f

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5f1c233ecf8f