Rename Animate as Begin(Main)Frame

Rename Animate as Begin(Main)Frame

Rename Animate as Begin(Main)Frame to harmonize the terminology used by
different parts of the system. The monotonic start time of the last frame
that is passed into this function is also renamed for consistency.

No functional changes.

Original patch by Simon Pick (picksi@chromium.org).

BUG=346230
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290248

[Sami, 2014-08-04 18:44:47]

Could you guys do a sanity check on this? I was thinking of just making the
parameter a BeginFrameArgs, but there are a few places where we call this
without having all the data needed to build a valid BeginFrameArgs (arguably
that's a bug.)

[Sami, 2014-08-08 16:50:43]

I bit the bullet and made us pass around a BeginFrameArgs instead, because I
think we want to hook the deadline information to the scheduler fairly soon.

It looks scary but it's mostly just a rename, I swear :)

[mithro-old, 2014-08-11 07:33:34]

Hi Sami,

Firstly, I support us doing this, both the making naming consistent and passing
around BeginFrameArgs rather than just TimeTicks. Before I headed out for three
weeks I was also attempting to make a similar changes, doing so I discovered a
number of things which should probably be sorted out before we actually do the
rename. Easier to get it right first time so we don't have to do this type of
thing multiple times :)

One important fact is, what is the actual relationship between "Animation" and
BeginFrame / BeginMainFrame?

As Sami added the ANIMATE step in the scheduler, so he can probably give good
guidance on this. I think the current division in this CL is mostly right, but
renaming Compositor::Animate to Compositor::BeginMainFrame seems misleading as
all the method is doing is updating animation state rather than interacting with
main thread in any way and wanted input on if it is actually right?

There are a couple of locations where small clean ups would make this change
smaller (IE Fixing picture_layer_impl.cc to properly understand tile time), I'm
don't think we should block doing this on them though.

Hope this helps!

Tim
PS Sami, now I'm back we should sync up. Will drop something in your calendar.

https://codereview.chromium.org/429743003/diff/100001/cc/resources/tile_manag...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager_perftest.cc:398: BeginFrameArgs::DefaultInterval());
src/cc/test/begin_frame_args_test.h has helpers for doing this.

https://codereview.chromium.org/429743003/diff/100001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:60: void
FakeLayerTreeHostImpl::SetCurrentFrameTimeTicks(
Should this be "SetCurrentBeginFrameArgs"?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_client.h:20: struct BeginFrameArgs;
What is the chrome policy of doing this rather than including
"cc/output/begin_frame_args.h" ?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:164:
layer_tree_host_impl_->CurrentBeginFrameArgs().frame_time);
An example of the Animate verse BeginFrame here?

It sounds like people want to rip out this background ticking though in
http://code.google.com/p/chromium/issues/detail?id=371747

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1677: begin_impl_frame_interval_ =
args.interval;
Should we log a bug to remove this value now it's stored in the args too?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3157: void
LayerTreeHostImpl::UpdateCurrentFrameTime(const BeginFrameArgs& args) {
UpdateCurrentFrameTime previously didn't take arguments; in this CL I think we
should keep that consistent?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3160: current_begin_frame_args_.frame_time =
gfx::FrameTime::Now();
Can we add a comment about this now usage. It's going to cause a WTF any time
someone new sees it.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3175: BeginFrameArgs::DefaultInterval());
I'm still trying to make this go away :(

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:430: virtual BeginFrameArgs
CurrentBeginFrameArgs() const;
Should these names be updated to "UpdateCurrentFrameArgs" or something?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1318:
BeginFrameArgs::DefaultInterval());
src/cc/test/begin_frame_args_test.h has helpers for doing this.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:58: virtual void
BeginMainFrame(const BeginFrameArgs& args) OVERRIDE {}
Maybe the order here should be;

 WillBeginMainFrame
 BeginMainFrame
 DidBeginMainFrame

https://codereview.chromium.org/429743003/diff/100001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:797:
layer_tree_host()->BeginMainFrame(begin_main_frame_state->begin_frame_args);
Another Animate verses BeginMainFrame.

