Make browser hit testing return correct SurfaceId with nested Surfaces

Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

BUG=589572
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9d8b8320a54fc65539d99be0e2a489e2e2b1b7bc
Cr-Commit-Position: refs/heads/master@{#378495}

[kenrb, 2016-02-29 16:52:33]

Description was changed from

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

BUG=589572
==========

to

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

[kenrb, 2016-02-29 16:53:24]

Description was changed from

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

to

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

[kenrb, 2016-02-29 16:53:25]

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

[kenrb, 2016-02-29 16:54:22]

Lucas: Do you agree with this change?

[lfg, 2016-02-29 18:25:12]

lgtm with one optional nit.

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:755:
"/frame_tree/page_with_positioned_nested_frames.html"));
This is fine, but if you use cross_site_iframe_factory.html, you can build your
frame tree without navigating.

[kenrb, 2016-02-29 19:59:53]

kenrb@chromium.org changed reviewers:
+ jbauman@chromium.org, nasko@chromium.org

[kenrb, 2016-02-29 19:59:54]

PTAL?
- jbauman for cc/surfaces/
- nasko for content/

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:755:
"/frame_tree/page_with_positioned_nested_frames.html"));
On 2016/02/29 18:25:11, lfg wrote:
> This is fine, but if you use cross_site_iframe_factory.html, you can build
your
> frame tree without navigating.

It looks like it would be a similar amount of code since I need to make the
frames positioned.

[jbauman, 2016-02-29 22:03:05]

lgtm

[nasko, 2016-02-29 22:15:37]

LGTM with nits.

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:755:
"/frame_tree/page_with_positioned_nested_frames.html"));
On 2016/02/29 19:59:54, kenrb wrote:
> On 2016/02/29 18:25:11, lfg wrote:
> > This is fine, but if you use cross_site_iframe_factory.html, you can build
> your
> > frame tree without navigating.
> 
> It looks like it would be a similar amount of code since I need to make the
> frames positioned.

Can't we define some positioning algorithm for the factory file and rely on it?
Lots of boilerplate navigation code can be saved if we could.

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:783:
RenderWidgetHostMouseEventMonitor child_frame_monitor(
nit: nested_frame_monitor?

[kenrb, 2016-03-01 15:50:30]

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:755:
"/frame_tree/page_with_positioned_nested_frames.html"));
On 2016/02/29 22:15:37, nasko wrote:
> On 2016/02/29 19:59:54, kenrb wrote:
> > On 2016/02/29 18:25:11, lfg wrote:
> > > This is fine, but if you use cross_site_iframe_factory.html, you can build
> > your
> > > frame tree without navigating.
> > 
> > It looks like it would be a similar amount of code since I need to make the
> > frames positioned.
> 
> Can't we define some positioning algorithm for the factory file and rely on
it?
> Lots of boilerplate navigation code can be saved if we could.

I will take a todo on that, since I would need to figure out how to do that and
I'd like to land this CL to fix the regression.

https://codereview.chromium.org/1743303002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:783:
RenderWidgetHostMouseEventMonitor child_frame_monitor(
On 2016/02/29 22:15:37, nasko wrote:
> nit: nested_frame_monitor?

Done.

[kenrb, 2016-03-01 15:51:14]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-03-01 15:51:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, nasko@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/1743303002/#ps60001
(title: "Nits addressed")

[commit-bot: I haz the power, 2016-03-01 15:51:36]

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

[commit-bot: I haz the power, 2016-03-01 18:53:37]

Description was changed from

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

to

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

[commit-bot: I haz the power, 2016-03-01 18:53:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-01 18:55:52]

Description was changed from

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

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

to

==========
Make browser hit testing return correct SurfaceId with nested Surfaces

A regression caused SurfaceHittest::GetTargetSurfaceAtPoint() to return
the incorrect SurfaceId when the correct Surface was nested more than
one layer from the root Surface. This fixes the regression.

BUG=589572
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9d8b8320a54fc65539d99be0e2a489e2e2b1b7bc
Cr-Commit-Position: refs/heads/master@{#378495}
==========

[commit-bot: I haz the power, 2016-03-01 18:55:53]

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