1329169 - Store StyleAnimation's name as nsAtom

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

See https://github.com/servo/servo/pull/14879#discussion_r94933215

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Thanks for filling. To be clear, I think it's worth to do it, at least from the stylo point of view, since we have much better integration between Servo and Gecko atoms that between Servo and Gecko strings.

I expect animation names to be relatively short, and copied frequently. If that is true also for Gecko, this might be a good thing to do.

Comment 2

[Cameron McCormack (:heycam)]

FWIW in Gecko copying a string is usually cheap, since (except in certain cases) the underlying string buffer is shared with a COW scheme.  It looks like nsString and friends don't do a pointer comaprison to short circuit comparisons.  Atoms helps there.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

bholley, this is what I mentioned in the meeting for using atom in animation name.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Oh, this is not a stylo bug, but something we want to change for stylo.

Comment 5

[Daniel Holbert [:dholbert]]

(It's OK that this isn't going to make it for the initial stylo release in 57, right?  Tentatively setting "wontfix" for 57 since this is unassigned and 57 is nearly off of Nightly, and this doesn't sound critical.)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Right. We have no time to do this in 57.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, this is a waste of memory and conversion on animation name strings... but that's probably not a big deal I guess.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/try/rev/d21519a5cbfafa636b8bb09e67acea3e525d8c4b

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1329169 - Use atom for animation-name property.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

https://reviewboard.mozilla.org/r/185536/#review191216

Forgot to set review request.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

https://reviewboard.mozilla.org/r/185536/#review191274

r=me with the issues addressed.

::: layout/style/ServoBindings.cpp:649
(Diff revision 1)
> +Gecko_CopyAnimationNames(RawGeckoStyleAnimationListBorrowed aDest,
> +                         RawGeckoStyleAnimationListBorrowed aSrc)
> +{
> +  nsStyleAutoArray<StyleAnimation>* destArray =
> +    const_cast<nsStyleAutoArray<StyleAnimation>*>(aDest);

You should add
```c++
DECL_BORROWED_MUT_REF_TYPE_FOR(RawGeckoStyleAnimationList)
```
in ServoBindingTypes.h, and use `RawGeckoStyleAnimationListBorrowedMut` for `aDest` rather than passing a const pointer and do `const_cast` like this.

This is important for soundness and static analysis. And you would probably need to add something to analyzeHeapWrites.js to tell it that `aDest` is a safe argument.

::: layout/style/nsCSSRules.cpp:1748
(Diff revision 1)
> -  aCssText.Append(mName);
> +  nsDependentAtomString name(mName);
> +  aCssText.Append(name);

I think you can do
```c++
aCssText.Append(nsDependentAtomString(mName));
```
directly.

::: servo/components/style/properties/gecko.mako.rs:3276
(Diff revision 1)
> +        use gecko_bindings::structs::nsIAtom;
> +        use gecko_string_cache::atom_macro::nsGkAtoms__empty;
>          use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
> -        // XXX: Is there any effective ways?
> -        let atom = &self.gecko.mAnimations[index].mName;
> -        if atom.is_empty() {
> +
> +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> +        if atom == unsafe { nsGkAtoms__empty } {

Why couldn't we just use `atom!("")` here?

::: servo/components/style/properties/gecko.mako.rs:3279
(Diff revision 1)
> -        // XXX: Is there any effective ways?
> -        let atom = &self.gecko.mAnimations[index].mName;
> -        if atom.is_empty() {
> +
> +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> +        if atom == unsafe { nsGkAtoms__empty } {
>              AnimationName(None)
>          } else {
> -            AnimationName(Some(KeyframesName::from_ident(&atom.to_string())))
> +            AnimationName(Some(KeyframesName::from_atom((atom as *mut nsIAtom).into())))

I'm confused. Why do you need the `as *mut nsIAtom` here? Shouldn't atom have already been that type?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Thanks for the review!

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> ::: layout/style/ServoBindings.cpp:649
> (Diff revision 1)
> > +Gecko_CopyAnimationNames(RawGeckoStyleAnimationListBorrowed aDest,
> > +                         RawGeckoStyleAnimationListBorrowed aSrc)
> > +{
> > +  nsStyleAutoArray<StyleAnimation>* destArray =
> > +    const_cast<nsStyleAutoArray<StyleAnimation>*>(aDest);
> 
> You should add
> ```c++
> DECL_BORROWED_MUT_REF_TYPE_FOR(RawGeckoStyleAnimationList)
> ```
> in ServoBindingTypes.h, and use `RawGeckoStyleAnimationListBorrowedMut` for
> `aDest` rather than passing a const pointer and do `const_cast` like this.
> 
> This is important for soundness and static analysis. And you would probably
> need to add something to analyzeHeapWrites.js to tell it that `aDest` is a
> safe argument.

Oh right.  I will fix it and push a try with hazzard tests.

> ::: servo/components/style/properties/gecko.mako.rs:3276
> (Diff revision 1)
> > +        use gecko_bindings::structs::nsIAtom;
> > +        use gecko_string_cache::atom_macro::nsGkAtoms__empty;
> >          use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
> > -        // XXX: Is there any effective ways?
> > -        let atom = &self.gecko.mAnimations[index].mName;
> > -        if atom.is_empty() {
> > +
> > +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> > +        if atom == unsafe { nsGkAtoms__empty } {
> 
> Why couldn't we just use `atom!("")` here?

Will do.

> ::: servo/components/style/properties/gecko.mako.rs:3279
> (Diff revision 1)
> > -        // XXX: Is there any effective ways?
> > -        let atom = &self.gecko.mAnimations[index].mName;
> > -        if atom.is_empty() {
> > +
> > +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> > +        if atom == unsafe { nsGkAtoms__empty } {
> >              AnimationName(None)
> >          } else {
> > -            AnimationName(Some(KeyframesName::from_ident(&atom.to_string())))
> > +            AnimationName(Some(KeyframesName::from_atom((atom as *mut nsIAtom).into())))
> 
> I'm confused. Why do you need the `as *mut nsIAtom` here? Shouldn't atom
> have already been that type?

You are right. The cast is not necessay, I did miss that mRawPtr is *mut.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=126edaad4f1f9d5d2cd15c0feda8f187dcae375f
Gecko_SetAnimationName was also needed to be added in treatAsSafeArgument in analyzeHeapWrites.js.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

FWIW, it seems `DECL_BORROWED_MUT_REF_TYPE_FOR` declarations are usually put before their corresponding `DECL_BORROWED_REF_TYPE_FOR` declarations.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> FWIW, it seems `DECL_BORROWED_MUT_REF_TYPE_FOR` declarations are usually put
> before their corresponding `DECL_BORROWED_REF_TYPE_FOR` declarations.

Thanks! Changed the order locally.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

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

Comment 19

[Jeremy Chen ([e-mail])]

Correct the summary since we renamed nsIAtom as nsAtom in Bug 1400460.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ed2f3c94155
Use atom for animation-name property. r=xidorn

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5ed2f3c94155