Implement parsing/serialization and glue for will-change property #15813

canova · 2017-03-03T19:12:29Z

Implement parsing/serialization and glue for will-change property

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Stylo needs will-change support #15706 (github issue number if applicable).

There are tests for these changes

This change is

highfive · 2017-03-03T19:12:32Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/properties/data.py, components/style/properties/longhand/effects.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
@emilio: components/style/properties/gecko.mako.rs, components/style/properties/data.py, components/style/properties/longhand/effects.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/svg.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs

highfive · 2017-03-03T19:12:33Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

canova · 2017-03-03T19:14:38Z

Parsing/serialization of the will-change property is completed but the current glue is crashing in a stylo build. I'm doing something wrong when assigning values to mWillChange(root::nsTArray<::nsstring::nsStringRepr>) but still investigating the problem.
Do you have suggestions for me at constructing nsTArray?
r? @Manishearth @emilio

Manishearth · 2017-03-03T23:14:23Z

r? @upsuper ? I'm not quite familiar with the will-change stuff.

@canaltinova there are methods on nsTarray defined in gecko_bindings/sugar/ns_t_array.rs. You can use those to set its length, and then mutate elements. Be careful, the Rust code doesn't know how to run C++ side constructors, so setting the length will give you an array full of garbage data, into which you have to move the strings.

upsuper · 2017-03-04T08:00:35Z

Reviewed 3 of 3 files at r1, 6 of 6 files at r2.
Review status: 7 of 9 files reviewed at latest revision, 6 unresolved discussions.

components/style/properties/properties.mako.rs, line 455 at r2 (raw file):

    /// Returns true if property requires a stacking context.
    #[cfg(feature = "gecko")]
    pub fn creates_stacking_context(&self) -> bool {

Having three individual functions for things which are mostly used together seems to be bad both for code size and for performance. Could you merge these functions together and make it a bitflag? I guess this can become a general mechanism for having flags on properties.

components/style/properties/longhand/box.mako.rs, line 1972 at r1 (raw file):

                    let mut iter = features.iter();
                    // handle head element
                    try!(iter.next().unwrap().to_css(dest));

You can use the ? syntax.

components/style/properties/longhand/box.mako.rs, line 1988 at r1 (raw file):

    pub enum AnimateableFeature {
        ScrollPosition,
        Contents,

Actually I'm not sure whether it's worth distinguishing these two values from other identifiers. Effectively they are just another two identifiers. Doing so makes the code look more similiar to what the spec describes, but it takes a bit runtime cost. I guess I can live either way, though.

components/style/properties/longhand/box.mako.rs, line 1989 at r1 (raw file):

        ScrollPosition,
        Contents,
        CustomIdent(String),

Probably better using Atom so that we can avoid copying the string all the time from specified value to computed value, and that would

components/style/properties/longhand/box.mako.rs, line 1994 at r1 (raw file):

    impl Parse for AnimateableFeature {
        fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result<Self, ()> {
            if input.try(|input| input.expect_ident_matching("scroll-position")).is_ok() {

I don't think you need to add separate if-blocks for these two values. They can be merged into the match_ignore_ascii_case! block, right?

components/style/properties/longhand/box.mako.rs, line 2002 at r1 (raw file):

            match_ignore_ascii_case! { &input.expect_ident()?,
                "will-change" | "none" | "all" | "auto" => Err(()),

This may need to exclude CSS-wide keywords as well... or probably expect_ident should not accept those keywords as identifier at all. Not sure whether the parser is implemented that way. cc @SimonSapin

Comments from Reviewable

canova · 2017-03-04T16:03:53Z

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions.

components/style/properties/longhand/box.mako.rs, line 1972 at r1 (raw file):