cc: Control activation from the Scheduler

cc: Control activation from the Scheduler

This threads the NotifyReadyToActivate signal all the way to the
Scheduler, so that the Scheduler is the final arbiter of when
to actually activate.

With this information, we are also able to change the
ScheduledActionActivateIfPossible to just ScheduledActionActivate,
which will help reduce the number of calls to ManageTiles.

This also checks if ManageTiles is needed after a commit. If it
is not needed, the pending tree is ready for activation immediately.

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

[brianderson, 2013-08-23 03:49:25]

PTAL. Tests still need a lot of love, but the patch is otherwise ready for
review.

[reveman, 2013-08-23 16:16:47]

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
active_tree_is_null);
has_active_tree? find it confusing that active tree flags is inverted but not
the pending tree flag.

[enne (OOO), 2013-08-23 17:54:08]

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
active_tree_is_null);
On 2013/08/23 16:16:48, David Reveman wrote:
> has_active_tree? find it confusing that active tree flags is inverted but not
> the pending tree flag.

Doesn't the scheduler already know this information?  For example, the
commit_results_in_pending_tree that you added in the other patch.  It seems like
either the Scheduler should assume the tree state based on commit/activate/abort
actions or it should be told explicitly, but not a little of both.

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:330: // We do not want to activate a
second tree before drawing the first one.
Is this handled by the HasActivatedPendingTreeThisFrame above?

[brianderson, 2013-08-23 18:17:37]

Another version incoming that removes the two hacks I said I would remove.

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
active_tree_is_null);
On 2013/08/23 16:16:48, David Reveman wrote:
> has_active_tree? find it confusing that active tree flags is inverted but not
> the pending tree flag.

Hmm. I used active_tree_is_null because, technically, there is an active tree in
LTHI, it's just that it's root layer is null. From the point of view of the
Scheduler though, we might as well use has_active_tree.  I'll change it if we
are going to keep it.

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
active_tree_is_null);
On 2013/08/23 17:54:09, enne wrote:
> On 2013/08/23 16:16:48, David Reveman wrote:
> > has_active_tree? find it confusing that active tree flags is inverted but
not
> > the pending tree flag.
> 
> Doesn't the scheduler already know this information?  For example, the
> commit_results_in_pending_tree that you added in the other patch.  It seems
like
> either the Scheduler should assume the tree state based on
commit/activate/abort
> actions or it should be told explicitly, but not a little of both.

Is it safe to assume that the active tree's root layer is not null after the
first commit/activation for the lifetime of the Scheduler class?

[danakj, 2013-08-23 18:29:55]

On Fri, Aug 23, 2013 at 2:17 PM, <brianderson@chromium.org> wrote:

> Another version incoming that removes the two hacks I said I would remove.
>
>
>
> https://codereview.chromium.**org/22926024/diff/1/cc/**
>
scheduler/scheduler.h<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h>
> File cc/scheduler/scheduler.h (right):
>
> https://codereview.chromium.**org/22926024/diff/1/cc/**
>
scheduler/scheduler.h#**newcode71<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newcode71>
> cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,
> bool active_tree_is_null);
> On 2013/08/23 16:16:48, David Reveman wrote:
>
>> has_active_tree? find it confusing that active tree flags is inverted
>>
> but not
>
>> the pending tree flag.
>>
>
> Hmm. I used active_tree_is_null because, technically, there is an active
> tree in LTHI, it's just that it's root layer is null. From the point of
> view of the Scheduler though, we might as well use has_active_tree.
> I'll change it if we are going to keep it.
>
>
> https://codereview.chromium.**org/22926024/diff/1/cc/**
>
scheduler/scheduler.h#**newcode71<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newcode71>
> cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,
> bool active_tree_is_null);
> On 2013/08/23 17:54:09, enne wrote:
>
>> On 2013/08/23 16:16:48, David Reveman wrote:
>> > has_active_tree? find it confusing that active tree flags is
>>
> inverted but not
>
>> > the pending tree flag.
>>
>
>  Doesn't the scheduler already know this information?  For example, the
>> commit_results_in_pending_tree that you added in the other patch.  It
>>
> seems like
>
>> either the Scheduler should assume the tree state based on
>>
> commit/activate/abort
>
>> actions or it should be told explicitly, but not a little of both.
>>
>
> Is it safe to assume that the active tree's root layer is not null after
> the first commit/activation for the lifetime of the Scheduler class?
>

