[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_sequence_number in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2632563003
Cr-Commit-Position: refs/heads/master@{#452997}
Committed: https://chromium.googlesource.com/chromium/src/+/37da9b3e656ac85c42e3f375e84b55c98b397da1

[Eric Seckler, 2017-01-13 09:11:49]

Description was changed from

==========
[cc] Calculate the correct latest_confirmed_frame in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_frame in cc::Scheduler.

BUG=646774, 401331
==========

to

==========
[cc] Calculate the correct latest_confirmed_frame in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_frame in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-01-13 09:12:31]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 09:12:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 10:51:37]

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

[commit-bot: I haz the power, 2017-01-13 10:51:38]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-01-13 11:17:14]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org, skyostil@chromium.org

[Eric Seckler, 2017-01-13 11:17:14]

Sami, Brian, I'm sending this to both of you - an additional set of eyes
probably can't hurt :)

Thanks!

[brianderson, 2017-01-13 19:13:49]

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:438: begin_main_frame_args_.source_id,
The begin_main_frame_args_.source_id at this point might not correspond to the
same source_id that created last_frame_number_compositor_frame_was_fresh. I
think you'll need to track multiple source_ids through the pipeline just like
you did with the last_frame_number*s.

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:862: current_frame_number_ =
sequence_number;
I think it would be nice to keep the frame_number and {source_id_,
sequence_number_} pair separate concepts.

Where frame_number monotonically increases by one each time as it did before
this patch, which let's us ignore begin frame source changes when looking at
traces or in tests.

Then track the pairs with some new struct for the purposes of acking. Maybe
include the words "main frame ack" in them.

[Sami, 2017-01-16 10:41:02]

Great points Brian!

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:438: begin_main_frame_args_.source_id,
On 2017/01/13 19:13:49, brianderson wrote:
> The begin_main_frame_args_.source_id at this point might not correspond to the
> same source_id that created last_frame_number_compositor_frame_was_fresh. I
> think you'll need to track multiple source_ids through the pipeline just like
> you did with the last_frame_number*s.

I think this is okay, because when the source_id changes we reset everything to
be fresh up to that point, which is effectively doing the same thing as would
checking source_id here. However Eric noticed we're not always telling the state
machine when the source_id might change, so he's gonna fix that still. FWIW we
also decided to reset to everything to kInvalidSequenceNumber to make it more
obvious that there was a discontinuity.

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine_unittest.cc:2168: {
nit: Consider splitting each scope to a separate unit test with a descriptive
name. That way it's easier to diagnose failures that only affect some of the
subtests.

[Eric Seckler, 2017-01-16 14:27:56]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 14:28:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-16 14:33:01]

Thank you, Brian & Sami!

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:438: begin_main_frame_args_.source_id,
On 2017/01/16 10:41:02, Sami wrote:
> On 2017/01/13 19:13:49, brianderson wrote:
> > The begin_main_frame_args_.source_id at this point might not correspond to
the
> > same source_id that created last_frame_number_compositor_frame_was_fresh. I
> > think you'll need to track multiple source_ids through the pipeline just
like
> > you did with the last_frame_number*s.
> 
> I think this is okay, because when the source_id changes we reset everything
to
> be fresh up to that point, which is effectively doing the same thing as would
> checking source_id here. However Eric noticed we're not always telling the
state
> machine when the source_id might change, so he's gonna fix that still. FWIW we
> also decided to reset to everything to kInvalidSequenceNumber to make it more
> obvious that there was a discontinuity.

What Sami said. :) Regarding telling the state machine about source_id changes,
I'm now telling it about every BeginFrame received by the Scheduler to update
the sequence numbers even in cases where we skip a BeginImplFrame().

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:862: current_frame_number_ =
sequence_number;
On 2017/01/13 19:13:49, brianderson wrote:
> I think it would be nice to keep the frame_number and {source_id_,
> sequence_number_} pair separate concepts.
> 
> Where frame_number monotonically increases by one each time as it did before
> this patch, which let's us ignore begin frame source changes when looking at
> traces or in tests.
> 
> Then track the pairs with some new struct for the purposes of acking. Maybe
> include the words "main frame ack" in them.
> 

Tracking BeginFrame sequence numbers separately from frame numbers now. As Sami
mentioned, we don't think it's necessary to track the source_id for all pipeline
stages, since we reset all last_begin_frame_sequence_number_* fields on
source_id change in OnBeginFrameReceived().

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine_unittest.cc:2168: {
On 2017/01/16 10:41:02, Sami wrote:
> nit: Consider splitting each scope to a separate unit test with a descriptive
> name. That way it's easier to diagnose failures that only affect some of the
> subtests.

Done.

[commit-bot: I haz the power, 2017-01-16 15:16:56]

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

[commit-bot: I haz the power, 2017-01-16 15:16:57]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-01-17 18:01:24]

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

