1344314 - stylo: Setup for pref checks is broken for aliases

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

<div style="width: 100px; height: 100px; background: green;
            transform: rotate(45deg); -moz-transform-origin: 0 0"></div>

Load that in stylo and Gecko, with "layout.css.prefixes.transforms" set to false.  stylo still applies the -moz-transform-origin.

Comment 1

[Bobby Holley (:bholley)]

Matt, unless you already have patches, I'm going to throw this back in the pile. I think bug 1349651 and others are more urgent.

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1344314 - stylo: Add an aliasid field to CSS_PROP_ALIAS macro and include them to bindings

This field will be needed for servo side alias pref handling on the
next patch.

Review commit: https://reviewboard.mozilla.org/r/167976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167976/

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

Review commit: https://reviewboard.mozilla.org/r/167978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167978/

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167978/diff/1-2/

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167978/diff/2-3/

Comment 6

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896670 [details]
Bug 1344314 - stylo: Add an aliasid field to CSS_PROP_ALIAS macro and include them to bindings

https://reviewboard.mozilla.org/r/167976/#review174266

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896670 [details]
Bug 1344314 - stylo: Add an aliasid field to CSS_PROP_ALIAS macro and include them to bindings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167976/diff/1-2/

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167978/diff/3-4/

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

https://reviewboard.mozilla.org/r/167978/#review174270

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167978/diff/4-5/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8896671 [details]
Bug 1344314 - stylo: Add a pref checking mechanism for alias properties

https://reviewboard.mozilla.org/r/167978/#review174362

r=me with nits below addressed.

::: servo/components/style/properties/properties.mako.rs:1012
(Diff revision 5)
>  
>  impl PropertyId {
>      /// Returns a given property from the string `s`.
>      ///
>      /// Returns Err(()) for unknown non-custom properties
> -    pub fn parse(property_name: &str) -> Result<Self, ()> {
> +    pub fn parse(property_name: &str, context: Option< &PropertyParserContext>) -> Result<Self, ()> {

Please mention in the doc that `None` would be treated as style rule context in author sheet by default.

::: servo/components/style/properties/properties.mako.rs:1270
(Diff revision 5)
> +    pub rule_type: CssRuleType,
> +}
> +
> +impl PropertyParserContext {
> +    /// Creates a PropertyParserContext with given stylesheet origin and rule type.
> +    pub fn new(stylesheet_origin: Origin, rule_type: CssRuleType) -> Self {

It is probably easier to use if it just takes a `ParserContext`. It seems the same pattern is used in different places.

::: servo/components/style/properties/properties.mako.rs:1278
(Diff revision 5)
> +impl Default for PropertyParserContext {
> +    fn default() -> Self {
> +        Self {
> +            stylesheet_origin: Origin::Author,
> +            rule_type: CssRuleType::Style,
> +        }
> +    }
> +}

If it is only used once... this probably doesn't need to exist at all.

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 8896670 [details]
Bug 1344314 - stylo: Add an aliasid field to CSS_PROP_ALIAS macro and include them to bindings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167976/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f84b7bbf5d8
stylo: Add an aliasid field to CSS_PROP_ALIAS macro and include them to bindings r=manishearth

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f84b7bbf5d8