cc: Control defer_commits logic by Scheduler

cc: Control defer_commits logic by Scheduler

So far, EarlyOut_DeferCommits is controlled by Proxy.
Because Scheduler doesn't know about this deferring, it triggers next
BeginMainFrame when main thread want to defer a commit.
This CL moves the EarlyOut_DeferCommits logic from Proxy to
Scheduler so Scheduler can stop next BeginMainFrame until defer commit is off.

R=brianderson@chromium.org, danakj@chromium.org, skyostil@chromium.org
BUG=382572, 453787
TEST=cc_unittests

Committed: https://crrev.com/1625b74468e4dd55f54ba209f8e91d05493875b9
Cr-Commit-Position: refs/heads/master@{#313816}

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

[simonhong, 2014-06-17 01:20:43]

Please check initial version.
I'll add more unit tests when reviewers confirms this cl
is going right way.
Thanks!

[brianderson, 2014-06-17 15:57:02]

Thanks Simon. Approach looks good.

@enne: Do you see any potential problems with aborting the commits the main
thread used to just defer?

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

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:617: state_machine_.SetDeferCommits(defer_commits);
Probably want to call ProcessScheduledActions here. Even though we don't expect
to initiate any new actions in response to this, we might want to enable or
disable requests for BeginFrames.

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

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:759: if (defer_commits_)
Comment?

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:797: if (defer_commits_)
Comment?

https://codereview.chromium.org/337693005/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/20001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:759:
base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
Would it make sense to reorganize the BeginMainFrameAbortedOnImplThread posting
code so we only PostTask from one place?

[simonhong, 2014-06-18 21:52:06]

kindly ping to enne.

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

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:617: state_machine_.SetDeferCommits(defer_commits);
On 2014/06/17 15:57:01, brianderson wrote:
> Probably want to call ProcessScheduledActions here. Even though we don't
expect
> to initiate any new actions in response to this, we might want to enable or
> disable requests for BeginFrames.

Done.

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

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:759: if (defer_commits_)
On 2014/06/17 15:57:01, brianderson wrote:
> Comment?

Done.

https://codereview.chromium.org/337693005/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:797: if (defer_commits_)
On 2014/06/17 15:57:01, brianderson wrote:
> Comment?

Done.

https://codereview.chromium.org/337693005/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/20001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:759:
base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
On 2014/06/17 15:57:01, brianderson wrote:
> Would it make sense to reorganize the BeginMainFrameAbortedOnImplThread
posting
> code so we only PostTask from one place?

Done.

[enne (OOO), 2014-06-18 22:25:24]

Sorry, I am trying to land https://codereview.chromium.org/134623005/ which adds
deferred commits to SingleThreadProxy and so I haven't quite wrapped my head
around this patch yet.

[simonhong, 2014-06-19 00:01:37]

On 2014/06/18 22:25:24, enne wrote:
> Sorry, I am trying to land https://codereview.chromium.org/134623005/ which
adds
> deferred commits to SingleThreadProxy and so I haven't quite wrapped my head
> around this patch yet.

No problem!
Please come back and check again after finishing your work. :)

[Sami, 2014-06-19 18:26:08]

I like the idea of moving this logic to the scheduler. Could use some tests :)

https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:748: main().commit_requested = false;
Seems like resetting these flags while deferring the commit could break things.
I wonder how good our test coverage for deferred commits is? I only found one
test that uses the feature.

[simonhong, 2014-06-20 00:12:22]

On 2014/06/19 18:26:08, Sami wrote:
> I like the idea of moving this logic to the scheduler. Could use some tests :)
> 
> https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc...
> cc/trees/thread_proxy.cc:748: main().commit_requested = false;
> Seems like resetting these flags while deferring the commit could break
things.
> I wonder how good our test coverage for deferred commits is? I only found one
> test that uses the feature.

Yes, we need more tests for deferred commit scenario.
As you said, there is just one test.
I'm pausing on this cl and will re-visit after enne's cl(Make SingleThreadProxy
a SchedulerClient) is landed.

[mithro-old, 2014-07-02 10:16:04]

On 2014/06/20 00:12:22, simonhong wrote:
> On 2014/06/19 18:26:08, Sami wrote:
> > I like the idea of moving this logic to the scheduler. Could use some tests
:)
> > 
> >
https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc
> > File cc/trees/thread_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc...
> > cc/trees/thread_proxy.cc:748: main().commit_requested = false;
> > Seems like resetting these flags while deferring the commit could break
> things.
> > I wonder how good our test coverage for deferred commits is? I only found
one
> > test that uses the feature.
> 
> Yes, we need more tests for deferred commit scenario.
> As you said, there is just one test.
> I'm pausing on this cl and will re-visit after enne's cl(Make
SingleThreadProxy
> a SchedulerClient) is landed.