https://codereview.chromium.org/2632563003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:438: begin_main_frame_args_.source_id,
On 2017/01/16 14:33:01, Eric Seckler wrote:
> On 2017/01/16 10:41:02, Sami wrote:
> > On 2017/01/13 19:13:49, brianderson wrote:
> > > The begin_main_frame_args_.source_id at this point might not correspond to
> the
> > > same source_id that created last_frame_number_compositor_frame_was_fresh.
I
> > > think you'll need to track multiple source_ids through the pipeline just
> like
> > > you did with the last_frame_number*s.
> > 
> > I think this is okay, because when the source_id changes we reset everything
> to
> > be fresh up to that point, which is effectively doing the same thing as
would
> > checking source_id here. However Eric noticed we're not always telling the
> state
> > machine when the source_id might change, so he's gonna fix that still. FWIW
we
> > also decided to reset to everything to kInvalidSequenceNumber to make it
more
> > obvious that there was a discontinuity.
> 
> What Sami said. :) Regarding telling the state machine about source_id
changes,
> I'm now telling it about every BeginFrame received by the Scheduler to update
> the sequence numbers even in cases where we skip a BeginImplFrame().

Badum.. I'm just realizing that this is broken since
BeginImplFrameWithDeadline() may trigger the previous BeginFrame's deadline.
I'll have to think about a different solution for this... :)

[Eric Seckler, 2017-01-18 18:13:03]

Next attempt :)

I think this should fix the issues with changing source_id by handling part of
that in the Scheduler rather than the SchedulerStateMachine.

Brian, could you have a look at the comment below please?

