Bug 82154 – [chromium] Route monotonic clock up from compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Route monotonic clock and willBeginFrame notification up from compositor

[Nat Duca]

Created attachment 133704 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Nat Duca]

Created attachment 133706 [details]
oops

[Nat Duca]

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.

[Nat Duca]

Moving willBeginFrame changes to standalone cl

[Nat Duca]

Created attachment 133735 [details]
Remove willBeginFrame code

[James Robinson]

Comment on attachment 133735 [details]
Remove willBeginFrame code

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

[James Robinson]

Comment on attachment 133735 [details]
Remove willBeginFrame code

(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)

[Nat Duca]

Created attachment 134178 [details]
Patch

[Nat Duca]

Moved WLTVI refactoring to another patch. This is the newer and lighter routing change. :)

[James Robinson]

Comment on attachment 134178 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 134178 [details]
Patch

Attachment 134178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12145862

[James Robinson]

Comment on attachment 134178 [details]
Patch

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

[Nat Duca]

Created attachment 134202 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 134202 [details]
Patch for landing

Clearing flags on attachment: 134202

Committed r112360: <http://trac.webkit.org/changeset/112360>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.