Bug 79537 – [chromium] Ensure we use the correct time when notifying animation started

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

We are currently plumbing back the same time that is passed to CCLayerTreeHostImpl::animate; this may not be the time expected by GraphicsLayerChromium's client, so some work may need to be done to normalize this time.

[Nat Duca]

How about we make the proxy send two times to the animate() paths --- both monotonic and regular clocks? Then we can send both down to the main thread and back to the client, and let the client figure out which of those two times it wants to use.

Created attachment 130503 [details]
Patch

Created attachment 130506 [details]
Patch

[Nat Duca]

Comment on attachment 130506 [details]
Patch

looks cool to me; methinks maybe we should take this opportunity to eradicate milliseconds from the system entirely?

Created attachment 130520 [details]
Patch

Created attachment 130643 [details]
Patch

[James Robinson]

Comment on attachment 130643 [details]
Patch

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

R=me but you're gonna have to sort the ChangeLog before this can land.

I think it would be good to have at least some unit tests that pass different values for monotonic / wall clock times through the system as a way to try to flush out code that uses the wrong one somewhere.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this won't fly - there's an SVN presubmit that will reject this on the server. just list the tests that cover this behavior

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:296
> +      double wallClockTime = WTF::currentTime();

fyi: you don't need the WTF::, <wtf/CurrentTime.h> pulls the function into the global namespace with a using statement (which is the standard pattern for wtf/ stuff)

Created attachment 130864 [details]
Patch

(In reply to comment #7)
> (From update of attachment 130643 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130643&action=review
> 
> R=me but you're gonna have to sort the ChangeLog before this can land.
> 
> I think it would be good to have at least some unit tests that pass different values for monotonic / wall clock times through the system as a way to try to flush out code that uses the wrong one somewhere.
I've added a unit test that ensures we use the correct times.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> this won't fly - there's an SVN presubmit that will reject this on the server. just list the tests that cover this behavior
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:296
> > +      double wallClockTime = WTF::currentTime();
> 
> fyi: you don't need the WTF::, <wtf/CurrentTime.h> pulls the function into the global namespace with a using statement (which is the standard pattern for wtf/ stuff)
Removed.

Created attachment 130878 [details]
Patch

Created attachment 130898 [details]
Patch

Noteworthy changes:
 - The controller is never given the wall clock time.
 - The wall clock time is passed to the CCLayerTreeHostImpl. This time is passed along with the animation events.
 - I've added a test to ensure that the events are not accompanied by the wrong time in CCLayerTreeHostTest

[James Robinson]

Comment on attachment 130898 [details]
Patch

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

nice!

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationEvents.h:66
> +    CCAnimationStartedEvent(int layerId);

explicit prz

Created attachment 130915 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130915 [details]
Patch

Rejecting attachment 130915 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
merTick()':
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:64: error: no matching function for call to 'WebCore::CCLayerTreeHostImpl::animate(double)'
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:79: note: candidates are: virtual void WebCore::CCLayerTreeHostImpl::animate(double, double)
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/11903327

Created attachment 131039 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131039 [details]
Patch

Clearing flags on attachment: 131039

Committed r110299: <http://trac.webkit.org/changeset/110299>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.