Thank you! :)

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:334: if (!observing_begin_frame_source_) {
I'm trying to test this branch (see scheduler_unittest.cc), but I can't find a
way to set things up so that I end up with !observing_begin_frame_source_ here.
Is there a way that this can be the case?

We can only end up in BeginImplFrameWithDeadline() if
state_machine_.BeginFrameNeeded() was true during OnBeginFrameDerivedImpl().
Plus a previous BeginImplFrame needs to be started but not yet finished.

Thus, if observing_begin_frame_source_ should be false at this point, that means
that some condition affecting state_machine_.BeginFrameNeeded() would have to
have changed during the BeginImplFrameDeadline. The only thing that I can see is
the state machine's needs_redraw_. Yet, if needs_redraw_ becomes false,
did_draw_in_last_frame_ becomes true, and the state machine will request a
proactive BeginFrame. Thus, in this case, state_machine_.BeginFrameNeeded()
wouldn't change.

I've played around with a few different interleavings of missed BeginFrames etc,
but I can't find one that puts me here with !observing_begin_frame_source.

Can you think of a situation that puts me here?

If not, would it make sense to get rid of this branch (replace by DCHECK with
explanation)? It'd also enable us to get rid of
OnBeginFrameDroppedNotObserving(). It would make this bit of code more brittle
to future StateMachine changes, though.

An alternative might be a DisableProactiveBeginFramesForTest() or something
alike that lets me build a test case that exercises this branch?

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:3436: // TODO(eseckler): This doesn't
actually work, because
Here's my current attempt at creating a situation that triggers the branch, but
this approach doesn't work for reasons described above.

[brianderson, 2017-01-18 19:53:54]

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:334: if (!observing_begin_frame_source_) {
On 2017/01/18 18:13:03, Eric Seckler wrote:
> I'm trying to test this branch (see scheduler_unittest.cc), but I can't find a
> way to set things up so that I end up with !observing_begin_frame_source_
here.
> Is there a way that this can be the case?
> 
> We can only end up in BeginImplFrameWithDeadline() if
> state_machine_.BeginFrameNeeded() was true during OnBeginFrameDerivedImpl().
> Plus a previous BeginImplFrame needs to be started but not yet finished.
> 
> Thus, if observing_begin_frame_source_ should be false at this point, that
means
> that some condition affecting state_machine_.BeginFrameNeeded() would have to
> have changed during the BeginImplFrameDeadline. The only thing that I can see
is
> the state machine's needs_redraw_. Yet, if needs_redraw_ becomes false,
> did_draw_in_last_frame_ becomes true, and the state machine will request a
> proactive BeginFrame. Thus, in this case, state_machine_.BeginFrameNeeded()
> wouldn't change.
> 
> I've played around with a few different interleavings of missed BeginFrames
etc,
> but I can't find one that puts me here with !observing_begin_frame_source.
> 
> Can you think of a situation that puts me here?
> 
> If not, would it make sense to get rid of this branch (replace by DCHECK with
> explanation)? It'd also enable us to get rid of
> OnBeginFrameDroppedNotObserving(). It would make this bit of code more brittle
> to future StateMachine changes, though.
> 
> An alternative might be a DisableProactiveBeginFramesForTest() or something
> alike that lets me build a test case that exercises this branch?

I think it doesn't show up in tests because the tests don't use IPCs. With IPC's
there can be a BeginFrame in flight after we stop requesting BeginFrames.

You may need to create a new fake BeginFrameSource that has a delay between when
the BeginFrame is created vs. when it is run; where it will still run even if
BeginFramesNeeded becomes false after creation. Or maybe a
DelayedBeginFrameQueue that forwards BeginFrames from a source with a delay.

Would that help?

[Eric Seckler, 2017-01-18 20:27:23]

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:334: if (!observing_begin_frame_source_) {
On 2017/01/18 19:53:54, brianderson wrote:
> On 2017/01/18 18:13:03, Eric Seckler wrote:
> > I'm trying to test this branch (see scheduler_unittest.cc), but I can't find
a
> > way to set things up so that I end up with !observing_begin_frame_source_
> here.
> > Is there a way that this can be the case?
> > 
> > We can only end up in BeginImplFrameWithDeadline() if
> > state_machine_.BeginFrameNeeded() was true during OnBeginFrameDerivedImpl().
> > Plus a previous BeginImplFrame needs to be started but not yet finished.
> > 
> > Thus, if observing_begin_frame_source_ should be false at this point, that
> means
> > that some condition affecting state_machine_.BeginFrameNeeded() would have
to
> > have changed during the BeginImplFrameDeadline. The only thing that I can
see
> is
> > the state machine's needs_redraw_. Yet, if needs_redraw_ becomes false,
> > did_draw_in_last_frame_ becomes true, and the state machine will request a
> > proactive BeginFrame. Thus, in this case, state_machine_.BeginFrameNeeded()
> > wouldn't change.
> > 
> > I've played around with a few different interleavings of missed BeginFrames
> etc,
> > but I can't find one that puts me here with !observing_begin_frame_source.
> > 
> > Can you think of a situation that puts me here?
> > 
> > If not, would it make sense to get rid of this branch (replace by DCHECK
with
> > explanation)? It'd also enable us to get rid of
> > OnBeginFrameDroppedNotObserving(). It would make this bit of code more
brittle
> > to future StateMachine changes, though.
> > 
> > An alternative might be a DisableProactiveBeginFramesForTest() or something
> > alike that lets me build a test case that exercises this branch?
> 
> I think it doesn't show up in tests because the tests don't use IPCs. With
IPC's
> there can be a BeginFrame in flight after we stop requesting BeginFrames.
> 
> You may need to create a new fake BeginFrameSource that has a delay between
when
> the BeginFrame is created vs. when it is run; where it will still run even if
> BeginFramesNeeded becomes false after creation. Or maybe a
> DelayedBeginFrameQueue that forwards BeginFrames from a source with a delay.
> 
> Would that help?

Hm, I don't think that would help. If there was an IPC delay during which
BeginFrameNeeded becomes false, the BeginFrame would be dropped during
OnBeginFrameDerivedImpl() already as well.

A delay between "BeginFrame created" and "BeginFrame received" wouldn't make a
difference here, I think. What matters it the point in time when it is received,
which I can choose via triggering the fake BFS at the right time.

What I'd be looking for is a situation where:
 1) A new BeginFrame was accepted when it is received by the Scheduler (i.e.,
BeginFrameNeeded is true) before the previous BeginImplFrameDeadline has run.
 2) Running the previous deadline synchronously in the line above has to then
clear the BeginFrameNeeded, so that we drop the new BeginFrame.

I can set up 1) - by issuing a BeginFrame before running the previous
BeginImplFrameDeadline while needs_redraw_ is true - , but I don't see a way to
achieve 2) without disabling proactive BeginFrames.

[Eric Seckler, 2017-01-20 17:31:32]

eseckler@chromium.org changed reviewers:
+ sunnyps@chromium.org

[Eric Seckler, 2017-01-20 17:31:32]

+Sunny for feedback regarding discussion below. Maybe you can think of a case
we're missing :)

