The default bug view has changed. See this FAQ.

stylo: Need will-change support

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
27 days ago
11 hours ago

People

(Reporter: bz, Assigned: canaltinova)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 affected, firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

https://github.com/servo/servo/issues/15706
Nazim is working on this.
Assignee: nobody → canaltinova
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 days ago
I needed to push here the PR in the Servo(https://github.com/servo/servo/pull/15813) because of gecko side changes. Part 1 and 2 is the same as Servo side. In the third part glue changed and 3 willChange bindings added. 

Manishearth suggested to write directly unsafe bindings instead of sugar because this is the only usage of nsCOMArray anyway. (And honestly, I think this was easier :) )

Comment 6

3 days ago
mozreview-review
Comment on attachment 8848865 [details]
Bug 1342139 - Stylo: Update test expectations for will-change property

https://reviewboard.mozilla.org/r/121744/#review123762

r=me with the `to_owned` issue addressed.

::: servo/components/style/properties/longhand/box.mako.rs:2003
(Diff revision 1)
> +    impl ToCss for SpecifiedValue {
> +        fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +            match *self {
> +                SpecifiedValue::Auto => dest.write_str("auto"),
> +                SpecifiedValue::AnimateableFeatures(ref features) => {
> +                    let mut iter = features.iter();

Using `let (first, rest) = features.split_first().unwrap();` here instead may make the code a bit nicer. I don't have strong preference here, though.

::: servo/components/style/properties/longhand/box.mako.rs:2033
(Diff revision 1)
> +                let ident = i.expect_ident()?;
> +                match_ignore_ascii_case! { &ident,
> +                    "will-change" | "none" | "all" | "auto" |
> +                    "initial" | "inherit" | "unset" => Err(()),
> +                    _  => {
> +                        Ok((Atom::from(ident.to_owned())))

Do we need `to_owned` here? `expect_ident` returns `Cow<'i, str>` and it seems `Atom::from` support `Cow<'a, str>` directly.
Attachment #8848865 - Flags: review?(xidorn+moz) → review+

Comment 7

3 days ago
mozreview-review
Comment on attachment 8848866 [details]
Bug 1342139 - Part 2: Add flags to longhands for gecko glue

https://reviewboard.mozilla.org/r/121746/#review123764

r=me if it is same as what I reviewed in GitHub.
Attachment #8848866 - Flags: review?(xidorn+moz) → review+
(In reply to Nazım Can Altınova [:canaltinova] from comment #5)
> Manishearth suggested to write directly unsafe bindings instead of sugar
> because this is the only usage of nsCOMArray anyway. (And honestly, I think
> this was easier :) )

True it is currently the only usage... but we will have more once we convert some other properties to use Atom as well, e.g. animation-name and transform-property.

But... well, I guess it's fine for now.

Comment 9

3 days ago
mozreview-review
Comment on attachment 8848867 [details]
Bug 1342139 - Implement gecko bindings for will-change

https://reviewboard.mozilla.org/r/121748/#review123768
Attachment #8848867 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> True it is currently the only usage... but we will have more once we convert
> some other properties to use Atom as well, e.g. animation-name and
> transform-property.

Oh, those store the string as part of a larger struct... so, ok, probably this would be the only usage for a while.
(Assignee)

Comment 11

3 days ago
mozreview-review
Comment on attachment 8848865 [details]
Bug 1342139 - Stylo: Update test expectations for will-change property

https://reviewboard.mozilla.org/r/121744/#review123856

::: servo/components/style/properties/longhand/box.mako.rs:2003
(Diff revision 1)
> +    impl ToCss for SpecifiedValue {
> +        fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +            match *self {
> +                SpecifiedValue::Auto => dest.write_str("auto"),
> +                SpecifiedValue::AnimateableFeatures(ref features) => {
> +                    let mut iter = features.iter();

Yes, this looks better. I'll change that.

::: servo/components/style/properties/longhand/box.mako.rs:2033
(Diff revision 1)
> +                let ident = i.expect_ident()?;
> +                match_ignore_ascii_case! { &ident,
> +                    "will-change" | "none" | "all" | "auto" |
> +                    "initial" | "inherit" | "unset" => Err(()),
> +                    _  => {
> +                        Ok((Atom::from(ident.to_owned())))

We can't remove `to_owned()` here because `match_ignore_ascii_case` macro borrows ident variable here and we need to own it inside `match_ignore_ascii_case` macro.
(Assignee)

Comment 12

3 days ago
mozreview-review-reply
Comment on attachment 8848866 [details]
Bug 1342139 - Part 2: Add flags to longhands for gecko glue

https://reviewboard.mozilla.org/r/121746/#review123764

Yes, I just imported the same commit here.

Comment 13

3 days ago
mozreview-review-reply
Comment on attachment 8848865 [details]
Bug 1342139 - Stylo: Update test expectations for will-change property

https://reviewboard.mozilla.org/r/121744/#review123856

> We can't remove `to_owned()` here because `match_ignore_ascii_case` macro borrows ident variable here and we need to own it inside `match_ignore_ascii_case` macro.

Then, `clone()`, I guess. Cloning a `Cow` should usually be cheap if it borrows the content rather than owns it.

Comment 14

3 days ago
mozreview-review
Comment on attachment 8848867 [details]
Bug 1342139 - Implement gecko bindings for will-change

https://reviewboard.mozilla.org/r/121748/#review123864

::: layout/style/ServoBindings.cpp:1121
(Diff revision 1)
> +void
> +Gecko_SetWillChangeItem(nsStyleDisplay* aDisplay, nsIAtom* aAtom, uint32_t aIndex)
> +{
> +  aDisplay->mWillChange.InsertElementAt(aIndex, aAtom);
> +}

You don't really need to use `InsertElement`. You should probably just append each atom at the end of the array.

::: layout/style/ServoBindings.cpp:1130
(Diff revision 1)
> +  aDest->mWillChange.SetCount(aSrc->mWillChange.Length());
> +  aDest->mWillChange.SwapElements(aSrc->mWillChange);

This should be
```c++
aDest->mWillChange.Clear();
aDest->mWillChange.AppendElements(aSrc->mWillChange);
```

Swaping elements doesn't seem to be the right action for "Copy...From".
Attachment #8848867 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

3 days ago
mozreview-review
Comment on attachment 8848867 [details]
Bug 1342139 - Implement gecko bindings for will-change

https://reviewboard.mozilla.org/r/121748/#review123872
Attachment #8848867 - Flags: review?(xidorn+moz) → review+

Comment 19

3 days ago
mozreview-review
Comment on attachment 8848867 [details]
Bug 1342139 - Implement gecko bindings for will-change

https://reviewboard.mozilla.org/r/121748/#review123874

::: servo/components/style/properties/gecko.mako.rs:1993
(Diff revision 2)
> +                            },
> +                        }
> +                    }
> +                }
> +            },
> +            T::Auto => (),

You probably also need to clear the array here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8848866 - Attachment is obsolete: true
Moved relevant Servo parts back to https://github.com/servo/servo/pull/15813 and updated the test expectations.

Comment 24

18 hours ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d08e6d268c84
Implement gecko bindings for will-change r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ce6384737e6e
Stylo: Update test expectations for will-change property r=xidorn

Comment 25

11 hours ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d08e6d268c84
https://hg.mozilla.org/mozilla-central/rev/ce6384737e6e
Status: NEW → RESOLVED
Last Resolved: 11 hours ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.