Adds support for hittesting points on surface quads

Adds support for hittesting points on surface quads.

This is the first patch towards adding support for browser-process quad hittesting. This will be used for out of process iframes as a way to determine which process should receive input events.

BUG=491334
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8d07c91d23ae5293087bdd8f6817459f9016a60c
Cr-Commit-Position: refs/heads/master@{#342362}

[lfg, 2015-07-31 19:15:17]

lfg@chromium.org changed reviewers:
+ piman@chromium.org

[lfg, 2015-07-31 19:15:17]

Hi piman, can you take a look?

The implementation is based on the design doc here:
https://docs.google.com/document/d/1RQUbxiK60Z7CZDcWeryqKeX-bewZzgsirjF2Dprfx...

[danakj, 2015-07-31 21:47:28]

danakj@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2015-07-31 22:00:28]

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:25: const CompositorFrame* surface_frame =
surface->GetEligibleFrame();
To avoid infinite recursion, this should check that it's not already being
called on the current surface higher in the callstack (see referenced_surfaces_
in SurfaceAggregator).

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:31: for (const auto& render_pass :
frame_data->render_pass_list) {
It would probably be best to look at the root pass (last pass), and look at
other passes only if the point hits a RenderPassDrawQuad that references them.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:32: for (const auto& quad :
render_pass->quad_list) {
This should go from the end of the list to the beginning to handle overlapping
surfaces - it should also stop if any quad contains the point.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:46: transformed_point.y() <
surface_quad->rect.height()) {
Should check clip rect as well.

[jbauman, 2015-07-31 22:49:27]

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
File cc/surfaces/surface_hittest.h (right):

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.h:23: SurfaceId Hittest(SurfaceId surface_id, const
gfx::Point& point);
You might also need to return the transformed location in the context of this
Surface; is there any other way to tell the renderer where the click (or
whatever) actually was?

[dcheng, 2015-08-01 00:48:47]

Drive-by.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
File cc/surfaces/surface_hittest.h (right):

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.h:26: SurfaceManager* manager_;
Nit: SurfaceManager* const, since it can't change for the lifetime of this
class.

[piman, 2015-08-03 22:36:21]

content/ lgtm

[lfg, 2015-08-04 21:14:59]

Patchset #4 (id:60001) has been deleted

[lfg, 2015-08-04 21:21:42]

jbauman@, please take another look.

Thanks!

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:25: const CompositorFrame* surface_frame =
surface->GetEligibleFrame();
On 2015/07/31 22:00:28, jbauman wrote:
> To avoid infinite recursion, this should check that it's not already being
> called on the current surface higher in the callstack (see
referenced_surfaces_
> in SurfaceAggregator).

Done.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:31: for (const auto& render_pass :
frame_data->render_pass_list) {
On 2015/07/31 22:00:28, jbauman wrote:
> It would probably be best to look at the root pass (last pass), and look at
> other passes only if the point hits a RenderPassDrawQuad that references them.

Done.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:32: for (const auto& quad :
render_pass->quad_list) {
On 2015/07/31 22:00:28, jbauman wrote:
> This should go from the end of the list to the beginning to handle overlapping
> surfaces - it should also stop if any quad contains the point.

This is the only point I'm not sure about. For rendering, yes, we want to go
from end to beginning. But for hittesting, I actually want to go from front to
back in quads, so that the quads that are drawn later are the first ones tested
for input.

The code here
https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/render_pa...
shows that the reverse iterator is used for back to front for quads, so isn't
the order I'm iterating the correct one?

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.cc:46: transformed_point.y() <
surface_quad->rect.height()) {
On 2015/07/31 22:00:28, jbauman wrote:
> Should check clip rect as well.

I've added the check to clip rect, but I'm wondering if the check to rect is not
necessary. Is it true that all visible_rects are contained inside their rects?

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
File cc/surfaces/surface_hittest.h (right):

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.h:23: SurfaceId Hittest(SurfaceId surface_id, const
gfx::Point& point);
On 2015/07/31 22:49:27, jbauman wrote:
> You might also need to return the transformed location in the context of this
> Surface; is there any other way to tell the renderer where the click (or
> whatever) actually was?

Indeed, you're right. For some context, today this transformation happens in the
parent renderer.

https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
cc/surfaces/surface_hittest.h:26: SurfaceManager* manager_;
On 2015/08/01 00:48:47, dcheng wrote:
> Nit: SurfaceManager* const, since it can't change for the lifetime of this
> class.

Done.

[jbauman, 2015-08-04 21:51:04]

On 2015/08/04 21:21:42, lfg wrote:
> jbauman@, please take another look.
> 
> Thanks!
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> File cc/surfaces/surface_hittest.cc (right):
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.cc:25: const CompositorFrame* surface_frame =
> surface->GetEligibleFrame();
> On 2015/07/31 22:00:28, jbauman wrote:
> > To avoid infinite recursion, this should check that it's not already being
> > called on the current surface higher in the callstack (see
> referenced_surfaces_
> > in SurfaceAggregator).
> 
> Done.
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.cc:31: for (const auto& render_pass :
> frame_data->render_pass_list) {
> On 2015/07/31 22:00:28, jbauman wrote:
> > It would probably be best to look at the root pass (last pass), and look at
> > other passes only if the point hits a RenderPassDrawQuad that references
them.
> 
> Done.

It now ignores passes other than the root render pass. It should check if it
hits a RenderPassDrawQuad and if so transform the point into the space of that
renderpass and then walk the renderpass.
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.cc:32: for (const auto& quad :
> render_pass->quad_list) {
> On 2015/07/31 22:00:28, jbauman wrote:
> > This should go from the end of the list to the beginning to handle
overlapping
> > surfaces - it should also stop if any quad contains the point.
> 
> This is the only point I'm not sure about. For rendering, yes, we want to go
> from end to beginning. But for hittesting, I actually want to go from front to
> back in quads, so that the quads that are drawn later are the first ones
tested
> for input.
> 
> The code here
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/render_pa...
> shows that the reverse iterator is used for back to front for quads, so isn't
> the order I'm iterating the correct one?
> 
Good point. This seems correct, though you need to check for hittesting against
every quad, to ensure other quads that occlude a subsurface are hit on.

>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.cc:46: transformed_point.y() <
> surface_quad->rect.height()) {
> On 2015/07/31 22:00:28, jbauman wrote:
> > Should check clip rect as well.
> 
> I've added the check to clip rect, but I'm wondering if the check to rect is
not
> necessary. Is it true that all visible_rects are contained inside their rects?

Checking visible_rect should be unnecessary, but you need to check
shared_quad_state->clip_rect (if shared_quad_state->is_clipped is set), which is
in target space, not in quad space.
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> File cc/surfaces/surface_hittest.h (right):
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.h:23: SurfaceId Hittest(SurfaceId surface_id,
const
> gfx::Point& point);
> On 2015/07/31 22:49:27, jbauman wrote:
> > You might also need to return the transformed location in the context of
this
> > Surface; is there any other way to tell the renderer where the click (or
> > whatever) actually was?
> 
> Indeed, you're right. For some context, today this transformation happens in
the
> parent renderer.
> 
>
https://codereview.chromium.org/1265013003/diff/20001/cc/surfaces/surface_hit...
> cc/surfaces/surface_hittest.h:26: SurfaceManager* manager_;
> On 2015/08/01 00:48:47, dcheng wrote:
> > Nit: SurfaceManager* const, since it can't change for the lifetime of this
> > class.
> 
> Done.

[lfg, 2015-08-05 22:57:12]

On 2015/08/04 21:51:04, jbauman wrote:
> 
> It now ignores passes other than the root render pass. It should check if it
> hits a RenderPassDrawQuad and if so transform the point into the space of that
> renderpass and then walk the renderpass.

I think I got it now. Sorry, I'm just not very familiar with the way
RenderPasses are drawn.

> Good point. This seems correct, though you need to check for hittesting
against
> every quad, to ensure other quads that occlude a subsurface are hit on.

Indeed. Done.

> Checking visible_rect should be unnecessary, but you need to check
> shared_quad_state->clip_rect (if shared_quad_state->is_clipped is set), which
is
> in target space, not in quad space.

Done.

[jbauman, 2015-08-05 23:33:22]

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:5: #include <iostream>
Remove this.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:76: (point_in_target_space.x() >
quad->shared_quad_state->clip_rect.x() &&
quad->shared_quad_state->clip_rect.Contains(point_in_target_space).

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:92: if (transformed_point.x() > quad->rect.x() &&
Contains here too.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:107:
referenced_surfaces_.insert(surface_quad->surface_id);
The root surface could still have a reference to itself with this (though at
least it wouldn't have infinite recursion).

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:132: quad_render_pass = render_pass;
It's possible that two passes could have RenderPassDrawQuads that each reference
the other pass, which would also cause stack overflow. Could you change
referenced_surfaces_ to referenced_passes_ instead, and store RenderPass*s in
it?

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:137: return HittestInternal(surface_id,
quad_render_pass,
Just thought of one last possible issue - it's possible that there are no quads
in the renderpass that the point would hit. In that case the code should
continue to walk the parent renderpass to try to find some other quad that
contains it.

[lfg, 2015-08-06 18:28:12]

Thanks for the quick reviews! I've addressed the comments, please take another
look.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:5: #include <iostream>
On 2015/08/05 23:33:22, jbauman wrote:
> Remove this.

Oops. Done.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:76: (point_in_target_space.x() >
quad->shared_quad_state->clip_rect.x() &&
On 2015/08/05 23:33:22, jbauman wrote:
> quad->shared_quad_state->clip_rect.Contains(point_in_target_space).

Done. Just a note: in the previous code I was also assuming > for top/left and
<= for bottom/right. Contains() uses >= for top/left and < for bottom/right.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:92: if (transformed_point.x() > quad->rect.x() &&
On 2015/08/05 23:33:22, jbauman wrote:
> Contains here too.

Done.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:107:
referenced_surfaces_.insert(surface_quad->surface_id);
On 2015/08/05 23:33:22, jbauman wrote:
> The root surface could still have a reference to itself with this (though at
> least it wouldn't have infinite recursion).

I've fixed it when moving to use referenced passes.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:132: quad_render_pass = render_pass;
On 2015/08/05 23:33:22, jbauman wrote:
> It's possible that two passes could have RenderPassDrawQuads that each
reference
> the other pass, which would also cause stack overflow. Could you change
> referenced_surfaces_ to referenced_passes_ instead, and store RenderPass*s in
> it?

Done.

https://codereview.chromium.org/1265013003/diff/100001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:137: return HittestInternal(surface_id,
quad_render_pass,
On 2015/08/05 23:33:22, jbauman wrote:
> Just thought of one last possible issue - it's possible that there are no
quads
> in the renderpass that the point would hit. In that case the code should
> continue to walk the parent renderpass to try to find some other quad that
> contains it.

Done. This was a bit more complicated, as I have to unwind the recursion stack
and continue iterating over the next quads. Also, I need to store the
transformed point relative to the Surface (not the RenderPass) because that's
what the renderer must receive.

I've also had to change the tests since my previous tests didn't have solid
quads in the child surface.

[jbauman, 2015-08-06 20:42:19]

lgtm with one nit.

https://codereview.chromium.org/1265013003/diff/140001/cc/surfaces/surface_hi...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/140001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:129: }
Check if quad_render_pass is nullptr and don't do the hittest if so (to protect
against invalid renderpassdrawquads).

[lfg, 2015-08-06 22:30:00]

https://codereview.chromium.org/1265013003/diff/140001/cc/surfaces/surface_hi...
File cc/surfaces/surface_hittest.cc (right):

https://codereview.chromium.org/1265013003/diff/140001/cc/surfaces/surface_hi...
cc/surfaces/surface_hittest.cc:129: }
On 2015/08/06 20:42:19, jbauman wrote:
> Check if quad_render_pass is nullptr and don't do the hittest if so (to
protect
> against invalid renderpassdrawquads).

Done.

[lfg, 2015-08-06 23:11:34]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-08-06 23:11:34]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1265013003/#ps160001
(title: "addressing comments")

[commit-bot: I haz the power, 2015-08-06 23:12:44]

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

[commit-bot: I haz the power, 2015-08-07 01:59:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 01:59:12]

Try jobs failed on following builders:
  mac_chromium_10.10_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.1...)

[lfg, 2015-08-07 16:07:20]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-08-07 16:07:50]

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

[commit-bot: I haz the power, 2015-08-07 16:13:30]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-08-07 16:14:03]

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