No the main thread can remove the root layer and we'd commit/activate that.


>
>
https://codereview.chromium.**org/22926024/<https://codereview.chromium.org/2...
>

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

[enne (OOO), 2013-08-23 18:30:35]

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
active_tree_is_null);
On 2013/08/23 18:17:38, Brian Anderson wrote:

> Is it safe to assume that the active tree's root layer is not null after the
> first commit/activation for the lifetime of the Scheduler class?

Ok, good point.  I don't think we do, but I think it's technically possible.

That said, it seems like you're only using active_tree_is_null to check in
ShouldActivatePendingTree() to avoid spurious aborted draws before the first
activation.  You could maybe replace that with bool has_ever_activated_, which
the scheduler definitely knows about.

[brianderson, 2013-08-23 20:49:19]

On 2013/08/23 18:30:35, enne wrote:
> https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newc...
> cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,  bool
> active_tree_is_null);
> On 2013/08/23 18:17:38, Brian Anderson wrote:
> 
> > Is it safe to assume that the active tree's root layer is not null after the
> > first commit/activation for the lifetime of the Scheduler class?
> 
> Ok, good point.  I don't think we do, but I think it's technically possible.
> 
> That said, it seems like you're only using active_tree_is_null to check in
> ShouldActivatePendingTree() to avoid spurious aborted draws before the first
> activation.  You could maybe replace that with bool has_ever_activated_, which
> the scheduler definitely knows about.

Good point too. I'm using it to avoid spurious activation during output surface
initialization, in which case, I could use (output_surface_state_ !=
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION) in place of active_tree_is_null.

I will remove active_tree_is_null.

[brianderson, 2013-08-23 20:50:17]

On 2013/08/23 18:29:55, danakj wrote:
> On Fri, Aug 23, 2013 at 2:17 PM, <mailto:brianderson@chromium.org> wrote:
> 
> > Another version incoming that removes the two hacks I said I would remove.
> >
> >
> >
> > https://codereview.chromium.**org/22926024/diff/1/cc/**
> >
>
scheduler/scheduler.h<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h>
> > File cc/scheduler/scheduler.h (right):
> >
> > https://codereview.chromium.**org/22926024/diff/1/cc/**
> >
>
scheduler/scheduler.h#**newcode71<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newcode71>
> > cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,
> > bool active_tree_is_null);
> > On 2013/08/23 16:16:48, David Reveman wrote:
> >
> >> has_active_tree? find it confusing that active tree flags is inverted
> >>
> > but not
> >
> >> the pending tree flag.
> >>
> >
> > Hmm. I used active_tree_is_null because, technically, there is an active
> > tree in LTHI, it's just that it's root layer is null. From the point of
> > view of the Scheduler though, we might as well use has_active_tree.
> > I'll change it if we are going to keep it.
> >
> >
> > https://codereview.chromium.**org/22926024/diff/1/cc/**
> >
>
scheduler/scheduler.h#**newcode71<https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler.h#newcode71>
> > cc/scheduler/scheduler.h:71: void SetHasTrees(bool has_pending_tree,
> > bool active_tree_is_null);
> > On 2013/08/23 17:54:09, enne wrote:
> >
> >> On 2013/08/23 16:16:48, David Reveman wrote:
> >> > has_active_tree? find it confusing that active tree flags is
> >>
> > inverted but not
> >
> >> > the pending tree flag.
> >>
> >
> >  Doesn't the scheduler already know this information?  For example, the
> >> commit_results_in_pending_tree that you added in the other patch.  It
> >>
> > seems like
> >
> >> either the Scheduler should assume the tree state based on
> >>
> > commit/activate/abort
> >
> >> actions or it should be told explicitly, but not a little of both.
> >>
> >
> > Is it safe to assume that the active tree's root layer is not null after
> > the first commit/activation for the lifetime of the Scheduler class?
> >
> 
> No the main thread can remove the root layer and we'd commit/activate that.