I'd like to add my +1 to the idea of making the scheduler the controller of
deferred commits. Please do revisit after enne finishes his patch (enne also
seems to be working on making the SingleThreadProxy a scheduler client too?)

Tim

[mithro-old, 2014-07-02 10:16:08]

On 2014/06/20 00:12:22, simonhong wrote:
> On 2014/06/19 18:26:08, Sami wrote:
> > I like the idea of moving this logic to the scheduler. Could use some tests
:)
> > 
> >
https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc
> > File cc/trees/thread_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/337693005/diff/30001/cc/trees/thread_proxy.cc...
> > cc/trees/thread_proxy.cc:748: main().commit_requested = false;
> > Seems like resetting these flags while deferring the commit could break
> things.
> > I wonder how good our test coverage for deferred commits is? I only found
one
> > test that uses the feature.
> 
> Yes, we need more tests for deferred commit scenario.
> As you said, there is just one test.
> I'm pausing on this cl and will re-visit after enne's cl(Make
SingleThreadProxy
> a SchedulerClient) is landed.

I'd like to add my +1 to the idea of making the scheduler the controller of
deferred commits. Please do revisit after enne finishes his patch (enne also
seems to be working on making the SingleThreadProxy a scheduler client too?)

Tim

[simonhong, 2014-11-19 16:16:54]

Patchset #4 (id:70001) has been deleted

[simonhong, 2014-11-19 16:19:08]

danakj@ what do you think this change?

