cc: Fix sublayer scale bug in hit testing

cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.

BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0da498c027ed0b0d1c4596892c7666afaa5a98af
Cr-Commit-Position: refs/heads/master@{#389873}

[jaydasika, 2016-04-25 23:41:50]

Description was changed from

==========
cc: Fix sublayer scale bug in hit testing

BUG=604731
==========

to

==========
cc: Fix sublayer scale bug in hit testing

BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-04-25 23:43:43]

Description was changed from

==========
cc: Fix sublayer scale bug in hit testing

BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.
 
BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-04-25 23:45:39]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-04-25 23:45:40]

PTAL

[ajuma, 2016-04-25 23:58:18]

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1644: DCHECK(!target_layer ||
target_layer->render_surface());
When is target_layer null here? (That is, when the the transform node have an
invalid owner id?)

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl_un...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl_un...
cc/trees/layer_tree_impl_unittest.cc:1920: // this layer is positioned, and hit
testing should correctly know where the
Nit: s/this/This

[jaydasika, 2016-04-26 00:49:32]

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1644: DCHECK(!target_layer ||
target_layer->render_surface());
On 2016/04/25 23:58:17, ajuma wrote:
> When is target_layer null here? (That is, when the the transform node have an
> invalid owner id?)

For the root transform node (transform node with id 0 has owner id -1).

[jaydasika, 2016-04-26 00:56:36]

On 2016/04/26 00:49:32, jaydasika wrote:
> https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl.cc...
> cc/trees/layer_tree_impl.cc:1644: DCHECK(!target_layer ||
> target_layer->render_surface());
> On 2016/04/25 23:58:17, ajuma wrote:
> > When is target_layer null here? (That is, when the the transform node have
an
> > invalid owner id?)
> 
> For the root transform node (transform node with id 0 has owner id -1).

Did a little more investigation on why we end up with the root transform node in
hit testing and not the transform node for the root layer (which will have id 1)
and figured it was because of of this :
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/property_...
, we set the transform id on root effect node to 0 and not 1(which will be the
transform tree index of root layer). Looking at the comment there, I think that
was intentional and so I am not sure if its a bug?

weiliangc: Any thoughts on this ?

[jaydasika, 2016-04-26 18:12:22]

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl_un...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/1917043002/diff/1/cc/trees/layer_tree_impl_un...
cc/trees/layer_tree_impl_unittest.cc:1920: // this layer is positioned, and hit
testing should correctly know where the
On 2016/04/25 23:58:17, ajuma wrote:
> Nit: s/this/This

Done.

https://codereview.chromium.org/1917043002/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1917043002/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:1646: transform_node->id == 0
Making it more explicit that transform node's id = 0 is the only case where we
won't have a target layer.

[ajuma, 2016-04-26 18:36:34]

Thanks, lgtm.

On 2016/04/26 00:56:36, jaydasika wrote:
> Did a little more investigation on why we end up with the root transform node
in
> hit testing and not the transform node for the root layer (which will have id
1)
> and figured it was because of of this :
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/property_...
> , we set the transform id on root effect node to 0 and not 1(which will be the
> transform tree index of root layer). Looking at the comment there, I think
that
> was intentional and so I am not sure if its a bug?

Summarizing an offline discussion: the root render surface's space is really
just screen space, so it makes sense that transform id 0 is used to identify the
space of the root effect node. Currently, the root RenderSurfaceImpl's transform
tree index is 1, but that's only because render surfaces currently get their
property tree indexes from their owning layers (and
TransformTree::ToScreenSpaceTransformWithoutSublayerScale works around this by
special-casing id 1). This is something that can be cleaned up when surface
ownership moves to effect tree nodes.

[jaydasika, 2016-04-26 19:29:46]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-04-26 19:30:20]

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

[commit-bot: I haz the power, 2016-04-26 20:26:55]

Description was changed from

==========
cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.
 
BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.
 
BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-26 20:26:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-26 20:28:15]

Description was changed from

==========
cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.
 
BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix sublayer scale bug in hit testing

In hit testing, we should use the render surface screen space
transform (which doesn't have sublayer scale) instead of the
screen_space_transform stored on the transform node (which
has the sublayer scale baked in) as the clips stored in clip
tree already account for the scale.

BUG=604731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0da498c027ed0b0d1c4596892c7666afaa5a98af
Cr-Commit-Position: refs/heads/master@{#389873}
==========

[commit-bot: I haz the power, 2016-04-26 20:28:17]

Patchset 2 (id:??) landed as
https://crrev.com/0da498c027ed0b0d1c4596892c7666afaa5a98af
Cr-Commit-Position: refs/heads/master@{#389873}