Bug 84605 - [chromium] Do not clobber synchronized start times.
Summary: [chromium] Do not clobber synchronized start times.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 09:55 PDT by vollick
Modified: 2012-04-25 10:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.44 KB, patch)
2012-04-23 09:58 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (8.89 KB, patch)
2012-04-23 10:13 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2012-04-25 09:09 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2012-04-25 10:09 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-04-23 09:55:41 PDT
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.
Comment 1 vollick 2012-04-23 09:58:15 PDT
Created attachment 138369 [details]
Patch
Comment 2 Dana Jansens 2012-04-23 10:06:36 PDT
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
Comment 3 vollick 2012-04-23 10:13:25 PDT
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.
Comment 4 James Robinson 2012-04-24 23:47:37 PDT
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?
Comment 5 vollick 2012-04-25 09:09:07 PDT
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.)
Comment 6 vollick 2012-04-25 10:09:22 PDT
Created attachment 138833 [details]
Patch
Comment 7 WebKit Review Bot 2012-04-25 10:31:41 PDT
Comment on attachment 138833 [details]
Patch

Clearing flags on attachment: 138833

Committed r115226: <http://trac.webkit.org/changeset/115226>
Comment 8 WebKit Review Bot 2012-04-25 10:31:45 PDT
All reviewed patches have been landed.  Closing bug.