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

RESOLVED FIXED in mozilla35

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: Mihatia Ghiorghe, Mentored)

Tracking

Trunk
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments, 2 obsolete attachments)

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.)
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.)
Mentor: dholbert@mozilla.com
Whiteboard: [lang=c++]
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
Created attachment 8503249 [details]
removed aNow function argument
Attachment #8503249 - Attachment is patch: false
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!
(Assignee)

Comment 5

3 years ago
Created attachment 8503621 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

Removed unused parameter
(Assignee)

Comment 6

3 years ago
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 on attachment 8503621 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

This looks mostly good, except you also need to patch nsAnimationManager.cpp (at line 145).
And, ideally, you should include the bug number in the commit message, e.g.:

Bug 1081013 - Remove unused aNow parameter to AnimationPlayerCollection constructor.
(Assignee)

Comment 9

3 years ago
Both changes are included in the same diff. Should a make a separate diff for each one?
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.
(Assignee)

Comment 11

3 years ago
Created attachment 8503651 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

Bug 1081013 - Remove unused aNow parameter to AnimationPlayerCollection constructor
r=dholbert
Attachment #8503249 - Attachment is obsolete: true
Attachment #8503621 - Attachment is obsolete: true
Attachment #8503651 - Flags: review?(dholbert)
(Assignee)

Comment 12

3 years ago
I made all the changes and compiled the source. I hope everything is all right now.
Thank you, Daniel and David.
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 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! :))
Attachment #8503651 - Flags: review?(dholbert) → review-
(Assignee)

Comment 15

3 years ago
Created attachment 8503690 [details] [diff] [review]
bug1081013_removedunusedparameters.diff

Bug 1081013 - Removed unused parameter from AnimationPlayerCollection constructor
r=dholbert
Attachment #8503690 - Flags: review?(dholbert)
(Assignee)

Comment 16

3 years ago
It should be fine this time. Sorry for bugging you so much for such a minor problem :)
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!
Attachment #8503690 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/a014629454d0
Assignee: nobody → mihaitaghiorghe
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.