cc: Convert backface LTHCommon tests from Layer->LayerImpl

cc: Convert backface LTHCommon tests from Layer->LayerImpl

R=ajuma@chromium.org
BUG=502041
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7b2a217751cdfbcfcf6dcf9793cb428082939da7
Cr-Commit-Position: refs/heads/master@{#338599}

[enne (OOO), 2015-07-07 23:31:00]

So, it turns out that the update_list includes layers that have backfacing
parents if they themselves are front facing.  Do you have any thoughts if/how
this should be addressed?

https://codereview.chromium.org/1221273003/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1221273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:3796: EXPECT_TRUE(
This didn't used to be included.

https://codereview.chromium.org/1221273003/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:4016:
EXPECT_TRUE(UpdateLayerListContains(back_facing_surface->id()));
This and child2 also didn't used to be included.

[ajuma, 2015-07-08 15:24:22]

On 2015/07/07 23:31:00, enne wrote:
> So, it turns out that the update_list includes layers that have backfacing
> parents if they themselves are front facing.  Do you have any thoughts if/how
> this should be addressed?

The IsLayerBackfaceVisible logic in draw_property_utils seems very similar to
CDP's IsLayerBackfaceVisible logic, except for a couple things:
1) LayerIsInExisting3DRenderingContext is missing the
"(layer->parent()->sorting_context_id() == layer->sorting_context_id())" check.
2) For layers that own render surfaces, CDP also calls IsSurfaceBackfaceVisible
(which is slightly different from IsLayerBackfaceVisible), and earlies-out
(skipping the subtree) if this returns true and the layer is single-sided.

I think (2) is probably what's causing the test differences.

[enne (OOO), 2015-07-10 22:22:35]

Addressed the test issues so that these all pass now.

I have to admit I'm a bit saddened to have to recreate all of the old behavior
of CDP here.  I think it's ok to have backfacing layers be updated on either
thread.  It's a lot of matrix testing and conditionals for most layers that
don't need it.  We can leave it for now; just expressing a vague concern about
wanting things to be more efficient in the common case.

https://codereview.chromium.org/1221273003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1221273003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:179: return
node->data.local.IsBackFaceVisible();
to_target is just identity for something that creates a surface, so I think
local is what I want, but I'm not 100% sure.

[ajuma, 2015-07-10 22:56:53]

On 2015/07/10 22:22:35, enne wrote:
> I have to admit I'm a bit saddened to have to recreate all of the old behavior
> of CDP here.  I think it's ok to have backfacing layers be updated on either
> thread.  It's a lot of matrix testing and conditionals for most layers that
> don't need it.  We can leave it for now; just expressing a vague concern about
> wanting things to be more efficient in the common case.

I agree. I think it's good to keep for now for the sake of treating CDP's
current output as the "correct" result we're trying to match, but we should
revisit this once CDP is gone. Even if we need to keep something like this
logic, we can probably make it more efficient; we should really only need to
compute backface visibility at each transform node rather than each layer, since
layers without transform nodes will necessarily have the same backface
visibility as their transform parent.

https://codereview.chromium.org/1221273003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1221273003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:179: return
node->data.local.IsBackFaceVisible();
On 2015/07/10 22:22:34, enne wrote:
> to_target is just identity for something that creates a surface, so I think
> local is what I want, but I'm not 100% sure.

I think we want the transform from this node to node->data.target_id rather than
just the local transform (e.g. maybe the local transform is a 180-degree
rotation, but the parent node also is a 180-degree rotation, and the target is
somewhere further up).

[ajuma, 2015-07-10 23:00:20]

On 2015/07/10 22:56:53, ajuma wrote:
> On 2015/07/10 22:22:35, enne wrote:
> > I have to admit I'm a bit saddened to have to recreate all of the old
behavior
> > of CDP here.  I think it's ok to have backfacing layers be updated on either
> > thread.  It's a lot of matrix testing and conditionals for most layers that
> > don't need it.  We can leave it for now; just expressing a vague concern
about
> > wanting things to be more efficient in the common case.
> 
> I agree. I think it's good to keep for now for the sake of treating CDP's
> current output as the "correct" result we're trying to match, but we should
> revisit this once CDP is gone. Even if we need to keep something like this
> logic, we can probably make it more efficient; we should really only need to
> compute backface visibility at each transform node rather than each layer,
since
> layers without transform nodes will necessarily have the same backface
> visibility as their transform parent.

(At least in terms of matrix math. The 3d rendering context stuff won't
necessarily be same for a layer and its transform parent, I don't think.)

[enne (OOO), 2015-07-13 21:57:50]

PTAL

[ajuma, 2015-07-13 22:58:45]

lgtm with nit

https://codereview.chromium.org/1221273003/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1221273003/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:181: tree.ComputeTransform(node->data.target_id,
node->id,
I think these arguments need to be reversed in order to compute the surface draw
transform (though this might not make a difference in terms of figuring out
whether the back face is visible).

[enne (OOO), 2015-07-13 23:01:50]

Oops, quite right!

[enne (OOO), 2015-07-13 23:03:06]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-07-13 23:03:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1221273003/#ps60001
(title: "Swap arg order")

[commit-bot: I haz the power, 2015-07-13 23:03:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1221273003/60001

[commit-bot: I haz the power, 2015-07-14 00:05:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-14 00:06:27]

Patchset 4 (id:??) landed as
https://crrev.com/7b2a217751cdfbcfcf6dcf9793cb428082939da7
Cr-Commit-Position: refs/heads/master@{#338599}