On 2017/01/18 20:27:23, Eric Seckler wrote:
>
https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/2632563003/diff/40001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:334: if (!observing_begin_frame_source_) {
> On 2017/01/18 19:53:54, brianderson wrote:
> > On 2017/01/18 18:13:03, Eric Seckler wrote:
> > > I'm trying to test this branch (see scheduler_unittest.cc), but I can't
find
> a
> > > way to set things up so that I end up with !observing_begin_frame_source_
> > here.
> > > Is there a way that this can be the case?
> > > 
> > > We can only end up in BeginImplFrameWithDeadline() if
> > > state_machine_.BeginFrameNeeded() was true during
OnBeginFrameDerivedImpl().
> > > Plus a previous BeginImplFrame needs to be started but not yet finished.
> > > 
> > > Thus, if observing_begin_frame_source_ should be false at this point, that
> > means
> > > that some condition affecting state_machine_.BeginFrameNeeded() would have
> to
> > > have changed during the BeginImplFrameDeadline. The only thing that I can
> see
> > is
> > > the state machine's needs_redraw_. Yet, if needs_redraw_ becomes false,
> > > did_draw_in_last_frame_ becomes true, and the state machine will request a
> > > proactive BeginFrame. Thus, in this case,
state_machine_.BeginFrameNeeded()
> > > wouldn't change.
> > > 
> > > I've played around with a few different interleavings of missed
BeginFrames
> > etc,
> > > but I can't find one that puts me here with !observing_begin_frame_source.
> > > 
> > > Can you think of a situation that puts me here?
> > > 
> > > If not, would it make sense to get rid of this branch (replace by DCHECK
> with
> > > explanation)? It'd also enable us to get rid of
> > > OnBeginFrameDroppedNotObserving(). It would make this bit of code more
> brittle
> > > to future StateMachine changes, though.
> > > 
> > > An alternative might be a DisableProactiveBeginFramesForTest() or
something
> > > alike that lets me build a test case that exercises this branch?
> > 
> > I think it doesn't show up in tests because the tests don't use IPCs. With
> IPC's
> > there can be a BeginFrame in flight after we stop requesting BeginFrames.
> > 
> > You may need to create a new fake BeginFrameSource that has a delay between
> when
> > the BeginFrame is created vs. when it is run; where it will still run even
if
> > BeginFramesNeeded becomes false after creation. Or maybe a
> > DelayedBeginFrameQueue that forwards BeginFrames from a source with a delay.
> > 
> > Would that help?
> 
> Hm, I don't think that would help. If there was an IPC delay during which
> BeginFrameNeeded becomes false, the BeginFrame would be dropped during
> OnBeginFrameDerivedImpl() already as well.
> 
> A delay between "BeginFrame created" and "BeginFrame received" wouldn't make a
> difference here, I think. What matters it the point in time when it is
received,
> which I can choose via triggering the fake BFS at the right time.
> 
> What I'd be looking for is a situation where:
>  1) A new BeginFrame was accepted when it is received by the Scheduler (i.e.,
> BeginFrameNeeded is true) before the previous BeginImplFrameDeadline has run.
>  2) Running the previous deadline synchronously in the line above has to then
> clear the BeginFrameNeeded, so that we drop the new BeginFrame.
> 
> I can set up 1) - by issuing a BeginFrame before running the previous
> BeginImplFrameDeadline while needs_redraw_ is true - , but I don't see a way
to
> achieve 2) without disabling proactive BeginFrames.

Had another look at this with Sami today - still can't find a way to trigger
this branch. If we're not missing something, we're thinking that it makes more
sense to get rid of this branch, since it seems rather unfortunate to introduce
unnecessary additional complexity to this patch in order to keep it around. (The
downside of this is that we're assuming internal behavior of the state machine
by removing the branch.)

Anyway, I got rid of it for now and replaced it by a DCHECK. Let's see whether
the try bots find a way to trigger it that we can't think of :)

Brian, Sunny, WDYT?

[Eric Seckler, 2017-01-20 17:31:40]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 17:32:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 18:52:05]

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

[commit-bot: I haz the power, 2017-01-20 18:52:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-01-23 10:45:50]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 10:46:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-01-23 10:53:31]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-23 10:54:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Eric Seckler, 2017-01-23 11:56:31]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 11:56:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 13:37:12]

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

[commit-bot: I haz the power, 2017-01-23 13:37:13]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-01-23 14:31:15]

On 2017/01/20 17:31:32, Eric Seckler wrote:
> Had another look at this with Sami today - still can't find a way to trigger
> this branch. If we're not missing something, we're thinking that it makes more
> sense to get rid of this branch, since it seems rather unfortunate to
introduce
> unnecessary additional complexity to this patch in order to keep it around.
(The
> downside of this is that we're assuming internal behavior of the state machine
> by removing the branch.)
> 
> Anyway, I got rid of it for now and replaced it by a DCHECK. Let's see whether
> the try bots find a way to trigger it that we can't think of :)
> 
> Brian, Sunny, WDYT?

Try bots helped - Turns out it's possible to trigger it afterall, using
SetNeedsPrepareTiles. I added corresponding a test case.

Brian, Could you have another look? Thanks! :)

Do we need to track the status / result of the tile manager, too?

[Eric Seckler, 2017-01-25 15:21:41]