I guess we'll still be safe, because can_draw should go false in that case and
we will start aborting draws without swapping to screen.

> 
> 
> >
> >
>
https://codereview.chromium.**org/22926024/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2013-08-23 20:51:59]

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:330: // We do not want to activate a
second tree before drawing the first one.
On 2013/08/23 17:54:09, enne wrote:
> Is this handled by the HasActivatedPendingTreeThisFrame above?

There's some overlap, but there are cases where
HasActivatedPendingTreeThisFrame() will be false and active_tree_has_been_drawn_
will also be false, in which case we still don't want to activate.

Now that I think about it, I should get rid of
HasActivatedPendingTreeThisFrame(). If activations are blocked by the active
tree having been drawn, activating twice in the same frame will be ok.

Also, we don't have to worry about infinite loops because
active_tree_has_been_drawn_ will go false to block the next activation.

[brianderson, 2013-08-23 21:22:20]

PTAL again.

[enne (OOO), 2013-08-23 21:38:36]

https://codereview.chromium.org/22926024/diff/13001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/13001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:71: void SetHasPendingTree(bool has_pending_tree);
I'm also not sure why the scheduler needs to be told explicitly about the
pending tree here when it's driving activation.  It's safe to assume that
activating = no pending tree and non-aborted commit with impl-side painting =
has pending tree.

I'm mostly balking at calculating it implicitly in
commit_results_in_pending_tree and then also getting it set explicitly here in
SetHasPendingTree.  It seems like it'd be cleaner to do one or the other but not
both.

[brianderson, 2013-08-23 23:11:52]

https://codereview.chromium.org/22926024/diff/13001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/22926024/diff/13001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:71: void SetHasPendingTree(bool has_pending_tree);
On 2013/08/23 21:38:36, enne wrote:
> I'm also not sure why the scheduler needs to be told explicitly about the
> pending tree here when it's driving activation.  It's safe to assume that
> activating = no pending tree and non-aborted commit with impl-side painting =
> has pending tree.
> 
> I'm mostly balking at calculating it implicitly in
> commit_results_in_pending_tree and then also getting it set explicitly here in
> SetHasPendingTree.  It seems like it'd be cleaner to do one or the other but
not
> both.

Ah, ok. I'm removing SetHasPendingTree now and it's definitely more clear.

[brianderson, 2013-08-23 23:19:42]

I'm working on the unit tests now and notice that tests that do readbacks are
hanging sometimes. I may have get the readback patch in before this one.

[brianderson, 2013-08-24 00:22:56]

PTAL. I got rid of SetHasPendingTree.  I also got rid of
ActivatePendingTreeIfNecessary, since other places calling it could activate the
pending tree without the Scheduler knowing.

[brianderson, 2013-08-24 02:33:28]

A number of test fixes, but I wonder if the LayerTreeHostImplClientInterposer is
too hacky.

https://codereview.chromium.org/22926024/diff/29001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/22926024/diff/29001/cc/test/layer_tree_test.c...
cc/test/layer_tree_test.cc:290: new
LayerTreeHostImplClientInterposer(host_impl_client));
I'm trying to think of a better way to do this... any ideas? Or is this
acceptable?

[brianderson, 2013-08-24 16:05:03]

On 2013/08/24 02:33:28, Brian Anderson wrote:
> A number of test fixes, but I wonder if the LayerTreeHostImplClientInterposer
is
> too hacky.
> 

