Closed Bug 1498948 Opened Last year Closed Last year

Refactor KeyframeEffect constructor to use r-value reference

Categories

(Core :: DOM: Animation, defect, P3)

64 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: dpino, Assigned: dpino)

References

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

https://bugzilla.mozilla.org/show_bug.cgi?id=1339675 refactors TimingParams to use `std::move`. After these refactoring, a local TimingParams variable is passed to KeyframeEffect by reference, since that's what KeyframeEffect constructor expects, instead of using `std::move`.

This issue is a follow-up of https://bugzilla.mozilla.org/show_bug.cgi?id=1339675. https://bugzilla.mozilla.org/show_bug.cgi?id=1339675#c17 suggest to pass `timing` using `std::move` and get rid of the TimingParams "call by reference" in KeyframeEffect constructor.
Here's a patch. I had to temporarily store some TimingParams values into local variables to make it work. I'm not sure if that's right.
Attachment #9017049 - Flags: review?(boris.chiou)
Depends on: 1339675
Assignee: nobody → dpino
Status: UNCONFIRMED → ASSIGNED
Component: General → DOM: Animation
Ever confirmed: true
Priority: -- → P3
Comment on attachment 9017049 [details] [diff] [review]
Refactor-KeyframeEffect-constructor-to-use-r-value-r.patch

Review of attachment 9017049 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I'm happy to give you r+ after addressed the following nits. Thanks.

::: dom/animation/KeyframeEffect.cpp
@@ +874,4 @@
>    RefPtr<KeyframeEffect> effect =
>      new KeyframeEffect(doc,
>                         aSource.mTarget,
> +                       std::move(timing),

A simpler way is to create a temporary object (i.e. r-value).

`TimingParams(aSource.SpecifiedTiming()),`

::: layout/style/nsTransitionManager.cpp
@@ +890,5 @@
>        pt->mReplacedTransition.emplace(
>          ElementPropertyTransition::ReplacedTransitionProperties({
>            oldPT->GetAnimation()->GetStartTime().Value(),
>            oldPT->GetAnimation()->PlaybackRate(),
> +          std::move(timing),

I prefer to keeping using `oldPT->SpecifiedTiming(),` here because this is just a simple struct constructor.
Comment on attachment 9017049 [details] [diff] [review]
Refactor-KeyframeEffect-constructor-to-use-r-value-r.patch

Review of attachment 9017049 [details] [diff] [review]:
-----------------------------------------------------------------

Please request me again after you update this. Thanks. :)
Attachment #9017049 - Flags: review?(boris.chiou)
Thanks for the review. I updated the patch fixing the commented issues.
Attachment #9017049 - Attachment is obsolete: true
Attachment #9017334 - Flags: review?(boris.chiou)
I'm sorry I didn't mention one thing:

It's better to also update the constructor of AnimationEffect to use move.

[1] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/animation/AnimationEffect.h#37
I updated the AnimationEffect constructor.

In this patch I also undo what I think it's an error I introduced when fixing the previous patch:

std::move(TimingParams(aSource.SpecifiedTiming())),`

should be,

TimingParams(aSource.SpecifiedTiming()

since the constructor returns a r-value, std::move is not needed.

If the patch looks right, I can merge it later with the previous one.
Attachment #9017412 - Flags: review?(boris.chiou)
Comment on attachment 9017412 [details] [diff] [review]
Bug-1498948-Update-AnimationEffect-constructor.patch

Review of attachment 9017412 [details] [diff] [review]:
-----------------------------------------------------------------

BTW, It's ok to merge these two patches for review. :)

::: dom/animation/AnimationEffect.cpp
@@ +40,2 @@
>    : mDocument(aDocument)
>    , mTiming(aTiming)

This should also be "move":

, mTiming(std::move(aTiming))


So we could use the move constructor of TimingParams.
Addressed last comment and merged both patches.
Attachment #9017334 - Attachment is obsolete: true
Attachment #9017412 - Attachment is obsolete: true
Attachment #9017334 - Flags: review?(boris.chiou)
Attachment #9017412 - Flags: review?(boris.chiou)
Attachment #9017419 - Flags: review?(boris.chiou)
Comment on attachment 9017419 [details] [diff] [review]
Bug-1498948-Refactor-KeyframeEffect-constructor-to-u.patch

Review of attachment 9017419 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks for doing this. You can mark "checkin-needed" if try looks good.
Attachment #9017419 - Flags: review?(boris.chiou) → review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6e7a30d050
Refactor KeyframeEffect constructor to use r-value reference. r=boris
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de6e7a30d050
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.