Mandoline: Support transforms and clipping of OOPIFs and events

Mandoline: Support transforms and clipping of OOPIFs and events

This CL enables basic transforms, clipping of iframe
graphical output and hit testing by leveraging
SurfaceAggregator, and SurfaceHittest.

Previously, DisplayManager's DrawViewTree was creating a
CompositorFrame with all the Surfaces in place.
SurfaceAggregator tries to put all the Surfaces together
but finds it has no work to do because all the
SurfaceDrawQuads are already in one CompositorFrame. By not
including subtrees of Views with Surfaces in the
CompositorFrame DisplayManager generates, we left
SurfaceAggregator do the heavy lifting for
transforms/clipping/opacity/etc.

BUG=496953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[Fady Samuel, 2015-09-16 20:45:21]

fsamuel@chromium.org changed reviewers:
+ rjkroege@chromium.org

[Fady Samuel, 2015-09-16 20:45:21]

Hi Rob,

Could you please take a look at this? This needs a bit of cleanup but I wanted
to get your thoughts/opinions.

Thanks!

Fady

[rjkroege, 2015-09-16 21:31:37]

https://codereview.chromium.org/1328953003/diff/60001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/1328953003/diff/60001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:64: SurfaceManager* manager() const { return
manager_; }
You can land this in isolation?

https://codereview.chromium.org/1328953003/diff/60001/components/mus/connecti...
File components/mus/connection_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/60001/components/mus/connecti...
components/mus/connection_manager.cc:453:
render_pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
where do we validate the contents of the provided CompositorFrame for malicious
payload? please put a TODO there.

why is connection_mangement doing graphics work? this seems like a failure to
separate concerns (more TODOs probably)

do you have a block-diagram of mus? I've seen fragments of one. Can you put one
together that's more detailed?

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:64: DrawViewTree(pass, *it,
absolute_bounds.OffsetFromOrigin(),
can you remind me why there are multiple view trees?

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:202: void DefaultDisplayManager::Draw() {
this is the top-level draw to the display function right?

btw: you could put more comments in your code... every time that I ask a
question like this, it means that someone else who is new to the code might also
want a helpful comment.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:206: scoped_ptr<cc::CompositorFrame>
frame(GenerateCompositorFrame());
you assemble the top-level OutputSurface CompositorFrame here? i.e.: on the
critical path for drawing.

Add TODO to bracket the assembly process with trace points.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:307: mojo_event->pointer_data->x =
transformed_point.x();
you don't translate the screen coords? they are in screen coordinates? it
strikes me that this possibly leaks information into the OOPIF. probably fine.

[Fady Samuel, 2015-09-17 18:31:43]

PTAL

https://codereview.chromium.org/1328953003/diff/60001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/1328953003/diff/60001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.h:64: SurfaceManager* manager() const { return
manager_; }
On 2015/09/16 21:31:37, rjkroege wrote:
> You can land this in isolation?

Done.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/connecti...
File components/mus/connection_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/60001/components/mus/connecti...
components/mus/connection_manager.cc:453:
render_pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
On 2015/09/16 21:31:37, rjkroege wrote:
> where do we validate the contents of the provided CompositorFrame for
malicious
> payload? please put a TODO there.
> 
> why is connection_mangement doing graphics work? this seems like a failure to
> separate concerns (more TODOs probably)
> 
> do you have a block-diagram of mus? I've seen fragments of one. Can you put
one
> together that's more detailed?

There's a TODO above.

Added a todo and filed a bug for moving this out of connectionmanager.

I'll write a block diagram for mus. I've added a doc to your phase 1 doc with a
partial diagram.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:64: DrawViewTree(pass, *it,
absolute_bounds.OffsetFromOrigin(),
On 2015/09/16 21:31:37, rjkroege wrote:
> can you remind me why there are multiple view trees?

We don't really. This is recursively visiting the view tree. I added a comment
above.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:202: void DefaultDisplayManager::Draw() {
On 2015/09/16 21:31:37, rjkroege wrote:
> this is the top-level draw to the display function right?
> 
> btw: you could put more comments in your code... every time that I ask a
> question like this, it means that someone else who is new to the code might
also
> want a helpful comment. 

To be fair, I didn't write this method :P I did add a comment to the header.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:206: scoped_ptr<cc::CompositorFrame>
frame(GenerateCompositorFrame());
On 2015/09/16 21:31:37, rjkroege wrote:
> you assemble the top-level OutputSurface CompositorFrame here? i.e.: on the
> critical path for drawing.
> 
> Add TODO to bracket the assembly process with trace points.

Done.

https://codereview.chromium.org/1328953003/diff/60001/components/mus/display_...
components/mus/display_manager.cc:307: mojo_event->pointer_data->x =
transformed_point.x();
On 2015/09/16 21:31:37, rjkroege wrote:
> you don't translate the screen coords? they are in screen coordinates? it
> strikes me that this possibly leaks information into the OOPIF. probably fine.

I'm not translating the screen coordinates. I'm actually not sure how
html_viewer uses screen coordinates...

[rjkroege, 2015-09-17 19:03:56]

Also, please wrap description.

https://codereview.chromium.org/1328953003/diff/80001/components/mus/connecti...
File components/mus/connection_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/connecti...
components/mus/connection_manager.cc:453: // TODO(fsamuel): This shouldn't be in
the ConnectionManager. Let's fine a
fine -> find

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:52: // skip this step and let
cc::SurfaceAggregator do the heavy lifting.
this code is all on the critical redraw path? 

Ongoing ask: please TODO any place where you think it would be a good idea to
have trace events recorded?

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
File components/mus/display_manager.h (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.h:107: // This method initiates a top level
redraw of the display.
When w.r.t. to vblank?

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.h:109: void DidDraw();
when is this called with regards to the vblank?

[rjkroege, 2015-09-17 19:08:05]

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:65: const float combined_opacity = opacity *
view->opacity();
do we have pre-multiplied alpha? if we do (pre-multiplied is the norm) then this
operation seems wrong. we need at least a TODO() to firmly rationalize the
handling of alpha

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:254: frame_data->device_scale_factor = 1.f;
? won't this break native apps that have their CF synthesized? They won't
rasterize correctly for dsf != 1.0

[Fady Samuel, 2015-09-17 20:00:22]

PTAL Rob

https://codereview.chromium.org/1328953003/diff/80001/components/mus/connecti...
File components/mus/connection_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/connecti...
components/mus/connection_manager.cc:453: // TODO(fsamuel): This shouldn't be in
the ConnectionManager. Let's fine a
On 2015/09/17 19:03:56, rjkroege wrote:
> fine -> find

Done.

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:52: // skip this step and let
cc::SurfaceAggregator do the heavy lifting.
On 2015/09/17 19:03:56, rjkroege wrote:
> this code is all on the critical redraw path? 
> 
> Ongoing ask: please TODO any place where you think it would be a good idea to
> have trace events recorded?

GenerateCompositorFrame is the top level call. Do we want a trace event for each
view?

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:65: const float combined_opacity = opacity *
view->opacity();
On 2015/09/17 19:08:05, rjkroege wrote:
> do we have pre-multiplied alpha? if we do (pre-multiplied is the norm) then
this
> operation seems wrong. we need at least a TODO() to firmly rationalize the
> handling of alpha

Done.

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.cc:254: frame_data->device_scale_factor = 1.f;
On 2015/09/17 19:08:05, rjkroege wrote:
> ? won't this break native apps that have their CF synthesized? They won't
> rasterize correctly for dsf != 1.0

Done.

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
File components/mus/display_manager.h (right):

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.h:107: // This method initiates a top level
redraw of the display.
On 2015/09/17 19:03:56, rjkroege wrote:
> When w.r.t. to vblank?

No :P I filed a bug for that. Added a reference and TODO here.

https://codereview.chromium.org/1328953003/diff/80001/components/mus/display_...
components/mus/display_manager.h:109: void DidDraw();
On 2015/09/17 19:03:56, rjkroege wrote:
> when is this called with regards to the vblank?

Done.

[rjkroege, 2015-09-17 20:21:20]

lgtm

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
components/mus/display_manager.cc:50: // DrawViewTree recursively visits
ServerViews, creating a SurfaceDrawQuad for
this is a very nice comment.

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
File components/mus/display_manager.h (right):

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
components/mus/display_manager.h:112: // This is called after cc::Display has
completed generating a new frame
I note in passing that this is where we should be kicking off the idle time
processing yes?

[Fady Samuel, 2015-09-17 21:44:18]

fsamuel@chromium.org changed reviewers:
+ sky@chromium.org

[Fady Samuel, 2015-09-17 21:44:18]

+sky@ for OWNER review.

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
components/mus/display_manager.cc:50: // DrawViewTree recursively visits
ServerViews, creating a SurfaceDrawQuad for
On 2015/09/17 20:21:20, rjkroege wrote:
> this is a very nice comment.

Acknowledged. I had a typo that I fixed.

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
File components/mus/display_manager.h (right):

https://codereview.chromium.org/1328953003/diff/100001/components/mus/display...
components/mus/display_manager.h:112: // This is called after cc::Display has
completed generating a new frame
On 2015/09/17 20:21:20, rjkroege wrote:
> I note in passing that this is where we should be kicking off the idle time
> processing yes?

Added a comment.

[sky, 2015-09-17 22:08:35]

https://codereview.chromium.org/1328953003/diff/120001/components/mus/display...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/120001/components/mus/display...
components/mus/display_manager.cc:291: if (event->IsLocatedEvent() &&
!!top_level_display_client_) {
As cc is updating during a draw cycle, might it be out of date when the event is
processed? For example, how do you know the view id is valid, or say didn't get
reused?

[Fady Samuel, 2015-09-17 22:13:11]

https://codereview.chromium.org/1328953003/diff/120001/components/mus/display...
File components/mus/display_manager.cc (right):

https://codereview.chromium.org/1328953003/diff/120001/components/mus/display...
components/mus/display_manager.cc:291: if (event->IsLocatedEvent() &&
!!top_level_display_client_) {
On 2015/09/17 22:08:34, sky wrote:
> As cc is updating during a draw cycle, might it be out of date when the event
is
> processed? For example, how do you know the view id is valid, or say didn't
get
> reused?

We are performing hit testing on the last display compositor frame. It is quite
possible that the ViewId no longer exists when we perform the hit test. We could
just drop it on the floor.

We could consider queuing up all ViewManager operations to execute once per
graphics frame followed by cc ops, followed by hit testing. Thoughts?

[Fady Samuel, 2015-09-17 22:34:33]

I've filed a bug to implement a UI scheduler:
https://code.google.com/p/chromium/issues/detail?id=533120&thanks=533120&ts=1...

PTAL

[sky, 2015-09-17 22:36:05]

https://codereview.chromium.org/1328953003/diff/120001/components/mus/view_tr...
File components/mus/view_tree_host_impl.cc (right):

https://codereview.chromium.org/1328953003/diff/120001/components/mus/view_tr...
components/mus/view_tree_host_impl.cc:154: if (view) {
How about a TODO here, as this should really be a DCHECK.

[sky, 2015-09-17 22:36:10]

LGTM

[Fady Samuel, 2015-09-17 23:58:01]

Added TODO. CQ'ing! Thanks!

https://codereview.chromium.org/1328953003/diff/120001/components/mus/view_tr...
File components/mus/view_tree_host_impl.cc (right):

https://codereview.chromium.org/1328953003/diff/120001/components/mus/view_tr...
components/mus/view_tree_host_impl.cc:154: if (view) {
On 2015/09/17 22:36:05, sky wrote:
> How about a TODO here, as this should really be a DCHECK.

Done.

[Fady Samuel, 2015-09-17 23:58:05]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-09-17 23:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1328953003/#ps140001
(title: "Added TODO")

[commit-bot: I haz the power, 2015-09-17 23:59:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1328953003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1328953003/140001

[commit-bot: I haz the power, 2015-09-18 00:14:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-18 00:15:18]

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