Support for drawing a debug border around animated layers

Support for drawing a debug border around animated layers

This is the groundwork for showing a border around the
animation bounds of a layer. The animation bounds are the
bounds of the layer transformed to screen space where layers
that have an animation  instead transform by an "inflate"
step.

To follow, compute the animation bounds of layers when
needed.

+ Add debug rect code to show a lime green border around
  layers that have animation bounds.
+ Add --show-layer-animation-bounds that will enable the
  drawing of the green box.

BUG=

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

[Ian Vollick, 2013-11-28 00:05:12]

This does a little more than plumbing a switch. It also adds the debug rects
machinery for showing the new rects as well as stubbing a function in LayerImpl.
The title is a bit misleading. Can you please update it, describe what these
animated bounds actually are, and what work is required for this visualization
to be correct?

https://codereview.chromium.org/92853002/diff/1/cc/debug/debug_colors.cc
File cc/debug/debug_colors.cc (right):

https://codereview.chromium.org/92853002/diff/1/cc/debug/debug_colors.cc#newc...
cc/debug/debug_colors.cc:256: // Animation bounds are line-green.
typo: lime.

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer.h#newcode377
cc/layers/layer.h:377: bool GetAnimationBounds(gfx::BoxF* out) const { return
false; }
I don't think this will be necessary. Ultimately, we'll only be asking for the
animation bounds of LayerImpls, right?

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer_impl.h#newcod...
cc/layers/layer_impl.h:354: 
Please add a comment saying what this will do and what the return value means.

[avallee, 2013-11-28 00:31:07]

https://codereview.chromium.org/92853002/diff/1/cc/debug/debug_colors.cc
File cc/debug/debug_colors.cc (right):

https://codereview.chromium.org/92853002/diff/1/cc/debug/debug_colors.cc#newc...
cc/debug/debug_colors.cc:256: // Animation bounds are line-green.
On 2013/11/28 00:05:12, Ian Vollick wrote:
> typo: lime.

Done.

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer.h#newcode377
cc/layers/layer.h:377: bool GetAnimationBounds(gfx::BoxF* out) const { return
false; }
On 2013/11/28 00:05:12, Ian Vollick wrote:
> I don't think this will be necessary. Ultimately, we'll only be asking for the
> animation bounds of LayerImpls, right?

It's not. I forgot to delete this in the CL here.

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/92853002/diff/1/cc/layers/layer_impl.h#newcod...
cc/layers/layer_impl.h:354: 
On 2013/11/28 00:05:12, Ian Vollick wrote:
> Please add a comment saying what this will do and what the return value means.

Done.

[Ian Vollick, 2013-11-28 00:44:58]

cc lgtm w/ nit. You'll need OWNERS for content/

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:357: // on success.
It's not quite success, IIRC. I thought we returned false if the layer was
unaffected by an animation? It returns true whenever we need to inflate bounds
to account for animation, right?

[danakj, 2013-11-28 17:48:56]

https://codereview.chromium.org/92853002/diff/20001/cc/debug/debug_rect_histo...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/92853002/diff/20001/cc/debug/debug_rect_histo...
cc/debug/debug_rect_history.cc:240: for (int surface_index =
render_surface_layer_list.size() - 1;
Why not use LayerIterator?

[Ian Vollick, 2013-12-06 20:24:27]

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:357: // on success.
On 2013/11/28 00:44:58, Ian Vollick wrote:
> It's not quite success, IIRC. I thought we returned false if the layer was
> unaffected by an animation? It returns true whenever we need to inflate bounds
> to account for animation, right?

Alex pointed out offline, that although we'll return false when we're not
animated, |box| in that case really won't be correct (I thought it would
previously). The problem is that for |box| to be correct, it would still have to
be transformed into screen space by all the static transforms in the hierarchy,
but we skip this work.

tl;dr - Ignore me :)

[avallee, 2013-12-06 22:08:32]

https://codereview.chromium.org/92853002/diff/20001/cc/debug/debug_rect_histo...
File cc/debug/debug_rect_history.cc (right):

