1081013 - AnimationPlayerCollection constructors "aNow" parameter is unused & should be dropped

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

I noticed that the AnimationPlayerCollection constructor has a parameter "aNow" which is unused. We should remove it.

Code snippet / MXR link:
> 152   AnimationPlayerCollection(dom::Element *aElement, nsIAtom *aElementProperty,
> 153                             mozilla::css::CommonAnimationManager *aManager,
> 154                             TimeStamp aNow)
> 155     : mElement(aElement)
> 156     , mElementProperty(aElementProperty)
> 157     , mManager(aManager)
> 158     , mAnimationGeneration(0)
> 159     , mNeedsRefreshes(true)
> 160 #ifdef DEBUG
> 161     , mCalledPropertyDtor(false)
> 162 #endif
> 163   {
> 164     MOZ_COUNT_CTOR(AnimationPlayerCollection);
> 165     PR_INIT_CLIST(this);
> 166   }

http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.h?rev=860a94d6002a#152

It looks like the last usage of this variable was removed here:
 http://hg.mozilla.org/mozilla-central/rev/12959495c74e#l3.98

(At that point, this class was called ElementAnimationCollection.)

Comment 1

[Daniel Holbert [:dholbert]]

Setting this as a mentored bug, for an easy targeted patch from a new contributor.  Basically, we just need to remove "Timestamp aNow" from the function-signature linked in comment 0, and also remove the value that we pass for that parameter in the one place where we invoke this constructor, which is here:

> 605     collection = new AnimationPlayerCollection(aElement, propName, this,
> 606       mPresContext->RefreshDriver()->MostRecentRefresh());
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp?rev=9ad680392072#605

(So, we'd need to drop "mPresContext->RefreshDriver()->MostRecentRefresh()" from that invocation there.)

(birtles, please chime in if you know of a need for this parameter -- i.e. if we're going to add back a usage of it.)

Comment 2

[Mihatia Ghiorghe]

Hello,

I have made the required changes to the both files. I am new to the mozilla submission system.
Can you help me submit the changes?
Thank you

Comment 3

[Mihatia Ghiorghe]

Attachment: removed aNow function argument

Comment 4

[Daniel Holbert [:dholbert]]

Hi Mihatia -- thanks for stepping up to take this!

It looks like what you attached is a copy of the two edited files (in their entirety).

That's not how we take changes (because e.g. if someone else modifies the file in the meantime, then when we copied in your version, we'd stomp on their changes!) We use patch files, also known as "unified diffs" -- files that *just* show your changes, plus some metadata.

So: you'll need to generate a patch for your changes (with a commit message and with your name/email address included in the patch headers), and attach that.

For instructions on how to do that, these videos will probably be helpful:
 http://www.codefirefox.com/video/mercurial-setup
 http://www.codefirefox.com/video/patch-creation

(If you're on linux or Mac, you can ignore the "start-msvc.bat" stuff in the videos.)

There are steps for making a patch file in this documentation, as well (though if you're just getting started, the videos may be more user-friendly):
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Thanks!

Comment 5

[Mihatia Ghiorghe]

Attachment: bug1081013_removedunusedparameters.diff

Removed unused parameter

Comment 6

[Mihatia Ghiorghe]

I've added the diff file. I hope it is good this time. Sorry for being so late, I had some trouble downloading mozilla-central :).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8503621 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

This looks mostly good, except you also need to patch nsAnimationManager.cpp (at line 145).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, ideally, you should include the bug number in the commit message, e.g.:

Bug 1081013 - Remove unused aNow parameter to AnimationPlayerCollection constructor.

Comment 9

[Mihatia Ghiorghe]

Both changes are included in the same diff. Should a make a separate diff for each one?

Comment 10

[Daniel Holbert [:dholbert]]

Nope, all the changes in the same patch would be best here.

dbaron is right about there being one more change needed, at nsAnimationManager.cpp:145. (Sorry for missing that in comment 1.) After you've made that change, you should probably also try to compile Firefox locally with your patch -- that will ensure you haven't made any typos here, and that there aren't any other changes that are needed for this bug.  (And this will be an important thing to be able to do, for any future Mozilla work that you do.)  That's documented here:
 https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

The quick-and-easy version should be to run this in your mozilla-central directory:
  ./mach bootstrap
  ./mach build
(The first line should install dependencies, and only needs to be run once.)
If you have trouble, feel free to ask questions in irc.mozilla.org #introduction or #developers.

And as dbaron noted, please include the bug number in your commit message, and include "r=dholbert" at the end to indicate that I'll be the reviewer. And when you re-post the patch, you can tag me for review by setting the "review" field to "?" and then typing dholbert into the textfield that appears.

Comment 11

[Mihatia Ghiorghe]

Attachment: bug1081013_removedunusedparameters.diff

Bug 1081013 - Remove unused aNow parameter to AnimationPlayerCollection constructor
r=dholbert

Comment 12

[Mihatia Ghiorghe]

I made all the changes and compiled the source. I hope everything is all right now.
Thank you, Daniel and David.

Comment 13

[Daniel Holbert [:dholbert]]

Thanks! It looks like the updated patch is the same as the old one, though.  In particular: It's still missing the nsAnimationManager.cpp change, and the commit message inside the patch itself doesn't have the bug number.

If you're using Mercurial Queues, (i.e. if you created the patch with "hg qnew"), you probably just need to run "hg qref" to update the patch file, and "hg qref -e" to update the commit message.

(The commit message that you typed into comment 11 looks good, though it should say "Remove...from" instead of "Remove...to".)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8503651 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

(Marking this one as "review-" since it's not quite complete yet, but it's  close! :))

Comment 15

[Mihatia Ghiorghe]

Attachment: bug1081013_removedunusedparameters.diff

Bug 1081013 - Removed unused parameter from AnimationPlayerCollection constructor
r=dholbert

Comment 16

[Mihatia Ghiorghe]

It should be fine this time. Sorry for bugging you so much for such a minor problem :)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8503690 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

No worries! Patch looks good, thanks!

(For future reference, you can speculatively include "r=dholbert" (or whoever) in the actual commit message inside the patch, too -- since that needs to be in there when the patch actually lands, to indicate who reviewed it.)

Normally at this point, you'd run a patch through a "try server" run, to get a run of builds & tests across platforms, and then you'd mark the bug as checkin-needed to indicate that it's been tested and it's ready to land. In this case, I don't think that's necessary, because this is the sort of patch that's correct if it compiles & doesn't really need testing.

So, I verified locally that it compiles, so I just went ahead and pushed it to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a014629454d0

It should be merged to mozilla-central in a day or so, and then this bug will be closed by a tree sheriff.

Thanks for the patch!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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