Bug 82154 - [chromium] Route monotonic clock up from compositor
: [chromium] Route monotonic clock up from compositor
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 81479 81740
  Show dependency treegraph
 
Reported: 2012-03-25 17:32 PST by
Modified: 2012-03-27 20:16 PST (History)


Attachments
Patch (30.24 KB, patch)
2012-03-25 17:33 PST, Nat Duca
no flags Review Patch | Details | Formatted Diff | Diff
oops (30.21 KB, patch)
2012-03-25 17:46 PST, Nat Duca
no flags Review Patch | Details | Formatted Diff | Diff
Remove willBeginFrame code (27.68 KB, patch)
2012-03-25 23:38 PST, Nat Duca
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.45 KB, patch)
2012-03-27 17:24 PST, Nat Duca
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (14.07 KB, patch)
2012-03-27 19:36 PST, Nat Duca
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-25 17:32:47 PST
[chromium] Route monotonic clock and willBeginFrame notification up from compositor
------- Comment #1 From 2012-03-25 17:33:10 PST -------
Created an attachment (id=133704) [details]
Patch
------- Comment #2 From 2012-03-25 17:36:28 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #3 From 2012-03-25 17:36:51 PST -------
Attachment 133704 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #4 From 2012-03-25 17:46:20 PST -------
Created an attachment (id=133706) [details]
oops
------- Comment #5 From 2012-03-25 17:49:19 PST -------
This patch provides infrastructure for two bugfixes:
1. It has the WebViewImpl tick fling animations with the monotonic clock. This allows b81479 to then do fling transfer without having to convert the timestamp. This in turn makes the unit test for that that feature easier.

2. It adds a notification to the WebViewImpl for willBeginFrame. This provides infrastructure to fix PDF rendering in threaded mode.

As part of doing this, I made WebLayerTreeHostImpl stop deriving from CCLayerTreeHost and instead (a) own a CCLayerTreeHost and (b) use an adapter class for its callbacks. While this requires more typing, it is I think more maintainable.
------- Comment #6 From 2012-03-25 23:30:31 PST -------
Moving willBeginFrame changes to standalone cl
------- Comment #7 From 2012-03-25 23:38:09 PST -------
Created an attachment (id=133735) [details]
Remove willBeginFrame code
------- Comment #8 From 2012-03-26 14:30:35 PST -------
(From update of attachment 133735 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=133735&action=review

Left various comments inline, but I think overall our lives will be simpler if we keep wall clock time concerns out of the compositor where possible (for example the CSS animation API doesn't let us do that, but RAF would I think).

> Source/Platform/chromium/public/WebLayerTreeView.h:145
> +    // FIXME: remove default arugment on monotonicFrameBeginTime when upstream dependencies are resolved.

there aren't any upstream dependencies or upstream implementations of WebLayerTreeView, so you don't need a default param or anything like that

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:41
> +    virtual void updateAnimations(double frameBeginTime) { }
> +    virtual void updateAnimations(double wallClockFrameBeginTime, double monotonicFrameBeginTime) { }

instead of having 2 versions can we just change the parameter value to be the monotonic clock and have WebViewImpl get a currentTime()-based value for RAF? we aren't doing anything terribly clever with the time value (like adjusting it for latency or expected frame delivery timer or anything)

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:402
> +    m_pendingBeginFrameRequest->wallClockFrameBeginTime = currentTime();

should the proxy have to know about the wall clock time at all? it seems equally valid to say that wall clock time for RAF is a WebViewImpl-specific thing and make it deal with generating the right time value. the overall direction we want to go in is to pass a monotonic clock into WebCore and have it figure out what the correct value for each Frame is, right?

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:193
> +    // FIXME: Remove this once upstream embedders pass monotonicFrameBeginTime
> +    if (!monotonicFrameBeginTime)
> +        monotonicFrameBeginTime = monotonicallyIncreasingTime();

don't need this, the only implementation of WebLayerTreeView is this one

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:37
> +class CCLayerTreeHostClientToWebLayerTreeViewClientAdapter;

i think if you make this a private inner class you can use a shorter class name and put the forward decl next to the use in the private section instead of up here

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:41
> +    virtual ~WebLayerTreeViewImpl();

it looks like this class doesn't need a vtable, so do we need a virtual d'tor?

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:62
> +    WebLayerTreeViewImpl(WebLayerTreeViewClient*);

explicit
------- Comment #9 From 2012-03-27 17:02:25 PST -------
(From update of attachment 133735 [details])
(r- just to take this out of the pending-review queue, I think Nat and I are in agreement to go another round on this)
------- Comment #10 From 2012-03-27 17:24:53 PST -------
Created an attachment (id=134178) [details]
Patch
------- Comment #11 From 2012-03-27 17:25:10 PST -------
Moved WLTVI refactoring to another patch. This is the newer and lighter routing change. :)
------- Comment #12 From 2012-03-27 17:37:13 PST -------
(From update of attachment 134178 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=134178&action=review

Me likey

> Source/WebKit/chromium/src/WebViewImpl.cpp:1338
> +    dobule monotonicFrameBeginTime;

nit: can you initialize this explicitly to make the behavior more predictable if someone accidentally refactors the code below to no longer set the value? local variables of primitive types have bitten us a few times before

> Source/WebKit/chromium/src/WebViewImpl.cpp:1341
> +    else {

pretty sure there are no callers to this function with a non-zero value, so this is a bit theoretical. rather than shifting might be simpler to just grab monotonicallyIncreasingTime() here no matter what
------- Comment #13 From 2012-03-27 18:13:24 PST -------
(From update of attachment 134178 [details])
Attachment 134178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12145862
------- Comment #14 From 2012-03-27 18:14:26 PST -------
(From update of attachment 134178 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=134178&action=review

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1338
>> +    dobule monotonicFrameBeginTime;
> 
> nit: can you initialize this explicitly to make the behavior more predictable if someone accidentally refactors the code below to no longer set the value? local variables of primitive types have bitten us a few times before

also dobule -> double
------- Comment #15 From 2012-03-27 19:36:17 PST -------
Created an attachment (id=134202) [details]
Patch for landing
------- Comment #16 From 2012-03-27 20:16:14 PST -------
(From update of attachment 134202 [details])
Clearing flags on attachment: 134202

Committed r112360: <http://trac.webkit.org/changeset/112360>
------- Comment #17 From 2012-03-27 20:16:20 PST -------
All reviewed patches have been landed.  Closing bug.