[danakj, 2014-11-19 16:31:34]

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

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:776: if (defer_commits_)
Why is the ShouldSendBeginMainFrame() change not enough? Doesn't that happen
before calling this always?

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:807: // New BeginFrame should not be
triggered during the commit is deferred.
ditto?

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:664: if (defer_commits_) {
nit: i mildly prefer a few early outs/returns instead of a elseif elseif else
chain.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:711: ScopedAbortRemainingSwapPromises
swap_promise_checker(layer_tree_host());
This patch reorders the deferred commit early out WRT the swap promise checker,
meaning swap promises would be aborted on a deferred commit when before they
would not be. And now it would send a BeginFrameAborted, as well. Is this
intentional? Is it the right thing to do?

[brianderson, 2014-11-19 19:55:07]

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

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:776: if (defer_commits_)
On 2014/11/19 16:31:33, danakj wrote:
> Why is the ShouldSendBeginMainFrame() change not enough? Doesn't that happen
> before calling this always?

Agree that this check isn't needed. Deferring commit should prevent us from
needing to animate or draw. Also, if for some reason we _did_ need to draw the
active tree but wanted to defer commits, we'll need BeginFrames to draw the
active tree.

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:807: // New BeginFrame should not be
triggered during the commit is deferred.
On 2014/11/19 16:31:33, danakj wrote:
> ditto?

ditto.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:667: layer_tree_host_->DidDeferCommit();
It looks like LTH::DidDeferCommit() is only used by tests.

After this patch, I think it'll be racy whether or not DidDeferCommit gets
called anymore, since the scheduler may never send a BeginMainFrame.

Should we get rid of LTH::DidDeferCommit and make tests rely on some other way
of aborting commits, such as setting the visibility instead?

LayerTreeHostTestDeferCommits will also need to change. Probably with some kind
of timeout that verifies WillBeginMainFrame isn't called after commits are
deferred.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:711: ScopedAbortRemainingSwapPromises
swap_promise_checker(layer_tree_host());
On 2014/11/19 16:31:34, danakj wrote:
> This patch reorders the deferred commit early out WRT the swap promise
checker,
> meaning swap promises would be aborted on a deferred commit when before they
> would not be. And now it would send a BeginFrameAborted, as well. Is this
> intentional? Is it the right thing to do?

We should keep the original behavior and not fulfill the swap promises on
deferred commit. My understanding is that the promises are used to track input
latency. If we abort because of a deferred commit, we are deferring input as
well. The other reasons we abort are because we aren't going to draw.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:722: TRACE_EVENT_SCOPE_THREAD);
If we abort a commit because we are deferring commits, we should probably call
SetNeedsCommit to make sure we eventually get another BeginMainFrame. Otherwise
we are assuming blink will SetNeedsCommit when it stops deferring commits, which
I don't think is a safe assumption to make. Even if it is a true assumption
today, it may not be in the future.

Same for the SingleThreadProxy.

[simonhong, 2014-11-26 15:52:53]

Sorry for late reply.
Please take a look again.

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

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:776: if (defer_commits_)
On 2014/11/19 19:55:07, brianderson wrote:
> On 2014/11/19 16:31:33, danakj wrote:
> > Why is the ShouldSendBeginMainFrame() change not enough? Doesn't that happen
> > before calling this always?
> 
> Agree that this check isn't needed. Deferring commit should prevent us from
> needing to animate or draw. Also, if for some reason we _did_ need to draw the
> active tree but wanted to defer commits, we'll need BeginFrames to draw the
> active tree.

Yep, I think you're right.
Deferring commit should not prevent BeginFrame triggering.

https://codereview.chromium.org/337693005/diff/90001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:807: // New BeginFrame should not be
triggered during the commit is deferred.
On 2014/11/19 19:55:07, brianderson wrote:
> On 2014/11/19 16:31:33, danakj wrote:
> > ditto?
> 
> ditto.

Done.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:664: if (defer_commits_) {
On 2014/11/19 16:31:34, danakj wrote:
> nit: i mildly prefer a few early outs/returns instead of a elseif elseif else
> chain.

Done.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:667: layer_tree_host_->DidDeferCommit();
On 2014/11/19 19:55:07, brianderson wrote:
> It looks like LTH::DidDeferCommit() is only used by tests.
> 
> After this patch, I think it'll be racy whether or not DidDeferCommit gets
> called anymore, since the scheduler may never send a BeginMainFrame.
> 
> Should we get rid of LTH::DidDeferCommit and make tests rely on some other way
> of aborting commits, such as setting the visibility instead?
> 
> LayerTreeHostTestDeferCommits will also need to change. Probably with some
kind
> of timeout that verifies WillBeginMainFrame isn't called after commits are
> deferred.
> 

This change breaks below three tests.
* LayerTreeHostTestDeferCommits
* LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
* LayerTreeHostTestBreakSwapPromiseForContextAbortedCommit

I think we don't need some tests for defer commit in here because all defer
commit logic is moved to Scheduler.
I removed above two tests(*BreakSwap*) and I added some test in SchedulerTest
and SchedulerStateMachineTest.
Does this make sense?

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:711: ScopedAbortRemainingSwapPromises
swap_promise_checker(layer_tree_host());
On 2014/11/19 16:31:34, danakj wrote:
> This patch reorders the deferred commit early out WRT the swap promise
checker,
> meaning swap promises would be aborted on a deferred commit when before they
> would not be. And now it would send a BeginFrameAborted, as well. Is this
> intentional? Is it the right thing to do?

As brian said, Aborting the BeginMainFrame when deferring commit is true is
intentional.
I want to handle deferring BeginMainFrame by aborting it and recreating new
BeginMainFrame
when deferring is canceled.
And I didn't think much about re-ordering swap promise.
It should work like before.

https://codereview.chromium.org/337693005/diff/90001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:722: TRACE_EVENT_SCOPE_THREAD);
On 2014/11/19 19:55:07, brianderson wrote:
> If we abort a commit because we are deferring commits, we should probably call
> SetNeedsCommit to make sure we eventually get another BeginMainFrame.
Otherwise
> we are assuming blink will SetNeedsCommit when it stops deferring commits,
which
> I don't think is a safe assumption to make. Even if it is a true assumption
> today, it may not be in the future.
> 
> Same for the SingleThreadProxy.

SetNeedsCommit() is added when SetDeferCommits(false) is called (when changed
from true to false).

[brianderson, 2014-12-03 02:12:11]

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/337693005/diff/90001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:667: layer_tree_host_->DidDeferCommit();
On 2014/11/26 15:52:53, simonhong wrote:
> On 2014/11/19 19:55:07, brianderson wrote:
> > It looks like LTH::DidDeferCommit() is only used by tests.
> > 
> > After this patch, I think it'll be racy whether or not DidDeferCommit gets
> > called anymore, since the scheduler may never send a BeginMainFrame.
> > 
> > Should we get rid of LTH::DidDeferCommit and make tests rely on some other
way
> > of aborting commits, such as setting the visibility instead?
> > 
> > LayerTreeHostTestDeferCommits will also need to change. Probably with some
> kind
> > of timeout that verifies WillBeginMainFrame isn't called after commits are
> > deferred.
> > 
> 
> This change breaks below three tests.
> * LayerTreeHostTestDeferCommits
> * LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
> * LayerTreeHostTestBreakSwapPromiseForContextAbortedCommit
> 
> I think we don't need some tests for defer commit in here because all defer
> commit logic is moved to Scheduler.
> I removed above two tests(*BreakSwap*) and I added some test in SchedulerTest
> and SchedulerStateMachineTest.
> Does this make sense?

The scheduler tests are good, but I think we should keep the LTH tests too. They
cover different things.

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1855:
state.OnBeginImplFrame(CreateBeginFrameArgsForTesting());
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);?

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1857:
state.OnBeginImplFrameDeadlinePending();
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);?

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4726: class
LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
Please don't delete these tests. The new scheduler tests don't cover what these
two tests cover.

