1339675 - Move TimingParams

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

We can move TimingParams on SetTimingParams.
The only one caller is here:
https://hg.mozilla.org/mozilla-central/file/1060668405a9/layout/style/nsAnimationManager.cpp#l373

I guess we can also add a KeyframeEffectReadOnly construct that receives TimingParams&&.

Comment 1

[shisun.xia]

Hi, I'm new to Bugzilla. I have set up the local firefox environment in a Linux virtual machine. Could I work on this bug?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Sure!  Thanks for taking this!

Comment 3

[Boris Chiou [:boris]]

(In reply to shisun.xia from comment #1)
> Hi, I'm new to Bugzilla. I have set up the local firefox environment in a
> Linux virtual machine. Could I work on this bug?

Hi shisun.xia, if you have any question, please feel free to ask us. :)

Comment 4

[shisun.xia]

(In reply to Boris Chiou [:boris] from comment #3)
> (In reply to shisun.xia from comment #1)
> > Hi, I'm new to Bugzilla. I have set up the local firefox environment in a
> > Linux virtual machine. Could I work on this bug?
> 
> Hi shisun.xia, if you have any question, please feel free to ask us. :)

Thanks for giving me this opportunity. My first question is why do we need to move TimingParams? Is this causing any problem to the current Mozilla system?

Comment 5

[Boris Chiou [:boris]]

(In reply to shisun.xia from comment #4)
> Thanks for giving me this opportunity. My first question is why do we need
> to move TimingParams? Is this causing any problem to the current Mozilla
> system?

There is no problem on Firefox caused by it now. Just kind of refinement for Mozilla system, i.e. Reduce the copy times as many as possible.

Comment 6

[shisun.xia]

After looking at the code, the "SetSpecifiedTiming" function in the "AnimationEffectReadOnly" class is only called once in the whole project. And the "SetTimingParams" function is only used once inside the SetSpecifiedTiming function. So what should I do?

Comment 7

[Boris Chiou [:boris]]

(In reply to shisun.xia from comment #6)
> After looking at the code, the "SetSpecifiedTiming" function in the
> "AnimationEffectReadOnly" class is only called once in the whole project.
> And the "SetTimingParams" function is only used once inside the
> SetSpecifiedTiming function. So what should I do?

If it is only called once, we can do that as comment 0 suggested. How about trying to revise the argument as TimingParams&&? So we can move it directly. Besides, you can use mozilla::Move() [1] to convert the l-value into r-value in Mozilla code. After this, you can also check if we could also add a KeyframeEffectReadOnly construct that receives TimingParams&&.

[1] http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/mfbt/Move.h#201

Comment 8

[shisun.xia]

(In reply to Boris Chiou [:boris] from comment #7)
> (In reply to shisun.xia from comment #6)
> > After looking at the code, the "SetSpecifiedTiming" function in the
> > "AnimationEffectReadOnly" class is only called once in the whole project.
> > And the "SetTimingParams" function is only used once inside the
> > SetSpecifiedTiming function. So what should I do?
> 
> If it is only called once, we can do that as comment 0 suggested. How about
> trying to revise the argument as TimingParams&&? So we can move it directly.
> Besides, you can use mozilla::Move() [1] to convert the l-value into r-value
> in Mozilla code. After this, you can also check if we could also add a
> KeyframeEffectReadOnly construct that receives TimingParams&&.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/mfbt/Move.h#201

According to my understanding, firstly I need to convert TimingParams& to TimingParams&& by using Move function. After that, change the parameter type of SetSpecifiedTiming and SetTimingParams from TimingParams& to TimingParams&&. Am I on the right track? Thanks.

Comment 9

[shisun.xia]

I've added mozilla::Move to AnimationEffectTimingReadOnly.

http://searchfox.org/mozilla-central/source/dom/animation/AnimationEffectTimingReadOnly.h#51

Comment 10

[Marco Castelluccio [:marco]]

Hello shisun.xia, you should move your patch from https://github.com/mozilla/gecko-dev/pull/114 to here and request review.

Comment 11

[Boris Chiou [:boris]]

Hi, shisun.xia

If you are a git user, you can download moz-git-tool first [1], and set the $PATH for its binary.

Then:
`git bz attach 1339675 <parent commit>..HEAD -e`

This will attaches the commits to Bugzilla with the bug number. We don't create PR to gecko-dev. Instead, we only patches into our mercurial repo, https://hg.mozilla.org/mozilla-central/. After you attach the patches into Bugzilla and get r+, we will help you verify your patches, and land them into mozilla-central. Thanks.

[1] https://github.com/mozilla/moz-git-tools

Comment 12

[Boris Chiou [:boris]]

Hi, shisun.xia

It seems there no update on this bug for more than one year. I'd like to make this bug unassigned. If you want to keep working on this, please feel free on re-taking this bug. Thanks.

Comment 13

[Diego Pino]

Attachment: Bug-1339675-Move-TimingParams-in-call-to-SetSpecifie.patch

The code has changed a bit since this bug was created. `SetSpecifiedTime` still exists and there are 2 callers now. I changed `TimingParams` in `SetSpecifiedTime` to an r-value reference and called `std:move` in the callers.

About the other part of the patch: "check if we could also add a KeyframeEffectReadOnly construct that receives TimingParams&&", I don't know if that part is still valid since now there's not AnimationEffectReadOnly.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9016577 [details] [diff] [review]
Bug-1339675-Move-TimingParams-in-call-to-SetSpecifie.patch

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

I haven't audited all relevant places, but at first glance, you need to modify the argument of UpdateOldAnimationPropertiesWithNew too.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thanks for taking this, anyway!

Comment 16

[Diego Pino]

Attachment: Bug-1339675-Move-TimingParams-in-call-to-SetSpecifie.patch

Thanks for the review Hiroyuki. I updated the patch.

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 9016747 [details] [diff] [review]
Bug-1339675-Move-TimingParams-in-call-to-SetSpecifie.patch

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

Thanks for updating this. LGTM.

::: layout/style/nsAnimationManager.cpp
@@ +516,5 @@
>      // them.  See
>      // http://lists.w3.org/Archives/Public/www-style/2011Apr/0079.html
>      // In order to honor what the spec said, we'd copy more data over.
>      UpdateOldAnimationPropertiesWithNew(*oldAnim,
> +                                        std::move(timing),

Now, we use "move" for the timing object here, but below (in the same function) still uses "call by reference" [1]. For consistency, it's better to update both. However, this is out of the scope of this bug because the constructor of KeyframeEffect still uses "call by reference" [2]. Anyway, I'm ok with this.

[1] https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/layout/style/nsAnimationManager.cpp#532
[2] https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/dom/animation/KeyframeEffect.cpp#74

Comment 18

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #17)
> Now, we use "move" for the timing object here, but below (in the same
> function) still uses "call by reference" [1]. For consistency, it's better
> to update both. However, this is out of the scope of this bug because the
> constructor of KeyframeEffect still uses "call by reference" [2].

Or maybe you could file a separate bug for this. This is also a good first bug, I think.

Comment 19

[Diego Pino]

Finally I decided to file a different bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1498948) to address #Comment 17.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56ff378a56d
Move TimingParams in call to SetSpecifiedTiming r=boris

Comment 21

[Natalia Csoregi [:nataliaCs]]

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