DevTools expects to emulate CSS media like "print" via nsDocumentViewer::EmulateMedium[1]. With Stylo, this appears to have no effect. This leads to failures like: * devtools/client/commandline/test/browser_cmd_media.js[2] TEST-UNEXPECTED-FAIL | devtools/client/commandline/test/browser_cmd_media.js | media correctly emulated - "rgb(255, 255, 255)" == "rgb(255, 255, 0)" - [1]: http://searchfox.org/mozilla-central/source/devtools/shared/gcli/commands/media.js#43 [2]: https://treeherder.mozilla.org/logviewer.html#?job_id=105301412&repo=try&lineNumber=4068

The proposed patch allows media emulation of supported media types. It still won't pass the devtools/client/commandline/test/browser_cmd_media.js test, since that test attempts a "media emulate braille", which is not implemented in Servo. The spec at https://drafts.csswg.org/mediaqueries/#media-types says: > User agents must recognize the following media types as valid, but must make them match nothing. And includes "braille" in that section. Apparently Gecko still allows media query matching of media types in that list.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review168624 I think we should go for the correct fix for this, which is to stop discerning between `Known` and `Unknown` `MediaQueryType`s, and make `MediaType` accept any kind of identifier: ``` enum MediaQueryType { All, Some(MediaType), } struct MediaType(CustomIdent); ``` Would that make sense? Thanks! ::: servo/components/style/gecko/media_queries.rs:144 (Diff revision 1) > /// Returns the current media type of the device. > pub fn media_type(&self) -> MediaType { > unsafe { > - // FIXME(emilio): Gecko allows emulating random media with > - // mIsEmulatingMedia / mMediaEmulated . Refactor both sides so that > - // is supported (probably just making MediaType an Atom). > + // Gecko allows emulating random media with mIsEmulatingMedia and > + // mMediaEmulated. > + let medium_to_use = match self.pres_context().mIsEmulatingMedia() { Let's move the pres context to its own variab.e ::: servo/components/style/gecko/media_queries.rs:145 (Diff revision 1) > pub fn media_type(&self) -> MediaType { > unsafe { > - // FIXME(emilio): Gecko allows emulating random media with > - // mIsEmulatingMedia / mMediaEmulated . Refactor both sides so that > - // is supported (probably just making MediaType an Atom). > - if self.pres_context().mMedium == atom!("screen").as_ptr() { > + // Gecko allows emulating random media with mIsEmulatingMedia and > + // mMediaEmulated. > + let medium_to_use = match self.pres_context().mIsEmulatingMedia() { > + 1u32 => self.pres_context().mMediaEmulated.raw::<nsIAtom>(), and I'd just check `pres_context.mIsEmulatingMedia() != 0`, for similarity with what c++ does, but that's not a big deal. ::: servo/components/style/gecko/media_queries.rs:154 (Diff revision 1) > MediaType::Screen > - } else { > + } else if medium_to_use == atom!("print").as_ptr() { > - debug_assert!(self.pres_context().mMedium == atom!("print").as_ptr()); > MediaType::Print > + } else { > + MediaType::Unsupported Maybe `unknown`? but I'm not sure we should really fake this and just print "Unsupported". I think we should add a `MediaType::Other`, or make `MediaType` a struct wrapping an atom instead: ``` #[derive(Debug, Eq, PartialEq)] struct MediaType(CustomIdent); ``` ::: servo/components/style/media_queries.rs:114 (Diff revision 1) > write!(dest, "all")?; > } > }, > MediaQueryType::Known(MediaType::Screen) => write!(dest, "screen")?, > MediaQueryType::Known(MediaType::Print) => write!(dest, "print")?, > + MediaQueryType::Known(MediaType::Unsupported) => write!(dest, "unsupported")?, Yeah, I think this is wrong, and unreachable so far...

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review168904 ::: servo/components/style/media_queries.rs:114 (Diff revision 1) > write!(dest, "all")?; > } > }, > MediaQueryType::Known(MediaType::Screen) => write!(dest, "screen")?, > MediaQueryType::Known(MediaType::Print) => write!(dest, "print")?, > + MediaQueryType::Known(MediaType::Unsupported) => write!(dest, "unsupported")?, Yes, but the other changes in this patch require the match to cover all possible values, and this is now one of those values (though no code path would ever assign this value). This will likely go away with other needed changes to this patch.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review169358 ::: servo/components/style/media_queries.rs:113 (Diff revision 3) > // 40px)" in "all (min-width: 40px)", which is unexpected. > if self.qualifier.is_some() || self.expressions.is_empty() { > write!(dest, "all")?; > } > }, > - MediaQueryType::Known(MediaType::Screen) => write!(dest, "screen")?, > + MediaQueryType::Matchable(MediaType(ref desc)) => write!(dest, "{:?}", desc)?, This needs to use `dest.to_css`, in order to handle escaping correctly. ::: servo/components/style/media_queries.rs:142 (Diff revision 3) > #[cfg_attr(feature = "servo", derive(HeapSizeOf))] > pub enum MediaQueryType { > /// A media type that matches every device. > All, > - /// A known media type, that we parse and understand. > - Known(MediaType), > + /// A specific media type, that we are willing to match. > + Matchable(MediaType), Why do we need to distinguish between `Matchable` and `Unmatchable`? Seems like not needed to me, they'll never match the current `Device` anyway. ::: servo/components/style/media_queries.rs:170 (Diff revision 3) > - Some(media_type) => MediaQueryType::Known(media_type), > - None => MediaQueryType::Unknown(Atom::from(ident)), > - }) > + // Servo accepts all types, per spec, but only allows matching > + // of "screen" and "print". Everything else is accepted as > + // unmatchable. > + if !ident.eq_ignore_ascii_case("screen") && > + !ident.eq_ignore_ascii_case("print") { > + return Ok(MediaQueryType::Unmatchable(MediaType::parse(ident))) Oh, so this is just for Servo? I think making Servo's `MediaType` match Gecko would be nice instead. ::: servo/components/style/media_queries.rs:194 (Diff revision 3) > - Print, > -} > > impl MediaType { > - fn parse(name: &str) -> Option<Self> { > - Some(match_ignore_ascii_case! { name, > + fn parse(name: &str) -> Self { > + MediaType(CustomIdent(Atom::from(name.to_lowercase()))) no need for `to_lowercase`, right? Just use `CusomIdent::from_ident(name, &["not", "or", "and", "only"])`, making this return a `Result`.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review169358 > Why do we need to distinguish between `Matchable` and `Unmatchable`? > > Seems like not needed to me, they'll never match the current `Device` anyway. It seems to matter for device emulation, where in Gecko we do match "braille" and "embossed" and others. Servo aspires to more closely match the spec, so it rejects everything other than "screen" and "print" as unmatchable. Servo will also reject "speech", which is against the spec. > Oh, so this is just for Servo? I think making Servo's `MediaType` match Gecko would be nice instead. Do you mean making MediaQueryType be the same thing as MediaType? That would change a bit more of the parser logic and I'd like to see that as a different bug if you think it's necessary.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review169358 > It seems to matter for device emulation, where in Gecko we do match "braille" and "embossed" and others. Servo aspires to more closely match the spec, so it rejects everything other than "screen" and "print" as unmatchable. Servo will also reject "speech", which is against the spec. Sure, but why do we need to put them into another variant? Seems to that since servo will never have a device with `braille`, it's pretty impossible for it to match. > Do you mean making MediaQueryType be the same thing as MediaType? That would change a bit more of the parser logic and I'd like to see that as a different bug if you think it's necessary. MediaQueryType should still have an `All` variant, so should be a different type. But making Servo's `MediaType` also a `MediaType(CustomIdent)` would be nice.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review169358 > Sure, but why do we need to put them into another variant? Seems to that since servo will never have a device with `braille`, it's pretty impossible for it to match. I'm happy to to make the change you're asking for, but I don't understand what it is. Both Servo and Gecko will have multiple enum values for the MediaQueryType, and I can't see a reason why they would need different values. There are three enum values for the MediaQueryType: All, something we'll match, and something we won't (but we want to remember so we can serialize it). That feels to me like the correct, minimal set of values. Are you staying these three cases should be changed or collapsed? Prior to the patch these three values were called All, Known, and Unknown; and Known and Unknown had subtly different types that nevertheless accomplished much the same thing. All this part of the patch does is rename Known -> Matchable and Unknown -> Unmatchable and unify those two values to use the Atom maintained by the MediaQuery. What are you proposing should be done differently? > MediaQueryType should still have an `All` variant, so should be a different type. But making Servo's `MediaType` also a `MediaType(CustomIdent)` would be nice. The only definition of MediaType that I can find is http://searchfox.org/mozilla-central/source/servo/components/style/media_queries.rs#182. This patch makes it a MediaType(CustomIdent). Where is the other one that needs to be changed?

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review169866 ::: servo/components/style/media_queries.rs:155 (Diff revision 4) > } > > // From https://drafts.csswg.org/mediaqueries/#mq-syntax: > // > // The <media-type> production does not include the keywords only, > // not, and, and or. (And this comment can be moved there, presumably) ::: servo/components/style/media_queries.rs:161 (Diff revision 4) > if ident.eq_ignore_ascii_case("not") || > ident.eq_ignore_ascii_case("or") || > ident.eq_ignore_ascii_case("and") || > ident.eq_ignore_ascii_case("only") { > return Err(()) > } This can go away with the `CustomIdent::from`.

