Closed Bug 1355408 Opened 7 years ago Closed 7 years ago

stylo: Support @-moz-document rule

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: ferjm)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1355408 - Part 1: Support @-moz-document rule in stylo.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1355408 - Part 2: Update test expectations after @-moz-document support for stylo.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Follow up
4.37 KB, patch
emilio
: review+
Details | Diff | Splinter Review 
We need to support @-moz-document basically because popular addons like Stylish uses it heavily in content stylesheet.
Fernando, can you look at this one?
Assignee: nobody → ferjmoreno
Priority: -- → P1
heycam and I discussed about this, and we think it is reasonable to evaluate this during parsing, and cache the result in the rule struct for cascading. With that, we don't need to worry about being in the wrong thread.

@-moz-document doesn't work prefectly even in Gecko, so it is fine to just have it match whatever the current document url is. We can probably add a hook later to re-evaluate it when url gets changed (at which time we are on main thread as well).
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #2)
> heycam and I discussed about this, and we think it is reasonable to evaluate
> this during parsing, and cache the result in the rule struct for cascading.
> With that, we don't need to worry about being in the wrong thread.

If we do it during cascading (like what Gecko currently does), we may not be able to handle the url matching and regexp things correctly.

This is how it currently work for evaluation: https://dxr.mozilla.org/mozilla-central/rev/abf145ebd05fe105efbc78b761858c34f7690154/layout/style/nsCSSRules.cpp#670-717

Probably we should abstract it into a function and call it from Servo side.
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

This patch implements Stylo support for @-moz-document. For now it only works for Gecko, but it should be pretty easy to make it work for Servo.

Instead of using the code at layout/style/nsCSSRule.cpp for evaluation as suggested above, I opted to do the evaluation in Servo. This way, we can reuse most part of the code for both Gecko and Servo. The only differing piece is the code to obtain the document URL.

I still need to see if there are existing tests for this and probably to write new tests. In the mean time, I would really appreciate some feedback.

By the way, this is my first patch for the Stylo team and the first time that I use MozReview (I normally use bz + Splinter), so apologies in advance if I am not following the workflow properly. I appreciate any feedback about this as well :)

Thank you!
Attachment #8859269 - Flags: feedback?(xidorn+moz)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> Comment on attachment 8859269 [details]
> 
> By the way, this is my first patch for the Stylo team and the first time
> that I use MozReview (I normally use bz + Splinter), so apologies in advance
> if I am not following the workflow properly. I appreciate any feedback about
> this as well :)

For instance, I don't see a way to ask for feedback instead of review from MozReview.
Nice work!

FWIW you can still use splinter if you like - stylo devs can get the bits to push directly to autoland from git/hg. I'll get you the bits.
Ah, good to know. Thank you!
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review134020

::: servo/components/style/Cargo.toml:50
(Diff revision 1)
>  ordered-float = "0.4"
>  parking_lot = "0.3.3"
>  pdqsort = "0.1.0"
>  precomputed-hash = "0.1"
>  rayon = "0.6"
> +regex = "0.2"

Please don't introduce regex into the dependency. It is too large for code size.

You can add a FFI to use `nsContentUtils::IsPatternMatching` instead.

::: servo/components/style/Cargo.toml:61
(Diff revision 1)
>  smallvec = "0.3"
>  style_traits = {path = "../style_traits"}
>  servo_url = {path = "../url", optional = true}
>  time = "0.1"
>  unicode-segmentation = "1.0"
> +url = "1.2"

Please don't reintroduce the url crate again, which was just removed soon ago... Probably using a FFI as well...
Attachment #8859269 - Flags: feedback?(xidorn+moz) → feedback-
Thank you for your feedback Xidorn!

I just updated the first patch removing the regex and url dependencies for Servo and implementing FFI alternatives. I also added a patch with the implementation for Servo, including the CSSOM bits.

I see that there are still some tests failing for Gecko and I still need to write some for Servo.
Attachment #8860046 - Flags: review?(xidorn+moz)
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review135094

