Bug 79537 - [chromium] Ensure we use the correct time when notifying animation started
Summary: [chromium] Ensure we use the correct time when notifying animation started
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: 76468 79536 80514
  Show dependency treegraph
 
Reported: 2012-02-24 15:04 PST by vollick
Modified: 2012-03-09 08:49 PST (History)
4 users (show)

See Also:


Attachments
Patch (36.71 KB, patch)
2012-03-06 18:02 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (38.83 KB, patch)
2012-03-06 18:17 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (56.73 KB, patch)
2012-03-06 19:16 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (56.85 KB, patch)
2012-03-07 10:21 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (58.48 KB, patch)
2012-03-08 11:23 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (58.48 KB, patch)
2012-03-08 12:28 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (57.62 KB, patch)
2012-03-08 13:54 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (57.41 KB, patch)
2012-03-08 15:13 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (63.28 KB, patch)
2012-03-09 08:00 PST, 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-02-24 15:04:48 PST
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.
Comment 1 Nat Duca 2012-02-28 12:25:57 PST
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.
Comment 2 vollick 2012-03-06 18:02:37 PST
Created attachment 130503 [details]
Patch
Comment 3 vollick 2012-03-06 18:17:49 PST
Created attachment 130506 [details]
Patch
Comment 4 Nat Duca 2012-03-06 18:27:35 PST
Comment on attachment 130506 [details]
Patch

looks cool to me; methinks maybe we should take this opportunity to eradicate milliseconds from the system entirely?
Comment 5 vollick 2012-03-06 19:16:57 PST
Created attachment 130520 [details]
Patch
Comment 6 vollick 2012-03-07 10:21:12 PST
Created attachment 130643 [details]
Patch
Comment 7 James Robinson 2012-03-07 23:52:09 PST
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)
Comment 8 vollick 2012-03-08 11:23:50 PST
Created attachment 130864 [details]
Patch
Comment 9 vollick 2012-03-08 11:25:24 PST
(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.
Comment 10 vollick 2012-03-08 12:28:28 PST
Created attachment 130878 [details]
Patch
Comment 11 vollick 2012-03-08 13:54:42 PST
Created attachment 130898 [details]
Patch
Comment 12 vollick 2012-03-08 13:57:11 PST
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
Comment 13 James Robinson 2012-03-08 15:02:52 PST
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
Comment 14 vollick 2012-03-08 15:13:44 PST
Created attachment 130915 [details]
Patch
Comment 15 WebKit Review Bot 2012-03-08 20:15:11 PST
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
Comment 16 vollick 2012-03-09 08:00:09 PST
Created attachment 131039 [details]
Patch
Comment 17 WebKit Review Bot 2012-03-09 08:49:19 PST
Comment on attachment 131039 [details]
Patch

Clearing flags on attachment: 131039

Committed r110299: <http://trac.webkit.org/changeset/110299>
Comment 18 WebKit Review Bot 2012-03-09 08:49:25 PST
All reviewed patches have been landed.  Closing bug.