Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate

Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate

[2/2] Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate

They basically do the same thing except that SetNeedsAnimate makes the next
commit non-cancellable. However there is really no reason why SetNeedsAnimate
need to enforce a commit even if no tiles are updated and no layer properties
changed.

SetNeedsAnimate is thus merged into SetNeedsUpdateLayers. The proper use of
it is when there are potential layout/tile changes, we can use it to defer
calculation until the next frame. A commit will be scheduled but can be
cancelled if no updates are needed after calculation.

This part of the patch changes code behavior slightly.
SingleThreadProxy::SetNeedsUpdateLayers was originally implemented as
RenderWidget::ScheduleComposite but now it is RenderWidget::ScheduleAnimation.
ThreadProxy::SetNeedsAnimate was non-cancellable but is now cancellable.

[1/2] Cleanup RenderWidget::scheduleComposite/scheduleAnimation

scheduleComposite has been renamed to ScheduleComposite as it is no longer
a part of WebWidgetClient API.

scheduleAnimation has been renamed to ScheduleAnimation. The semantics is to
schedule a composite and also (potentially) animating WebWidget.

A new WebWidgetClient API scheduleUpdate has been added, to replace the old
scheduleAnimation. The semantics is to notify the embedder that something in
the WebWidget may change in 0 seconds. (i.e. it is allowed to be called
during a redraw, in such case another redraw will be scheduled after frame
delay.

This part of the patch should not change code behavior.

BUG=316929
R=danakj,jamesr,piman

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

[danakj, 2013-11-18 09:09:50]

Cool plan, but I propose that we not rename SetNeedsUpdateLayers, which will
keep this patch much smaller, and also keep the extra context (it causes
LTH::UpdateLayers() to happen).

[trchen, 2013-11-18 23:44:41]

On 2013/11/18 09:09:50, danakj wrote:
> Cool plan, but I propose that we not rename SetNeedsUpdateLayers, which will
> keep this patch much smaller, and also keep the extra context (it causes
> LTH::UpdateLayers() to happen).

Done. By the way does anyone know why RenderWidgetCompositor::setNeedsRedraw
is implemented in such weird way?

[jamesr, 2013-11-19 00:54:26]

https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:510: // TODO(trchen): This
function is not doing what the name indicates?
the purpose of this function is to request that whichever system is in control
of scheduling frames kick another one.  In single-threaded mode, that's the
RenderWidget so this calls RenderWidget::scheduleAnimation().  In threaded mode,
it's the scheduler inside cc so this calls cc::SetNeedsAnimate().  Both have the
practical effect of kicking the make-a-frame logic without causing any specific
damage.

It sounds like this is exactly the semantics you want with
SetNeedsUpdateLayers()

I don't think TODO is at all beneficial to have in the code

[enne (OOO), 2013-11-19 01:48:46]

https://codereview.chromium.org/68893031/diff/50001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/68893031/diff/50001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_animation.cc:729: // TODO(trchen): This test
is broken.
Add a FakeContentLayer and set_needs_always_update_resources?

[trchen, 2013-11-19 02:33:06]

On 2013/11/19 00:54:26, jamesr wrote:
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.cc:510: // TODO(trchen): This
> function is not doing what the name indicates?
> the purpose of this function is to request that whichever system is in control
> of scheduling frames kick another one.  In single-threaded mode, that's the
> RenderWidget so this calls RenderWidget::scheduleAnimation().  In threaded
mode,
> it's the scheduler inside cc so this calls cc::SetNeedsAnimate().  Both have
the
> practical effect of kicking the make-a-frame logic without causing any
specific
> damage.
>
> It sounds like this is exactly the semantics you want with
> SetNeedsUpdateLayers()
>
> I don't think TODO is at all beneficial to have in the code

I think the purpose is for the window system to tell us our framebuffer is
damaged, and needs a redraw without changing contents. That is, we shouldn't
schedule a commit at all. We're just abusing the fact that requesting a commit
always results in a redraw.

I suggest we shouldn't change the implementation of it for now, as some of the
call sites use setNeedsRedraw when it really means setNeedsUpdateLayers. But in
the long term we should make it layer_tree_host_->SetNeedsRedraw() when all such
call sites are corrected.

[jamesr, 2013-11-19 02:35:31]

On 2013/11/19 02:33:06, trchen wrote:
> On 2013/11/19 00:54:26, jamesr wrote:
> >
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> > File content/renderer/gpu/render_widget_compositor.cc (right):
> > 
> >
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> > content/renderer/gpu/render_widget_compositor.cc:510: // TODO(trchen): This
> > function is not doing what the name indicates?
> > the purpose of this function is to request that whichever system is in
control
> > of scheduling frames kick another one.  In single-threaded mode, that's the
> > RenderWidget so this calls RenderWidget::scheduleAnimation().  In threaded
> mode,
> > it's the scheduler inside cc so this calls cc::SetNeedsAnimate().  Both have
> the
> > practical effect of kicking the make-a-frame logic without causing any
> specific
> > damage.
> >
> > It sounds like this is exactly the semantics you want with
> > SetNeedsUpdateLayers()
> >
> > I don't think TODO is at all beneficial to have in the code
> 
> I think the purpose is for the window system to tell us our framebuffer is
> damaged, and needs a redraw without changing contents. That is, we shouldn't
> schedule a commit at all. We're just abusing the fact that requesting a commit
> always results in a redraw.
> 
> I suggest we shouldn't change the implementation of it for now, as some of the
> call sites use setNeedsRedraw when it really means setNeedsUpdateLayers. But
in
> the long term we should make it layer_tree_host_->SetNeedsRedraw() when all
such
> call sites are corrected.

SetNeedsRedraw() tells us there is window damage.  This call is used when we
want an update but don't have any damage.

[trchen, 2013-11-19 02:51:27]

On 2013/11/19 02:35:31, jamesr wrote:
> On 2013/11/19 02:33:06, trchen wrote:
> > On 2013/11/19 00:54:26, jamesr wrote:
> > >
> >
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> > > File content/renderer/gpu/render_widget_compositor.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/68893031/diff/50001/content/renderer/gpu/rend...
> > > content/renderer/gpu/render_widget_compositor.cc:510: // TODO(trchen):
This
> > > function is not doing what the name indicates?
> > > the purpose of this function is to request that whichever system is in
> control
> > > of scheduling frames kick another one.  In single-threaded mode, that's
the
> > > RenderWidget so this calls RenderWidget::scheduleAnimation().  In threaded
> > mode,
> > > it's the scheduler inside cc so this calls cc::SetNeedsAnimate().  Both
have
> > the
> > > practical effect of kicking the make-a-frame logic without causing any
> > specific
> > > damage.
> > >
> > > It sounds like this is exactly the semantics you want with
> > > SetNeedsUpdateLayers()
> > >
> > > I don't think TODO is at all beneficial to have in the code
> > 
> > I think the purpose is for the window system to tell us our framebuffer is
> > damaged, and needs a redraw without changing contents. That is, we shouldn't
> > schedule a commit at all. We're just abusing the fact that requesting a
commit
> > always results in a redraw.
> > 
> > I suggest we shouldn't change the implementation of it for now, as some of
the
> > call sites use setNeedsRedraw when it really means setNeedsUpdateLayers. But
> in
> > the long term we should make it layer_tree_host_->SetNeedsRedraw() when all
> such
> > call sites are corrected.
> 
> SetNeedsRedraw() tells us there is window damage.  This call is used when we
> want an update but don't have any damage.

Let's merge all three functions then.
People will be surprised to see RenderWidgetCompositor::setNeedsRedraw is not a
wrapper of LayerTreeHost::SetNeedsRedraw.

And there are so many place(RenderWidgetCompositor, WebViewImpl) we do this
branch:
if (threaded_)
    layer_tree_host_->SetNeedsAnimate();
else
    widget_->scheduleAnimation();

I think the best way to do it is by overriding the virtual function in Proxy.
(as I did in this patch)

[trchen, 2013-12-05 04:20:27]

Hello reviewers, here is an updated patch. The broken cc animation unittest has
been fixed. PTAL, thanks!

In this patch I also refactored WebWidgetClient::scheduleAnimation as discussed
with jamesr@ in another thread.
https://codereview.chromium.org/80203002/#msg2

[danakj, 2013-12-05 18:28:16]

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

https://codereview.chromium.org/68893031/diff/160001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:181: client_->ScheduleAnimation();
Should we be renaming this ScheduleAnimation() function as well?

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
true. We need to force that call
Can you explain this? The SetNeedsUpdateLayers() function only checks for
update_requested_ in its early out. So would this not have been called and
post-tasked already if update_requested_ is true here?

This looks to me like old code that is not needed any more. Do any tests fail
without it?

+ajuma as well

https://codereview.chromium.org/68893031/diff/160001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/68893031/diff/160001/content/renderer/render_...
content/renderer/render_widget.cc:576: compositor_->setNeedsAnimate();
git cl format

[danakj, 2013-12-05 18:28:32]

+ajuma for sanity checking me in threadproxy

[ajuma, 2013-12-05 19:01:06]

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
true. We need to force that call
On 2013/12/05 18:28:16, danakj wrote:
> Can you explain this? The SetNeedsUpdateLayers() function only checks for
> update_requested_ in its early out. So would this not have been called and
> post-tasked already if update_requested_ is true here?

SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
gap between when update_requested_ is cleared and when
commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
that calls to SetNeedsUpdateLayers during this gap (e.g. those that
occur during UpdateClientAnimations) do eventually cause a commit request.
So I think we either need to close that gap by moving up the clearing of
commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
check.

[danakj, 2013-12-05 19:18:50]

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
true. We need to force that call
On 2013/12/05 19:01:06, ajuma wrote:
> On 2013/12/05 18:28:16, danakj wrote:
> > Can you explain this? The SetNeedsUpdateLayers() function only checks for
> > update_requested_ in its early out. So would this not have been called and
> > post-tasked already if update_requested_ is true here?
> 
> SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
> earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
> gap between when update_requested_ is cleared and when
> commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
> that calls to SetNeedsUpdateLayers during this gap (e.g. those that
> occur during UpdateClientAnimations) do eventually cause a commit request.
> So I think we either need to close that gap by moving up the clearing of
> commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
> check.

Ah, thanks. I see. SendCommitRequestToImplThreadIfNeeded() will early out and
that is still on the main thread.

I think this comment isn't entirely true though. We could have called
SetNeedsUpdate() inside UpdateLayers() or inside WillCommit(), both of which are
called when commit_request_sent_to_impl_thread_ is false.

I guess if the comment is wrong, the the SetNeedsUpdateLayers() call here will
not do anything because commit_request_sent_to_impl_thread_ will be true again.
It might make sense to move this all the way up to line 846, just below where we
actually set commit_request_sent_to_impl_thread = false.

[trchen, 2013-12-06 03:42:38]

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

https://codereview.chromium.org/68893031/diff/160001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:181: client_->ScheduleAnimation();
On 2013/12/05 18:28:16, danakj wrote:
> Should we be renaming this ScheduleAnimation() function as well?

Hmm it may be a good idea. There should be no concept of "layers" in the thread
proxy. +1 vote for it.

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
true. We need to force that call
On 2013/12/05 19:18:51, danakj wrote:
> On 2013/12/05 19:01:06, ajuma wrote:
> > On 2013/12/05 18:28:16, danakj wrote:
> > > Can you explain this? The SetNeedsUpdateLayers() function only checks for
> > > update_requested_ in its early out. So would this not have been called and
> > > post-tasked already if update_requested_ is true here?
> > 
> > SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
> > earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
> > gap between when update_requested_ is cleared and when
> > commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
> > that calls to SetNeedsUpdateLayers during this gap (e.g. those that
> > occur during UpdateClientAnimations) do eventually cause a commit request.
> > So I think we either need to close that gap by moving up the clearing of
> > commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
> > check.
> 
> Ah, thanks. I see. SendCommitRequestToImplThreadIfNeeded() will early out and
> that is still on the main thread.
> 
> I think this comment isn't entirely true though. We could have called
> SetNeedsUpdate() inside UpdateLayers() or inside WillCommit(), both of which
are
> called when commit_request_sent_to_impl_thread_ is false.
> 
> I guess if the comment is wrong, the the SetNeedsUpdateLayers() call here will
> not do anything because commit_request_sent_to_impl_thread_ will be true
again.
> It might make sense to move this all the way up to line 846, just below where
we
> actually set commit_request_sent_to_impl_thread = false.

It is doing the right thing. The comment is a bit misleading though.

The purpose of it is to make sure a commit will be scheduled again when the 
update request happened during a commit. Depending on during which phase of
the commit an update is requested, commit_request_sent_to_impl_thread_ may be
true or false. Either way we clear the early-out flag here and request update
again to make sure a commit is always scheduled.

Whether commit_request_sent_to_impl_thread_ is false here is irrelevant. If it
is true that means something in UpdateLayers or WillCommit scheduled a commit,
and we can get a free ride here. If it's false, then we schedule a commit
ourselves.

I think we can both revise the comment and the code:
if (update_requested_)
  SetNeedsCommit();

This should be much less confusing.

https://codereview.chromium.org/68893031/diff/160001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/68893031/diff/160001/content/renderer/render_...
content/renderer/render_widget.cc:576: compositor_->setNeedsAnimate();
On 2013/12/05 18:28:16, danakj wrote:
> git cl format

Done.

[danakj, 2013-12-06 17:21:32]

Sorry for the confusion *^^*. After the discussion I think PS3 LGTM. I'd prefer
to keep the SetNeedsUpdateLayers names I think, as that is the internal process
we are scheduling inside the compositor.

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

https://codereview.chromium.org/68893031/diff/160001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:181: client_->ScheduleAnimation();
On 2013/12/06 03:42:39, trchen wrote:
> On 2013/12/05 18:28:16, danakj wrote:
> > Should we be renaming this ScheduleAnimation() function as well?
> 
> Hmm it may be a good idea. There should be no concept of "layers" in the
thread
> proxy. +1 vote for it.

Ah, sorry for being unclear here. I think SetNeedsUpdateLayers() is the better
name here given that it is the public API, where embedders can request that the
layer they own be updated. "UpdateLayers" is a bit of canonical step in the
compositor flow.

I was referring to the client_->ScheduleAnimation() call here, and wondering if
it should perhaps be client_->ScheduleUpdateLayers? But I am not sure, I think
it would be a follow up patch if anything. I think a comment here why
SetNeedsUpdateLayers is calling ScheduleAnimation would be valuable.

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
true. We need to force that call
On 2013/12/06 03:42:39, trchen wrote:
> On 2013/12/05 19:18:51, danakj wrote:
> > On 2013/12/05 19:01:06, ajuma wrote:
> > > On 2013/12/05 18:28:16, danakj wrote:
> > > > Can you explain this? The SetNeedsUpdateLayers() function only checks
for
> > > > update_requested_ in its early out. So would this not have been called
and
> > > > post-tasked already if update_requested_ is true here?
> > > 
> > > SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
> > > earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
> > > gap between when update_requested_ is cleared and when
> > > commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
> > > that calls to SetNeedsUpdateLayers during this gap (e.g. those that
> > > occur during UpdateClientAnimations) do eventually cause a commit request.
> > > So I think we either need to close that gap by moving up the clearing of
> > > commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
> > > check.
> > 
> > Ah, thanks. I see. SendCommitRequestToImplThreadIfNeeded() will early out
and
> > that is still on the main thread.
> > 
> > I think this comment isn't entirely true though. We could have called
> > SetNeedsUpdate() inside UpdateLayers() or inside WillCommit(), both of which
> are
> > called when commit_request_sent_to_impl_thread_ is false.
> > 
> > I guess if the comment is wrong, the the SetNeedsUpdateLayers() call here
will
> > not do anything because commit_request_sent_to_impl_thread_ will be true
> again.
> > It might make sense to move this all the way up to line 846, just below
where
> we
> > actually set commit_request_sent_to_impl_thread = false.
> 
> It is doing the right thing. The comment is a bit misleading though.
> 
> The purpose of it is to make sure a commit will be scheduled again when the 
> update request happened during a commit. Depending on during which phase of
> the commit an update is requested, commit_request_sent_to_impl_thread_ may be
> true or false. Either way we clear the early-out flag here and request update
> again to make sure a commit is always scheduled.
> 
> Whether commit_request_sent_to_impl_thread_ is false here is irrelevant. If it
> is true that means something in UpdateLayers or WillCommit scheduled a commit,
> and we can get a free ride here. If it's false, then we schedule a commit
> ourselves.
> 
> I think we can both revise the comment and the code:
> if (update_requested_)
>   SetNeedsCommit();
> 
> This should be much less confusing.

Ah, that will do something different though, as SNC() causes an un-abortable
commit, but we want to cause an abortable one, so either SetNeedsUpdate or
SendCommitRequestToImpl should be happening here. I guess the comment needs a
bit of love but we're good otherwise.

[trchen, 2013-12-06 22:13:28]

On 2013/12/06 17:21:32, danakj wrote:
> Sorry for the confusion *^^*. After the discussion I think PS3 LGTM. I'd
prefer
> to keep the SetNeedsUpdateLayers names I think, as that is the internal
process
> we are scheduling inside the compositor.

Thanks for the valuable feedback!

>
https://codereview.chromium.org/68893031/diff/160001/cc/trees/single_thread_p...
> File cc/trees/single_thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/68893031/diff/160001/cc/trees/single_thread_p...
> cc/trees/single_thread_proxy.cc:181: client_->ScheduleAnimation();
> On 2013/12/06 03:42:39, trchen wrote:
> > On 2013/12/05 18:28:16, danakj wrote:
> > > Should we be renaming this ScheduleAnimation() function as well?
> > 
> > Hmm it may be a good idea. There should be no concept of "layers" in the
> thread
> > proxy. +1 vote for it.
> 
> Ah, sorry for being unclear here. I think SetNeedsUpdateLayers() is the better
> name here given that it is the public API, where embedders can request that
the
> layer they own be updated. "UpdateLayers" is a bit of canonical step in the
> compositor flow.
> 
> I was referring to the client_->ScheduleAnimation() call here, and wondering
if
> it should perhaps be client_->ScheduleUpdateLayers? But I am not sure, I think
> it would be a follow up patch if anything. I think a comment here why
> SetNeedsUpdateLayers is calling ScheduleAnimation would be valuable.

I see. Yes I think we can rename RenderWidgetCompositor::ScheduleAnimation to
ScheduleUpdateLayers. (It was added by me in a recent patch. We didn't think of
a perfect name for it.)
However RenderWidget::ScheduleAnimation should keep as-is, since the code path
is shared with software mode.

> https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/68893031/diff/160001/cc/trees/thread_proxy.cc...
> cc/trees/thread_proxy.cc:860: // when commit_request_sent_to_impl_thread_ =
> true. We need to force that call
> On 2013/12/06 03:42:39, trchen wrote:
> > On 2013/12/05 19:18:51, danakj wrote:
> > > On 2013/12/05 19:01:06, ajuma wrote:
> > > > On 2013/12/05 18:28:16, danakj wrote:
> > > > > Can you explain this? The SetNeedsUpdateLayers() function only checks
> for
> > > > > update_requested_ in its early out. So would this not have been called
> and
> > > > > post-tasked already if update_requested_ is true here?
> > > > 
> > > > SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
> > > > earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
> > > > gap between when update_requested_ is cleared and when
> > > > commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
> > > > that calls to SetNeedsUpdateLayers during this gap (e.g. those that
> > > > occur during UpdateClientAnimations) do eventually cause a commit
request.
> > > > So I think we either need to close that gap by moving up the clearing of
> > > > commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
> > > > check.
> > > 
> > > Ah, thanks. I see. SendCommitRequestToImplThreadIfNeeded() will early out
> and
> > > that is still on the main thread.
> > > 
> > > I think this comment isn't entirely true though. We could have called
> > > SetNeedsUpdate() inside UpdateLayers() or inside WillCommit(), both of
which
> > are
> > > called when commit_request_sent_to_impl_thread_ is false.
> > > 
> > > I guess if the comment is wrong, the the SetNeedsUpdateLayers() call here
> will
> > > not do anything because commit_request_sent_to_impl_thread_ will be true
> > again.
> > > It might make sense to move this all the way up to line 846, just below
> where
> > we
> > > actually set commit_request_sent_to_impl_thread = false.
> > 
> > It is doing the right thing. The comment is a bit misleading though.
> > 
> > The purpose of it is to make sure a commit will be scheduled again when the 
> > update request happened during a commit. Depending on during which phase of
> > the commit an update is requested, commit_request_sent_to_impl_thread_ may
be
> > true or false. Either way we clear the early-out flag here and request
update
> > again to make sure a commit is always scheduled.
> > 
> > Whether commit_request_sent_to_impl_thread_ is false here is irrelevant. If
it
> > is true that means something in UpdateLayers or WillCommit scheduled a
commit,
> > and we can get a free ride here. If it's false, then we schedule a commit
> > ourselves.
> > 
> > I think we can both revise the comment and the code:
> > if (update_requested_)
> >   SetNeedsCommit();
> > 
> > This should be much less confusing.
> 
> Ah, that will do something different though, as SNC() causes an un-abortable
> commit, but we want to cause an abortable one, so either SetNeedsUpdate or
> SendCommitRequestToImpl should be happening here. I guess the comment needs a
> bit of love but we're good otherwise.

Good catch! I'll use the original code but revise the comment then.

[trchen, 2013-12-06 22:50:33]

Hello James, does the content/renderer change looks okay to you? Thanks!

[jamesr, 2013-12-06 22:55:24]

lgtm! This looks great.  As a sanity check could you confirm that this page:

http://webstuff.nfshost.com/anim-timing/raftime.html

continues to animate in single-threaded non-compositing, single-threaded
compositing, and multi-threaded compositing paths?  They should work fine but I
don't have a lot of trust in our automated test coverage here.

[trchen, 2013-12-06 23:21:13]

On 2013/12/06 22:55:24, jamesr wrote:
> lgtm! This looks great.  As a sanity check could you confirm that this page:
> 
> http://webstuff.nfshost.com/anim-timing/raftime.html
> 
> continues to animate in single-threaded non-compositing, single-threaded
> compositing, and multi-threaded compositing paths?  They should work fine but
I
> don't have a lot of trust in our automated test coverage here.

Oops it breaks single-thread compositing mode. Commits are continuously
scheduled but the compositor refuses to redraw. I'm figuring out what went
wrong.

[jamesr, 2013-12-07 00:19:22]

On 2013/12/06 23:21:13, trchen wrote:
> Oops it breaks single-thread compositing mode. Commits are continuously
> scheduled but the compositor refuses to redraw. I'm figuring out what went
> wrong.

D'oh!  Thanks for checking.

[trchen, 2013-12-07 00:21:08]

On 2013/12/06 23:21:13, trchen wrote:
> On 2013/12/06 22:55:24, jamesr wrote:
> > lgtm! This looks great.  As a sanity check could you confirm that this page:
> > 
> > http://webstuff.nfshost.com/anim-timing/raftime.html
> > 
> > continues to animate in single-threaded non-compositing, single-threaded
> > compositing, and multi-threaded compositing paths?  They should work fine
but
> I
> > don't have a lot of trust in our automated test coverage here.
> 
> Oops it breaks single-thread compositing mode. Commits are continuously
> scheduled but the compositor refuses to redraw. I'm figuring out what went
> wrong.

I got this. Unlike threaded compositing mode, the single-thread mode will only
redraw if invalidation rect is not empty (even if tree is changed, or layers are
invalidated). Adding artificial invalidation workarounds the bug. (See
render_widget_compositor.cc in the latest patch)

[jamesr, 2013-12-07 00:22:04]

Yeah, that 1x1 nonsense.  lgtm!

[trchen, 2013-12-07 00:26:30]

Hello Antoine, can I have a owner approval for content/test? Thank you!

[piman, 2013-12-07 01:08:57]

lgtm

[commit-bot: I haz the power, 2013-12-07 02:13:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/210001

[commit-bot: I haz the power, 2013-12-07 04:11:39]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-09 20:52:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/210001

[commit-bot: I haz the power, 2013-12-09 20:52:51]

Failed to apply patch for cc/trees/layer_tree_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host.cc
  Hunk #1 FAILED at 519.
  1 out of 1 hunk FAILED -- saving rejects to file
cc/trees/layer_tree_host.cc.rej

Patch:       cc/trees/layer_tree_host.cc
Index: cc/trees/layer_tree_host.cc
diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index
081d96bd59d7448bb8ea8fc1e1f08a925f9d0790..cca0d5da5f1e222d143ae0f937f71f2ebcab8edc
100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc
@@ -519,10 +519,6 @@ const RendererCapabilities&
LayerTreeHost::GetRendererCapabilities() const {
   return proxy_->GetRendererCapabilities();
 }
 
-void LayerTreeHost::SetNeedsAnimate() {
-  proxy_->SetNeedsAnimate();
-}
-
 void LayerTreeHost::SetNeedsUpdateLayers() { proxy_->SetNeedsUpdateLayers(); }
 
 void LayerTreeHost::SetNeedsCommit() {

[commit-bot: I haz the power, 2013-12-09 21:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/230001

[commit-bot: I haz the power, 2013-12-10 01:19:55]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-12-10 01:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/270001

[commit-bot: I haz the power, 2013-12-10 02:14:41]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-10 17:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/230001

[commit-bot: I haz the power, 2013-12-10 18:37:30]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-10 18:59:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/230001

[commit-bot: I haz the power, 2013-12-10 20:22:25]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-10 21:16:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/230001

[commit-bot: I haz the power, 2013-12-10 22:36:18]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-10 22:55:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/290001

[commit-bot: I haz the power, 2013-12-10 23:20:12]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-10 23:25:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/310001

[commit-bot: I haz the power, 2013-12-10 23:51:26]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-10 23:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/310001

[commit-bot: I haz the power, 2013-12-11 01:33:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/68893031/330001

[commit-bot: I haz the power, 2013-12-11 04:11:37]

Change committed as 240008