Closed Bug 1337229 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: boris, Assigned: shinglyu, Mentored)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → slyu
Attachment #8837028 - Flags: feedback?(boris.chiou)
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
Attachment #8837028 - Flags: review?(hikezoe)
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;
Attachment #8837028 - Flags: review?(hikezoe) → review+
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 `==`.
Attachment #8837028 - Flags: review?(manishearth)
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.
Flags: needinfo?(slyu)
Got it!
Flags: needinfo?(slyu)
Comment on attachment 8837028 [details]
Bug 1337229 - Implement the deep operator== for RawServoAnimationValue

https://reviewboard.mozilla.org/r/112300/#review114616
Attachment #8837028 - Flags: review?(manishearth) → review+
Blocks: 1338927
Status: NEW → ASSIGNED
Just finished the rebase, pushing a try to prevent any surprise

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4047db2eab8393d34af9675650acf7b059dbd7fb
Shing, you should include StyleAnimationValueInlines.h in KeyframeEffectReadOnly.h instead of KeyframeEffectReadOnly.cpp because there is operator==.
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 :(
(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.
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.
(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 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
Attachment #8837028 - Flags: review?(hikezoe)
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.
Attachment #8837028 - Flags: review?(hikezoe) → review+
I just removed the line and it works fine, I'll trigger an autoland from MozReview then.
Pushed by slyu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d21596af124
Implement deep operator== for RawServoAnimationValue r=hiro,manishearth
Shing, we should strip servo side changes and send a PR to servo separately.

Thank you,
Flags: needinfo?(slyu)
Sorry. I've stripped the servo side patch into https://github.com/servo/servo/pull/15720
Flags: needinfo?(slyu)
Pushed by slyu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7967050a8fa0
Implement the deep operator== for RawServoAnimationValue r=hiro,manishearth
https://hg.mozilla.org/mozilla-central/rev/7967050a8fa0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.