Send compositor frame IPC with metadata.

Send compositor frame IPC with metadata.

This makes CompositorFrame contain one of two payloads: DelegatedFrameData for ubercomp and GLFrameData for the Aura model.  This also adds CompositorFrameMetadata containing information useful for positioning subwindows relative to the webpage scroll position.

The message is sent when a new switch --enable-compositor-frame-message is set.  This is enabled by default on Android and also suppresses Android-specific UpdateFrameInfo messages, which will be superceded by CompositorFrameMetadata.

BUG=152337, 161945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173634

[aelias_OOO_until_Jul13, 2012-11-21 03:27:12]

Hi Antoine and Dana,

Here's this data structure.  Currently hanging out without any users, but I
thought I might as well make it available to get plugged in when the rest of
ubercomp exists.

[danakj, 2012-11-21 03:39:07]

https://codereview.chromium.org/11316128/diff/1/cc/compositor_frame_metadata.h
File cc/compositor_frame_metadata.h (right):

https://codereview.chromium.org/11316128/diff/1/cc/compositor_frame_metadata....
cc/compositor_frame_metadata.h:20: gfx::Vector2dF rootScrollOffset;
new files in chromium style

https://codereview.chromium.org/11316128/diff/1/cc/compositor_frame_metadata....
cc/compositor_frame_metadata.h:24: // determine if scrolling/scaling in a
particular direction is possible.
The browser compositor isn't going to be answering scroll questions though, is
it? I thought we're going to have the renderer deal with scroll.

[aelias_OOO_until_Jul13, 2012-11-21 03:46:56]

On 2012/11/21 03:39:07, danakj wrote:
> The browser compositor isn't going to be answering scroll questions though, is
> it? I thought we're going to have the renderer deal with scroll.

This is for various "other" uses that don't fit well in our multiprocess
architecture.  The motivating case for me is Android WebView child views, that
expect to be scrolled in perfect sync with the web document.  If we cared about
making NPAPI plugins work well with impl-thread scrolling, that would be another
example.

[aelias_OOO_until_Jul13, 2012-12-14 21:32:20]

Hi Antoine, PTAL.  This now pulls in some pieces of your ubercomp patch to make
the minimum useful patch for metadata.  This doesn't yet included the swap
integration, the ack message sending or the GLOutputDevice -- let's do those in
later patches.