Had a thought on my drive home: I'll just add a
BlockNotifyReadyToActivateForTesting to LayerTreeHostImpl. Much more
straightforward than the LayerTreeHostImplClientInterposer.

[enne (OOO), 2013-08-26 21:55:13]

This looks really solid.  I have a few small questions:

https://codereview.chromium.org/22926024/diff/29001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/29001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:548: void
SchedulerStateMachine::UpdateStateOnDraw(bool did_swap) {
All of these UpdateStateOnFoo functions make the code super readable.  Thanks!

https://codereview.chromium.org/22926024/diff/29001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/22926024/diff/29001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.h:218: bool has_active_tree_;
unused?

https://codereview.chromium.org/22926024/diff/29001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/22926024/diff/29001/cc/test/layer_tree_test.c...
cc/test/layer_tree_test.cc:290: new
LayerTreeHostImplClientInterposer(host_impl_client));
On 2013/08/24 02:33:29, Brian Anderson wrote:
> I'm trying to think of a better way to do this... any ideas? Or is this
> acceptable?

This seems fine to me.  It's unfortunate that because of the way other code is
structured you have to do this with a has-a relationship instead of an is-a
(where you could inherit all the functions you are just thunking through).  That
said, you keep all of the complication in testing code, so this feels like a
pretty elegant solution to me.  :)

