1107351 - Simplify AnimationPlayer::GetCurrentTime() with early-returns

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

Noticed while reviewing bug 1104424: The method AnimationPlayer::GetCurrentTime() could be made a bit more skimmable if it used early-returns instead of its current if/else + 'result' structure.

Filing this bug on doing that cleanup.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This ended up being maybe a bit more complicated, but that's partly because I've made it more efficient than the existing code, by preventing a call to mTimeline->GetCurrentTime() if we don't actually need it (if mStartTime is null).

Brian, what do you think?

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Created attachment 8531801 [details] [diff] [review]
> fix v1
> 
> This ended up being maybe a bit more complicated, but that's partly because
> I've made it more efficient than the existing code, by preventing a call to
> mTimeline->GetCurrentTime() if we don't actually need it (if mStartTime is
> null).
> 
> Brian, what do you think?

I think avoiding the call to GetCurrentTime is really nice. It's a shame we lose RVO with this though.

How about:

>Nullable<TimeDuration>
>AnimationPlayer::GetCurrentTime() const
>{
>  Nullable<TimeDuration> result;
>
>  if (!mHoldTime.IsNull()) {
>    result = mHoldTime;
>    return result;
>  }
>
>  if (!mStartTime.IsNull()) {
>    Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
>    if (!timelineTime.IsNull()) {
>      result = timelineTime.Value() - mStartTime.Value();
>    }
>  }
>
>  return result;
>}

We could avoid the nested branches like so:

>Nullable<TimeDuration>
>AnimationPlayer::GetCurrentTime() const
>{
>  Nullable<TimeDuration> result;
>
>  if (!mHoldTime.IsNull()) {
>    result = mHoldTime;
>    return result;
>  }
>
>  if (mStartTime.IsNull()) {
>    return result;
>  }
>
>  Nullable<TimeDuration> timelineTime = mTimeline->GetCurrentTime();
>  if (timelineTime.IsNull()) {
>    return result;
>  }
>
>  result = timelineTime.Value() - mStartTime.Value();
>  return result;
>}

I prefer the first, shorter version however.

Comment 3

[Daniel Holbert [:dholbert]]

Ah, right -- I wasn't thinking about RVO.  I'm actually not sure RVO does much for us here; from a quick skim, it looks like TimeDuration only has one member-var -- "int64_t mValue" -- so copying the return value into the caller's local variable isn't particularly expensive.  Though I suppose on a 32-bit system, it helps a bit.

The first, shorter version looks good to me -- I'll rewrite to match that & repost. Thanks!

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Here's the updated patch. This really only changes two things w.r.t. the existing code:
 - adds 1 early-return to convert "if/else" into just "if"
 - Promotes the !mStartTime.IsNull() check to avoid an unneeded GetCurrentTime call.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8531871 [details] [diff] [review]
fix v2

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

Looks great. Thanks Daniel!

Comment 6

[Daniel Holbert [:dholbert]]

Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/69b7229fa7a0

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69b7229fa7a0