On 2017/01/23 14:31:15, Eric Seckler wrote:
> On 2017/01/20 17:31:32, Eric Seckler wrote:
> > Had another look at this with Sami today - still can't find a way to trigger
> > this branch. If we're not missing something, we're thinking that it makes
more
> > sense to get rid of this branch, since it seems rather unfortunate to
> introduce
> > unnecessary additional complexity to this patch in order to keep it around.
> (The
> > downside of this is that we're assuming internal behavior of the state
machine
> > by removing the branch.)
> > 
> > Anyway, I got rid of it for now and replaced it by a DCHECK. Let's see
whether
> > the try bots find a way to trigger it that we can't think of :)
> > 
> > Brian, Sunny, WDYT?
> 
> Try bots helped - Turns out it's possible to trigger it afterall, using
> SetNeedsPrepareTiles. I added corresponding a test case.
> 
> Brian, Could you have another look? Thanks! :)
> 
> Do we need to track the status / result of the tile manager, too?

Brian: Friendly ping :)

Chatted to Sami regarding the tile preparation. For our use in headless, maybe
an alternative to tracking the tile status would be to enable a mode similar to
waitForReadyToDraw in the renderers, effectively disabling drawing the active
tree without the corresponding tiles? In any case, it seems like that's
something we might be better off looking at separately from this patch?

[Eric Seckler, 2017-01-25 16:20:39]

Description was changed from

==========
[cc] Calculate the correct latest_confirmed_frame in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_frame in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_sequence_number in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-01-25 16:21:00]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 16:21:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 17:10:16]

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

[commit-bot: I haz the power, 2017-01-25 17:10:17]

Dry run: This issue passed the CQ dry run.

[brianderson, 2017-02-14 22:24:54]

> Chatted to Sami regarding the tile preparation. For our use in headless, maybe
> an alternative to tracking the tile status would be to enable a mode similar
to
> waitForReadyToDraw in the renderers, effectively disabling drawing the active
> tree without the corresponding tiles? In any case, it seems like that's
> something we might be better off looking at separately from this patch?

Yeah, let's worry about that in another patch.

[brianderson, 2017-02-14 22:51:09]

Still need to review the tests.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:338:
state_machine_.OnBeginFrameDroppedNotObserving(args.source_id,
Why don't we need to do this for the other places where kBeginFrameSkipped is
used?

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:391: if (ShouldRecoverMainLatency(adjusted_args,
can_activate_before_deadline)) {
For headless, you'll probably want to disable main and impl latency recovery.
Doesn't have to be part of this patch - but just wanted to mention it while it
was fresh in my head.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:885: DCHECK_GE(sequence_number,
begin_frame_sequence_number_);
DCHECK_GT?

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:893: if (begin_main_frame_state_ ==
BEGIN_MAIN_FRAME_STATE_IDLE &&
Can you restructure this with early returns to reduce nesting?

[Eric Seckler, 2017-02-14 23:43:04]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[Eric Seckler, 2017-02-14 23:43:45]

Thank you :)

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:338:
state_machine_.OnBeginFrameDroppedNotObserving(args.source_id,
On 2017/02/14 22:51:08, brianderson wrote:
> Why don't we need to do this for the other places where kBeginFrameSkipped is
> used?

In all other cases, we skip the frame but actually have (or at least estimated
that we will have) pending updates (otherwise we wouldn't be observing the
source), so we won't confirm it. That means, no state in the state machine has
to change, so we don't need to tell it about those skipped frames.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:391: if (ShouldRecoverMainLatency(adjusted_args,
can_activate_before_deadline)) {
On 2017/02/14 22:51:08, brianderson wrote:
> For headless, you'll probably want to disable main and impl latency recovery.
> Doesn't have to be part of this patch - but just wanted to mention it while it
> was fresh in my head.

Thanks, you're right :)

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:885: DCHECK_GE(sequence_number,
begin_frame_sequence_number_);
On 2017/02/14 22:51:08, brianderson wrote:
> DCHECK_GT?

This requires that we set begin_frame_sequence_number_ = kInvalidFrameNumber
initially (currently its kStartingFrameNumber). Let's see if that works on the
bots :)

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:893: if (begin_main_frame_state_ ==
BEGIN_MAIN_FRAME_STATE_IDLE &&
On 2017/02/14 22:51:09, brianderson wrote:
> Can you restructure this with early returns to reduce nesting?

Done.

[commit-bot: I haz the power, 2017-02-14 23:43:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brianderson, 2017-02-14 23:58:56]

Still need to look at the state machine unit tests in detail, but I noticed some
missing coverage (even though you've already added so many tests!) In particular
1) what should happen when we lose the CompositorFrameSink, 2)
commit_to_active_tree is true, and 3) using_synchronous_renderer_compositor is
true. Hopefully you can just add some extra expectations to existing tests.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:620:
last_begin_frame_sequence_number_begin_main_frame_sent_;
Do you also need to do:
if (...active_tree_was_fresh_ == ...compositor_frame_was_fresh_)
  ...compositor_frame_was_fresh_ = ...begin_main_frame_sent_

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:3298: args = SendNextBeginFrame();
EXPECT_NE(args.sequence_number, latest_confirmed_sequence_number);

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:3399: // BeginFrame, too.
Comment wrapping could use cleanup here.

[brianderson, 2017-02-15 00:13:55]

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2295:
EXPECT_SEQUENCE_NUMBERS(12u, 10u, 12u, BeginFrameArgs::kInvalidFrameNumber,
Can you verify that before the deadline, the pending tree's frame number hasn't
been updated? (Just in case SetNeedsCommit arrives before the deadline.)

[commit-bot: I haz the power, 2017-02-15 01:34:47]

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

[commit-bot: I haz the power, 2017-02-15 01:34:48]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-02-17 00:07:27]

On 2017/02/14 23:58:56, brianderson wrote:
> Still need to look at the state machine unit tests in detail, but I noticed
some
> missing coverage (even though you've already added so many tests!) In
particular
> 1) what should happen when we lose the CompositorFrameSink, 2)
> commit_to_active_tree is true, and 3) using_synchronous_renderer_compositor is
> true. Hopefully you can just add some extra expectations to existing tests.

Haven't added them yet, will do soon :)

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:620:
last_begin_frame_sequence_number_begin_main_frame_sent_;
On 2017/02/14 23:58:56, brianderson wrote:
> Do you also need to do:
> if (...active_tree_was_fresh_ == ...compositor_frame_was_fresh_)
>   ...compositor_frame_was_fresh_ = ...begin_main_frame_sent_

Brian and I chatted about why this works, even if impl thread redraws are
pending at the same time. Essentially, we assume that:
(a) any imp-side needs_redraw_=true changes are aligned with BeginFrames. This
isn't completely true at the moment, but work is underway to ensure this.
(b) a main-thread commit without updates has considered the effects of any
(impl-side) inputs/animations/etc that were pending at the time of
BeginMainFrame. That is, any such effects also did not cause updates.

Given this, it is fine to update the compositor frame freshness if it previously
was as fresh as the active tree.

Added the branch and also an expectation to the state machine tests.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine_unittest.cc:2295:
EXPECT_SEQUENCE_NUMBERS(12u, 10u, 12u, BeginFrameArgs::kInvalidFrameNumber,
On 2017/02/15 00:13:54, brianderson wrote:
> Can you verify that before the deadline, the pending tree's frame number
hasn't
> been updated? (Just in case SetNeedsCommit arrives before the deadline.)

Done.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:3298: args = SendNextBeginFrame();
On 2017/02/14 23:58:56, brianderson wrote:
> EXPECT_NE(args.sequence_number, latest_confirmed_sequence_number);

Done, here and in similar places.

https://codereview.chromium.org/2632563003/diff/140001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:3399: // BeginFrame, too.
On 2017/02/14 23:58:56, brianderson wrote:
> Comment wrapping could use cleanup here.

Done.

https://codereview.chromium.org/2632563003/diff/180001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:702: // The draw either didn't have
damage or had damage and submitted a
To cover successful draws with and without damages, I moved the update of
compositor frame freshness from DidSubmitCompositorFrame into DidDraw:

Brian mentioned that successful draw doesn't always produce a CompositorFrame.
If a successful draw produces no damage, then no CompositorFrame will be
submitted. On the other hand, judging by ProxyImpl::DrawInternal /
SingleThreadProxy::DoComposite, when SchedulerStateMachine::DidDraw() is called,
DidSubmitCompositorFrame() would have already been called if the draw was
successful and produced damages.

[Eric Seckler, 2017-02-22 12:09:01]

Brian, PTAL :)

On 2017/02/17 00:07:27, Eric Seckler wrote:
> On 2017/02/14 23:58:56, brianderson wrote:
> > Still need to look at the state machine unit tests in detail, but I noticed
> some
> > missing coverage (even though you've already added so many tests!) In
> particular
> > 1) what should happen when we lose the CompositorFrameSink, 2)
> > commit_to_active_tree is true, and 3) using_synchronous_renderer_compositor
is
> > true. Hopefully you can just add some extra expectations to existing tests.
> 
> Haven't added them yet, will do soon :)

