1337229 - stylo: Implement the deep operator==() for RawServoAnimationValue

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Boris Chiou [:boris]]

We need to implement a deep operator==() for RawServoAnimationValue [1]. BtW, it will be moved into AnimationValue::operator==() by Bug 1335942 Comment 20.

[1] http://searchfox.org/mozilla-central/rev/f5077ad52f8b90183e73038869f6140f0afbf427/dom/animation/KeyframeEffectReadOnly.h#64

Hi, ShingLyu, I think you might be interesting in this.

Comment 1

[Shing Lyu [:shinglyu]]

Attachment: Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

What a nice! I did a tweak in my patch to avoid this issue. Now I can drop it! Thanks!

https://hg.mozilla.org/try/rev/65e5cbec218ee57ab207b409fccbb7ccefc73ea6#l7.111

Comment 3

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

https://reviewboard.mozilla.org/r/112300/#review113606

I thinks servo part needs to be reviewed by Manish.

::: layout/style/StyleAnimationValue.h:594
(Diff revision 2)
>    StyleAnimationValue mGecko;
>    RefPtr<RawServoAnimationValue> mServo;
>  
>    bool operator==(const AnimationValue& aOther) const {
> -    // FIXME: Bug 1337229: add a deep == impl for RawServoAnimationValue.
> -    return mGecko == aOther.mGecko && mServo == aOther.mServo;
> +    // mGecko and mServo are mutual exclusive, one of them must be null
> +    if (!mServo) {

You might want to use if (mServo) here.

::: layout/style/StyleAnimationValue.h:596
(Diff revision 2)
> +    } else {
> +      return mGecko == aOther.mGecko;
> +    }

We usually does not add else block in case of early return there.
We can write it to:

if (mServo) {
  return ...
}

return mGecko == aOther.mGecko;

Comment 5

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 6

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

Manish, would you mind reviewing the Servo side? I believe the derived PartialEq impls are OK so I just use `==`.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Shing, I just landed bug 1340033 which moves several inline functions from StyleAnimationValue.h to StyleAnimationValueInlines.h to remove the dependency from StyleAnimationValue.h to ServoBindings.h. You may need to rebase your patch on top of that and move the new operator== to StyleAnimationValueInlines.h as well.

Comment 8

[Shing Lyu [:shinglyu]]

Got it!

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

https://reviewboard.mozilla.org/r/112300/#review114616

Comment 10

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 11

[Shing Lyu [:shinglyu]]

Just finished the rebase, pushing a try to prevent any surprise

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4047db2eab8393d34af9675650acf7b059dbd7fb

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Shing, you should include StyleAnimationValueInlines.h in KeyframeEffectReadOnly.h instead of KeyframeEffectReadOnly.cpp because there is operator==.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Ah, that's sad... KeyframeEffectReadOnly.h is included by nsTransitionManager.h, which is included by tons of files... My effort to reduce file number to compile for changes to ServoBindings.h regresses immediately :(

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> Ah, that's sad... KeyframeEffectReadOnly.h is included by
> nsTransitionManager.h, which is included by tons of files... My effort to
> reduce file number to compile for changes to ServoBindings.h regresses
> immediately :(

Oh, nsTransitionmanager.h is included in so many files?  I think most of them are not necessary.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

OK. I think both of AnimationPropertySegment::operator== and AnimationProperty::operator== can be moved into KeyframeEffectReadOnly.cpp.  I did check we only use the operator in KeyframeEffectReadOnly::UpdateProperties(), if I am missing something.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> > Ah, that's sad... KeyframeEffectReadOnly.h is included by
> > nsTransitionManager.h, which is included by tons of files... My effort to
> > reduce file number to compile for changes to ServoBindings.h regresses
> > immediately :(
> 
> Oh, nsTransitionmanager.h is included in so many files?  I think most of
> them are not necessary.

It doesn't seem to be true anymore. I think the last time I checked, it was included in RestyleManager.h which is widely included. But it seems it is now only included in GeckoRestyleManager.h, which is not included that widely, so it's probably fine.

Comment 17

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 18

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

Hiro, would you mind a quick double check for my rebased patch? 

Here are the test results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcc852f9609

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

I can't stamp r+ on MozReview.
We can drop the line of include "mozilla/StyleAnimationValueInline.h" from KeyframeEffectReadOnly.cpp.

Comment 20

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

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

Comment 21

[Shing Lyu [:shinglyu]]

I just removed the line and it works fine, I'll trigger an autoland from MozReview then.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d21596af124
Implement deep operator== for RawServoAnimationValue r=hiro,manishearth

Comment 23

[Iris Hsiao [:ihsiao]]

backed this out for heycam's request
https://hg.mozilla.org/integration/autoland/rev/8a6084bc234c

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Shing, we should strip servo side changes and send a PR to servo separately.

Thank you,

Comment 25

[Shing Lyu [:shinglyu]]

Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112300/diff/6-7/

Comment 26

[Shing Lyu [:shinglyu]]

Sorry. I've stripped the servo side patch into https://github.com/servo/servo/pull/15720

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7967050a8fa0
Implement the deep operator== for RawServoAnimationValue r=hiro,manishearth

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7967050a8fa0