1498948 - Refactor KeyframeEffect constructor to use r-value reference

Status: RESOLVED FIXED

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

Description

[Diego Pino]

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.

Comment 1

[Diego Pino]

Attachment: Refactor-KeyframeEffect-constructor-to-use-r-value-r.patch

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.

Comment 2

[Boris Chiou [:boris]]

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 3

[Boris Chiou [:boris]]

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. :)

Comment 4

[Diego Pino]

Attachment: Bug-1498948-Refactor-KeyframeEffect-constructor-to-u.patch

Thanks for the review. I updated the patch fixing the commented issues.

Comment 5

[Diego Pino]

Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=393304aca229383f87a6d07d9474aa6b407592e8

Comment 6

[Boris Chiou [:boris]]

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

Comment 7

[Diego Pino]

Attachment: Bug-1498948-Update-AnimationEffect-constructor.patch

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.

Comment 8

[Boris Chiou [:boris]]

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.

Comment 9

[Diego Pino]

Attachment: Bug-1498948-Refactor-KeyframeEffect-constructor-to-u.patch

Addressed last comment and merged both patches.

Comment 10

[Boris Chiou [:boris]]

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.

Comment 11

[Diego Pino]

Thanks for the multiple reviews and feedback. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9f033b6fba210d9ff438b6af82aa9d26b3463b

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6e7a30d050
Refactor KeyframeEffect constructor to use r-value reference. r=boris

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/de6e7a30d050