Instead of using SetDeferCommits, can you set the Visibility instead to trigger
the aborted commits?

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2036: PostSetNeedsCommitToMainThread();
We should make sure we don't need to PostSetNeedsCommitToMainThread to make
forward progress.The scheduler should do that automatically after we stop
deferring commits. Please delete this line.

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2046: PostSetVisibleToMainThread(false);
Instead of playing with visibility in this test, it would be better to post a
delayed task, say 100ms later, and verify WillBeginMainFrame is never called.

[simonhong, 2015-01-19 14:53:33]

Let's fix this old issue :)
Please take another look!

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1855:
state.OnBeginImplFrame(CreateBeginFrameArgsForTesting());
On 2014/12/03 02:12:10, brianderson wrote:
> EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);?

Done.

https://codereview.chromium.org/337693005/diff/150001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1857:
state.OnBeginImplFrameDeadlinePending();
On 2014/12/03 02:12:10, brianderson wrote:
> EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);?

This is removed.

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4726: class
LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
On 2014/12/03 02:12:11, brianderson wrote:
> Please don't delete these tests. The new scheduler tests don't cover what
these
> two tests cover.
> 
> Instead of using SetDeferCommits, can you set the Visibility instead to
trigger
> the aborted commits?

I think scenarios that these two tests want to simulate cannot be occurred in
real.
These tests cover visibility change from true to false when there is a deferred
commit.
These tests will only have a meaning when pending(deferred) commit is resumed
with invisible state.
With this cl, however, deferred commit is not pending.
It will be aborted and rescheduled when deferred is turned off.
Of course, new frame will not be started with invisible state when deferring is
turned off.
So, I think this test can be removed.

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2036: PostSetNeedsCommitToMainThread();
On 2014/12/03 02:12:11, brianderson wrote:
> We should make sure we don't need to PostSetNeedsCommitToMainThread to make
> forward progress.The scheduler should do that automatically after we stop
> deferring commits. Please delete this line.

Yep, this is not needed.
When defer commit is turned off, new commit will be started based on following
condition.
* BeginMainFrame is aborted because of defer commit.
* When new commit is requested during defer commit is off.

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2046: PostSetVisibleToMainThread(false);
On 2014/12/03 02:12:11, brianderson wrote:
> Instead of playing with visibility in this test, it would be better to post a
> delayed task, say 100ms later, and verify WillBeginMainFrame is never called.

Done.

[enne (OOO), 2015-01-23 01:35:21]

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4726: class
LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
On 2015/01/19 14:53:33, simonhong wrote:
> On 2014/12/03 02:12:11, brianderson wrote:
> > Please don't delete these tests. The new scheduler tests don't cover what
> these
> > two tests cover.
> > 
> > Instead of using SetDeferCommits, can you set the Visibility instead to
> trigger
> > the aborted commits?
> 
> I think scenarios that these two tests want to simulate cannot be occurred in
> real.
> These tests cover visibility change from true to false when there is a
deferred
> commit.
> These tests will only have a meaning when pending(deferred) commit is resumed
> with invisible state.
> With this cl, however, deferred commit is not pending.
> It will be aborted and rescheduled when deferred is turned off.
> Of course, new frame will not be started with invisible state when deferring
is
> turned off.
> So, I think this test can be removed.

I do not think this is true.  These tests use deferred commits to control when
commits happen, as commits happen immediately when undeferring.

The goal of these tests is to make sure that swap promises get the right result
during aborted commits.  The use of deferred commits is incidental, I think.

https://codereview.chromium.org/337693005/diff/190001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/190001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1941:
state.SetDeferCommits(true);
Can you make this test set defer commits back to false and verify that it goes
back to the right state?