https://codereview.chromium.org/92853002/diff/20001/cc/debug/debug_rect_histo...
cc/debug/debug_rect_history.cc:240: for (int surface_index =
render_surface_layer_list.size() - 1;
On 2013/11/28 17:48:56, danakj wrote:
> Why not use LayerIterator?

Done. I think we're iterating front to back now instead of back to front, but it
doesn't really matter.

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/92853002/diff/20001/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:357: // on success.
On 2013/12/06 20:24:28, Ian Vollick wrote:
> On 2013/11/28 00:44:58, Ian Vollick wrote:
> > It's not quite success, IIRC. I thought we returned false if the layer was
> > unaffected by an animation? It returns true whenever we need to inflate
bounds
> > to account for animation, right?
> 
> Alex pointed out offline, that although we'll return false when we're not
> animated, |box| in that case really won't be correct (I thought it would
> previously). The problem is that for |box| to be correct, it would still have
to
> be transformed into screen space by all the static transforms in the
hierarchy,
> but we skip this work.
> 
> tl;dr - Ignore me :)

Tweaked the comment.

[danakj, 2013-12-06 22:12:05]

Thanks, LayerIterator LGTM

[avallee, 2013-12-09 19:12:14]

kbr@chromium.org: Please review changes in 

content/

[Ken Russell (switch to Gerrit), 2013-12-09 19:19:14]

content/renderer/gpu LGTM

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

https://codereview.chromium.org/92853002/diff/40001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:184: // These flags should be
mirrored by UI versions in ui/compositor/.
Nit: I don't see any changes to ui/compositor/ in this patch. Should there be?

[Ken Russell (switch to Gerrit), 2013-12-09 19:20:09]

content/browser/renderer_host also LGTM

[avallee, 2013-12-09 21:30:26]

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

https://codereview.chromium.org/92853002/diff/40001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:184: // These flags should be
mirrored by UI versions in ui/compositor/.
On 2013/12/09 19:19:14, Ken Russell wrote:
> Nit: I don't see any changes to ui/compositor/ in this patch. Should there be?

Our bounds are in window space for now. This means that when a window animated
its bounds are just the window bounds, not the total screen space bounds. I'll
disable the drawing in the ui compositor for now.

[danakj, 2013-12-09 21:43:52]

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

https://codereview.chromium.org/92853002/diff/40001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:184: // These flags should be
mirrored by UI versions in ui/compositor/.
On 2013/12/09 21:30:26, avallee wrote:
> On 2013/12/09 19:19:14, Ken Russell wrote:
> > Nit: I don't see any changes to ui/compositor/ in this patch. Should there
be?
> 
> Our bounds are in window space for now. This means that when a window animated
> its bounds are just the window bounds, not the total screen space bounds. I'll
> disable the drawing in the ui compositor for now.

What this comment means is these flags should be possible to have --ui-same-flag
to enable it in the UI compositor so you can use it with ui compositor
animations, the same way that you would with renderer animations. Each
compositor can display these boxes independently of the other. They each have
their own notion of screen space.

[avallee, 2013-12-09 21:48:05]

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

https://codereview.chromium.org/92853002/diff/40001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:184: // These flags should be
mirrored by UI versions in ui/compositor/.
On 2013/12/09 21:43:52, danakj wrote:
> On 2013/12/09 21:30:26, avallee wrote:
> > On 2013/12/09 19:19:14, Ken Russell wrote:
> > > Nit: I don't see any changes to ui/compositor/ in this patch. Should there
> be?
> > 
> > Our bounds are in window space for now. This means that when a window
animated
> > its bounds are just the window bounds, not the total screen space bounds.
I'll
> > disable the drawing in the ui compositor for now.
> 
> What this comment means is these flags should be possible to have
--ui-same-flag
> to enable it in the UI compositor so you can use it with ui compositor
> animations, the same way that you would with renderer animations. Each
> compositor can display these boxes independently of the other. They each have
> their own notion of screen space.

Done.

[danakj, 2013-12-09 21:51:23]

https://codereview.chromium.org/92853002/diff/80001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/92853002/diff/80001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1061: // also be added
to chrome/browser/chromeos/login/chrome_restart_request.cc.
You need to duplicate this switch here as well, so it works on chromeos
correctly.

[avallee, 2013-12-10 18:33:00]

stevenjb@chromium.org: Please review changes in 

chrome/browser/chromeos

Thanks.

[stevenjb, 2013-12-10 18:36:24]

c/b/chromeos lgtm

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/92853002/100001

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

Change committed as 239863