[piman, 2012-12-14 22:02:22]

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h
File cc/compositor_frame.h (right):

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h#newc...
cc/compositor_frame.h:16: ~CompositorFrame();
Don't we need a virtual destructor?

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame_metada...
File cc/compositor_frame_metadata.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame_metada...
cc/compositor_frame_metadata.cc:11: , min_page_scale_factor(0)
nit: chromium style please for , and { below

https://codereview.chromium.org/11316128/diff/9001/cc/gl_compositor_frame.cc
File cc/gl_compositor_frame.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/gl_compositor_frame.cc#...
cc/gl_compositor_frame.cc:10: : sync_point(0) {
nit: indent = 4

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:876: return CompositorFrameMetadata();
nit: (I know it sucks) consistency for indents? +4 here since we're still using
webkit style

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:923: if (settings().compositorFrameMessage &&
outputSurface()->Context3D()) {
Shouldn't this happen in GLRenderer::drawFrame instead?

https://codereview.chromium.org/11316128/diff/9001/cc/switches.cc
File cc/switches.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/switches.cc#newcode18
cc/switches.cc:18: const char kEnableCompositorFrameMessage[] =
"enable-compositor-frame-message";
Do we need this flag in the long run? It feels like we're passing a flag to make
a decision at run-time for something that will (eventually?) be compiled just
one way or the other, hence won't work if the flag is not properly set (or
unset).
If we need it for the transition, that's ok, but I hope we're going to eliminate
it soon.

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1523: if (view_)
nit: needs braces per style

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1525:
gfx::Vector2d(static_cast<int>(scroll_offset.x()),
Use gfx::ToFlooredVector2d
Should it be Rounded instead?

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1530:
gfx::Size(static_cast<int>(content_size.width()),
use gfx::ToFlooredSize
Should it be Ceiled?

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages.cc
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages...
content/common/cc_messages.cc:587: if (!ReadParam(m, iter,
static_cast<cc::CompositorFrame*>(p)))
Also fail if the read type is not DELEGATED.

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages.h
File content/common/cc_messages.h (right):

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages...
content/common/cc_messages.h:230: IPC_STRUCT_TRAITS_MEMBER(type)
Somewhere, we need to check the enum values, verify it's only DELEGATED or GL,
and refuse the message if it's not. Where should we do that?

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages...
content/common/cc_messages.h:235: IPC_STRUCT_TRAITS_PARENT(cc::CompositorFrame)
We should check somewhere that type == GL.

https://codereview.chromium.org/11316128/diff/9001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:97: case
CompositorFrame::DELEGATED:
nit: indent according to chrome style (case at +2)

[danakj, 2012-12-14 22:23:34]

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h
File cc/compositor_frame.h (right):

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h#newc...
cc/compositor_frame.h:24: FrameType type;
Can we set the type in the constructor and make this constructor protected?

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:878: CompositorFrameMetadata metadata;
How about making a constructor that takes all these parameters so the compiler
can make sure we're setting everything?

[aelias_OOO_until_Jul13, 2012-12-14 22:41:01]

OK, to avoid the type enum, Antoine suggested offline that we avoid the
inheritance hierarchy and just have CompositorFrame contain
scoped_ptr<GLFrameData> and scoped<DelegatedFrameData>, so I'll do that in my
next patch (not yet uploaded).

[aelias_OOO_until_Jul13, 2012-12-15 00:06:17]

All done, PTAL.  Responses to the nits below.

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h
File cc/compositor_frame.h (right):

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame.h#newc...
cc/compositor_frame.h:16: ~CompositorFrame();
On 2012/12/14 22:02:22, piman wrote:
> Don't we need a virtual destructor?

Done.

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame_metada...
File cc/compositor_frame_metadata.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/compositor_frame_metada...
cc/compositor_frame_metadata.cc:11: , min_page_scale_factor(0)
On 2012/12/14 22:02:22, piman wrote:
> nit: chromium style please for , and { below

Done.

https://codereview.chromium.org/11316128/diff/9001/cc/gl_compositor_frame.cc
File cc/gl_compositor_frame.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/gl_compositor_frame.cc#...
cc/gl_compositor_frame.cc:10: : sync_point(0) {
On 2012/12/14 22:02:22, piman wrote:
> nit: indent = 4

Done.

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:876: return CompositorFrameMetadata();
On 2012/12/14 22:02:22, piman wrote:
> nit: (I know it sucks) consistency for indents? +4 here since we're still
using
> webkit style

Done.

https://codereview.chromium.org/11316128/diff/9001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:923: if (settings().compositorFrameMessage &&
outputSurface()->Context3D()) {
Done.  I gave the GLRenderer an OutputSurface in the constructor to do this.

https://codereview.chromium.org/11316128/diff/9001/cc/switches.cc
File cc/switches.cc (right):

https://codereview.chromium.org/11316128/diff/9001/cc/switches.cc#newcode18
cc/switches.cc:18: const char kEnableCompositorFrameMessage[] =
"enable-compositor-frame-message";
We don't need it in the long run, it's only for the transition.

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1523: if (view_)
On 2012/12/14 22:02:22, piman wrote:
> nit: needs braces per style

Done.

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1525:
gfx::Vector2d(static_cast<int>(scroll_offset.x()),
Switched to rounded.  On Android, the float is currently expected to be an
integral value modulo floating-point errors so it doesn't matter much.  In the
future, we'll pass through floats all the way.

https://codereview.chromium.org/11316128/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1530:
gfx::Size(static_cast<int>(content_size.width()),
Switched to Ceiled.

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages.cc
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/common/cc_messages...
content/common/cc_messages.cc:587: if (!ReadParam(m, iter,
static_cast<cc::CompositorFrame*>(p)))
On 2012/12/14 22:02:22, piman wrote:
> Also fail if the read type is not DELEGATED.

Done.

https://codereview.chromium.org/11316128/diff/9001/content/renderer/gpu/compo...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/11316128/diff/9001/content/renderer/gpu/compo...
content/renderer/gpu/compositor_output_surface.cc:97: case
CompositorFrame::DELEGATED:
On 2012/12/14 22:02:22, piman wrote:
> nit: indent according to chrome style (case at +2)

Done.

[aelias_OOO_until_Jul13, 2012-12-15 00:07:58]

Adding jschuh@ for IPC security review.

[piman, 2012-12-15 01:10:36]

LGTM+nit

https://codereview.chromium.org/11316128/diff/20001/cc/gl_frame_data.h
File cc/gl_frame_data.h (right):

https://codereview.chromium.org/11316128/diff/20001/cc/gl_frame_data.h#newcode20
cc/gl_frame_data.h:20: std::string mailbox_name;
nit: do you want to use cc::Mailbox here? The advantage is that the size is
checked in IPCs. I was going to mention that we may need to check it
somewhere...

[aelias_OOO_until_Jul13, 2012-12-15 01:19:16]

Switched to cc::Mailbox.

[piman, 2012-12-15 02:36:26]

lgtm

[aelias_OOO_until_Jul13, 2012-12-17 19:10:58]

Ping for security review.

[jschuh, 2012-12-17 20:19:30]

Lgtm in isolation.

[aelias_OOO_until_Jul13, 2012-12-17 22:19:58]

FYI, the #include "third_party/khronos/GLES2/gl2.h" in transferable_resource.h
was conflicting with platform OpenGL header on Mac in view_messages.h, so I
removed that include and replaced GLbyte/GLenum with int8/uint32.

[piman, 2012-12-17 22:23:06]

On Mon, Dec 17, 2012 at 2:19 PM, <aelias@chromium.org> wrote:

> FYI, the #include "third_party/khronos/GLES2/**gl2.h" in
> transferable_resource.h
> was conflicting with platform OpenGL header on Mac in view_messages.h, so I
> removed that include and replaced GLbyte/GLenum with int8/uint32.
>

LGTM - though we should fix that some time, it's going to bite us again I
bet.


>
>
https://codereview.chromium.**org/11316128/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-12-17 23:23:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11316128/27001

[commit-bot: I haz the power, 2012-12-18 00:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11316128/27001

[commit-bot: I haz the power, 2012-12-18 01:48:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/11316128/27001

[commit-bot: I haz the power, 2012-12-18 03:42:22]

Change committed as 173634