Removing the use of base::Time inside the LayerTreeHost system.

Removing the use of base::Time inside the LayerTreeHost system.

First step before removing Now() usage from the class.

DEPS=194543002
BUG=345459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256303

[mithro-old, 2014-02-28 00:54:22]

Hi guys,

Here is the first CL as discussed in the meeting.

As suggested, I'm adding ajuma on CC. 

Tim

[jamesr, 2014-02-28 00:58:16]

I wasn't in the meeting, but here are some comments.

https://codereview.chromium.org/178103004/diff/50001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/178103004/diff/50001/base/time/time.h#newcode618
base/time/time.h:618: // (Jan 1, 1970).  Webkit uses this format to represent
time.
if this is just needed for interacting with Blink (we don't use WebKit any more)
then it doesn't belong in base/, it should probably go somewhere in the
chromium-blink integration code that's currently split between content/ and
webkit/

https://codereview.chromium.org/178103004/diff/50001/base/time/time.h#newcode701
base/time/time.h:701: os << "TimeTicks(";
i'm pretty sure we don't want to have these stream operator based functions
defined in every single translation unit that #includes base/time.h - they're
probably huge.

[mithro-old, 2014-02-28 00:59:18]

Hi Ali,

I'm adding you as a CC on this bug after talking to Dana and Brian as they
suggested you had been working in this area. Please feel free to upgrade
yourself to a reviewer if you would like but don't feel obliged to.

This CL related to the work I'm doing to get everything to use a consistent
frame time through the whole Chrome stack (root bug can be found at
https://code.google.com/p/chromium/issues/detail?id=346230). 

This CL will be followed by https://codereview.chromium.org/184023002/ which is
not quite working yet (it currently breaks background ticking) but hopefully you
can see where I'm going.

Thank you for your time!

Tim

[mithro-old, 2014-02-28 01:18:30]

Hi James!

Dana and Brian suggest I talk to you to better understand how LayerHostImpl and
input work together, I have some concerns that the changes to try and remove
::Now() might break related code. I'll try and catch you on IM/IRC and have a
talk.

On 2014/02/28 00:58:16, jamesr wrote:
> I wasn't in the meeting, but here are some comments.
> 
> https://codereview.chromium.org/178103004/diff/50001/base/time/time.h
> File base/time/time.h (right):
> 
>
https://codereview.chromium.org/178103004/diff/50001/base/time/time.h#newcode618
> base/time/time.h:618: // (Jan 1, 1970).  Webkit uses this format to represent
> time.
> if this is just needed for interacting with Blink (we don't use WebKit any
more)
> then it doesn't belong in base/, it should probably go somewhere in the
> chromium-blink integration code that's currently split between content/ and
> webkit/

I was unsure the correct location for this code, very open to a better location.

Dana suggested that we should make sure these conversions only happen in the
webkit compositor bindings and make sure everything inside the compositor is
using TimeTicks instead. I think is probably the right approach and will update
this code to make that happen.

time.h has the following on base::Time which is what the ToWebKit/FromWebKit
code was based on. Hopefully we can remove these functions soon too.
----------------------------------------------
  // Converts time to/from a double which is the number of seconds since epoch
  // (Jan 1, 1970).  Webkit uses this format to represent time.
  // Because WebKit initializes double time value to 0 to indicate "not
  // initialized", we map it to empty Time object that also means "not
  // initialized".
  static Time FromDoubleT(double dt);
  double ToDoubleT() const;
----------------------------------------------

Maybe we can eventually get Blink to use a TimeTicks rather than the double
WebKit used, I don't see that happening any time soon however. The work that the
Sydney team is doing to make Blink have a single animation engine would makes it
much easier to do!

>
https://codereview.chromium.org/178103004/diff/50001/base/time/time.h#newcode701
> base/time/time.h:701: os << "TimeTicks(";
> i'm pretty sure we don't want to have these stream operator based functions
> defined in every single translation unit that #includes base/time.h - they're
> probably huge.

This should be in either time_unittest.cc or a time_test_helper.h but I'm having
trouble convincing gtest to find it properly. Still investigating.

[ajuma, 2014-02-28 02:32:03]

I'm happy to see wall clock times being removed. Since AnimationEvents already
include a monotonic time, there's no need to replace the removed wall clock
times with monotonic times; they can be removed without replacement.

https://codereview.chromium.org/178103004/diff/50001/cc/test/fake_layer_tree_...
File cc/test/fake_layer_tree_host_impl_client.h (right):

https://codereview.chromium.org/178103004/diff/50001/cc/test/fake_layer_tree_...
cc/test/fake_layer_tree_host_impl_client.h:31: base::TimeTicks clock_time)
OVERRIDE {}
AnimationEvents already store a monotonic_time, so having a base::TimeTicks here
seems redundant.

https://codereview.chromium.org/178103004/diff/50001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/178103004/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:582: clock_time.ToWebKit());
This seems incorrect, since the callee is expecting a wall clock time (expressed
as a double). As noted elsewhere, this time is ultimately unused now, though.

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

https://codereview.chromium.org/178103004/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2685: last_animation_time_);
Since last_animation_time_ is no longer being set, it shouldn't be used here any
longer. The ultimate consumer of this time used to be the old Blink animation
framework, but now that that's gone, there aren't any consumers and it's safe to
no longer send a real wall clock time back to Blink. (We should ultimately just
remove the wall clock time argument from WebToCCAnimationDelegateAdapter.)

[mithro-old, 2014-03-11 09:08:17]

Hi Ali,

PTAL. 

This CL has got dramatically smaller because changes were moved to other CLs
(https://codereview.chromium.org/194543002/ and
https://codereview.chromium.org/184433007/). I believe all your comments have
been addresses, so if you see something I missed, please do comment.

Tim

[ajuma, 2014-03-11 13:29:18]

Looks good, just a nit about naming.

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

https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2651: double blink_monotonic_time =
This time isn't actually sent to Blink (it's sent to cc::PageScaleAnimation), so
using 'blink' in this name seems misleading.

https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2698: double blink_monotonic_time =
Same naming comment as before. While this time is eventually sent to blink (in
AnimationEvents), the initial consumer is cc::LayerAnimationController.

[mithro-old, 2014-03-11 13:31:20]

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

https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2651: double blink_monotonic_time =
On 2014/03/11 13:29:18, ajuma wrote:
> This time isn't actually sent to Blink (it's sent to cc::PageScaleAnimation),
so
> using 'blink' in this name seems misleading.

