Brian Grinstead [:bgrins] Reporter
	Description • 10 years ago

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

	alexharris6 Assignee
	Comment 1 • 10 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

	Brian Grinstead [:bgrins] Reporter
	Comment 2 • 10 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

	alexharris6 Assignee
	Comment 3 • 10 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

	Tim Nguyen :ntim
	Comment 4 • 10 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.

	Brian Grinstead [:bgrins] Reporter
	Comment 5 • 10 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.

	alexharris6 Assignee
	Comment 6 • 10 years ago

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

	Brian Grinstead [:bgrins] Reporter
	Comment 7 • 10 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

	alexharris6 Assignee
	Comment 8 • 10 years ago

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

	Brian Grinstead [:bgrins] Reporter
	Comment 9 • 10 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

	Brian Grinstead [:bgrins] Reporter
	Comment 10 • 10 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

	Tim Nguyen :ntim
	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.

	Brian Grinstead [:bgrins] 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

	alexharris6 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 :)

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 15 • 10 years ago

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