Closed Bug 1330146 Opened 7 years ago Closed 7 years ago

Remove windows-{xp,vista} values for the -moz-os-version CSS property

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox56 --- fixed

People

(Reporter: n.nethercote, Assigned: miketaylr)

References

Details

Attachments

(2 files)

Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
Places that need changing include the following:

- Update osVersionStrings in layout/style/nsMediaFeatures.cpp

- Update OperatingSystemVersion in widget/LookAndFeel.h

- Update windows_versions in layout/style/test/chrome/bug418986-2.js

- Remove uses in layout/style/test/test_media_queries.html and browser/components/extensions/extension-win-panel.css
Also remove a use in browser/themes/shared/tab-selected.svg.
Just to confirm: this won't cause existing pages that use xp/vista media queries to go crazy (error out catastrophically, etc), right?
(In reply to David Major [:dmajor] from comment #3)
> Just to confirm: this won't cause existing pages that use xp/vista media
> queries to go crazy (error out catastrophically, etc), right?

Good question! I don't know. I'm not planning to work on this myself. Perhaps someone who know more about CSS can answer. jimm, do you know?
Flags: needinfo?(jmathies)
Hmm, we probably should keep the queries, and just make sure they alwasy return false. This is more a question for web compat, ni'ing mtaylor.
Flags: needinfo?(jmathies) → needinfo?(miket)
Media features with invalid values are just ignored, which is the same as always returning false. So this change won't break any sites. Unless of course, if your site was relying on them for some (sad) reason.

(I guess we add a "Found invalid value for media feature." message in the console as well).
Flags: needinfo?(miket)
No longer blocks: xp-eol
Depends on: xp-eol
Blocks: xp-eol
No longer depends on: xp-eol
Priority: -- → P3
(Looks like this was partially done in Bug 1325503)
Assignee: nobody → miket
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review163942

This seems reasonable to me but I don't really know anything about these properties, so I don't think I'm qualified to review this. Sorry!
Attachment #8887650 - Flags: review?(n.nethercote)
No worries! Lemme ping bwinton since he last reviewed code for extension-win-panel.css, and I'll ping a layout peer.
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review164264

::: commit-message-1f894:1
(Diff revision 2)
> +Bug 1330146. Remoev windows-xp and windows-vista values from extension-win-panel.css. r=bwinton

drive-by -- typo:  "Remoev"
Comment on attachment 8887981 [details]
Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests.

https://reviewboard.mozilla.org/r/158850/#review164266

r=me, perhaps with a bit more detail in the extended commit message as noted below:

::: commit-message-41349:1
(Diff revision 1)
> +Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
> +
> +The functionality was removed in Bug 1325503.
> +

Am I correct in inferring that these tests simply checked that these were *parseable values*? (and not that they were ever actually enabled)  i.e. these tests might as well have had "foobar" instead of "windows-xp" and would have behaved just the same?

If so, that might be worth noting in the extended commit message here.  Otherwise, it's a bit mysterious. ("hey why didn't these tests start failing when the functionality was removed way back in that other bug")
Attachment #8887981 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #13)
> drive-by -- typo:  "Remoev"

Thx - just noticed that myself. :)
(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8887981 [details]
> Bug 1330146. Remove windows-xp and windows-vista as values for
> -moz-os-version from tests.
> 
> https://reviewboard.mozilla.org/r/158850/#review164266
> 
> r=me, perhaps with a bit more detail in the extended commit message as noted
> below:
> 
> ::: commit-message-41349:1
> (Diff revision 1)
> > +Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
> > +
> > +The functionality was removed in Bug 1325503.
> > +
> 
> Am I correct in inferring that these tests simply checked that these were
> *parseable values*? (and not that they were ever actually enabled)  i.e.
> these tests might as well have had "foobar" instead of "windows-xp" and
> would have behaved just the same?

Yeah, it seems so. Which explains why they didn't start failing once we actually removed support for windows-xp and windows-vista values. I thought about leaving them in, but someone might be more confused in the future... 

> If so, that might be worth noting in the extended commit message here. 
> Otherwise, it's a bit mysterious. ("hey why didn't these tests start failing
> when the functionality was removed way back in that other bug")

Sounds good, will update.
Attachment #8887650 - Flags: review?(bwinton) → review?(mwein)
Oh god, I am so the wrong person to review this!  Redirecting to Matt, who (I believe) has been working on WebExtension styling stuff recently… 
I have, but I haven't worked with extension-win-panel.css, so I think Kris would be better to review this.
Attachment #8887650 - Flags: review?(mwein) → review?(kmaglione+bmo)
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review164292
Attachment #8887650 - Flags: review?(kmaglione+bmo) → review+
Thanks everyone!
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49f30c8fefa
Remove windows-xp and windows-vista values from extension-win-panel.css. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b626aca18e66
Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: