Last Comment Bug 1313830 - Replace @media not all and (-moz-windows-default-theme) with @media (-moz-windows-default-theme: 0)
: Replace @media not all and (-moz-windows-default-theme) with @media (-moz-win...
Status: RESOLVED FIXED
[good first bug][lang=css]
: good-first-bug
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
P3 normal (vote)
: Firefox 53
Assigned To: katecastellano
:
: Dão Gottwald [:dao]
Mentors: Dão Gottwald [:dao]
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-29 00:36 PDT by Dão Gottwald [:dao]
Modified: 2016-12-19 16:47 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
not-at-all-replacement.patch (8.97 KB, patch)
2016-10-29 04:01 PDT, katecastellano
no flags Details | Diff | Splinter Review
not-at-all-replacement-no-superfluos-spaces.patch (1.86 KB, patch)
2016-10-29 04:43 PDT, katecastellano
no flags Details | Diff | Splinter Review
Bug-1313830.patch (8.01 KB, patch)
2016-10-29 07:46 PDT, katecastellano
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Dão Gottwald [:dao] 2016-10-29 00:36:15 PDT
We use "@media not all and (-moz-windows-default-theme)" in a couple of places:

https://dxr.mozilla.org/mozilla-central/search?q=not+all+and+(-moz-windows-default-theme)&redirect=false

and it can be simplified to "@media (-moz-windows-default-theme: 0)" like here:

https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/browser/themes/shared/icon-colors.inc.svg#16
Comment 1 User image Dão Gottwald [:dao] 2016-10-29 01:21:06 PDT
While we're at it, we could do the same for -moz-windows-compositor and -moz-windows-classic:

https://dxr.mozilla.org/mozilla-central/search?q=not+all+and+(-moz-windows-compositor)&redirect=true

https://dxr.mozilla.org/mozilla-central/search?q=not+all+and+(-moz-windows-classic)&redirect=true
Comment 2 User image Michael Henretty [:mhenretty] 2016-10-29 02:34:57 PDT
Kate is going to take a look at this one.
Comment 3 User image katecastellano 2016-10-29 04:01:47 PDT
Created attachment 8805760 [details] [diff] [review]
not-at-all-replacement.patch
Comment 5 User image Dão Gottwald [:dao] 2016-10-29 04:22:17 PDT
Comment on attachment 8805760 [details] [diff] [review]
not-at-all-replacement.patch

Some feedback while we're waiting for results from the Try server run:

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -187,8 +187,8 @@ toolbar:-moz-lwtheme {
>   transition: min-height 170ms ease-out, max-height 170ms ease-out, visibility 170ms linear;
> }
> 
>-@media not all and (-moz-windows-compositor),
>-       not all and (-moz-windows-default-theme) {
>+@media (-moz-windows-compositor : 0),

nit: there's an extra space before : that shouldn't be there

> @media not all and (-moz-os-version: windows-vista) and (-moz-windows-default-theme) {
>-  @media not all and (-moz-os-version: windows-win7) and (-moz-windows-default-theme) {
>+  @media not all and  (-moz-os-version: windows-win7) and (-moz-windows-default-theme) {

nit: you've added a superfluous space here

>--- a/browser/themes/windows/devedition.css
>+++ b/browser/themes/windows/devedition.css
>@@ -191,8 +191,8 @@
>   }
> 
>   /* Use proper menu text styling in Win7 classic mode (copied from browser.css) */
>-  @media not all and (-moz-windows-compositor),
>-         not all and (-moz-windows-default-theme) {
>+  @media (-moz-windows-compositor : 0),

superfluous space before : again

>--- a/devtools/client/webconsole/test/test-file-location.js
>+++ b/devtools/client/webconsole/test/test-file-location.js
>@@ -3,6 +3,7 @@
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/ */
> 
>+
> console.log("message for level log");
> console.info("message for level info");
> console.warn("message for level warn");

Unrelated change, please revert this

Looks good otherwise!
Comment 6 User image katecastellano 2016-10-29 04:43:15 PDT
Created attachment 8805763 [details] [diff] [review]
not-at-all-replacement-no-superfluos-spaces.patch
Comment 7 User image katecastellano 2016-10-29 07:07:25 PDT
Comment on attachment 8805763 [details] [diff] [review]
not-at-all-replacement-no-superfluos-spaces.patch

tests passed and I updated your changes
Comment 8 User image katecastellano 2016-10-29 07:46:27 PDT
Created attachment 8805787 [details] [diff] [review]
Bug-1313830.patch

Fixed broken patch file and added previous said fixes
Comment 9 User image Dão Gottwald [:dao] 2016-10-29 07:52:55 PDT
Comment on attachment 8805787 [details] [diff] [review]
Bug-1313830.patch

Looks good. Thanks!
Comment 10 User image Dão Gottwald [:dao] 2016-12-19 03:35:57 PST
Oops, looks like we never landed this. Next time you can just add the checkin-needed keyword after getting review granted on your patch.
Comment 11 User image Pulsebot 2016-12-19 03:39:39 PST
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec29f454b89
fixed media queries to not use 'not at all', r=dao
Comment 12 User image Wes Kocher (:KWierso) 2016-12-19 16:47:39 PST
https://hg.mozilla.org/mozilla-central/rev/6ec29f454b89

Note You need to log in before you can comment on or make changes to this bug.