Bug 84605 – [chromium] Do not clobber synchronized start times.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

If an animation starts on the impl thread before it starts on the main thread, we currently clobber the synchronized start time. This is a bug.

Created attachment 138369 [details]
Patch

[Dana Jansens]

Comment on attachment 138369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138369&action=review

> Source/WebCore/ChangeLog:5
> +

The comment from your test would go well here as well.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:84
> +    void setStartTime(double startTime) { m_startTime = startTime; m_hasSetStartTime = true; }

one statement per line

Created attachment 138373 [details]
Patch

(In reply to comment #2)
> (From update of attachment 138369 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138369&action=review
>
> > Source/WebCore/ChangeLog:5
> > +
>
> The comment from your test would go well here as well.
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:84
> > +    void setStartTime(double startTime) { m_startTime = startTime; m_hasSetStartTime = true; }
>
> one statement per line
Moved to the cpp.

[James Robinson]

Comment on attachment 138373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138373&action=review

Looks OK but try to get rid of the new member if you don't really need it.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:130
> +    bool m_hasSetStartTime;

we don't really need a bool, do we? m_startTime is initialized to 0 and can only become non-zero by a setStartTime() call or code internal to CCActiveAnimation, so would it be enough to just check for 0?

Created attachment 138821 [details]
Patch

(In reply to comment #4)
> (From update of attachment 138373 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138373&action=review
>
> Looks OK but try to get rid of the new member if you don't really need it.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:130
> > +    bool m_hasSetStartTime;
>
> we don't really need a bool, do we? m_startTime is initialized to 0 and can only become non-zero by a setStartTime() call or code internal to CCActiveAnimation, so would it be enough to just check for 0?

That's true. Removed. (Also, I had to update the unit test because I was setting the time to 0 and expecting hasSetStartTime() to be true.)

Created attachment 138833 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138833 [details]
Patch

Clearing flags on attachment: 138833

Committed r115226: <http://trac.webkit.org/changeset/115226>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.