Rather than converting between strings and url forth and back, would it be better to just pass document, pattern string, and matching type (url, url prefix, domain, regex) into a single FFI function? You can probably factor out part of `DocumentRule::UseForPresentation` so that Gecko and Stylo share the same code path for evaluation this.

Also, I'm not sure whether we want to implement `@-moz-document` for Servo as well. Manish, emilio, or Simon should probably review that. I suppose we don't want to blindly add unstandard moz-prefixed feature to Servo just because of Stylo. Maybe you should try making all the corresponding code to be Gecko-specific.

::: layout/style/ServoBindings.cpp:1809
(Diff revision 2)
>  }
>  
> +bool
> +Gecko_nsIURI_ToString(nsIURI* aURI, nsACString* aResult)
> +{
> +  nsresult rv = aURI->GetSpec(*aResult);

You probably need to add `MOZ_ASSERT(NS_IsMainThread());` in all three functions you add to satisisfy static analyser.
Attachment #8859269 - Flags: review?(xidorn+moz)
Comment on attachment 8860045 [details]
Bug 1355408 - Part 2: Update test expectations after @-moz-document support for stylo.

https://reviewboard.mozilla.org/r/132068/#review135110

As I said before, I'm not sure whether we should support this in Servo. Servo people should probably rubberstamp that.
Attachment #8860045 - Flags: review?(xidorn+moz)
Comment on attachment 8860046 [details]
Bug 1355408 - stylo: Support @-moz-document rule. Part 3: Update test expectations.

https://reviewboard.mozilla.org/r/132070/#review135114
Attachment #8860046 - Flags: review+
Thank you Xidorn!

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #15)
> Comment on attachment 8859269 [details]
> 
> Rather than converting between strings and url forth and back, would it be
> better to just pass document, pattern string, and matching type (url, url
> prefix, domain, regex) into a single FFI function? You can probably factor
> out part of `DocumentRule::UseForPresentation` so that Gecko and Stylo share
> the same code path for evaluation this.
> 

As I mentioned in comment 5, I opted to do it this way to share as much code as possible between Stylo and Servo, so we don't have two different ways of evaluating the same thing in Servo's code. But my assumption was obviously that we wanted @-moz-document for Servo as well. If this is not the case, I agree that what you propose is better. Thanks for the suggestion.

Manish, Emilio, Simon, may I have your opinion about whether we should support @-moz-document in Servo or not? Thank you!
Flags: needinfo?(simon.sapin)
Flags: needinfo?(manishearth)
Flags: needinfo?(emilio+bugs)
I think even if we want to support @-moz-document, it is still not that bad to just have separate functions for evaluation for Servo and Stylo.
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review135182

::: layout/style/ServoBindings.cpp:1814
(Diff revision 2)
> +Gecko_IsPatternMatching(const uint8_t* aValue, uint32_t aValueLength,
> +                        const uint8_t* aPattern, uint32_t aPatternLength,

I think we can construct a `nsCString` which shares the string buffer, and pass it into C++ code. But it seems we never use it anywhere, so I'm fine if you don't do that here.

The first use of that probably also need check from manish or mystor.

::: layout/style/ServoBindings.cpp:1821
(Diff revision 2)
> +                        RawGeckoDocumentBorrowedOrNull aDocument)
> +{
> +  nsString valueStr;
> +  nsDependentCSubstring value(reinterpret_cast<const char*>(aValue),
> +                              aValueLength);
> +  AppendUTF8toUTF16(value, valueStr);

You can just use `NS_ConvertUTF8ToUTF16(value)` at the place you need the `nsAString`.
> Manish, Emilio, Simon, may I have your opinion about whether we should support @-moz-document in Servo or not? Thank you!

On this I think we should do whatever is easiest for Stylo and not worry about support in non-Stylo Servo, especially as long as this rule is not standardized. (It used to be in https://drafts.csswg.org/css-conditional/ but was deferred so that unresolved issues in @document would not block @supports.)
Flags: needinfo?(simon.sapin)
I feel like we should do the Stylo bits now and not worry about the servo part, but design  it in a way that makes it easy to add servo support later.

We had discussed this rule a bit when implementing @supports, I think. The regex would have to use geckos regex so as not to change behavior; Rusts regex syntax is not the JS syntax gecko uses (however it would be okay to use rust regexes for pure servo support IMO)

I don't have an objection to landing support in servo aside from it being extra work. It's nonstandard, and we avoid implementing nonstandard stuff in servo, but it's also something that will probably happen in Level 4 ... *shrug*
Flags: needinfo?(manishearth)
I do have an objection with exposing this to web content in Servo before it is standardized, especially if it is with Rust regex syntax. “What is even a regex” is one of the unresolved issues that blocked standardization.
Ok, thank you for the feedback, folks. I'll remove Servo's support for now then.
Flags: needinfo?(emilio+bugs)
Attachment #8860046 - Attachment is obsolete: true
I have addressed all the feedback and fixed the code to make @-moz-document work not only on author sheets but also with user and UA sheets. The patch is only implementing support for Stylo and it is using DocumentRule::UseForPresentation as suggested.

Apart from bug 1358524, I think there's only one missing piece to make the tests pass. I am not parsing the URL matching functions properly. All the examples in https://www.w3.org/TR/2012/WD-css3-conditional-20120911/#at-document are using strings for the URL matching functions. However the tests in Gecko aren't. And so with these patches something like url_prefix("http://localhost") works, but url_prefix(http://localhost) isn't working.
Depends on: 1358524
Wow, Gecko makes the tokenizer go into a special mode just for the url-prefix() and domain() functions…

layout/style/nsCSSScanner.h:

  // Get the body of an URL token (everything after the 'url(').
  // This is exposed for use by nsCSSParser::ParseMozDocumentRule,
  // which, for historical reasons, must make additional function
  // tokens behave like url().  Please do not add new uses to the
  // parser.
  void NextURL(nsCSSToken& aTokenResult);

Is this "must" still relevant? Can we remove this syntax quirk and require quotes? I only find a couple uses without quotes in the tree. I rather strongly would prefer not to add this quirk to rust-cssparser.
Given that this is not a standard feature yet and that the draft attempt for CSS3 seems to consider the quotes requirement [1], I'm fine simply fixing the tests to use quotes. However, we should update the MDN docs [2] and notify to extension authors about this change as well. FWIW, Stylish seems to be using the unquoted form only for its unit tests.

[1] https://www.w3.org/TR/2012/WD-css3-conditional-20120911/#at-document
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/@document
So the risk is breaking not only Stylish’s unit tests but also the user stylesheets of many Stylish users…
Looping in the Stylish maintainer.

Jason, as context here, we're replacing Gecko's CSS engine with Servo's, with the intent to ship in Firefox 57. Stylish will be a WebExtension at that point, right? Can you give us a sense of whether a lot of user sheets would break if we dropped support for the unquoted form?
Flags: needinfo?(jason.barnabe)
The question is whether user stylesheets depend on that syntax. I think we can probably check data on userstyles.org and see how common is that syntax used, if possible.
I did an analysis of the styles at userstyles.org and these are my findings:

- Total number of styles analyzed: 14,732
- 493 appearances of unquoted domain() rules in 361 styles. This is ~2.45% of styles.
- 2137 appearances of unquoted url-prefix() rules in 410 styles. This is ~2.78% of styles.
- No appearances of unquoted regexp() rules.
It doesn't seem to me unquoted regexp() is allowed, so that is not surprised.

Given the number, I guess it is probably better supporting them. Is it very complicated? I suppose we just need to extend the parsing of <url-token> to support them.
One option is to use something like this after finding a Function("url-prefix") or Function("domain") token:

input.parse_nested_block(|input| {
    let start = input.position();
    while let Ok(_) = input.next() {}
    Ok(input.slice_from(start).to_string())
})

… ignoring tokens and then taking a slice of the CSS input. This is not quite the same because it requires {}, [], and () blocks to be properly nested whereas the unquoted url() token doesn’t and just looks for the next ')'. (https://drafts.csswg.org/css-syntax/#consume-url-token) It also doesn’t do backslash-escaping. But maybe that’s close enough for real stylesheets?
I am no longer the maintainer of Stylish.
Flags: needinfo?(jason.barnabe)
Thanks for the suggestion Simon.

The attached patches make test_moz_document_rules.html and test_bug511909.html pass. It seems that there are still some CSSOM changes required to make test_rule_serialization.html and test_condition_text.html pass though.
(In reply to Simon Sapin (:SimonSapin) from comment #35)
> One option is to use something like this after finding a
> Function("url-prefix") or Function("domain") token:
> 
> input.parse_nested_block(|input| {
>     let start = input.position();
>     while let Ok(_) = input.next() {}
>     Ok(input.slice_from(start).to_string())
> })
> 
> … ignoring tokens and then taking a slice of the CSS input. This is not
> quite the same because it requires {}, [], and () blocks to be properly
> nested whereas the unquoted url() token doesn’t and just looks for the next
> ')'. (https://drafts.csswg.org/css-syntax/#consume-url-token) It also
> doesn’t do backslash-escaping. But maybe that’s close enough for real
> stylesheets?

Couldn't we just use input.parse_until_before with proper delimiters set?
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review137524

::: layout/style/URLMatchingFunction.h:17
(Diff revision 4)
> +
> +/**
> + * Enum defining the type of URL matching function for a @-moz-document rule
> + * condition.
> + */
> +enum URLMatchingFunction {

`enum class` please, so that these values are scoped.

::: layout/style/nsCSSRules.h:131
(Diff revision 4)
>    virtual bool UseForPresentation(nsPresContext* aPresContext,
> -                                    nsMediaQueryResultCacheKey& aKey) override;
> +                                  nsMediaQueryResultCacheKey& aKey) override;
>  
>    bool UseForPresentation(nsPresContext* aPresContext);
>  
> -  enum Function {
> +  static bool UseForPresentation(nsIDocument* aDoc,

I would probably prefer we use a different name for this function from the methods... but I guess I can live with just using this.

::: servo/components/style/Cargo.toml:68
(Diff revision 4)
>  kernel32-sys = "0.2"
>  
>  [build-dependencies]
>  lazy_static = "0.2"
>  log = "0.3"
> -bindgen = { version = "0.22", optional = true }
> +bindgen = {version = "0.22", optional = true}

This seems to be a unrelated change.

::: servo/components/style/build_gecko.rs:306
(Diff revision 4)
>              .include(add_include("mozilla/LookAndFeel.h"))
>              .include(add_include("mozilla/ServoBindings.h"))
>              .include(add_include("nsCSSFontFaceRule.h"))
>              .include(add_include("nsMediaFeatures.h"))
>              .include(add_include("nsMediaList.h"))
> +            .include(add_include("mozilla/css/Loader.h"))

Why? Why not just add `mozilla/css/URLMatchingFunction.h`?

::: servo/components/style/document.rs:5
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +//! [@document rules](https://www.w3.org/TR/2012/WD-css3-conditional-20120911/#at-document)

`document` doesn't sound like a good mod name here. It feels like it is something about the document, but it is just some helpers for document rule.

It's not clear what name should it be. Probably just `document_rule` and define `DocumentRule` in this mod as well.

::: servo/components/style/document.rs:43
(Diff revision 4)
> +                loop {
> +                    match input.next() {
> +                        Ok(Token::QuotedString(value)) => {
> +                            return Ok(UrlMatchingFunction::UrlPrefix(value.into_owned()));
> +                        },
> +                        Ok(_) => {},
> +                        Err(_) => { break; }
> +                    };

This doesn't look very correct... If I have something like `url-prefix(a "b" c)`, it seems this would just return `b`.

Although this is probably rare... and invalid...

I guess we can do something like
```rust
input.parse_nested_block(|input| {
    input.try(|input| {
        input.parse_entirely(|input| {
            match input.next() {
                Ok(Token::QuotedString(value) => Ok(value.into_owned()),
                _ => Err(())
            }
        })
    }).or_else(|_| {
        while let Ok(_) = input.next() {}
        Ok(input.slice_from(start).to_string())
    })
})
```
which should be more correct.

::: servo/components/style/document.rs:55
(Diff revision 4)
> +            input.parse_nested_block(|input| {
> +                let start = input.position();
> +                loop {
> +                    match input.next() {
> +                        Ok(Token::QuotedString(value)) => {
> +                            return Ok(UrlMatchingFunction::Domain(value.into_owned()));
> +                        },
> +                        Ok(_) => {},
> +                        Err(_) => { break; }
> +                    };
> +                }
> +                Ok(UrlMatchingFunction::Domain(input.slice_from(start).to_string()))
> +            })

It would be great if you can abstract this into a separate function so that the same code can be shared between `url-prefix` and `domain`.

::: servo/components/style/document.rs:83
(Diff revision 4)
> +            /// Exact URL matching function. It evaluates to true whenever the
> +            /// URL of the document being styled is exactly the URL given.

I don't think it makes much sense to do documentation comment here. They should probably go to the declarations in enum.

::: servo/components/style/document.rs:86
(Diff revision 4)
> +                unsafe {
> +                    let url = nsCString::from(url.as_str());
> +                    Gecko_DocumentRule_UseForPresentation(&*device.pres_context,
> +                                                          &*url,
> +                                                          GeckoUrlMatchingFunction::eURL)
> +                }

Would it be simplier to just do something like
```rust
let func = match *self {
    UrlMatchingFunction::Url(_) => GeckoUrlMatchingFunction::eURL,
    ...
};
let pattern = nsCString::from(match *self {
    UrlMatchingFunction::Url(ref url) => url.as_str(),
    UrlMatchingFunction::UrlPrefix(ref pat) |
    UrlMatchingFunction::Domain(ref pat) |
    ... => pat,
});
unsafe {
    Gecko_DocumentRule_UseForPresentation(...);
}
```
?

::: servo/components/style/document.rs:150
(Diff revision 4)
> +            UrlMatchingFunction::Url(ref url) => {
> +                url.to_css(dest)
> +            },
> +            UrlMatchingFunction::UrlPrefix(ref url_prefix) => {
> +                dest.write_str("url-prefix(")?;
> +                dest.write_str(url_prefix)?;

You probably should use `serialize_string`.

::: servo/components/style/document.rs:155
(Diff revision 4)
> +                dest.write_str(url_prefix)?;
> +                dest.write_str(")")
> +            },
> +            UrlMatchingFunction::Domain(ref domain) => {
> +                dest.write_str("domain(")?;
> +                dest.write_str(domain)?;

And here.

::: servo/components/style/document.rs:160
(Diff revision 4)
> +                dest.write_str(domain)?;
> +                dest.write_str(")")
> +            },
> +            UrlMatchingFunction::RegExp(ref regex) => {
> +                dest.write_str("regexp(")?;
> +                dest.write_str(regex)?;

And here.

::: servo/components/style/stylesheets.rs:1192
(Diff revision 4)
> +            "-moz-document" => {
> +                if cfg!(feature = "gecko") {
> +                    let cond = DocumentCondition::parse(self.context, input)?;
> +                    Ok(AtRuleType::WithBlock(AtRulePrelude::Document(cond)))
> +                } else {
> +                    unimplemented!()

It should be `Err(())`. It is not good to crash Servo when someone just adds a `@-moz-document` to their stylesheet.

::: servo/components/style/stylesheets.rs:1247
(Diff revision 4)
> +                    Ok(CssRule::Document(Arc::new(self.shared_lock.wrap(DocumentRule {
> +                        condition: cond,
> +                        rules: self.parse_nested_rules(input, CssRuleType::Document),
> +                    }))))
> +                } else {
> +                    unimplemented!()

It should be `unreachable!` instead.
Attachment #8859269 - Flags: review?(xidorn+moz)
Comment on attachment 8860045 [details]
Bug 1355408 - Part 2: Update test expectations after @-moz-document support for stylo.

https://reviewboard.mozilla.org/r/132068/#review137566
Attachment #8860045 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #40)
> Couldn't we just use input.parse_until_before with proper delimiters set?

That is effectively what parse_nested_block does. Both maintain block nesting.

From cssparser/src/parser.rs:

    pub fn next_including_whitespace_and_comments(&mut self) -> Result<Token<'i>, ()> {
        if let Some(block_type) = self.at_start_of.take() {
            consume_until_end_of_block(block_type, &mut *self.tokenizer);
        }
        // ...
Blocks: 1361994
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review139438

r=me with comments addressed.

::: layout/style/nsCSSRules.cpp:701
(Diff revision 5)
> +                                 URLMatchingFunction aUrlMatchingFunction)
> +{
> +  switch (aUrlMatchingFunction) {
> +    case URLMatchingFunction::eURL: {
> +      if (aDocURISpec == aPattern)
> -          return true;
> +        return true;

As far as you are here, please wrap the body of `if` with `{` and `}`.

::: layout/style/nsCSSRules.cpp:710
(Diff revision 5)
> -          return true;
> +        return true;
> -      } break;
> +    } break;
> -      case eDomain: {
> +    case URLMatchingFunction::eDomain: {
> -        nsAutoCString host;
> +      nsAutoCString host;
> -        if (docURI)
> -          docURI->GetHost(host);
> +      if (aDocURI)
> +        aDocURI->GetHost(host);

ditto.

::: layout/style/nsCSSRules.cpp:714
(Diff revision 5)
> -        if (docURI)
> -          docURI->GetHost(host);
> -        int32_t lenDiff = host.Length() - url->url.Length();
> +      if (aDocURI)
> +        aDocURI->GetHost(host);
> +      int32_t lenDiff = host.Length() - aPattern.Length();
> -        if (lenDiff == 0) {
> +      if (lenDiff == 0) {
> -          if (host == url->url)
> +        if (host == aPattern)
> -            return true;
> +          return true;

ditto.

::: layout/style/nsCSSRules.cpp:718
(Diff revision 5)
> -          if (host == url->url)
> +        if (host == aPattern)
> -            return true;
> +          return true;
> -        } else {
> +      } else {
> -          if (StringEndsWith(host, url->url) &&
> +        if (StringEndsWith(host, aPattern) &&
> -              host.CharAt(lenDiff - 1) == '.')
> +            host.CharAt(lenDiff - 1) == '.')
> -            return true;
> +          return true;

ditto.

::: servo/components/style/document_condition.rs:59
(Diff revision 5)
> +                    Ok(Token::QuotedString(value)) =>
> +                        Ok($url_matching_function(value.into_owned())),
> +                    _ => Err(()),
> +                }
> +            }).or_else(|_| {
> +                while let Ok(_) = input.next() {}

Please file a bug against bug 1337166 to describe the difference of this and the existing behavior in Gecko (which reuses the same mechanism as parsing a url-token).

We can probably live with this, but it should be documented as far as we know we are changing the behavior.

(Since a url-token is invalid when there is any bracket or quotes inside, we can probably simulate that via checking the token in the loop. It may not be worth that effort, though.)

::: servo/components/style/document_condition.rs:146
(Diff revision 5)
> +///
> +/// The `@document` rule's condition is written as a comma-separated list of
> +/// URL matching functions, and the condition evaluates to true whenever any
> +/// one of those functions evaluates to true.
> +#[derive(Debug)]
> +pub struct DocumentCondition(Vec<UrlMatchingFunction>);

You can just do
```rust
pub type DocumentCondition = Vec<...>;
impl OneOrMoreCommaSeparated for DocumentCondition {}
```
then you would get the `Parse` and `ToCss` for free.

I'm fine either way.

::: servo/components/style/stylesheets.rs:374
(Diff revision 5)
>      /// Note that only some types of rules can contain rules. An empty slice is
>      /// used for others.
>      ///
>      /// This will not recurse down unsupported @supports rules
> -    pub fn with_nested_rules_and_mq<F, R>(&self, guard: &SharedRwLockReadGuard, mut f: F) -> R
> -    where F: FnMut(&[CssRule], Option<&MediaList>) -> R {
> +    pub fn with_nested_rules_mq_and_doc_rule<F, R>(&self, guard: &SharedRwLockReadGuard, mut f: F) -> R
> +    where F: FnMut(&[CssRule], Option<&MediaList>, Option<&DocumentRule>) -> R {

It seems the callback would be invoked only in one of three states:
1. None for both media query and document condition
2. Only with media query
3. Only with document rule

Would it be better to make it a tri-state enum rather than two `Option`s?

::: servo/components/style/stylist.rs:606
(Diff revision 5)
>                      if let Some(mq) = mq {
>                          if mq.evaluate(before, quirks_mode) != mq.evaluate(after, quirks_mode) {
>                              return true
>                          }
>                      }
>                      mq_eval_changed(guard, rules, before, after, quirks_mode)

Should we skip unmatched document rule here? Otherwise, if a user has `@media` in some of their user stylesheet scoped by `@-moz-document`, that media would be evaluate and potentially trigger a restyle in all documents even unrelated.

I guess that may not be a big deal, but it's probably worth at least having a comment here noting that.
Attachment #8859269 - Flags: review?(xidorn+moz) → review+
Also I still think it might be better to evaluate the document rule condition during parsing rather than during collecting effective rules, since document rules are more expensive to evaluate.

That could be in a followup low-priority bug, though.
Comment on attachment 8860045 [details]
Bug 1355408 - Part 2: Update test expectations after @-moz-document support for stylo.

https://reviewboard.mozilla.org/r/132068/#review139452

Please remember to remove fails-if(stylo) from usercss-moz-document.html that bug 1358524 added.
Comment on attachment 8859269 [details]
Bug 1355408 - Part 1: Support @-moz-document rule in stylo.

https://reviewboard.mozilla.org/r/131284/#review139438

> Please file a bug against bug 1337166 to describe the difference of this and the existing behavior in Gecko (which reuses the same mechanism as parsing a url-token).
> 
> We can probably live with this, but it should be documented as far as we know we are changing the behavior.
> 
> (Since a url-token is invalid when there is any bracket or quotes inside, we can probably simulate that via checking the token in the loop. It may not be worth that effort, though.)

Bug 1362333
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/452b80f72738
Part 1: Support @-moz-document rule in stylo. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/2b61a7f13710
Part 2: Update test expectations after @-moz-document support for stylo. r=xidorn
Attached patch Follow up — — Splinter Review 

        Attachment #8865904 -
        Flags: review?(emilio+bugs)

        Attachment #8865904 -
        Flags: review?(emilio+bugs) → review+

    
    

    
  
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/991e7eaf8b03
Follow up: properly update test expectations. r=emilio

    
    

    
  
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc5e49a131d5
Re-update test expectation data a=bustage CLOSED TREE

    
    

    
  
Comment on attachment 8865904 [details] [diff] [review]
Follow up

Review of attachment 8865904 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/stylo-failures.md
@@ +79,5 @@
>    * test_bug829816.html [8]
>  * \@counter-style support bug 1328319
>    * test_counter_descriptor_storage.html [1]
>    * test_counter_style.html [5]
> +  * test_rule_insertion.html `@counter-style` and bug 1361994 (CSSOM support for @-moz-document) [22]

This doesn't match the syntax.

It should be
* test_rule_insertion.html `@counter-style` [22]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: