cc: Add SetChildrenNeedBeginFrames() & SendBeginFramesToChildren()

cc: Add SetChildrenNeedBeginFrames() & SendBeginFramesToChildren()

This cl is preparation for unified BeginFrame in cc layer.
This implements handling of forwarding_begin_frames_to_children in cc.
And this forwarding is not yet propagated to content layer.

R=brianderson@chromium.org
BUG=372086
TEST=cc_unittests

Committed: https://crrev.com/d3d5f7f94c494dc1289d3ed8ffdc0cc17ba894bc
Cr-Commit-Position: refs/heads/master@{#305226}

[simonhong, 2014-11-12 22:26:58]

PTAL!

[simonhong, 2014-11-15 00:17:36]

brianderson@, should we stop implementing unified BeginFrame until initial
SurfaceScheduler is added?

[mithro-old, 2014-11-17 04:43:58]

I've yet to have a chat with Brian, but I think we should continue with
this work and it will be pretty reusable for any future DisplayScheduler.

Tim

On 15 November 2014 11:17, <simonhong@chromium.org> wrote:

> brianderson@, should we stop implementing unified BeginFrame until initial
> SurfaceScheduler is added?
>
> https://codereview.chromium.org/723713003/
>

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

[jbauman, 2014-11-18 00:49:26]

Yeah, this particular code probably won't be useful with the DisplayScheduler,
but a bunch of things in content/ that depend on it will remain useful, so I
think it still makes sense to work on this patch.

On 2014/11/17 04:43:58, mithro wrote:
> I've yet to have a chat with Brian, but I think we should continue with
> this work and it will be pretty reusable for any future DisplayScheduler.
> 
> Tim
> 
> On 15 November 2014 11:17, <mailto:simonhong@chromium.org> wrote:
> 
> > brianderson@, should we stop implementing unified BeginFrame until initial
> > SurfaceScheduler is added?
> >
> > https://codereview.chromium.org/723713003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-11-18 01:08:19]

John, is it right to assume that the Display Surfaces *could* use the same
BeginFrame API, just the sources end up being the DisplayScheduler and the
observers being the Surfaces?

(Will also add this comment to the surfaces discussion on scheduler-dev..)

Tim

On 18 November 2014 11:49, <jbauman@chromium.org> wrote:

> Yeah, this particular code probably won't be useful with the
> DisplayScheduler,
> but a bunch of things in content/ that depend on it will remain useful, so
> I
> think it still makes sense to work on this patch.
>
> On 2014/11/17 04:43:58, mithro wrote:
>
>> I've yet to have a chat with Brian, but I think we should continue with
>> this work and it will be pretty reusable for any future DisplayScheduler.
>>
>
>  Tim
>>
>
>  On 15 November 2014 11:17, <mailto:simonhong@chromium.org> wrote:
>>
>
>  > brianderson@, should we stop implementing unified BeginFrame until
>> initial
>> > SurfaceScheduler is added?
>> >
>> > https://codereview.chromium.org/723713003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/723713003/
>

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

[brianderson, 2014-11-19 19:03:35]

Patch is looking good. I only have a few small comments.

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:455: ProcessScheduledActions();
This should not trigger any new actions in the Browser, similar to how
DidSwapBuffers will never trigger new actions.

Can you copy the DCHECK from DidSwapBuffers instead of calling
ProcessScheduledActions?

https://codereview.chromium.org/723713003/diff/20001/cc/trees/proxy.cc
File cc/trees/proxy.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/trees/proxy.cc#newcode92
cc/trees/proxy.cc:92: void Proxy::SetChildrenNeedBeginFrames(bool
children_need_begin_frames) {
Thought this was pure virtual in Proxy. Does this implementation belong in
ThreadProxy?

[picksi1, 2014-11-20 13:43:17]

picksi@chromium.org changed reviewers:
+ picksi@chromium.org

[picksi1, 2014-11-20 13:43:18]

Just nosing around in this CL... I have two readability comments that have no
functional relevance!

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:727: 
nit: [Background: I'm a fan of keeping !'s out of if statements] It might be
more readable to remove the ! and swap the body of the if... e.g.

if (HasInitializedOutputSurface())
  return children_need_begin_frames_;
return false;

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:745: ProactiveBeginFrameWanted());
The above code is starting to get hairy IMO!  BeginFrameNeededToAnimateOrDraw()
is [always] called in two separate places and the or'ing together of functions
in the return is starting to get a bit long winded. Is it worth refactoring this
to clean this up before a future dev follows this pattern and compounds the
problem?... The best I can do is as follows, but I don't know if this logic
follows the intention of the implementation.

if (BeginFrameNeededToAnimateOrDraw())
   return true;
if (SupportsProactiveBeginFrame())
   return BeginFrameNeededForChildren() || ProactiveBeginFrameWanted();
return false;

[simonhong, 2014-11-20 16:24:09]

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:455: ProcessScheduledActions();
On 2014/11/19 19:03:34, brianderson wrote:
> This should not trigger any new actions in the Browser, similar to how
> DidSwapBuffers will never trigger new actions.
> 
> Can you copy the DCHECK from DidSwapBuffers instead of calling
> ProcessScheduledActions?

Done.

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:727: 
On 2014/11/20 13:43:18, picksi1 wrote:
> nit: [Background: I'm a fan of keeping !'s out of if statements] It might be
> more readable to remove the ! and swap the body of the if... e.g.
> 
> if (HasInitializedOutputSurface())
>   return children_need_begin_frames_;
> return false;

Thanks! Looks more readable :)

https://codereview.chromium.org/723713003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:745: ProactiveBeginFrameWanted());
On 2014/11/20 13:43:18, picksi1 wrote:
> The above code is starting to get hairy IMO! 
BeginFrameNeededToAnimateOrDraw()
> is [always] called in two separate places and the or'ing together of functions
> in the return is starting to get a bit long winded. Is it worth refactoring
this
> to clean this up before a future dev follows this pattern and compounds the
> problem?... The best I can do is as follows, but I don't know if this logic
> follows the intention of the implementation.
> 
> if (BeginFrameNeededToAnimateOrDraw())
>    return true;
> if (SupportsProactiveBeginFrame())
>    return BeginFrameNeededForChildren() || ProactiveBeginFrameWanted();
> return false;

Hmm, I think we can understand more easily by dividing synchronous or not.
How about below? this removes "!", too.

if (SupportsProactiveBeginFrame())
   return (BeginFrameNeededToAnimateOrDraw() ||
               BeginFrameNeededForChildren() ||
               ProactiveBeginFrameWanted());

DCHECK(!children_need_begin_frames_);
return BeginFrameNeededToAnimateOrDraw();

https://codereview.chromium.org/723713003/diff/20001/cc/trees/proxy.cc
File cc/trees/proxy.cc (right):

https://codereview.chromium.org/723713003/diff/20001/cc/trees/proxy.cc#newcode92
cc/trees/proxy.cc:92: void Proxy::SetChildrenNeedBeginFrames(bool
children_need_begin_frames) {
On 2014/11/19 19:03:34, brianderson wrote:
> Thought this was pure virtual in Proxy. Does this implementation belong in
> ThreadProxy?

Oops. my mistake.
Removed.

[picksi1, 2014-11-20 16:35:08]

Great! The more '!'s you kill the happier I get!

[brianderson, 2014-11-21 02:08:06]

lgtm

https://codereview.chromium.org/723713003/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/723713003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:732: if (SupportsProactiveBeginFrame())
Please use brackets here.

[simonhong, 2014-11-21 15:42:53]

https://codereview.chromium.org/723713003/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/723713003/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:732: if (SupportsProactiveBeginFrame())
On 2014/11/21 02:08:05, brianderson wrote:
> Please use brackets here.

Done.

[simonhong, 2014-11-21 15:43:02]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2014-11-21 15:44:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/723713003/80001

[commit-bot: I haz the power, 2014-11-21 16:38:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-21 16:38:56]

Patchset 5 (id:??) landed as
https://crrev.com/d3d5f7f94c494dc1289d3ed8ffdc0cc17ba894bc
Cr-Commit-Position: refs/heads/master@{#305226}