Bikeshedding, I would maybe name it Proxy instead of Interposer, since for me
that's a more common name for its job: it sits in front of the real client and
forwards requests to it.

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1461: if (!tile_manager_ ||
tile_manager_->AreTilesRequiredForActivationReady()) {
Where does this removed !tile_manager logic end up in your patch?  Maybe this
should go with the !manage_tiles_needed conditional to immediately activate
during commit?

Could you add a basic LayerTreeHostTest with impl-side painting to make sure
that a TileManager-less LTHI will still continue to activate and not get stuck?

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1481: client_->DidActivatePendingTree();
Why did this move from the end of the function? (Just trying to understand.)

[enne (OOO), 2013-08-26 21:57:52]

Oops, I missed that you'd updated this patch.

https://codereview.chromium.org/22926024/diff/33001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22926024/diff/33001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:175:
block_notify_ready_to_activate_for_testing_(false),
I kind of liked the previous interposer in that it kept the testing code out of
production code.  Could you move this LTHI code to some derived LTHI type and
move the block tracking there?

[brianderson, 2013-08-26 22:47:25]

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1461: if (!tile_manager_ ||
tile_manager_->AreTilesRequiredForActivationReady()) {
On 2013/08/26 21:55:13, enne wrote:
> Where does this removed !tile_manager logic end up in your patch?  Maybe this
> should go with the !manage_tiles_needed conditional to immediately activate
> during commit?
> 
> Could you add a basic LayerTreeHostTest with impl-side painting to make sure
> that a TileManager-less LTHI will still continue to activate and not get
stuck?

Good idea. I will add handle the !time_manager case and add a test that fails if
that case is not handled.

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1481: client_->DidActivatePendingTree();
On 2013/08/26 21:55:13, enne wrote:
> Why did this move from the end of the function? (Just trying to understand.)

I did this a long time ago and copied it over, so I forget my original
motivation... but I think it was to unblock other threads as soon as possible. I
wonder if I could move this up to the top of the function even and rename it to
WillActivatePendingTree().

In any case, it's only tangentially related to this patch, so I should probably
make it a separate patch for easier bisecting if anything goes wrong.

https://codereview.chromium.org/22926024/diff/33001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22926024/diff/33001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:175:
block_notify_ready_to_activate_for_testing_(false),
On 2013/08/26 21:57:52, enne wrote:
> I kind of liked the previous interposer in that it kept the testing code out
of
> production code.  Could you move this LTHI code to some derived LTHI type and
> move the block tracking there?

I will put the code into LayerTreeHostImplForTesting, which is where a lot of
this kind of stuff already lives. I would be tempted to keep the client proxy if
it had more uses that couldn't be implemented in LayerTreeHostImplForTesting.

I will still need a stub implementation of
LayerTreeHostImpl::BlockNotifyReadyToActivateForTesting, for which I will just
call NOTREACHED().

[brianderson, 2013-08-26 23:06:25]

All tests should be passing now in the most recent patch. I've addressed all of
enne's comments except that the !tile_manager test is still pending.

[brianderson, 2013-08-26 23:33:20]

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/22926024/diff/29001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:1461: if (!tile_manager_ ||
tile_manager_->AreTilesRequiredForActivationReady()) {
On 2013/08/26 21:55:13, enne wrote:
> Where does this removed !tile_manager logic end up in your patch?  Maybe this
> should go with the !manage_tiles_needed conditional to immediately activate
> during commit?
> 
> Could you add a basic LayerTreeHostTest with impl-side painting to make sure
> that a TileManager-less LTHI will still continue to activate and not get
stuck?

In looking at how to implement this test, it seems like we're only missinga
tile_manager when we don't have a renderer / output surface.  In which case, the
SchedulerStateMachine::PendingActicationsShouldBeForced() will force activations
and there are many context lost tests that exercise this code path.

Is there a case where we can have an output surface but no tile_manager_ that
i'm missing?

[enne (OOO), 2013-08-26 23:41:13]

On 2013/08/26 23:33:20, Brian Anderson wrote:

> In looking at how to implement this test, it seems like we're only missinga
> tile_manager when we don't have a renderer / output surface.  In which case,
the
> SchedulerStateMachine::PendingActicationsShouldBeForced() will force
activations
> and there are many context lost tests that exercise this code path.
> 
> Is there a case where we can have an output surface but no tile_manager_ that
> i'm missing?

Oh!  My apologies.  This was just my poor memory remembering a time where
Android WebView used to support !tile_manager, but in reality they switched to
just using a zero memory policy.  All the checks for !tile_manager should
probably just get removed (in some other patch).

[enne (OOO), 2013-08-26 23:50:10]

lgtm

https://codereview.chromium.org/22926024/diff/36001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/22926024/diff/36001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.h:190: virtual void
BlockNotifyReadyToActivateForTesting(bool block);
I think maybe these "ForTesting" functions could get moved out of LTHI if
TestHooks took a LayerTreeHostImplForTesting instead of just a vanilla
LayerTreeHostImpl.

That said, doing that would be a giant yak shave for little relative benefit. 
I'm ok with landing this as-is.

[danakj, 2013-08-27 00:59:12]

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:218: // These are all the cases where,
if we do not abort draws to make
"if we do not perform aborted draws while needs_redraw_ is true, even though we
would not normally try to draw at all, to make"

can you make this say something like ^ to help clarify why returning true if
PendingActivationsShouldBeForced() is correct, so I can remember this in the
future?

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:277: // If we are going to abort draws,
we should do so ASAP.
"...we should do so ASAP. If we're waiting for the first draw, then perform the
aborted draw to keep the scheduler moving forward. If we're not then do not draw
just to have it aborted for no reason."

can you say something along those lines so i can remember this too?

[danakj, 2013-08-27 01:24:02]

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:311: output_surface_state_ !=
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
What if the active tree hasn't been drawn because it can't?  I suppose then we'd
draw and abort to make active_tree_has_been_drawn be true, pushing this
activation out a vsync.

Why not check can_draw here, or even PendingDrawsShouldBeAborted() - name seems
out of place, but when that's true we won't draw, right? Would it make sense to
shorten that cycle and make this more direct?

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:332: return ShouldActivatePendingTree()
|| ShouldDraw() ||
Can you comment explaining why these bools are used for this method? Maybe split
the 3 up with a comment for each?

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:523: // Update state if we have a new
active tree to draw.
, or the active tree was unchanged but we're going to do a readback.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:526: needs_redraw_ = true;
2 space indent

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:602: // Do not be proactive when
invisible.
can you update this comment to explain the output surface part?

https://codereview.chromium.org/22926024/diff/36001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/22926024/diff/36001/cc/test/layer_tree_test.h...
cc/test/layer_tree_test.h:34: virtual void
WillBeginFrameOnThread(LayerTreeHostImpl* host_impl,
can this be WillBeginImplFrameOnThread? (that is what it is, right?)

https://codereview.chromium.org/22926024/diff/36001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/22926024/diff/36001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.h:190: virtual void
BlockNotifyReadyToActivateForTesting(bool block);
On 2013/08/26 23:50:10, enne wrote:
> I think maybe these "ForTesting" functions could get moved out of LTHI if
> TestHooks took a LayerTreeHostImplForTesting instead of just a vanilla
> LayerTreeHostImpl.
> 
> That said, doing that would be a giant yak shave for little relative benefit. 
> I'm ok with landing this as-is.

Ya, this function is cool and I like how you did it, but you could avoid it if
we instead passed the LayerTreeHostImplForTesting created in LayerTreeTest to
all the test hooks. For another patch.

[brianderson, 2013-08-27 01:45:37]

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:218: // These are all the cases where,
if we do not abort draws to make
On 2013/08/27 00:59:12, danakj wrote:
> "if we do not perform aborted draws while needs_redraw_ is true, even though
we
> would not normally try to draw at all, to make"
> 
> can you make this say something like ^ to help clarify why returning true if
> PendingActivationsShouldBeForced() is correct, so I can remember this in the
> future?

Done.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:277: // If we are going to abort draws,
we should do so ASAP.
On 2013/08/27 00:59:12, danakj wrote:
> "...we should do so ASAP. If we're waiting for the first draw, then perform
the
> aborted draw to keep the scheduler moving forward. If we're not then do not
draw
> just to have it aborted for no reason."
> 
> can you say something along those lines so i can remember this too?

Done.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:311: output_surface_state_ !=
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
On 2013/08/27 01:24:02, danakj wrote:
> What if the active tree hasn't been drawn because it can't?  I suppose then
we'd
> draw and abort to make active_tree_has_been_drawn be true, pushing this
> activation out a vsync.

It is true that the draw will be aborted to unblock the activation. But the
aborted draw is not tied to vsync and the activation (forced or not) is not tied
to vsync either, so everything should happen very quickly.

> 
> Why not check can_draw here, or even PendingDrawsShouldBeAborted() - name
seems
> out of place, but when that's true we won't draw, right? Would it make sense
to
> shorten that cycle and make this more direct?

I initially used PendingDrawsShouldBeAborted to force activations, but that
results in checkerboarding in many cases, which I describe in the comments at
the end of PendingDrawsShouldBeAborted.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:332: return ShouldActivatePendingTree()
|| ShouldDraw() ||
On 2013/08/27 01:24:02, danakj wrote:
> Can you comment explaining why these bools are used for this method? Maybe
split
> the 3 up with a comment for each?

Thanks for making me do this exercise! ShouldActivatePendingTree is not needed
anymore because we don't need to poll for activation anymore (like when it was
called SHouldAttemptTreeActivation).

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:523: // Update state if we have a new
active tree to draw.
On 2013/08/27 01:24:02, danakj wrote:
> , or the active tree was unchanged but we're going to do a readback.

Done.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:526: needs_redraw_ = true;
On 2013/08/27 01:24:02, danakj wrote:
> 2 space indent

Done.

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:602: // Do not be proactive when
invisible.
On 2013/08/27 01:24:02, danakj wrote:
> can you update this comment to explain the output surface part?

Done.

https://codereview.chromium.org/22926024/diff/36001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/22926024/diff/36001/cc/test/layer_tree_test.h...
cc/test/layer_tree_test.h:34: virtual void
WillBeginFrameOnThread(LayerTreeHostImpl* host_impl,
On 2013/08/27 01:24:02, danakj wrote:
> can this be WillBeginImplFrameOnThread? (that is what it is, right?)

Yes that's what it is. I would like to do the BeginFrame renaming patch (round
2) separately though.

[danakj, 2013-08-27 02:03:47]

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:311: output_surface_state_ !=
OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
On 2013/08/27 01:45:37, Brian Anderson wrote:
> On 2013/08/27 01:24:02, danakj wrote:
> > What if the active tree hasn't been drawn because it can't?  I suppose then
> we'd
> > draw and abort to make active_tree_has_been_drawn be true, pushing this
> > activation out a vsync.
> 
> It is true that the draw will be aborted to unblock the activation. But the
> aborted draw is not tied to vsync and the activation (forced or not) is not
tied
> to vsync either, so everything should happen very quickly.
> 
> > 
> > Why not check can_draw here, or even PendingDrawsShouldBeAborted() - name
> seems
> > out of place, but when that's true we won't draw, right? Would it make sense
> to
> > shorten that cycle and make this more direct?
> 
> I initially used PendingDrawsShouldBeAborted to force activations, but that
> results in checkerboarding in many cases, which I describe in the comments at
> the end of PendingDrawsShouldBeAborted.

Ah, so it's like PendingDrawsShouldBeAborted() == ForcePendingActivations() ||
UnableToDraw(). And then here we could allow activation when we haven't drawn if
UnableToDraw()?

I'm not sure where your next patches take this logic, so do you think that would
be better or worse than what's here now?

[danakj, 2013-08-27 02:05:58]

On 2013/08/27 02:03:47, danakj wrote:
>
https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/22926024/diff/36001/cc/scheduler/scheduler_st...
> cc/scheduler/scheduler_state_machine.cc:311: output_surface_state_ !=
> OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
> On 2013/08/27 01:45:37, Brian Anderson wrote:
> > On 2013/08/27 01:24:02, danakj wrote:
> > > What if the active tree hasn't been drawn because it can't?  I suppose
then
> > we'd
> > > draw and abort to make active_tree_has_been_drawn be true, pushing this
> > > activation out a vsync.
> > 
> > It is true that the draw will be aborted to unblock the activation. But the
> > aborted draw is not tied to vsync and the activation (forced or not) is not
> tied
> > to vsync either, so everything should happen very quickly.
> > 
> > > 
> > > Why not check can_draw here, or even PendingDrawsShouldBeAborted() - name
> > seems
> > > out of place, but when that's true we won't draw, right? Would it make
sense
> > to
> > > shorten that cycle and make this more direct?
> > 
> > I initially used PendingDrawsShouldBeAborted to force activations, but that
> > results in checkerboarding in many cases, which I describe in the comments
at
> > the end of PendingDrawsShouldBeAborted.
> 
> Ah, so it's like PendingDrawsShouldBeAborted() == ForcePendingActivations() ||
> UnableToDraw(). And then here we could allow activation when we haven't drawn
if
> UnableToDraw()?
> 
> I'm not sure where your next patches take this logic, so do you think that
would
> be better or worse than what's here now?

UnableToDraw is kinda sketchy actually, cuz you can draw when it would return
false, if the reason was visible, and you want to draw to do a readback.

So please ignore this for now. :)

[danakj, 2013-08-27 02:06:09]

Oh, and LGTM!

[danakj, 2013-08-27 02:06:28]

https://codereview.chromium.org/22926024/diff/77001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/22926024/diff/77001/cc/test/layer_tree_test.h...
cc/test/layer_tree_test.h:35: const BeginFrameArgs& args) {}
(indenting)

[commit-bot: I haz the power, 2013-08-27 02:21:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/22926024/61018

[commit-bot: I haz the power, 2013-08-27 06:33:49]

Change committed as 219714