Here it seems like UpdateClientAnimations was always actually logically
equivalent to BeginMainFrame?

https://codereview.chromium.org/429743003/diff/100001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (left):

https://codereview.chromium.org/429743003/diff/100001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:49: void
UpdateAnimations(base::TimeTicks time);
Why is this removed totally?

https://codereview.chromium.org/429743003/diff/100001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/429743003/diff/100001/ui/compositor/composito...
ui/compositor/compositor.cc:235: host_->Composite(now);
Maybe
  cc::BeginFrameArgs args = cc::BeginFrameArgs::Create(
          gfx::FrameTime::Now(), base::TimeTicks(),
cc::BeginFrameArgs::DefaultInterval());
  BeginMainFrame(args);
  Layout();
  host_->Composite(args.frame_time);

I think that leaves open the question on why host_->Composite takes a frame_time
rather than a BeginFrameArgs.

[Sami, 2014-08-11 17:51:27]

Thanks Tim, all comments addressed.

https://codereview.chromium.org/429743003/diff/100001/cc/resources/tile_manag...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager_perftest.cc:398: BeginFrameArgs::DefaultInterval());
On 2014/08/11 07:33:33, mithro wrote:
> src/cc/test/begin_frame_args_test.h has helpers for doing this.

Handy!

https://codereview.chromium.org/429743003/diff/100001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host_impl.cc:60: void
FakeLayerTreeHostImpl::SetCurrentFrameTimeTicks(
On 2014/08/11 07:33:33, mithro wrote:
> Should this be "SetCurrentBeginFrameArgs"?

Yes, let's make them symmetric.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_client.h (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_client.h:20: struct BeginFrameArgs;
On 2014/08/11 07:33:33, mithro wrote:
> What is the chrome policy of doing this rather than including
> "cc/output/begin_frame_args.h" ?

It's to cut down on time spent parsing the same headers over and over, and also
from keeping transitive header dependencies in check. One day C++ modules will
save us from this madness but we're not there yet.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:164:
layer_tree_host_impl_->CurrentBeginFrameArgs().frame_time);
On 2014/08/11 07:33:33, mithro wrote:
> An example of the Animate verse BeginFrame here?
> 
> It sounds like people want to rip out this background ticking though in
> http://code.google.com/p/chromium/issues/detail?id=371747 

We chatted about this and came to the conclusion that let's keep this level of
Animate/BeginFrame separation until we have a good reason to do something about
it (and figure out what to do with background ticking).

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1677: begin_impl_frame_interval_ =
args.interval;
On 2014/08/11 07:33:33, mithro wrote:
> Should we log a bug to remove this value now it's stored in the args too?

It doesn't look like it's completely redundant since current_begin_frame_args_
gets reset often.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3157: void
LayerTreeHostImpl::UpdateCurrentFrameTime(const BeginFrameArgs& args) {
On 2014/08/11 07:33:33, mithro wrote:
> UpdateCurrentFrameTime previously didn't take arguments; in this CL I think we
> should keep that consistent?

Now that this is called UpdateCurrentBeginFrameArgs I think it makes to pass the
args here. WDYT?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3160: current_begin_frame_args_.frame_time =
gfx::FrameTime::Now();
On 2014/08/11 07:33:33, mithro wrote:
> Can we add a comment about this now usage. It's going to cause a WTF any time
> someone new sees it.

Can you suggest something to write, because I don't know why we override the
time like this anyway? :) Maybe it needs a TODO saying this is wrong?

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:430: virtual BeginFrameArgs
CurrentBeginFrameArgs() const;
On 2014/08/11 07:33:33, mithro wrote:
> Should these names be updated to "UpdateCurrentFrameArgs" or something?

I was a little on the fence since they do actually just reset the time in the
BeginFrameArgs, but let's rename then anyway.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1318:
BeginFrameArgs::DefaultInterval());
On 2014/08/11 07:33:33, mithro wrote:
> src/cc/test/begin_frame_args_test.h has helpers for doing this.

