Rename AnimationPlayer::GetCurrentTimeDuration

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Since I discovered BinaryName in WebIDL I'm a bit unsure how much to use it in the Web Animations work.

There are a few cases where we have functions which we use internally and from the DOM but where they behave differently.

In bug 1037321 we have both:

  PlayState()
  PlayStateFromJS()

The latter flushes style first. However, we often want to query to play state internally so it seems to make sense to give the nice name to the one intended for internal use rather than risk people accidentally flushing style.

I was thinking of doing the same thing for methods that get/set times. Internally we work with TimeDuration objects but for DOM callers we convert this to a DOMHighResTimeStamp (a double).

So I was thinking we should have:

  Nullable<TimeDuration> GetCurrentTime()
  Nullable<double>       GetCurrentTimeAsDouble()

We fetch the current time a lot internally so this is nicer than having to write GetCurrentTimeAsDuration() or something similar.

The only problem I can see is that nearly every method in the public API will end up being renamed for one of the above reasons. I'm not sure if this is a problem or not.
(Assignee)

Comment 1

3 years ago
Created attachment 8500256 [details] [diff] [review]
Rename AnimationTimeline/AnimationPlayer GetCurrentTimeDuration

This patch performs the following renaming:

  AnimationPlayer::GetCurrentTime -> GetCurrentTimeAsDouble
  AnimationPlayer::GetCurrentTimeDuration -> GetCurrentTime
  AnimationTimeline::GetCurrentTime -> GetCurrentTimeAsDouble
  AnimationTimeline::GetCurrentTimeDuration -> GetCurrentTime
Attachment #8500256 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Blocks: 1078122
(Assignee)

Comment 2

3 years ago
Created attachment 8500257 [details] [diff] [review]
Rename AnimationTimeline/AnimationPlayer GetCurrentTimeDuration

Forgot to drop a declaration of GetCurrentTimeDuration
Attachment #8500257 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8500256 - Attachment is obsolete: true
Attachment #8500256 - Flags: review?(bzbarsky)
Comment on attachment 8500257 [details] [diff] [review]
Rename AnimationTimeline/AnimationPlayer GetCurrentTimeDuration

r=me

In this case another option would have been to return some class with auto-conversions to both double and TimeDuration (e.g. a subclass of TimeDuration with a conversion operator to double).  But that seems a lot more confusing than just having separate methods.
Attachment #8500257 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6866bdaa73d
https://hg.mozilla.org/mozilla-central/rev/f6866bdaa73d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.