stylo: Parse and evaluate media queries using nsMediaFeatures

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(13 attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
xidorn
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1328655
(Assignee)

Updated

2 years ago
Summary: stylo: Parse media queries using nsMediaFeatures → stylo: Parse and evaluate media queries using nsMediaFeatures
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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+
(Assignee)

Comment 12

2 years ago
mozreview-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 :)
(Assignee)

Comment 13

2 years ago
mozreview-review
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 14

2 years ago
mozreview-review-reply
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 15

2 years ago
mozreview-review
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 16

2 years ago
mozreview-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+
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
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 18

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 years ago
mozreview-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+
(Assignee)

Updated

2 years ago
Attachment #8827081 - Flags: review?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

2 years ago
mozreview-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/#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 35

2 years ago
mozreview-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 36

2 years ago
mozreview-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 37

2 years ago
mozreview-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. :-)
(Assignee)

Comment 38

2 years ago
mozreview-review
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.
(Assignee)

Comment 39

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 40

2 years ago
mozreview-review-reply
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 41

2 years ago
mozreview-review
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 42

2 years ago
mozreview-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 43

2 years ago
mozreview-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 44

2 years ago
mozreview-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 45

2 years ago
mozreview-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+
(Assignee)

Comment 46

2 years ago
mozreview-review-reply
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 :)
(Assignee)

Updated

2 years ago
See Also: → bug 1331581
(Assignee)

Updated

2 years ago
See Also: → bug 1331584

Comment 47

2 years ago
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
(Assignee)

Updated

a year ago
Depends on: 1347273
You need to log in before you can comment on or make changes to this bug.