Done.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:58: virtual void
BeginMainFrame(const BeginFrameArgs& args) OVERRIDE {}
On 2014/08/11 07:33:33, mithro wrote:
> Maybe the order here should be;
> 
>  WillBeginMainFrame
>  BeginMainFrame
>  DidBeginMainFrame 

Done.

https://codereview.chromium.org/429743003/diff/100001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/429743003/diff/100001/cc/trees/thread_proxy.c...
cc/trees/thread_proxy.cc:797:
layer_tree_host()->BeginMainFrame(begin_main_frame_state->begin_frame_args);
On 2014/08/11 07:33:33, mithro wrote:
> Another Animate verses BeginMainFrame.
> 
> Here it seems like UpdateClientAnimations was always actually logically
> equivalent to BeginMainFrame?

Yes, that's how I figured.

https://codereview.chromium.org/429743003/diff/100001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (left):

https://codereview.chromium.org/429743003/diff/100001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:49: void
UpdateAnimations(base::TimeTicks time);
On 2014/08/11 07:33:34, mithro wrote:
> Why is this removed totally?

I couldn't find any callers for it.

https://codereview.chromium.org/429743003/diff/100001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/429743003/diff/100001/ui/compositor/composito...
ui/compositor/compositor.cc:235: host_->Composite(now);
On 2014/08/11 07:33:34, mithro wrote:
> Maybe
>   cc::BeginFrameArgs args = cc::BeginFrameArgs::Create(
>           gfx::FrameTime::Now(), base::TimeTicks(),
> cc::BeginFrameArgs::DefaultInterval());
>   BeginMainFrame(args);
>   Layout();
>   host_->Composite(args.frame_time);

Good idea, done.

> I think that leaves open the question on why host_->Composite takes a
frame_time
> rather than a BeginFrameArgs.

One step at a time :)

[Sami, 2014-08-14 16:46:27]

I think brian's still at SIGGRAPH, so enne could you have a look at the cc parts
in here?

[enne (OOO), 2014-08-14 17:21:32]

lgtm

[Sami, 2014-08-14 18:03:42]

Thanks enne. Adding some more owners:

mkosiba@chromium.org: android_webview/ 
qsr@chromium.org: mojo/ 
piman@chromium.org: content/ and ui/ 

PTAL, thanks.

[piman, 2014-08-15 03:04:59]

lgtm

[mkosiba (inactive), 2014-08-15 11:34:40]

android_webview/ LGTM

[brianderson, 2014-08-16 00:11:51]

lgtm. Thanks for routing BeginFrameArgs everywhere too.

https://codereview.chromium.org/429743003/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3159: current_begin_frame_args_.frame_time =
gfx::FrameTime::Now();
I'd say just add a TODO, so it's clear that we intend to fix this and to prevent
this patch from affecting behavior. Tim is getting close to removing the call to
Now here and below.

[qsr, 2014-08-18 08:38:24]

mojo LGTM

[Sami, 2014-08-18 10:48:16]

https://codereview.chromium.org/429743003/diff/180001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/429743003/diff/180001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3159: current_begin_frame_args_.frame_time =
gfx::FrameTime::Now();
On 2014/08/16 00:11:51, brianderson wrote:
> I'd say just add a TODO, so it's clear that we intend to fix this and to
prevent
> this patch from affecting behavior. Tim is getting close to removing the call
to
> Now here and below.

Good idea, done.

[Sami, 2014-08-18 10:51:48]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-18 10:52:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/429743003/220001

[commit-bot: I haz the power, 2014-08-18 11:35:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43010)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/48189)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-18 11:39:16]

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

[commit-bot: I haz the power, 2014-08-18 11:39:17]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2014-08-18 11:45:22]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-18 11:46:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/429743003/240001

[commit-bot: I haz the power, 2014-08-18 13:02:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-18 13:38:45]

Committed patchset #13 (240001) as 290248