Creating utility for converting TimeTicks to WebKit double and use it in the Chrome compositor.

Creating utility for converting TimeTicks to WebKit double and use it in the Chrome compositor.

From third_party/WebKit/public/platform/Platform.h
    // Monotonically increasing time in seconds from an arbitrary fixed point in the past.
    // This function is expected to return at least millisecond-precision values. For this reason,
    // it is recommended that the fixed point be no further in the past than the epoch.
    virtual double monotonicallyIncreasingTime() { return 0; }

TimeTicks deliberately doesn't have a way to be converted to a double in seconds
because doing so inside Chrome shouldn't occur. This patch adds a method for
doing the conversion (and tests that it works) at the boundary. The conversion 
functions also check for common problematic conversions.

 
BUG=349328

[mithro-old, 2014-03-10 14:51:15]

This CL needs a little more work but I wanted to get it in front of you.

What do you think? Is this the right location for this code?

Tim

[ajuma, 2014-03-10 15:01:31]

> WebKit uses a double in seconds (since 1970 Epoch) for animation

I don't think this is the case any longer -- as I understand it, the new
animation framework uses monotonic time (in particular, the time sent from
cc::LayerTreeHost::UpdateClientAnimations, which gets converted to a double by
RenderWidgetCompositor::Animate).

So a conversion to double is needed (for now, though once Blink moves into the
chromium repo, we should be able to just use TimeTicks) but a conversion from
monotonic time to wall-clock time is not.

[danakj, 2014-03-10 17:13:55]

Seems like the right place to me. +jamesr for blink<->cc API boundary stuff.

[danakj, 2014-03-10 17:14:14]

Seems like the right place to me. +jamesr for blink<->cc API boundary stuff.

(try again)

[jamesr, 2014-03-10 20:37:28]

https://codereview.chromium.org/192433002/diff/1/webkit/renderer/compositor_b...
File webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc
(left):

https://codereview.chromium.org/192433002/diff/1/webkit/renderer/compositor_b...
webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc:19:
(monotonic_time - base::TimeTicks()).InSecondsF(),
what's wrong with this?

[mithro-old, 2014-03-11 12:26:13]

On 2014/03/10 17:14:14, danakj wrote:
> Seems like the right place to me. +jamesr for blink<->cc API boundary stuff.
> 
> (try again)

Hi Dana,

I'm not sure now that this is the correct location. I updated this CL to use the
functions in cc/trees/layer_tree_host_impl.cc and found that it violates the
checkdeps rules in the PRESUBMIT.

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  cc/trees/layer_tree_host_impl.cc
      Illegal include: "webkit/renderer/compositor_bindings/webkit_time.h"
    Because of no rule applying.

Tim

[mithro-old, 2014-03-11 12:56:42]

On 2014/03/10 20:37:28, jamesr wrote:
>
https://codereview.chromium.org/192433002/diff/1/webkit/renderer/compositor_b...
> File
webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc
> (left):
> 
>
https://codereview.chromium.org/192433002/diff/1/webkit/renderer/compositor_b...
>
webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc:19:
> (monotonic_time - base::TimeTicks()).InSecondsF(),
> what's wrong with this?

Hi James,

I have rebased my other CLs so they are not dependent on this change. We can
abandon this change if we decide it is not the correct approach. 

I have updated the conversion functions to be significantly simpler now that I
understand that the epoch conversion attempt is wrong. I have also added a
couple of other locations which would use this function (although this doesn't
pass the presubmit yet, see my question to Dana).

As we agreed on IRC, we should only be converting from TimeTicks when
interfacing with an API that doesn't use them. I feel this CL is a step in
making this both clearer and more reliable. Expanding on what I was mentioning
on IRC, the reasons why I think this CL is useful are;

 a) The new functions make it clear to a reviewer/reader what subsystem
(Blink/WebKit) we are converting the TimeTicks (and hence why we are doing the
conversion). Even a noob like myself is going to understand
ToWebKitMonotonicTime(ticks) compared to the more cryptic "(ticks -
base::TimeTicks()).InSecondsF())".

 b) The new functions checks for potentially dangerous conversion cases. They
will fail early making it easier to find the source of bad values rather than
Chrome just exhibiting weird behaviour. The recent thread on "Representing time
in Blink" on blink-dev, the fact we have a base::TimeTicks class in Chrome and
the output from some of my frame time tracing experiments all indicate to me
that these type of issues are worth protecting against.

 c) The new functions give us one place to change in the future if/when
WebKit/Blink starts using something other than doubles.

 d) The new functions are much easier to audit, allowing us to make sure that
Chrome isn't converting the values at the wrong point (such as too early when
they should be still be using TimeTicks).

Thoughts? Am I totally wrong?

Thank you for your time and help!

Tim

[danakj, 2014-03-11 17:53:37]

On Tue, Mar 11, 2014 at 8:26 AM, <mithro@mithis.com> wrote:

> On 2014/03/10 17:14:14, danakj wrote:
>
>> Seems like the right place to me. +jamesr for blink<->cc API boundary
>> stuff.
>>
>
>  (try again)
>>
>
> Hi Dana,
>
> I'm not sure now that this is the correct location. I updated this CL to
> use the
> functions in cc/trees/layer_tree_host_impl.cc and found that it violates
> the
> checkdeps rules in the PRESUBMIT.
>
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   cc/trees/layer_tree_host_impl.cc
>       Illegal include: "webkit/renderer/compositor_bindings/webkit_time.h"
>     Because of no rule applying.
>

The outputs from cc to the bindings would be base::TimeTicks. The bindings
here would do the conversion before passing the value on to blink. cc
itself would never convert out of TimeTicks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-03-11 20:09:54]

Not lgtm.  I think this is less understandable than the previous code.  As Dana
said, converting all of cc/ and other chromium code to using the base::Time*
values is valuable.

(ticks - base::TimeTicks()).InSecondsF() is a pretty clear way (imo) to express
that a timestamp is being expressed as a delta, in seconds, from an arbitrary
point in the past.

https://codereview.chromium.org/192433002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/192433002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:65: #include
"webkit/renderer/compositor_bindings/webkit_time.h"
this is a bad dependency. the compositor bindings depend on cc, you can't have a
dependency the other way around or that would create a cycle

[mithro-old, 2014-03-12 05:51:42]

FYI As jamesr requested, I'm abandoning this change.