Added tests (for the sync compositor case, in scheduler_unittest.cc only). I
made two changes:
(1) I'm handling DRAW_ABORTED_DRAINING_PIPELINE differently from DRAW_SUCCESS
now. (We shouldn't update the ..compositor_frame_was_fresh_ if
DRAW_ABORTED_DRAINING_PIPELINE.) This was surfaced by the
CompositorFrameSinkLost test.
(2) If using_synchronous_renderer_compositor is true, I'm updating BeginFrame
numbers during OnBeginImplFrameIdle() instead of OnBeginImplFrameDeadline(),
because the cc::Scheduler only triggers the deadline when drawing happens and
finishes BeginFrames before drawing. I think, there's still an issue with this -
see below.

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:2921: // forwarding the one attached to the
later submitted CompositorFrame.
Here's the problem I mentioned :)

Because cc::Scheduler finishes the BeginFrame during
BeginImplFrameSynchronous(), no drawing has happened yet, so has_damage in the
ack is false - and the BeginFrame is not confirmed.

I wouldn't mind that it isn't confirmed yet, because the actual ack for cases
where drawing happens would (eventually) be provided together with the
CompositorFrame through the Draw. However, for that to work, this Ack needs to
be ignored by CompositorExternalBeginFrameSource (so that it is not forwarded to
the browser compositor). In my prototype, I use the has_damage field to decide
whether the ack should be packaged with the CompositorFrame. But that's |false|
here...

I'm considering whether it makes sense to change the cc::Scheduler to wait with
finishing the BeginFrame in sync-renderer-comp mode in case a draw is pending?

In any case, I guess it's not super important since sync-renderer-comp mode
isn't used with a DisplayScheduler or in headless mode, so there's currently no
use case for BeginFrameAcks there. Should we leave it as TODO?

[brianderson, 2017-02-22 23:27:04]

brianderson@chromium.org changed reviewers:
+ boliu@chromium.org

[brianderson, 2017-02-22 23:27:05]

lgtm.

Do we need any more commit_to_active_tree tests? Feel free to land them before I
have a chance to review to save us a day of review latency. I'll take a look.

https://codereview.chromium.org/2632563003/diff/180001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2632563003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:702: // The draw either didn't have
damage or had damage and submitted a
On 2017/02/17 00:07:27, Eric Seckler wrote:
> To cover successful draws with and without damages, I moved the update of
> compositor frame freshness from DidSubmitCompositorFrame into DidDraw:
> 
> Brian mentioned that successful draw doesn't always produce a CompositorFrame.
> If a successful draw produces no damage, then no CompositorFrame will be
> submitted. On the other hand, judging by ProxyImpl::DrawInternal /
> SingleThreadProxy::DoComposite, when SchedulerStateMachine::DidDraw() is
called,
> DidSubmitCompositorFrame() would have already been called if the draw was
> successful and produced damages.

Glad we found that!

https://codereview.chromium.org/2632563003/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:919: if (!needs_redraw_)
{}

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:2921: // forwarding the one attached to the
later submitted CompositorFrame.
On 2017/02/22 12:09:01, Eric Seckler wrote:
> Here's the problem I mentioned :)
> 
> Because cc::Scheduler finishes the BeginFrame during
> BeginImplFrameSynchronous(), no drawing has happened yet, so has_damage in the
> ack is false - and the BeginFrame is not confirmed.
> 
> I wouldn't mind that it isn't confirmed yet, because the actual ack for cases
> where drawing happens would (eventually) be provided together with the
> CompositorFrame through the Draw. However, for that to work, this Ack needs to
> be ignored by CompositorExternalBeginFrameSource (so that it is not forwarded
to
> the browser compositor). In my prototype, I use the has_damage field to decide
> whether the ack should be packaged with the CompositorFrame. But that's
|false|
> here...
> 
> I'm considering whether it makes sense to change the cc::Scheduler to wait
with
> finishing the BeginFrame in sync-renderer-comp mode in case a draw is pending?
> 
> In any case, I guess it's not super important since sync-renderer-comp mode
> isn't used with a DisplayScheduler or in headless mode, so there's currently
no
> use case for BeginFrameAcks there. Should we leave it as TODO?

We can leave it as a TODO for now and follow up when needed.

+boliu and sunnyps as FYI: Does WebView ever have to wait for multiple
producers, where a BeginFrameAck when there was no damage might be useful.

