Closed Bug 1331213 Opened 8 years ago Closed 8 years ago

stylo: Parse and evaluate media queries using nsMediaFeatures

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(13 files)

Bug 1331213: Export nsMediaFeatures.h and nsMediaList.h.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Add a descriptive error message when an include isn't found.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1331213: Generate nsMediaFeatures and nsMediaList, along with a bunch of other stuff in nsCSSProps.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1331213: Regenerate bindings.
59 bytes, text/x-review-board-request
Details
Bug 1331213: Add an API to get nsMediaFeatures::features.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Bootstrap a Gecko-side Device, and track it's dirtiness manually in the per-doc data.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Add an API to drop a nsCSSValue in the stack.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array.
59 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
Bug 1331213: Allow parsing media query expressions without colons.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Implement the bulk of media query evaluation.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Don't make the pres context pointers opaque.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Implement Device::media_type, without supporting overrides for now.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331213: Implement the resolution override.
59 bytes, text/x-review-board-request
heycam
: review+
Details
I finally had some time for bug 1325878. It's a decently-sized task, so I wanted to spin this off from there. This bug is to parse media queries in stylesheets and evaluate them using nsMediaFeatures.
Blocks: 1328655
Summary: stylo: Parse media queries using nsMediaFeatures → stylo: Parse and evaluate media queries using nsMediaFeatures
Comment on attachment 8826963 [details] Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array. https://reviewboard.mozilla.org/r/104760/#review105470 ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:17 (Diff revision 1) > + > +impl nsCSSValue { > + /// Create a CSSValue with null unit, useful to be used as a return value. > + #[inline] > + pub fn null() -> Self { > + unsafe { mem::zeroed() } Can we rely on the guarantee that the null unit has a zeroed discriminant? ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:38 (Diff revision 1) > + unsafe { *self.mValue.mFloat.as_ref() } > + } > + > + /// Returns this nsCSSValue as a nsCSSValue::Array, unchecked in release > + /// builds. > + pub fn array_unchecked(&self) -> &nsCSSValue_Array { unsafe fn ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:53 (Diff revision 1) > + unsafe { Gecko_CSSValue_Drop(self) }; > + } > +} > + > +impl nsCSSValue_Array { > + /// Return the length of this `nsCSSShadowArray` wrong comment?
Attachment #8826963 - Flags: review?(manishearth) → review+
Comment on attachment 8826961 [details] Bug 1331213: Bootstrap a Gecko-side Device, and track it's dirtiness manually in the per-doc data. https://reviewboard.mozilla.org/r/104756/#review105472 ::: servo/components/style/gecko/media_queries.rs:25 (Diff revision 1) > +use style_traits::ToCss; > +use style_traits::viewport::ViewportConstraints; > +use values::{CSSFloat, specified}; > + > +/// The `Device` in Gecko is only a newtype on top of a `nsIDocument`, which > +/// contains all the media query evaluation cache, etc. This comment is remminiscent from my initial attempt, so it should be rewritten :)
Comment on attachment 8826963 [details] Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array. https://reviewboard.mozilla.org/r/104760/#review105474 ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:17 (Diff revision 1) > + > +impl nsCSSValue { > + /// Create a CSSValue with null unit, useful to be used as a return value. > + #[inline] > + pub fn null() -> Self { > + unsafe { mem::zeroed() } I think so. I can also manually initialize it I guess. ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:38 (Diff revision 1) > + unsafe { *self.mValue.mFloat.as_ref() } > + } > + > + /// Returns this nsCSSValue as a nsCSSValue::Array, unchecked in release > + /// builds. > + pub fn array_unchecked(&self) -> &nsCSSValue_Array { Why this and not the others? You can't leak the array reference, which is what would justify that IMO. ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:53 (Diff revision 1) > + unsafe { Gecko_CSSValue_Drop(self) }; > + } > +} > + > +impl nsCSSValue_Array { > + /// Return the length of this `nsCSSShadowArray` Good catch :)
Comment on attachment 8826963 [details] Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array. https://reviewboard.mozilla.org/r/104760/#review105474 > Why this and not the others? You can't leak the array reference, which is what would justify that IMO. It could be a unit other than an array and then you're reading from an arbitrary pointer.
Comment on attachment 8826957 [details] Bug 1331213: Add a descriptive error message when an include isn't found. https://reviewboard.mozilla.org/r/104748/#review105484
Attachment #8826957 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8826958 [details] Bug 1331213: Generate nsMediaFeatures and nsMediaList, along with a bunch of other stuff in nsCSSProps. https://reviewboard.mozilla.org/r/104750/#review105508 ::: servo/components/style/build_gecko.rs:294 (Diff revision 1) > + // FIXME(emilio): These three can go away once > + // https://github.com/servo/rust-bindgen/pull/397 has landed. Actually I guess there is already some redundant in the list. There are some types listed twice: unprefixed and prefixed. When that is fixed, could we get rid of some more types from the list? ::: servo/components/style/build_gecko.rs:320 (Diff revision 1) > "nsFont", > "nsIAtom", > "nsMainThreadPtrHandle", > "nsMainThreadPtrHolder", > "nsMargin", > + "nsMedia.*", Would this be a bit too broad? There are some `nsMedia*` types I guess we don't really want to include. Probably list all we need like `nsMedia(List|Query|Features|Expression)`?
Attachment #8826958 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8826958 [details] Bug 1331213: Generate nsMediaFeatures and nsMediaList, along with a bunch of other stuff in nsCSSProps. https://reviewboard.mozilla.org/r/104750/#review105508 > Actually I guess there is already some redundant in the list. There are some types listed twice: unprefixed and prefixed. > > When that is fixed, could we get rid of some more types from the list? Yes, I think we can get rid of a bunch of them after the bindgen whitelisting improvements from a while ago. Probably worth cleaning them up in a followup. > Would this be a bit too broad? There are some `nsMedia*` types I guess we don't really want to include. Probably list all we need like `nsMedia(List|Query|Features|Expression)`? Maybe? It doesn't hurt, and I was lazy :) Will list them explicitly.
Comment on attachment 8826956 [details] Bug 1331213: Export nsMediaFeatures.h and nsMediaList.h. https://reviewboard.mozilla.org/r/104766/#review105620
Attachment #8826956 - Flags: review?(cam) → review+
Comment on attachment 8826960 [details] Bug 1331213: Add an API to get nsMediaFeatures::features. https://reviewboard.mozilla.org/r/104754/#review105622
Attachment #8826960 - Flags: review?(cam) → review+
Attachment #8827081 - Flags: review?(cam)
Comment on attachment 8826961 [details] Bug 1331213: Bootstrap a Gecko-side Device, and track it's dirtiness manually in the per-doc data. https://reviewboard.mozilla.org/r/104756/#review105626 s/it's/its/ in the commit message. r=me if you can explain the FnMut stuff to me. :-) ::: servo/components/style/gecko/data.rs:113 (Diff revision 2) > > /// Get an mutable reference to this style data. > pub fn borrow_mut(&self) -> AtomicRefMut<PerDocumentStyleDataImpl> { > self.0.borrow_mut() > } > + Nit: don't need this blank line? ::: servo/components/style/gecko/media_queries.rs:88 (Diff revision 2) > + self.viewport_override.as_ref().map(|v| { > + Size2D::new(Au::from_f32_px(v.size.width), > + Au::from_f32_px(v.size.height)) > + }).unwrap_or_else(|| { > + // TODO(emilio): Grab from pres context. > + Size2D::new(Au(0), Au(0)) Might be worth faking a more plausible size like 1024x768 otherwise we might default to mobile sites, once @media starts working. ::: servo/components/style/gecko/media_queries.rs:217 (Diff revision 2) > +fn find_feature<F>(mut f: F) -> Option<&'static nsMediaFeature> > + where F: FnMut(&'static nsMediaFeature) -> bool, I'm not really familiar with FnMut. Why does this need to be FnMut (and the |f| argument mut)? ::: servo/components/style/gecko/media_queries.rs:338 (Diff revision 2) > + let _table = feature.mData.mKeywordTable.as_ref(); > + 0 Add an XXX/TODO comment in here? ::: servo/components/style/stylist.rs:60 (Diff revision 2) > + /// That's why we need to look for viewport rules on a style update (though > + /// given new viewport rules may appear, this may have been a previous Servo > + /// bug). Nit: If this is a previous Servo bug that this change might have fixed, that would be better mentioned in the commit message (describing the change) rather than a comment (describing the state of the code).
Attachment #8826961 - Flags: review?(cam) → review+
Comment on attachment 8826962 [details] Bug 1331213: Add an API to drop a nsCSSValue in the stack. https://reviewboard.mozilla.org/r/104758/#review105632
Attachment #8826962 - Flags: review?(cam) → review+
Comment on attachment 8826963 [details] Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array. https://reviewboard.mozilla.org/r/104760/#review105634 ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:38 (Diff revision 2) > + pub unsafe fn array_unchecked(&self) -> &nsCSSValue_Array { > + debug_assert!(self.mUnit == nsCSSUnit::eCSSUnit_Array); > + let array = unsafe { *self.mValue.mArray.as_ref() }; > + debug_assert!(!array.is_null()); > + unsafe { &*array } > + } Why make the fn unsafe in addition to the unsafe bits inside it? ::: servo/components/style/gecko_bindings/sugar/ns_css_value.rs:39 (Diff revision 2) > + } > + > + /// Returns this nsCSSValue as a nsCSSValue::Array, unchecked in release > + /// builds. > + pub unsafe fn array_unchecked(&self) -> &nsCSSValue_Array { > + debug_assert!(self.mUnit == nsCSSUnit::eCSSUnit_Array); Might be worth copying the condition exactly from nsCSSValue.h (which allows other units, although we don't need them for @media).
Attachment #8826963 - Flags: review?(cam) → review+
Comment on attachment 8826964 [details] Bug 1331213: Allow parsing media query expressions without colons. https://reviewboard.mozilla.org/r/104762/#review105636 ::: servo/components/style/gecko/media_queries.rs:305 (Diff revision 2) > + if input.try(|i| i.expect_colon()).is_err() { > + return Ok(Expression::new(feature, None, range)); > + } I'm not sure this is right. Don't we need to return Err(()) if we failed to get a colon because we found something else (as opposed to end of input)? ::: servo/components/style/gecko/media_queries.rs:351 (Diff revision 2) > MediaExpressionValue::Resolution(Resolution::parse(input)?) > } > nsMediaFeature_ValueType::eEnumerated => { > let index = unsafe { > let _table = feature.mData.mKeywordTable.as_ref(); > + // TODO Oh, here it is. :-)
Comment on attachment 8826964 [details] Bug 1331213: Allow parsing media query expressions without colons. https://reviewboard.mozilla.org/r/104762/#review105752 ::: servo/components/style/gecko/media_queries.rs:305 (Diff revision 2) > + if input.try(|i| i.expect_colon()).is_err() { > + return Ok(Expression::new(feature, None, range)); > + } Yeah, it is, the `parse_nested_block` call contains a call to `parse_entirely`, the final result will be an error if the input isn't exhausted.
Comment on attachment 8826961 [details] Bug 1331213: Bootstrap a Gecko-side Device, and track it's dirtiness manually in the per-doc data. https://reviewboard.mozilla.org/r/104756/#review105626 > Might be worth faking a more plausible size like 1024x768 otherwise we might default to mobile sites, once @media starts working. This is using to evaluate viewport units (we get the correct viewport size from `nsMediaFeatures` for evaluating media queries). I didn't implement it because it's a bit tricky in the sense that it needs to take into account scrollbars some times (something we should get filled for Servo I guess). I don't expect it to be harder than a FFI function calling to CalcViewportUnitsScale (http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.cpp#398), but that function needs to be audited for thread safety (I guess we're fine), and potentially add some caching, but nothing important. Still would prefer it to be a separate patch, so yeah, I'll change to a more realistic size before landing this. > I'm not really familiar with FnMut. Why does this need to be FnMut (and the |f| argument mut)? It doesn't _need_ to be `FnMut`, though it can. Usually with functions I just use the one that has less restrictions for the caller. `mut` is needed to call it, because otherwise two references to the same function could be called mutating the same data, I guess.
Comment on attachment 8826963 [details] Bug 1331213: Add sugar for nsCSSValue and nsCSSValue::Array. https://reviewboard.mozilla.org/r/104760/#review105634 > Why make the fn unsafe in addition to the unsafe bits inside it? Right, apparently was too fast addressing review comments. > Might be worth copying the condition exactly from nsCSSValue.h (which allows other units, although we don't need them for @media). Will do.
Comment on attachment 8826965 [details] Bug 1331213: Implement the bulk of media query evaluation. https://reviewboard.mozilla.org/r/104764/#review105802 ::: servo/components/style/gecko/media_queries.rs:227 (Diff revision 3) > + return None; > + } > + > + match for_expr.feature.mValueType { > + nsMediaFeature_ValueType::eLength => { > + debug_assert!(css_value.mUnit == nsCSSUnit::eCSSUnit_Pixel); The comment in nsMediaFeatures.h says that eLength supports any nsCSSValue that IsLengthUnit(), although it does look like all of the length-typed media features we have defined will only return eCSSUnit_Pixel values. Can you update the comment to say that the getter must return px but values to test against can be any length-typed value? ::: servo/components/style/gecko/media_queries.rs:460 (Diff revision 3) > + // FIXME(emilio): This doesn't seem posible from reading gecko code, > + // probably we should just clean up that function and return void. s/posible/possible/ I think you're right. Please file a followup bug to do that. ::: servo/components/style/gecko/media_queries.rs:479 (Diff revision 3) > + debug_assert!(self.range == nsMediaExpression_Range::eEqual || > + self.feature.mRangeType == nsMediaFeature_RangeType::eMinMaxAllowed, > + "Whoops, wrong range"); > + use self::MediaExpressionValue::*; > + use std::cmp::Ordering; Nit: I guess use statements usually go above everything else. ::: servo/components/style/gecko/media_queries.rs:502 (Diff revision 3) > + }; > + > + let required_value = match self.value { > + Some(ref v) => v, > + None => { > + // If there's no value, always match except it's a zero length s/except/unless/ ::: servo/components/style/gecko/media_queries.rs:519 (Diff revision 3) > + (&Integer(one), &Integer(ref other)) => one.cmp(other), > + (&BoolInteger(one), &BoolInteger(ref other)) => one.cmp(other), > + (&Float(one), &Float(ref other)) => one.partial_cmp(other).unwrap(), We can drop the ref on these, I guess? And you can combine the Integer and BoolInteger arms, if you want. ::: servo/components/style/gecko/media_queries.rs:544 (Diff revision 3) > + nsMediaExpression_Range::eMin => cmp == Ordering::Less, > + nsMediaExpression_Range::eEqual => cmp == Ordering::Equal, > + nsMediaExpression_Range::eMax => cmp == Ordering::Greater, For eMin don't you want to allow Ordering::Less and Ordering::Equal? (Similar issue for eMax.)
Attachment #8826965 - Flags: review?(cam) → review+
Comment on attachment 8827081 [details] Bug 1331213: Don't make the pres context pointers opaque. https://reviewboard.mozilla.org/r/104912/#review105808
Attachment #8827081 - Flags: review?(cam) → review+
Comment on attachment 8827082 [details] Bug 1331213: Implement Device::media_type, without supporting overrides for now. https://reviewboard.mozilla.org/r/104914/#review105810
Attachment #8827082 - Flags: review?(cam) → review+
Comment on attachment 8827096 [details] Bug 1331213: Implement the resolution override. https://reviewboard.mozilla.org/r/104926/#review105812
Attachment #8827096 - Flags: review?(cam) → review+
Comment on attachment 8826964 [details] Bug 1331213: Allow parsing media query expressions without colons. https://reviewboard.mozilla.org/r/104762/#review105814
Attachment #8826964 - Flags: review?(cam) → review+
Comment on attachment 8826965 [details] Bug 1331213: Implement the bulk of media query evaluation. https://reviewboard.mozilla.org/r/104764/#review105802 > We can drop the ref on these, I guess? And you can combine the Integer and BoolInteger arms, if you want. Well, if we drop the ref then we need to take it manually for `cmp`/`partial_cmp`, that's why I'm only taking by ref the right hand side. Those arms can't be combined because `BoolInteger` stores a `bool`, so that'd be a type error. > For eMin don't you want to allow Ordering::Less and Ordering::Equal? (Similar issue for eMax.) Great catch :)
See Also: → 1331581
See Also: → 1331584
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e49a7ecc2c Export nsMediaFeatures.h and nsMediaList.h. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0cfd12cab5 Add an API to get nsMediaFeatures::features. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/822e011a61c0 Bootstrap a Gecko-side Device, and track it's dirtiness manually in the per-doc data. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/08dcea8c0001 Add an API to drop a nsCSSValue in the stack. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/74790f4d30de Implement the bulk of media query evaluation. r=heycam
Depends on: 1347273
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: