Aura: Resize locks with --ui-enable-threaded-compositing

Aura: Resize locks with --ui-enable-threaded-compositing

We temporarily defer commits while resizing so that the renderer has a chance to catch up with the UI window size.

Noteable changes from master:
- RWHVs can fast ACK the GPU process (not flip in the browser or TextureImageTransportSurface); useful for allowing the renderer to catch up when it gets too far behind.
- RWHVA will insist kicking a renderer frame after the lock times out on resize (wasn't necessary before)
- ui::Compositor vends draw locks (which largely just wraps to cc::Proxy) of ui::aura::RootWindow

BUG=136366
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164460

[jonathan.backer, 2012-07-31 22:46:09]

The WK side will need some vetting, but I think the Cr side is close.

[piman, 2012-07-31 23:42:16]

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_aura.cc:601: // Remove
locks if we've got got the right size.
typo: got got

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_aura.cc:750:
AddCompositingStartedCallback(
So, in the single-thread case, we're moving the sync point from before the draw
to after the swap , which is a little sad. Is it not possible to keep that
feature?

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
content/common/gpu/gpu_messages.h:238: // will not be presented and can be
reused.
Could we make this explicit instead? Linking both independent concepts may cause
trouble in the future.

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
content/common/gpu/texture_image_transport_surface.cc:373: front_ = back();
Mmh, is it ok to move this here wrt e.g. dropping front surface? If
AdjustFrontBufferAllocation gets called between the swap and the swap ack, then
it'll change the allocation of the old-front-now-back buffer instead of the
old-back-now-front buffer.

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.cc:267: NotifyEnd();
Should we also send the OnCompositingStarted callbacks? Some observers might
expect the Started/Ended to come in pairs.

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
File ui/compositor/compositor.h (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.h:294: // We've told |root_layer_| to enable updates,
but no update has cleared.
"update" is a very overloaded term... Should we use "commit" instead (which has
a specific meaning in the compositor)? (for comments, and enum names)

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.h:304: int updates_allowed_;
UpdatesAllowedState instead of int?

[jonathan.backer, 2012-08-01 17:22:43]

I took a good hard look at some traces and concluded that I was doing the resize
fast track ACKing wrong --- I was unnecessarily slowing down frames with
--ui-enable-threaded-compositing (waiting for a display) and blowing my budget.

This allowed me to simplify the code dramatically. It's much smaller and appears
correct.

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_aura.cc:601: // Remove
locks if we've got got the right size.
On 2012/07/31 23:42:17, piman wrote:
> typo: got got

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_aura.cc:750:
AddCompositingStartedCallback(
On 2012/07/31 23:42:17, piman wrote:
> So, in the single-thread case, we're moving the sync point from before the
draw
> to after the swap , which is a little sad. Is it not possible to keep that
> feature?

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
File content/common/gpu/gpu_messages.h (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
content/common/gpu/gpu_messages.h:238: // will not be presented and can be
reused.
On 2012/07/31 23:42:17, piman wrote:
> Could we make this explicit instead? Linking both independent concepts may
cause
> trouble in the future.

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/content/common/gpu/...
content/common/gpu/texture_image_transport_surface.cc:373: front_ = back();
On 2012/07/31 23:42:17, piman wrote:
> Mmh, is it ok to move this here wrt e.g. dropping front surface? If
> AdjustFrontBufferAllocation gets called between the swap and the swap ack,
then
> it'll change the allocation of the old-front-now-back buffer instead of the
> old-back-now-front buffer.

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.cc:267: NotifyEnd();
On 2012/07/31 23:42:17, piman wrote:
> Should we also send the OnCompositingStarted callbacks? Some observers might
> expect the Started/Ended to come in pairs.

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
File ui/compositor/compositor.h (right):

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.h:294: // We've told |root_layer_| to enable updates,
but no update has cleared.
On 2012/07/31 23:42:17, piman wrote:
> "update" is a very overloaded term... Should we use "commit" instead (which
has
> a specific meaning in the compositor)? (for comments, and enum names)

Done.

https://chromiumcodereview.appspot.com/10690168/diff/2001/ui/compositor/compo...
ui/compositor/compositor.h:304: int updates_allowed_;
On 2012/07/31 23:42:17, piman wrote:
> UpdatesAllowedState instead of int?

Done.

[piman, 2012-08-01 18:25:34]

The patch definitely looks a lot better this way.
I only have one concern, and I'm not sure the best course (maybe doing the
"right" thing would be to complicated and not worth it, and this would be "good
enough"), but I just want to see if we can do a bit better.

https://chromiumcodereview.appspot.com/10690168/diff/13001/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10690168/diff/13001/ui/compositor/comp...
ui/compositor/compositor.cc:434: commits_allowed_ = DISABLED_PENDING_COMMIT;
So, I think I understand why we're doing this - the case where we unlock the
compositor (we got the right-sized buffer) and draw a frame with it, before we
lock it again for the next resize.
My worry is that by waiting on a commit (it can take a long time in the
single-thread case, if the unlock comes while we're waiting for the swap), the
next resize might "sneak in" before we have a chance to lock again, causing us
to draw a "bad" frame.
The logic in RWHVA should prevent it from happening (we block resizes until the
commit did happen), but then, it feels that this logic (don't lock again until
the next commit) also belongs there. Otherwise an unrelated RWHVA (e.g. another
window) may want to lock as well, but will silently fail to do so (at least,
until the next commit).
WDYT? I'm afraid moving the logic there will make the logic more complicated,
and maybe this is "good enough" but OTOH I'm afraid that we may have to
work-around the limitation and complexity will build up at the wrong place.

[jonathan.backer, 2012-10-16 17:48:53]

*sigh*

I flip-flopped. The dropping flips for fast ACKs is back. I needed it for
performance.

The CL, I've uploaded feels pretty good performance wise. I lost a little bit of
async by dropping OnCompositingWillStart, but since
http://codereview.chromium.org/10974008/ this is less critical. And I think it's
worth it to keep threaded/non-threaded similar.

There's one FIXME in RWHVA. This is nice to have, but probably not essential.

>
https://chromiumcodereview.appspot.com/10690168/diff/13001/ui/compositor/comp...
> File ui/compositor/compositor.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10690168/diff/13001/ui/compositor/comp...
> ui/compositor/compositor.cc:434: commits_allowed_ = DISABLED_PENDING_COMMIT;
> So, I think I understand why we're doing this - the case where we unlock the
> compositor (we got the right-sized buffer) and draw a frame with it, before we
> lock it again for the next resize.

Yup.

> My worry is that by waiting on a commit (it can take a long time in the
> single-thread case, if the unlock comes while we're waiting for the swap), the
> next resize might "sneak in" before we have a chance to lock again, causing us
> to draw a "bad" frame.

Yup.

> The logic in RWHVA should prevent it from happening (we block resizes until
the
> commit did happen), but then, it feels that this logic (don't lock again until
> the next commit) also belongs there. Otherwise an unrelated RWHVA (e.g.
another
> window) may want to lock as well, but will silently fail to do so (at least,
> until the next commit).

I think that it's going to be hard to intermediate between multiple RWHVAs
trying to lock. To me failing seems like a reasonable option (we get some
guttering).

I'm all for moving the logic up to RWHVA. I think that it may help with the case
where we're waiting for a commit but ask for a resize lock before the commit
goes through. In this case we could fast track the pending ACK and lock. WDYT? I
don't think that this happens very often (maybe resizing video or animation).

[jonathan.backer, 2012-10-16 21:27:35]

> 
> I think that it's going to be hard to intermediate between multiple RWHVAs
> trying to lock. To me failing seems like a reasonable option (we get some
> guttering).

I take this back. I've piggybacked on CompositorObserver to notify all RWHVAs
and it works. Seems clean. I'll upload a revised patch tomorrow after some more
local testing.

[jonathan.backer, 2012-10-17 19:53:24]

@piman: Please take a look.

[piman, 2012-10-17 21:26:32]

I like the way this is going. There's complexity, but at least it's mostly
localized to RWHVA (which is already overly complicated, and we should fix that,
e.g. extracting the stateful parts, but in a separate change)

You'll need to include an OWNER for the cc part. I can already tell you that
they'll want a unit test :)
(You may want to separate out that change).

http://codereview.chromium.org/10690168/diff/32002/.gitmodules
File .gitmodules (right):

http://codereview.chromium.org/10690168/diff/32002/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
I don't think you intended to include this file.

http://codereview.chromium.org/10690168/diff/32002/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

http://codereview.chromium.org/10690168/diff/32002/cc/tree_synchronizer.cc#ne...
cc/tree_synchronizer.cc:46:
collectExistingCCLayerImplRecursive(children[i].get(), oldLayers,
oldChildrenMap.take(children[i]->id()));
Why is it a problem to collect all the older layers?

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:241: if
(defer_compositor_lock_) {
&& root_window_

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:405:
ui::Compositor* compositor = GetCompositor();
nit: ui::Compositor* compositor = root_window ? root_window->compositor() :
NULL;
That avoids traversing the tree a second time.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:831: if
(!compositor || ShouldFastACK(params_in_pixel.surface_handle)) {
I think we still want to update the layer tree (UpdateExternalTexture) if we
don't have a compositor (we're outside of the window tree) but everything else
allows it. While I'm not 100% sure of all the conditions why we may be outside
the tree (I think prerendering and stuff), once we get back into the tree, we'd
want to have the up-to-date frame, and we may not even get a notification to
force one through the pipeline.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1656:
(*it)->GrabDeferredLock();
should we set can_lock_compositor_ to YES_DID_LOCK if we did grab the lock?

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1676: if
(!compositor->IsLocked() && can_lock_compositor_ == YES_DID_LOCK) {
It's worth a comment to point out that this happens when the lock timed out.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.h:400: // No. A lock
timed out, we need to kick a new frame before locking again.
Since the lock timed out, we just "kicked" a frame. Shouldn't that mean we just
need to wait for the commit before locking again? In which case, shouldn't this
be merged into the NO_PENDING_COMMIT state?

I'm probably missing something, it's been a while since I reviewed your original
change and I may not have paged in all the constraints.

Looking at the code, I believe the only transition out of this state is to
NO_PENDING_COMMIT when we receive a swap/post from the renderer. If the
intention is to never lock from this RWHVA until we did receive a frame from the
renderer, I think we should rename the state into something like
"NO_PENDING_RENDERER_FRAME" and update the comment (otherwise I'm super confused
if "frame" refers to a UI frame or a renderer frame).

http://codereview.chromium.org/10690168/diff/32002/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10690168/diff/32002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:418: }
Just a suggestion: would it help to move the 2 lines from PostSubBuffer:

front_ = back();
previous_damage_rect_ = new_damage_rect;

(and corresponding from SwapBuffers) onto here?

It kinda feels we're trying to "undo" whatever was done there in some cases.

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:264: // Fast track draw because visible state could
not have changed.
There is a case where we may want to paint anyway, is if we got damage in the
root window - may not be critical on Chrome OS, but on desktop win/linux aura we
probably want to paint the old content as fast as possible.

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:418: if (!IsLocked())
nit: style requires braces because the statement spans multiple lines.

[jonathan.backer, 2012-10-18 20:20:35]

I can separate out the cc/ changes if you think that will improve landability. I
kinda of like having it all as one chunk. It motivates the setDeferUpdates API.

I will write the unit tests once I have OK for the approach :D Who should I ask:
jamesr or enne (nduca is out)?

http://codereview.chromium.org/10690168/diff/32002/.gitmodules
File .gitmodules (right):

http://codereview.chromium.org/10690168/diff/32002/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
On 2012/10/17 21:26:32, piman wrote:
> I don't think you intended to include this file.

Thanks. I didn't. But I couldn't figure out how to not to (after 30 minutes of
trying). So I went ahead and uploaded the CL anyway. I'll be sure that this is
dropped before landing.

http://codereview.chromium.org/10690168/diff/32002/cc/tree_synchronizer.cc
File cc/tree_synchronizer.cc (right):

http://codereview.chromium.org/10690168/diff/32002/cc/tree_synchronizer.cc#ne...
cc/tree_synchronizer.cc:46:
collectExistingCCLayerImplRecursive(children[i].get(), oldLayers,
oldChildrenMap.take(children[i]->id()));
On 2012/10/17 21:26:32, piman wrote:
> Why is it a problem to collect all the older layers?

Collecting all the old layers detroys the LayerImpl tree structure, which I want
to partially reuse.

My understanding of how the code used to work is:
- collect all the old LayerImpls
- traverse the Layer tree and reuse as many LayerImpls as possible
- discard the LayerImpls we didn't reuse

Now I want to skip those subtrees of the Layer tree rooted at a deferUpdates
(the tree structure may have changed since last commit). In this case, I to
reuse the corresponding tree structure that I had in the LayerImpl tree.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:241: if
(defer_compositor_lock_) {
On 2012/10/17 21:26:32, piman wrote:
> && root_window_

Can do. It's more robust. But if root_window_ = NULL, we already
UnlockCompositor() and defer_compositor_lock = false.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:405:
ui::Compositor* compositor = GetCompositor();
On 2012/10/17 21:26:32, piman wrote:
> nit: ui::Compositor* compositor = root_window ? root_window->compositor() :
> NULL;
> That avoids traversing the tree a second time.

Done.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:831: if
(!compositor || ShouldFastACK(params_in_pixel.surface_handle)) {
On 2012/10/17 21:26:32, piman wrote:
> I think we still want to update the layer tree (UpdateExternalTexture) if we
> don't have a compositor (we're outside of the window tree) but everything else
> allows it. While I'm not 100% sure of all the conditions why we may be outside
> the tree (I think prerendering and stuff), once we get back into the tree,
we'd
> want to have the up-to-date frame, and we may not even get a notification to
> force one through the pipeline.

Done.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1656:
(*it)->GrabDeferredLock();
On 2012/10/17 21:26:32, piman wrote:
> should we set can_lock_compositor_ to YES_DID_LOCK if we did grab the lock?

Done.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1676: if
(!compositor->IsLocked() && can_lock_compositor_ == YES_DID_LOCK) {
On 2012/10/17 21:26:32, piman wrote:
> It's worth a comment to point out that this happens when the lock timed out.

Done.

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

http://codereview.chromium.org/10690168/diff/32002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.h:400: // No. A lock
timed out, we need to kick a new frame before locking again.
On 2012/10/17 21:26:32, piman wrote:
> Since the lock timed out, we just "kicked" a frame. Shouldn't that mean we
just
> need to wait for the commit before locking again? In which case, shouldn't
this
> be merged into the NO_PENDING_COMMIT state?

Fast ACKs complicate the logic. When we fast ACK, we don't
UpdateExternalTexture. So if we just fast ACKed, the first commit after the lock
times out won't show an updated tab contents (it will gutter because the UI
window size changed). By delaying new compositor locks until we display an
updated renderer frame (no fast ACK), the renderer appears to keep up better
(en.wikipedia.org/wiki/Cat). It may cost us a bit because we miss a fast ACK,
but I find it feels much more responsive.

An alternative (where we always fast ACK) is that the window size changes, but
the tab contents may not change until the user stops resizing. It may
technically be faster, but it feels slower.

> 
> Looking at the code, I believe the only transition out of this state is to
> NO_PENDING_COMMIT when we receive a swap/post from the renderer. If the
> intention is to never lock from this RWHVA until we did receive a frame from
the
> renderer, I think we should rename the state into something like
> "NO_PENDING_RENDERER_FRAME" and update the comment (otherwise I'm super
confused
> if "frame" refers to a UI frame or a renderer frame).

I hope the reasoning above explains why I have two states (fast ACKs). I'll
rename to NO_PENDING_FRAME ---> NO_PENDING_RENDERER_FRAME. That's clearer. FWIW,
there are one or two places where we just jump to NO_PENDING_COMMIT (e.g. when
we defer SetSurfaceNotInUseByCompositor until a commit occurs).

http://codereview.chromium.org/10690168/diff/32002/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10690168/diff/32002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:418: }
On 2012/10/17 21:26:32, piman wrote:
> Just a suggestion: would it help to move the 2 lines from PostSubBuffer:
> 
> front_ = back();
> previous_damage_rect_ = new_damage_rect;
> 
> (and corresponding from SwapBuffers) onto here?
> 
> It kinda feels we're trying to "undo" whatever was done there in some cases.

I was motivated by a comment you made on an earlier version of this CL where I
did the flip here: "Mmh, is it ok to move this [the flip] here wrt e.g. dropping
front surface? If AdjustFrontBufferAllocation gets called between the swap and
the swap ack, then it'll change the allocation of the old-front-now-back buffer
instead of the old-back-now-front buffer."

I'm not sure how big a concern this is. I don't think that the front buffer
allocation can change like this (mid swap), but I really don't know.

I don't think that deferring the flip until we know that it goes through would
help substantially with the previous_damage_rect_ logic here. When we fast ACK,
we don't need to copy on the next post sub buffer. When we go from fast ACKing
to flipping, our two frame damage tracking is screwed up. We could do better by
accumulating damage with a union, but this is such an uncommon case that I'd
rather go suboptimal and obviously correct.

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:264: // Fast track draw because visible state could
not have changed.
On 2012/10/17 21:26:32, piman wrote:
> There is a case where we may want to paint anyway, is if we got damage in the
> root window - may not be critical on Chrome OS, but on desktop win/linux aura
we
> probably want to paint the old content as fast as possible.

LayerAnimator::step on the window resize triggers a redraw even though it's
subsequently no-oped by setDeferUpdates (the damage occurs well before we hit
RWHVA::SetBounds). I can get rid of this, but we'll draw an unnecessary frame.

This isn't a dramatic perf hit --- we take it with threaded compositing enabled
anyway. Your call.

http://codereview.chromium.org/10690168/diff/32002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:418: if (!IsLocked())
On 2012/10/17 21:26:32, piman wrote:
> nit: style requires braces because the statement spans multiple lines.

Done.

[jonathan.backer, 2012-10-18 21:25:15]

+jamesr for cc/

[jamesr, 2012-10-18 21:32:03]

What exactly do you want setDeferUpdates() to do?  Just stop commits?

[jonathan.backer, 2012-10-18 21:43:25]

On 2012/10/18 21:32:03, jamesr wrote:
> What exactly do you want setDeferUpdates() to do?  Just stop commits?

Stop changes to the subtree. Right now we're applying it to the root layer (so
it's similar to stopping commits). But in the future we could just stop changes
to that particular window (and let other windows changes).

nduca@ pushed for this approach (rather than defering commits) because it
simplifies the scheduling logic (it doesn't need to change).

[jamesr, 2012-10-18 21:49:26]

I think this should be at the tree level, not something set on individual
layers.  Your TreeSynchronizer changes won't work properly if the tree hierarchy
changes significantly - i.e. consider the case where a LayerChromium moves in or
out of the deferUpdates subtree between commits.  Since this isn't a feature you
need today let's start without it.

[jamesr, 2012-10-18 21:51:02]

Doing ~nothing in the commit but letting it go through (as opposed to completely
dropping it) is definitely simpler for scheduling.  I just don't think you can
actually do it on a subtree only, not without significantly more complex
changes.

[jonathan.backer, 2012-10-19 18:08:58]

Taking a much coarser grained approach to deferring impl-side updates.

[jonathan.backer, 2012-10-23 17:35:04]

@jamesr: I've deferred the commit as per my discussion with piman@. If this
LGTY, I will write the unit tests for cc changes.

@piman: Perf looks good. This gets rid of some extraneous draws that we had
before with --ui-enable-threaded-compositing. I feel like the two paths
(threaded and non-threaded) have converged. The single-threaded path is slightly
faster (2-3 ms per frame). Not sure why (probably commit scheduling). But we're
not guttering with threaded UI compositor on nytimes.com on 30" display
connected to stumpy (my litmus test). Could you take another look?

[jamesr, 2012-10-23 17:50:44]

Do you have traces showing threaded mode being 2-3ms slower than single
threaded?

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:113: , m_deferCommits(false)
There's no reason to shadow state here on LayerTreeHost - just pass the call
through to the proxy

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:335: setDeferCommits(false);
This doesn't look right at all - what this will do if we have a pending commit
is post a task to run that commit, then run a forced commit (due to
compositeAndReadback), then run the deferred commit, then be left in
non-deferred commit mode.

There are two options here:
1.) Calling compositeAndReadback() with deferCommits true is a caller error and
we just dcheck() here
2.) We let the compositeAndReadback proceed synchronously - meaning we run any
commit that is currently deferred run synchronously, then do the
composite+readback path (which will synchronously run another commit)

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:349: setDeferCommits(false);
There are the same sorts of problems here as with compositeAndReadback().  Do
you ever actually call finishAllRendering() from aura/ash code?

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:280: bool m_deferCommits;
This bool belongs on ThreadProxy, it's the class that makes decisions based on
it

http://codereview.chromium.org/10690168/diff/41025/cc/proxy.h
File cc/proxy.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/proxy.h#newcode68
cc/proxy.h:68: virtual void setDeferCommits(bool) = 0;
Document

http://codereview.chromium.org/10690168/diff/41025/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:324: TRACE_EVENT0("cc", "EarlyOut_DeferCommits");
I don't think this is right.  In single-threaded mode, the embedder of CC is in
complete control of when commits and composites happen.  They also get to decide
when to call deferCommits().  CC should just do exactly what the embedder asks
and not bail.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode292
cc/thread_proxy.cc:292: // FIXME(backer): Necessary?
No, this is wrong.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode505
cc/thread_proxy.cc:505: DCHECK(!m_deferredBeginFrameRequest.get());
!m_deferredBeginFrameRequest works fine. no need for .get()

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode506
cc/thread_proxy.cc:506:
m_deferredBeginFrameRequest.swap(m_pendingBeginFrameRequest);
This swap is unnecessary

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.h
File cc/thread_proxy.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.h#newcode95
cc/thread_proxy.h:95: scoped_ptr<BeginFrameAndCommitState>
m_deferredBeginFrameRequest;
This is unnecessary.

If commits are deferred, what you want to do is hold on to the
m_pendingBeginFrameRequest and just avoid running through the rest of the
beginFrame() logic until commits become un-deferred

[jonathan.backer, 2012-10-23 19:26:42]

Thanks for the quick review jamesr@. I've added some traces on the associated
bug (http://code.google.com/p/chromium/issues/detail?id=136366).

My comment about threaded compositing resize being slower was wrong: I was
counting the duration of RootWindow::HoldMouseMoves. But this is slightly
different because of when commits the UI compositor is told that commits are
completed. However, this does not affect the throughput. The throughput is
statistically the same for throughput: 22.8 +/- 5 (sigma) FPS for
single-threaded vs 22.1 +/- 6 FPS for threaded.

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:113: , m_deferCommits(false)
On 2012/10/23 17:50:44, jamesr wrote:
> There's no reason to shadow state here on LayerTreeHost - just pass the call
> through to the proxy

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:335: setDeferCommits(false);
On 2012/10/23 17:50:44, jamesr wrote:
> This doesn't look right at all - what this will do if we have a pending commit
> is post a task to run that commit, then run a forced commit (due to
> compositeAndReadback), then run the deferred commit, then be left in
> non-deferred commit mode.

We're more than happy to bail out of non-deferred commit mode. In
ui/compositor/compositor.cc, we have a timeout on the deferred commit, so we
have to structure our code so that it is always safe to bail out early. Given
that it must always be safe to bail out early, I like the shortcut here -- I
think it simplifies the logic.

> 
> There are two options here:
> 1.) Calling compositeAndReadback() with deferCommits true is a caller error
and
> we just dcheck() here

We use this for window snapshotting in Aura. I'd prefer to handle this case if
it does not impose too much complexity.

> 2.) We let the compositeAndReadback proceed synchronously - meaning we run any
> commit that is currently deferred run synchronously, then do the
> composite+readback path (which will synchronously run another commit)

This is totally an option. Although as I mentioned above, I prefer just bailing
--- it's simpler and we have to handle this case anyway.

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:349: setDeferCommits(false);
On 2012/10/23 17:50:44, jamesr wrote:
> There are the same sorts of problems here as with compositeAndReadback().  Do
> you ever actually call finishAllRendering() from aura/ash code?

'git gs' says no. Just doing it for completeness. I'm more than happy to DCHECK
here. SG?

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:280: bool m_deferCommits;
On 2012/10/23 17:50:44, jamesr wrote:
> This bool belongs on ThreadProxy, it's the class that makes decisions based on
> it

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/proxy.h
File cc/proxy.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/proxy.h#newcode68
cc/proxy.h:68: virtual void setDeferCommits(bool) = 0;
On 2012/10/23 17:50:44, jamesr wrote:
> Document

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:324: TRACE_EVENT0("cc", "EarlyOut_DeferCommits");
On 2012/10/23 17:50:44, jamesr wrote:
> I don't think this is right.  In single-threaded mode, the embedder of CC is
in
> complete control of when commits and composites happen.  They also get to
decide
> when to call deferCommits().  CC should just do exactly what the embedder asks
> and not bail.

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode292
cc/thread_proxy.cc:292: // FIXME(backer): Necessary?
On 2012/10/23 17:50:44, jamesr wrote:
> No, this is wrong.

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode505
cc/thread_proxy.cc:505: DCHECK(!m_deferredBeginFrameRequest.get());
On 2012/10/23 17:50:44, jamesr wrote:
> !m_deferredBeginFrameRequest works fine. no need for .get()

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.cc#newcode506
cc/thread_proxy.cc:506:
m_deferredBeginFrameRequest.swap(m_pendingBeginFrameRequest);
On 2012/10/23 17:50:44, jamesr wrote:
> This swap is unnecessary

Done.

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.h
File cc/thread_proxy.h (right):

http://codereview.chromium.org/10690168/diff/41025/cc/thread_proxy.h#newcode95
cc/thread_proxy.h:95: scoped_ptr<BeginFrameAndCommitState>
m_deferredBeginFrameRequest;
On 2012/10/23 17:50:44, jamesr wrote:
> This is unnecessary.
> 
> If commits are deferred, what you want to do is hold on to the
> m_pendingBeginFrameRequest and just avoid running through the rest of the
> beginFrame() logic until commits become un-deferred

Done.

[jamesr, 2012-10-23 21:24:34]

Still needs work but this is on the right track.

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:333: // Clear any pending commits.
No, this is definitely not right.  You're imposing lots of complexity on cc even
if the use case for aura is simplified

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:347: // Clear any pending commits.
DCHECK, don't handle

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:146: bool deferCommits() const;
I don't think you need a getter - nobody calls it and it's really up to the
embedder to keep track of this since they need to track state anyway (so they
remember to un-defer)

http://codereview.chromium.org/10690168/diff/51001/cc/proxy.h
File cc/proxy.h (right):

http://codereview.chromium.org/10690168/diff/51001/cc/proxy.h#newcode69
cc/proxy.h:69: // a task with a reset in the common case). So it is important
that we always be in a comittable state.
not sure what exactly this comment is referring to - are you trying to say that
we might commit even if deferCommits is set, so the LayerTreeHost has to be
committable?  Such a comment would better fit on LayerTreeHost

I think it's really important to think about what this API means in terms of
interactions between the embedder and cc.  This call is a way for the embedder
to communicate to cc that it doesn't want cc to commit of its own volition, but
if the embedder makes a sync call that requires a commit (like
compositeAndReadback) it will still happen.

http://codereview.chromium.org/10690168/diff/51001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:235: void SingleThreadProxy::setDeferCommits(bool
deferCommits)
I would say don't implement this at all - it doesn't make any sense to set
deferCommits() in single threaded mode since it's the embedder (not cc) that
decides when commits happen.  defersCommits is a hook to control the threaded
mode behavior only, since in threaded mode cc decides when to commit not the
embedder

http://codereview.chromium.org/10690168/diff/51001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/thread_proxy.cc#newcode380
cc/thread_proxy.cc:380: if (m_deferCommits == deferCommits)
redundant calls would probably be a caller error and i think we should DCHECK()
on them. for instance, if a caller calls setDeferCommits(true) when commits are
already deferred, it probably means that there will also be an unbalanced set of
setDeferCommits(false) calls and commits will not be deferred after the first
call even if whoever made the second call probably expects commits to still be
deferred

or, if you really want a recursive lock, you'll need a counter here

[jonathan.backer, 2012-10-24 16:42:14]

@jamesr: I've made this thread-only (NOTIMPLEMENTED in SingleThreadProxy). This
feels like the right thing to do. We don't need it in ui::Compositor for the
single threaded case. I wasn't sure if I should still add the API to thread
proxy --- there's one place in LTH where we statically cast for proxy
implementation specific access (for STP::compositeImmediately()). I prefer the
NOTIMPLEMENTED approach.

I made two Chromium style changes.
- using NOTIMPLEMENTED
- commenting out unused params: void foo(bool /* not_used */)
Let me know if this is appropriate.

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:333: // Clear any pending commits.
On 2012/10/23 21:24:34, jamesr wrote:
> No, this is definitely not right.  You're imposing lots of complexity on cc
even
> if the use case for aura is simplified

Done. Replaced with a DCHECK in ThreadProxy. Enforcing unlock in ui::Compositor.

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:347: // Clear any pending commits.
On 2012/10/23 21:24:34, jamesr wrote:
> DCHECK, don't handle

Done. Added DCHECK to ThreadProxy.

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

http://codereview.chromium.org/10690168/diff/51001/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:146: bool deferCommits() const;
On 2012/10/23 21:24:34, jamesr wrote:
> I don't think you need a getter - nobody calls it and it's really up to the
> embedder to keep track of this since they need to track state anyway (so they
> remember to un-defer)

Done.

http://codereview.chromium.org/10690168/diff/51001/cc/proxy.h
File cc/proxy.h (right):

http://codereview.chromium.org/10690168/diff/51001/cc/proxy.h#newcode69
cc/proxy.h:69: // a task with a reset in the common case). So it is important
that we always be in a comittable state.
On 2012/10/23 21:24:34, jamesr wrote:
> not sure what exactly this comment is referring to - are you trying to say
that
> we might commit even if deferCommits is set, so the LayerTreeHost has to be
> committable?  Such a comment would better fit on LayerTreeHost
> 
> I think it's really important to think about what this API means in terms of
> interactions between the embedder and cc.  This call is a way for the embedder
> to communicate to cc that it doesn't want cc to commit of its own volition,
but
> if the embedder makes a sync call that requires a commit (like
> compositeAndReadback) it will still happen.

Based on some of your earlier comments, it is an error to make a sync call if it
is deferred. We'll let aura do what it wants. But there is no requirement that
we be in a commitable state at the cc level.

As this is a thread only feature, I feel like the comment belongs here. But I am
more than happy to move it.

http://codereview.chromium.org/10690168/diff/51001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:235: void SingleThreadProxy::setDeferCommits(bool
deferCommits)
On 2012/10/23 21:24:34, jamesr wrote:
> I would say don't implement this at all - it doesn't make any sense to set
> deferCommits() in single threaded mode since it's the embedder (not cc) that
> decides when commits happen.  defersCommits is a hook to control the threaded
> mode behavior only, since in threaded mode cc decides when to commit not the
> embedder

Done. NOTREACHED.

http://codereview.chromium.org/10690168/diff/51001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/51001/cc/thread_proxy.cc#newcode380
cc/thread_proxy.cc:380: if (m_deferCommits == deferCommits)
On 2012/10/23 21:24:34, jamesr wrote:
> redundant calls would probably be a caller error and i think we should
DCHECK()
> on them. for instance, if a caller calls setDeferCommits(true) when commits
are
> already deferred, it probably means that there will also be an unbalanced set
of
> setDeferCommits(false) calls and commits will not be deferred after the first
> call even if whoever made the second call probably expects commits to still be
> deferred
> 
> or, if you really want a recursive lock, you'll need a counter here

Done.

Good idea. Let's not make this recursive (I can't imagine wanting that).
ui::Compositor currently prevents this from happening.

[jamesr, 2012-10-24 19:34:27]

Logic in cc/ looks solid - just need tests.  Thanks for the refinements.

http://codereview.chromium.org/10690168/diff/53001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/53001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:343: 
spurious newline here, remove

http://codereview.chromium.org/10690168/diff/53001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/53001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:234: void SingleThreadProxy::setDeferCommits(bool /*
deferCommits */)
I can't find anything in the chromium or google style guides to suggest
commenting out parameter names.  This is sometimes done to avoid unused variable
warnings, but  I've never seen it in chromium code.  I would not do this.

[jonathan.backer, 2012-10-24 21:29:37]

Added a unit test (and a LTH hook to support it).

http://codereview.chromium.org/10690168/diff/53001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

http://codereview.chromium.org/10690168/diff/53001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:343: 
On 2012/10/24 19:34:27, jamesr wrote:
> spurious newline here, remove

Done.

http://codereview.chromium.org/10690168/diff/53001/cc/single_thread_proxy.cc
File cc/single_thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/53001/cc/single_thread_proxy.cc#...
cc/single_thread_proxy.cc:234: void SingleThreadProxy::setDeferCommits(bool /*
deferCommits */)
On 2012/10/24 19:34:27, jamesr wrote:
> I can't find anything in the chromium or google style guides to suggest
> commenting out parameter names.  This is sometimes done to avoid unused
variable
> warnings, but  I've never seen it in chromium code.  I would not do this.

Done.

[piman, 2012-10-25 00:14:08]

A few things, but otherwise this is looking pretty good (in particular the RWVHA
stuff).

http://codereview.chromium.org/10690168/diff/55002/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/55002/cc/thread_proxy.cc#newcode390
cc/thread_proxy.cc:390: if (!m_deferCommits && m_pendingBeginFrameRequest)
There is a race condition here.
m_pendingBeginFrameRequest is set by scheduledActionBeginFrame on the impl
thread and then the struct is filled up - without a lock. However it's not meant
to be seen by the main thread only after the beginFrame task is posted - adding
an explicit coherency-ensuring ordering relationship
scheduledActionBeginFrame->postTask->runTask->beginFrame.

You can construct a case where the impl thread has set
m_pendingBeginFrameRequest, then the main thread calls setDeferCommits(false)
before the task is posted, and then executes beginFrame before the struct is
filled up (and visible by the main thread) causing inconsistencies.

You can probably solve it by adding a bool, that you would set in beginFrame
when commits are deferred, and here post a task (and reset it) when it is set.
That would guarantee that there's only a single beginFrame "in flight", and so
it would never see inconsistent state.

http://codereview.chromium.org/10690168/diff/55002/content/browser/gpu/gpu_pr...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/10690168/diff/55002/content/browser/gpu/gpu_pr...
content/browser/gpu/gpu_process_host.cc:125: host->Send(new
AcceleratedSurfaceMsg_BufferPresented(route_id, false, 0));
Should it really be false here? I think this path is taken for both success and
failure, though admittedly probably only on non-aura builds (I haven't checked
all the call chains), where it doesn't matter.
Maybe we need to pipe it through?

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:286: Unlock();
I think this should really be:
if (compositor_lock_)
  compositor_lock_->CancelLock();

Several reasons:
- calling Unlock will result in a DCHECK if it wasn't locked
- the CompositorLock object would still be live, but we wouldn't reset its back
pointer on Compositor destruction, possibly resulting in a use-after-free.

Maybe putting this into a (private?) Compositor::CancelLock wrapper, and calling
that from Compositor::~Compositor too.

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:411: if (!IsLocked()) {
Is there actually a case where we'll call didCommit with IsLocked() == true?
Can we assert instead?

[jonathan.backer, 2012-10-25 15:19:04]

I think this is converging nicely.

I modified the unittest slightly for consistency (I don't there there was a race
there, but I should only touch test member variables from one thread if
possible).

@jamesr: Can I get an R+/CQ+ on wkb.ug/100291 ? I need that to roll before I can
send this to the try bots.

http://codereview.chromium.org/10690168/diff/55002/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/55002/cc/thread_proxy.cc#newcode390
cc/thread_proxy.cc:390: if (!m_deferCommits && m_pendingBeginFrameRequest)
On 2012/10/25 00:14:08, piman wrote:
> There is a race condition here.
> m_pendingBeginFrameRequest is set by scheduledActionBeginFrame on the impl
> thread and then the struct is filled up - without a lock. However it's not
meant
> to be seen by the main thread only after the beginFrame task is posted -
adding
> an explicit coherency-ensuring ordering relationship
> scheduledActionBeginFrame->postTask->runTask->beginFrame.
> 
> You can construct a case where the impl thread has set
> m_pendingBeginFrameRequest, then the main thread calls setDeferCommits(false)
> before the task is posted, and then executes beginFrame before the struct is
> filled up (and visible by the main thread) causing inconsistencies.
> 
> You can probably solve it by adding a bool, that you would set in beginFrame
> when commits are deferred, and here post a task (and reset it) when it is set.
> That would guarantee that there's only a single beginFrame "in flight", and so
> it would never see inconsistent state.

Nice catch! That would have been impossible to debug.

http://codereview.chromium.org/10690168/diff/55002/content/browser/gpu/gpu_pr...
File content/browser/gpu/gpu_process_host.cc (right):

http://codereview.chromium.org/10690168/diff/55002/content/browser/gpu/gpu_pr...
content/browser/gpu/gpu_process_host.cc:125: host->Send(new
AcceleratedSurfaceMsg_BufferPresented(route_id, false, 0));
On 2012/10/25 00:14:08, piman wrote:
> Should it really be false here? I think this path is taken for both success
and
> failure, though admittedly probably only on non-aura builds (I haven't checked
> all the call chains), where it doesn't matter.
> Maybe we need to pipe it through?

Piping it through. I'll send presented = false on error and presented = true on
success.

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:286: Unlock();
On 2012/10/25 00:14:08, piman wrote:
> I think this should really be:
> if (compositor_lock_)
>   compositor_lock_->CancelLock();
> 
> Several reasons:
> - calling Unlock will result in a DCHECK if it wasn't locked
> - the CompositorLock object would still be live, but we wouldn't reset its
back
> pointer on Compositor destruction, possibly resulting in a use-after-free.
> 
> Maybe putting this into a (private?) Compositor::CancelLock wrapper, and
calling
> that from Compositor::~Compositor too.

Done.

http://codereview.chromium.org/10690168/diff/55002/ui/compositor/compositor.c...
ui/compositor/compositor.cc:411: if (!IsLocked()) {
On 2012/10/25 00:14:08, piman wrote:
> Is there actually a case where we'll call didCommit with IsLocked() == true?
> Can we assert instead?

You are right. This is should no longer happen. I'm going to leave a DCHECK here
to cover the single threaded case (we are covered by DCHECKs in cc::ThreadProxy
for forced commits).

[piman, 2012-10-25 16:18:58]

http://codereview.chromium.org/10690168/diff/53009/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/53009/cc/thread_proxy.cc#newcode515
cc/thread_proxy.cc:515: m_deferredCommitPending = false;
I would reset this when posting the task in setDeferCommits, right before
posting the task.
Otherwise, if the main thread quickly does several setDeferCommits(true);
setDeferCommits(false), we'll post several beginFrame tasks. The first one is
fine but the second one might catch a race as well (as soon as the first one
executes, the impl thread could send a new task, setting
m_pendingBeginFrameRequest, that the second task may catch while it's being
constructed).

http://codereview.chromium.org/10690168/diff/53009/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/53009/ui/compositor/compositor.c...
ui/compositor/compositor.cc:454: UnlockCompositor();
you really want compositor_lock_->Unlock() here (which will call
UnlockCompositor, but set the CompositorLock in a consistent state).

[jonathan.backer, 2012-10-25 16:38:11]

Addressed piman@s comments (on the mark) and did a rebase to ToT.

http://codereview.chromium.org/10690168/diff/53009/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

http://codereview.chromium.org/10690168/diff/53009/cc/thread_proxy.cc#newcode515
cc/thread_proxy.cc:515: m_deferredCommitPending = false;
On 2012/10/25 16:18:58, piman wrote:
> I would reset this when posting the task in setDeferCommits, right before
> posting the task.
> Otherwise, if the main thread quickly does several setDeferCommits(true);
> setDeferCommits(false), we'll post several beginFrame tasks. The first one is
> fine but the second one might catch a race as well (as soon as the first one
> executes, the impl thread could send a new task, setting
> m_pendingBeginFrameRequest, that the second task may catch while it's being
> constructed).

Done.

http://codereview.chromium.org/10690168/diff/53009/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

http://codereview.chromium.org/10690168/diff/53009/ui/compositor/compositor.c...
ui/compositor/compositor.cc:454: UnlockCompositor();
On 2012/10/25 16:18:58, piman wrote:
> you really want compositor_lock_->Unlock() here (which will call
> UnlockCompositor, but set the CompositorLock in a consistent state).

Done.

[piman, 2012-10-25 16:43:42]

lgtm

[jonathan.backer, 2012-10-26 18:44:55]

On 2012/10/25 16:43:42, piman wrote:
> lgtm

@jamesr: poke.

[jamesr, 2012-10-26 19:29:42]

cc/ lgtm - thanks for the tests!

[jamesr, 2012-10-26 19:30:40]

webkit/ lgtm as well. I think piman has owners everwhere else I do

http://codereview.chromium.org/10690168/diff/57042/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

http://codereview.chromium.org/10690168/diff/57042/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:49: virtual void
setDeferCommits(bool deferCommits) OVERRIDE;
You'll have an easier time landing patches like this if you leave out the
OVERRIDE - then you can land the implementation and the WebKit side in any
order, instead of having to land+roll one first.

[sky, 2012-10-26 19:46:31]

LGTM

[commit-bot: I haz the power, 2012-10-26 19:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/10690168/76001

[commit-bot: I haz the power, 2012-10-26 20:50:25]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-26 21:09:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/10690168/84002

[commit-bot: I haz the power, 2012-10-27 00:44:32]

Change committed as 164460