(In reply to Brad Werth [:bradwerth] from comment #13) > I'm happy to to make the change you're asking for, but I don't understand > what it is. Sorted this out with Emilio in IRC; here's the summary: Since Servo doesn't support media emulation (and doesn't intend to), there's no need for an Unmatched type. Every media type can be a Concrete type, and we just won't successfully match those types against the valid ones used by the Device. ("screen" and "print" in Servo). Conversely, although Gecko does support media emulation, it also doesn't reject any media type as unmatchable, so it also just needs a Concrete type. If Gecko ever aligns to spec and treats some types as unmatchable, then we may need additional types beyond Concrete.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review170198 Looks good! Just one question about `to_lowercase`, and one thing I overlooked about `CustomIdent::from_ident` ::: servo/components/style/media_queries.rs:150 (Diff revision 5) > - "not" | "or" | "and" | "only" => return Err(()), > _ => (), > }; > > - Ok(match MediaType::parse(ident) { > - Some(media_type) => MediaQueryType::Known(media_type), > + // If parseable, accept this type as a concrete type. > + match MediaType::parse(ident) { nit: This can just be `MediaType::parse(ident).map(MediaQueryType::Concrete)`. ::: servo/components/style/media_queries.rs:175 (Diff revision 5) > - fn parse(name: &str) -> Option<Self> { > - Some(match_ignore_ascii_case! { name, > - "screen" => MediaType::Screen, > - "print" => MediaType::Print, > - _ => return None > - }) > + fn parse(name: &str) -> Result<Self, ()> { > + // From https://drafts.csswg.org/mediaqueries/#mq-syntax: > + // > + // The <media-type> production does not include the keywords not, or, and, and only. > + let custom_ident = > + CustomIdent::from_ident(&name.to_lowercase().into(), nit: This jan just be: ``` CustomIdent::from_ident(name).map(MediaType) ``` Why is the `to_lowercase` needed? It shouldn't be. Also, I just noticed that this would fail to parse `@media default` and similar stuff, due to the check `CustomIdent::from_ident` does. So we probably still need to do the manual parsing :/. Could we add a test for this?

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review170198 > nit: This jan just be: > > ``` > CustomIdent::from_ident(name).map(MediaType) > ``` > > Why is the `to_lowercase` needed? It shouldn't be. > > Also, I just noticed that this would fail to parse `@media default` and similar stuff, due to the check `CustomIdent::from_ident` does. So we probably still need to do the manual parsing :/. > > Could we add a test for this? to_lowercase is used to be compliant with tests like /testing/web-platform/tests/css/CSS2/media/media-dependency-004.xht which has an @import rule with media type "ScReEn". I'll add a test for the media default case and other cases I can think of, and if necessary go back to a approach that avoids CustomIdent::from_ident.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review170576 ::: servo/components/style/media_queries.rs:175 (Diff revision 6) > - Some(match_ignore_ascii_case! { name, > - "screen" => MediaType::Screen, > - "print" => MediaType::Print, > - _ => return None > - }) > + // From https://drafts.csswg.org/mediaqueries/#mq-syntax: > + // > + // The <media-type> production does not include the keywords not, or, and, and only. > + match_ignore_ascii_case! { name, > + "not" | "or" | "and" | "only" => Err(()), > + _ => Ok(MediaType(CustomIdent(Atom::from(name.to_ascii_lowercase())))), So, seems like other browsers don't preserve case-sensitivity on serialization of media lists: ``` <!doctype html> <style> @media scReEn { } </style> <script> alert(document.styleSheets[0].cssRules[0].cssText); </script> ``` This is spec'd, so mention: https://drafts.csswg.org/cssom/#serializing-media-queries > Let type be the serialization as an identifier of the media type of the media query, converted to ASCII lowercase. And please add a test for it. Also, it's probably worth to avoid the extra allocation in the case it's already ascii-lowercase, so let's do: ``` let atom = if name.bytes().any(|c| matches!(c, b'A'..b'Z')) { Atom::from(name.to_ascii_lowercase()) } else { // Already ascii lowercase. Atom::from(name) }; ``` Bonus points for making it a function in `components/style/str.rs` like: ``` pub fn string_as_ascii_lowercase(input: &'a str) -> Cow<'a, str>; ```

Btw, your patch fixes a stylo bug in the serialization of invalid media types \o/. For example, this fails on stylo right now: <!doctype html> <style> @media scReEna { } </style> <script> let pass = document.styleSheets[0].cssRules[0].cssText.indexOf("screena") !== -1; alert(pass ? "pass" : "fail"); </script>

Comment on attachment 8894624 [details] Bug 1371395 Part 1: Create a Servo method for cheaply getting an ascii lowercase converted string. https://reviewboard.mozilla.org/r/165774/#review170910 ::: servo/components/style/lib.rs:40 (Diff revision 1) > //#![deny(unsafe_code)] > #![allow(unused_unsafe)] > > #![recursion_limit = "500"] // For define_css_keyword_enum! in -moz-appearance > > +#![feature(exclusive_range_pattern)] // Useful for some ascii char detection. We can't use this on stylo, we need to use the stable channel unfortunately. But looking at the patch, is it really needed? ::: servo/components/style/str.rs:9 (Diff revision 1) > > //! String utils for attributes and similar stuff. > > #![deny(missing_docs)] > > + nit: stray newline. ::: servo/components/style/str.rs:15 (Diff revision 1) > use num_traits::ToPrimitive; > use std::ascii::AsciiExt; > use std::convert::AsRef; > use std::iter::{Filter, Peekable}; > use std::str::Split; > +use std::borrow::Cow; nit: This will need to be ordered.

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review170912 r=me, thanks!

Comment on attachment 8894625 [details] Bug 1371395 Part 5: Add a test of media query list serialization of valid, invalid, and malformed types. https://reviewboard.mozilla.org/r/165776/#review170914 It'd be nice to upstream this test to WPT... But not bother too much if it's a hassle, I guess. ::: layout/style/test/test_media_query_serialization.html:33 (Diff revision 1) > + // Invalid types > + ["garbage7", "Invalid media types are ascii lowercased."], > + > + // Malformed types > + ["not all", "Malformed media types are serialized to 'not all'."], > + ["not all, not all", "Multiple malformed media types are each serialized to 'not all'."], Just curious, does this match other browsers' behavior? ::: layout/style/test/test_media_query_serialization.html:48 (Diff revision 1) > + let rule = sheet.cssRules[index]; > + let serializedList = rule.media.mediaText; > + is(serializedList, entry[0], entry[1]); > + }); > + > + SimpleTest.finish(); I'm not sure the waitForExplicitFinish dance is needed, do we need to wait until onload for any particular reason?

Comment on attachment 8894016 [details] Bug 1371395 Part 4: Test media emulation of an unsupported type. https://reviewboard.mozilla.org/r/165102/#review170916 Probably Jryans can take a look here faster than I can ::: devtools/client/commandline/test/browser_cmd_media.js:59 (Diff revision 2) > }, > > + testEmulateBadMedia: function (options) { > + return helpers.audit(options, [ > + { > + setup: "media emulate nonsense", I'm not really familiar with this test, nor the expected behavior of the devtools protocol. Is it expected to fail in this case?

Comment on attachment 8892250 [details] Bug 1371395 Part 2: Servo-side allow emulation of supported media types. https://reviewboard.mozilla.org/r/163196/#review170918 ::: commit-message-b82c4:1 (Diff revision 7) > +Bug 1371395 Part 2: Servo-side allow emulation of supported media types (currently only screen and print). I don't think the `currently only screen and print` part is true, is it?

Comment on attachment 8894016 [details] Bug 1371395 Part 4: Test media emulation of an unsupported type. https://reviewboard.mozilla.org/r/165102/#review170928 The general idea looks right, but you can probably be a bit more specific in the test. ::: devtools/client/commandline/test/browser_cmd_media.js:59 (Diff revision 2) > }, > > + testEmulateBadMedia: function (options) { > + return helpers.audit(options, [ > + { > + setup: "media emulate nonsense", It's probably worth getting a little more detailed in the test, something like the following should work: http://searchfox.org/mozilla-central/source/devtools/client/commandline/test/browser_cmd_rulers.js#28-36 So, something like: ``` { setup: "media emulate nonsense", check: { input: "media emulate nonsense", markup: "VVVVVVVVVVVVVVEEEEEEEE", status: "ERROR", }, exec: { output: "Can't use 'nonsense'", error: true, }, } ```

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 4 changesets with 6 changes to 6 files remote: (e7ded6e22d7d modifies servo/components/style/gecko/media_queries.rs from Servo; not enforcing peer review) remote: (e7ded6e22d7d modifies servo/components/style/media_queries.rs from Servo; not enforcing peer review) remote: (71ec41ecec66 modifies servo/components/style/str.rs from Servo; not enforcing peer review) remote: (2 changesets contain changes to protected servo/ directory: 71ec41ecec66, e7ded6e22d7d) remote: ************************************************************************ remote: you do not have permissions to modify files under servo/ remote: remote: the servo/ directory is kept in sync with the canonical upstream remote: repository at https://github.com/servo/servo remote: remote: changes to servo/ are only allowed by the syncing tool and by sheriffs remote: performing cross-repository "merges" remote: remote: to make changes to servo/, submit a Pull Request against the servo/servo remote: GitHub project remote: ************************************************************************ remote: transaction abort! remote: rollback completed remote: pretxnchangegroup.e_prevent_vendored hook failed abort: push failed on remote

Comment on attachment 8895246 [details] Bug 1371395 Part 1: Servo: Create a method for cheaply getting an ascii lowercase converted string. https://reviewboard.mozilla.org/r/166404/#review171610 Pretty sure I already reviewed this? Anyway, r=me regardless. Thanks!

Comment on attachment 8895247 [details] Bug 1371395 Part 2: Servo: Allow emulation of supported media types. https://reviewboard.mozilla.org/r/166406/#review171614 I already reviewed this, and there doesn't seem there is any other significant changes apart of review comments, so I'm re-stamping this. If there's anything more concrete you need me to review, please let me know :)

Comment on attachment 8895248 [details] Bug 1371395 Part 3: Servo: MediaType usages updated to use screen() and print() convenience functions. https://reviewboard.mozilla.org/r/166408/#review171616 ::: servo/components/style/media_queries.rs:172 (Diff revision 1) > > +lazy_static! { > + /// This static is provided as a convenience for Device initialization. > + pub static ref MEDIA_TYPE_SCREEN: MediaType = MediaType(CustomIdent(Atom::from("screen"))); > + /// This static is provided as a convenience for Device initialization. > + pub static ref MEDIA_TYPE_PRINT: MediaType = MediaType(CustomIdent(Atom::from("print"))); instead of this, let's add `print` and `screen` to `servo/components/atoms/static_atoms.txt`. Then: ``` impl MediaType { /// The `screen` media type. pub fn screen() -> Self { MediaType(CustomIdent(atom!("screen")) } /// The `print` media type. pub fn print() -> Self { MediaType(CustomIdent(atom!("print")) } } ``` Then change `MEDIA_TYPE_SCREEN` for `MediaType::screen()`, and so on.

Comment on attachment 8895249 [details] Bug 1371395 Part 4: Servo: Add string_cache crate. https://reviewboard.mozilla.org/r/166410/#review171620 ::: servo/components/style/Cargo.toml:24 (Diff revision 1) > gecko = ["nsstring_vendor", "num_cpus", "style_traits/gecko"] > use_bindgen = ["bindgen", "regex", "toml"] > servo = ["serde", "heapsize", "heapsize_derive", > "style_traits/servo", "servo_atoms", "servo_config", "html5ever", > "cssparser/heapsize", "cssparser/serde", "encoding", "smallvec/heapsizeof", > + "string_cache", I think there's no need for this patch, per my previous comment. `servo_atoms` should be enough. If there is a reason, please explain the reasoning on the commit message and re-request review. Thanks!

Comment on attachment 8895248 [details] Bug 1371395 Part 3: Servo: MediaType usages updated to use screen() and print() convenience functions. https://reviewboard.mozilla.org/r/166408/#review171750 Nice! r=me with the updated commit message, thanks! ::: commit-message-6eece:1 (Diff revision 2) > +Bug 1371395 Part 3: Servo: MediaType usages updated to use statics. nit: The commit message needs an update.

Comment on attachment 8895519 [details] Bug 1371395 Part 3: Add a test of media query list serialization of valid, invalid, and malformed types. https://reviewboard.mozilla.org/r/166726/#review171936 Per the IRC chat, this are the tests previously reviewed without changes, so r=me

Comment on attachment 8895518 [details] Bug 1371395 Part 2: Test media emulation of an unsupported type. https://reviewboard.mozilla.org/r/166724/#review171940

Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/231ae11cc314 Part 2: Test media emulation of an unsupported type. r=jryans https://hg.mozilla.org/integration/autoland/rev/c0b4c13955e7 Part 3: Add a test of media query list serialization of valid, invalid, and malformed types. r=emilio

https://hg.mozilla.org/mozilla-central/rev/231ae11cc314 https://hg.mozilla.org/mozilla-central/rev/c0b4c13955e7