https://codereview.chromium.org/337693005/diff/190001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/190001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2059: // PostDelayedTask to check whether
WillBeginMainFrame is called.
This time delay is a little weird.  Could you just post SetDeferCommits from
WillBeginMainFrame and check the counts somewhere else?

[brianderson, 2015-01-27 01:55:15]

I don't have any comments on top of what enne has already said. If you address
enne's comments, I think this patch will be ready.

[simonhong, 2015-01-28 19:44:25]

I think all comments are cleared. :)
PTAL again!

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/337693005/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4726: class
LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit
On 2015/01/23 01:35:21, enne_OOO_until_jan_26 wrote:
> On 2015/01/19 14:53:33, simonhong wrote:
> > On 2014/12/03 02:12:11, brianderson wrote:
> > > Please don't delete these tests. The new scheduler tests don't cover what
> > these
> > > two tests cover.
> > > 
> > > Instead of using SetDeferCommits, can you set the Visibility instead to
> > trigger
> > > the aborted commits?
> > 
> > I think scenarios that these two tests want to simulate cannot be occurred
in
> > real.
> > These tests cover visibility change from true to false when there is a
> deferred
> > commit.
> > These tests will only have a meaning when pending(deferred) commit is
resumed
> > with invisible state.
> > With this cl, however, deferred commit is not pending.
> > It will be aborted and rescheduled when deferred is turned off.
> > Of course, new frame will not be started with invisible state when deferring
> is
> > turned off.
> > So, I think this test can be removed.
> 
> I do not think this is true.  These tests use deferred commits to control when
> commits happen, as commits happen immediately when undeferring.
> 
> The goal of these tests is to make sure that swap promises get the right
result
> during aborted commits.  The use of deferred commits is incidental, I think.

Done.

https://codereview.chromium.org/337693005/diff/190001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/190001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1941:
state.SetDeferCommits(true);
On 2015/01/23 01:35:21, enne_OOO_until_jan_26 wrote:
> Can you make this test set defer commits back to false and verify that it goes
> back to the right state?

Done.

https://codereview.chromium.org/337693005/diff/190001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/190001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2059: // PostDelayedTask to check whether
WillBeginMainFrame is called.
On 2015/01/23 01:35:21, enne_OOO_until_jan_26 wrote:
> This time delay is a little weird.  Could you just post SetDeferCommits from
> WillBeginMainFrame and check the counts somewhere else?

Done.

[brianderson, 2015-01-29 04:30:53]

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2074: EXPECT_EQ(3,
num_will_begin_impl_frame_);
Assuming an exact number of BeginImplFrames have past might be flaky if the main
thread is slow to respond sometimes.