Why does cc::PageScaleAnimation use a double? 

Is it a case of something leftover from when cc was moved from WebKit in to
chromium?

Should it be converted to use TimeTicks?

[ajuma, 2014-03-11 13:37:05]

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

https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2651: double blink_monotonic_time =
On 2014/03/11 13:31:20, mithro wrote:
> Why does cc::PageScaleAnimation use a double? 
> 
> Is it a case of something leftover from when cc was moved from WebKit in to
> chromium?
> 
> Should it be converted to use TimeTicks?

Yes to both: it's leftover from the move, and should ideally be using TimeTicks
instead.

[mithro-old, 2014-03-11 14:08:51]

On 2014/03/11 13:37:05, ajuma wrote:
>
https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/178103004/diff/70001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:2651: double blink_monotonic_time =
> On 2014/03/11 13:31:20, mithro wrote:
> > Why does cc::PageScaleAnimation use a double? 
> > 
> > Is it a case of something leftover from when cc was moved from WebKit in to
> > chromium?
> > 
> > Should it be converted to use TimeTicks?
> 
> Yes to both: it's leftover from the move, and should ideally be using
TimeTicks
> instead.

Done! PTAL.

[ajuma, 2014-03-11 14:29:22]

Thanks, lgtm.

[mithro-old, 2014-03-11 15:38:45]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-03-11 15:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/178103004/110001

[commit-bot: I haz the power, 2014-03-11 15:54:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 15:54:54]

Try jobs failed on following builders: linux_chromium_rel

[mithro-old, 2014-03-11 15:59:23]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-03-11 16:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/178103004/110001

[danakj, 2014-03-11 17:48:04]

LGTM2

[commit-bot: I haz the power, 2014-03-11 18:56:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 18:57:00]

Internal error: failed to checkout. Please try again.

[danakj, 2014-03-11 18:57:54]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-11 19:44:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/178103004/110001

[commit-bot: I haz the power, 2014-03-11 20:42:16]

Change committed as 256303