[boliu, 2017-02-23 05:28:24]

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2632563003/diff/200001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:2921: // forwarding the one attached to the
later submitted CompositorFrame.
On 2017/02/22 23:27:05, brianderson wrote:
> On 2017/02/22 12:09:01, Eric Seckler wrote:
> > Here's the problem I mentioned :)
> > 
> > Because cc::Scheduler finishes the BeginFrame during
> > BeginImplFrameSynchronous(), no drawing has happened yet, so has_damage in
the
> > ack is false - and the BeginFrame is not confirmed.
> > 
> > I wouldn't mind that it isn't confirmed yet, because the actual ack for
cases
> > where drawing happens would (eventually) be provided together with the
> > CompositorFrame through the Draw. However, for that to work, this Ack needs
to
> > be ignored by CompositorExternalBeginFrameSource (so that it is not
forwarded
> to
> > the browser compositor). In my prototype, I use the has_damage field to
decide
> > whether the ack should be packaged with the CompositorFrame. But that's
> |false|
> > here...
> > 
> > I'm considering whether it makes sense to change the cc::Scheduler to wait
> with
> > finishing the BeginFrame in sync-renderer-comp mode in case a draw is
pending?
> > 
> > In any case, I guess it's not super important since sync-renderer-comp mode
> > isn't used with a DisplayScheduler or in headless mode, so there's currently
> no
> > use case for BeginFrameAcks there. Should we leave it as TODO?
> 
> We can leave it as a TODO for now and follow up when needed.
> 
> +boliu and sunnyps as FYI: Does WebView ever have to wait for multiple
> producers, where a BeginFrameAck when there was no damage might be useful.

Webview does not have a "browser" compositor, so the "display" compositor only
needs to wait for the renderer. So only thing I can think of that can use
multiple producers is OOPIF, which isn't hooked up in webview rendering yet, so
I guess no?

OTOH, it's really not possible to determine damage in BeginFrame in webview,
because input to that frame could still be changing. So the best cc can hope to
do is "is there damage so far", which probably isn't all that useful.

[Eric Seckler, 2017-02-23 10:13:10]

On 2017/02/22 23:27:05, brianderson wrote:
> lgtm.
> 
> Do we need any more commit_to_active_tree tests? Feel free to land them before
I
> have a chance to review to save us a day of review latency. I'll take a look.

Thanks :) I added some expectations to the only other SchedulerStateMachineTest
for commit_to_active_tree. I don't think that there's any need for further tests
- the sequence of state machine steps seems to be generally the same with
commit_to_active_tree (from what I can tell, it doesn't skip any steps). If you
were thinking of something else, let me know and I'll add tests separately.

[Eric Seckler, 2017-02-23 11:54:17]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 11:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 12:49:24]

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

[commit-bot: I haz the power, 2017-02-23 12:49:26]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-02-23 12:58:00]

Had to make another small change, PTAL :)

https://codereview.chromium.org/2632563003/diff/240001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2632563003/diff/240001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:267: SendBeginFrameAck(args, kBeginFrameSkipped);
Since we require an ack here now, I've added a comment and test for this.

[brianderson, 2017-02-23 20:20:36]

lgtm after small nit.

https://codereview.chromium.org/2632563003/diff/240001/cc/test/scheduler_test...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/2632563003/diff/240001/cc/test/scheduler_test...
cc/test/scheduler_test_common.h:147: bool StateMachineNeedsBeginFrames() const {
Remove the StateMachine prefix to be consistent with other accessor names.

[Eric Seckler, 2017-02-24 15:35:31]

Thanks!

https://codereview.chromium.org/2632563003/diff/240001/cc/test/scheduler_test...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/2632563003/diff/240001/cc/test/scheduler_test...
cc/test/scheduler_test_common.h:147: bool StateMachineNeedsBeginFrames() const {
On 2017/02/23 20:20:36, brianderson wrote:
> Remove the StateMachine prefix to be consistent with other accessor names.

Done.

[Eric Seckler, 2017-02-24 15:35:40]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-02-24 15:35:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2632563003/#ps260001
(title: "rename accessor.")

[commit-bot: I haz the power, 2017-02-24 15:35:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 15:37:40]

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

[commit-bot: I haz the power, 2017-02-24 15:37:41]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Eric Seckler, 2017-02-24 18:18:16]

Patchset #14 (id:280001) has been deleted

[Eric Seckler, 2017-02-24 21:49:54]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-02-24 21:49:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2632563003/#ps320001
(title: "add todo for impl-side invalidations.")

[commit-bot: I haz the power, 2017-02-24 21:51:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 23:37:00]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1487972994739460,
"parent_rev": "ae3f8d350e89f14776c9e3bf6c8a330315ec8d8c", "commit_rev":
"37da9b3e656ac85c42e3f375e84b55c98b397da1"}

[commit-bot: I haz the power, 2017-02-24 23:37:42]

Description was changed from

==========
[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_sequence_number in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

We track freshness frame numbers for the different trees in
cc::SchedulerStateMachine and use them to fill the BeginFrameAck's
latest_confirmed_sequence_number in cc::Scheduler.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2632563003
Cr-Commit-Position: refs/heads/master@{#452997}
Committed:
https://chromium.googlesource.com/chromium/src/+/37da9b3e656ac85c42e3f375e84b...
==========

[commit-bot: I haz the power, 2017-02-24 23:37:43]

Committed patchset #15 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/37da9b3e656ac85c42e3f375e84b...