Maybe make this EXPECT_GE, which will verify that we skipped sending a
BeginMainFrame for at least one BeginImplFrame.

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4899: bool output_surface_lost_;
This name is a slight lie. Maybe call it output_surface_lost_triggered_?

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_context.cc:1779: void
DeferCommitsOnMainThread(bool defer_commits) {
Can you delete this now?

[simonhong, 2015-01-29 16:22:17]

All done.
PTAL!

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2074: EXPECT_EQ(3,
num_will_begin_impl_frame_);
On 2015/01/29 04:30:53, brianderson wrote:
> Assuming an exact number of BeginImplFrames have past might be flaky if the
main
> thread is slow to respond sometimes.
> 
> Maybe make this EXPECT_GE, which will verify that we skipped sending a
> BeginMainFrame for at least one BeginImplFrame.

Done.

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4899: bool output_surface_lost_;
On 2015/01/29 04:30:53, brianderson wrote:
> This name is a slight lie. Maybe call it output_surface_lost_triggered_?

yep. output_surface_lost_triggered_ is correct!

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/337693005/diff/230001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_context.cc:1779: void
DeferCommitsOnMainThread(bool defer_commits) {
On 2015/01/29 04:30:53, brianderson wrote:
> Can you delete this now?

Done.

[brianderson, 2015-01-29 22:26:32]

lgtm

[simonhong, 2015-01-29 22:57:25]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2015-01-29 22:58:29]

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

[commit-bot: I haz the power, 2015-01-29 23:03:46]

Committed patchset #12 (id:250001)

[commit-bot: I haz the power, 2015-01-29 23:05:27]

Patchset 12 (id:??) landed as
https://crrev.com/1625b74468e4dd55f54ba209f8e91d05493875b9
Cr-Commit-Position: refs/heads/master@{#313816}

[danakj, 2015-01-30 17:47:54]

A revert of this CL (patchset #12 id:250001) has been created in
https://codereview.chromium.org/870183004/ by danakj@chromium.org.

The reason for reverting is: seems to flakily not stop commits
https://code.google.com/p/chromium/issues/detail?id=453787.

[simonhong, 2015-01-30 23:56:44]

@danakj, brianderson, I uploaded a patch to fix flaky test
(LayerTreeHostTestDeferCommits).
The reason is ImplFrame is one more triggered after EndTest() is called. brian
pointed out that in the last comment but I just modified at the AfterTest().
PTAL!

[danakj, 2015-01-30 23:57:50]

On 2015/01/30 23:56:44, simonhong wrote:
> @danakj, brianderson, I uploaded a patch to fix flaky test
> (LayerTreeHostTestDeferCommits).
> The reason is ImplFrame is one more triggered after EndTest() is called. brian
> pointed out that in the last comment but I just modified at the AfterTest().
> PTAL!

Is it okay that we'll do another commit after setting defer commits?

[simonhong, 2015-01-30 23:58:03]

On 2015/01/30 23:56:44, simonhong wrote:
> @danakj, brianderson, I uploaded a patch to fix flaky test
> (LayerTreeHostTestDeferCommits).
> The reason is ImplFrame is one more triggered after EndTest() is called. brian
> pointed out that in the last comment but I just modified at the AfterTest().
> PTAL!

Oh.. sould I create new CL for new patchset?
commit check button is still disabled.

[danakj, 2015-01-30 23:59:52]

On Fri, Jan 30, 2015 at 3:58 PM, <simonhong@chromium.org> wrote:

> On 2015/01/30 23:56:44, simonhong wrote:
>
>> @danakj, brianderson, I uploaded a patch to fix flaky test
>> (LayerTreeHostTestDeferCommits).
>> The reason is ImplFrame is one more triggered after EndTest() is called.
>> brian
>> pointed out that in the last comment but I just modified at the
>> AfterTest().
>> PTAL!
>>
>
> Oh.. sould I create new CL for new patchset?
> commit check button is still disabled.
>

You have to un-close the CL to re-commit it.

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

[simonhong, 2015-01-31 00:06:24]

On 2015/01/30 23:57:50, danakj wrote:
> On 2015/01/30 23:56:44, simonhong wrote:
> > @danakj, brianderson, I uploaded a patch to fix flaky test
> > (LayerTreeHostTestDeferCommits).
> > The reason is ImplFrame is one more triggered after EndTest() is called.
brian
> > pointed out that in the last comment but I just modified at the AfterTest().
> > PTAL!
> 
> Is it okay that we'll do another commit after setting defer commits?

Yep, This test will check number of begin impl frame count is greater than
number of begin main frame count.
During this test, defer commit is turned off and turned on.
So, this check can verify defer commit is ok.

[simonhong, 2015-01-31 00:45:44]

Patchset #14 (id:290001) has been deleted

[simonhong, 2015-01-31 00:46:14]

Patchset #14 (id:310001) has been deleted

[brianderson, 2015-01-31 01:45:24]

https://codereview.chromium.org/337693005/diff/320001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2071: EXPECT_GT(num_will_begin_impl_frame_,
num_send_begin_main_frame_);
I don't think it should be possible for there to be more than 2 BeginMainFrames
in this test. I like the checks you had in the previous patch set.

[simonhong, 2015-01-31 02:03:53]

https://codereview.chromium.org/337693005/diff/320001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/337693005/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:2071: EXPECT_GT(num_will_begin_impl_frame_,
num_send_begin_main_frame_);
On 2015/01/31 01:45:24, brianderson wrote:
> I don't think it should be possible for there to be more than 2
BeginMainFrames
> in this test. I like the checks you had in the previous patch set.

Done.

[simonhong, 2015-01-31 02:13:02]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2015-01-31 02:14:14]

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

[commit-bot: I haz the power, 2015-01-31 12:06:51]

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

[commit-bot: I haz the power, 2015-01-31 12:06:52]

Try jobs failed on following builders:
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)

[simonhong, 2015-01-31 12:52:41]

The CQ bit was checked by simonhong@chromium.org

[commit-bot: I haz the power, 2015-01-31 12:53:46]

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

[commit-bot: I haz the power, 2015-01-31 15:47:43]

Committed patchset #15 (id:340001)

[commit-bot: I haz the power, 2015-01-31 15:48:40]

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