Add Mojo API for Blink hit testing

Add Mojo API for Blink hit testing

Add Mojo API in the same place as other input
mojo APIs. So to be able to have this API
enabled kMojoInputMessages as the hit testing
will also go through the same path.

BUG=732405
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Navid Zolghadr, 2017-06-22 17:15:32]

Description was changed from

==========
Add Mojo API for Blink hit testing

Add Mojo API in the same place as other input
mojo APIs. So to be able to have this API
enabled kMojoInputMessages as the hit testing
will also go through the same path.

BUG=732405
==========

to

==========
Add Mojo API for Blink hit testing

Add Mojo API in the same place as other input
mojo APIs. So to be able to have this API
enabled kMojoInputMessages as the hit testing
will also go through the same path.

BUG=732405
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[rjkroege, 2017-06-26 17:46:42]

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

[rjkroege, 2017-06-26 18:29:52]

also: shouldn't this CL include some code in content/renderer/mus so that ws can
directly interrogate renderers?

https://codereview.chromium.org/2951053005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/input/legacy_ipc_frame_input_handler.cc (right):

https://codereview.chromium.org/2951053005/diff/1/content/browser/frame_host/...
content/browser/frame_host/input/legacy_ipc_frame_input_handler.cc:127: // We do
not support this API over old IPC.
NOTREACHED(); ?

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
File content/common/input/input_handler.mojom (right):

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:61: // Returns the hit-tested remote
frame id for the given coordinates.
how are we suppose to use this API from ui/ws? Couldn't this be a component?

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:62: HitTestFrameAt(gfx.mojom.PointF
point) => (int32 frame_id);
What is your expectation of the coordinate system of point? I've been presuming
that the CTS of point is the CTS of the target frame?

API seems reasonable to me.

[dcheng, 2017-06-26 20:56:37]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org, kenrb@chromium.org

[dcheng, 2017-06-26 20:56:38]

How does the new Blink API interact with the browser-side hit testing for
OOPIFs?

[Navid Zolghadr, 2017-06-26 20:58:18]

On 2017/06/26 20:56:38, dcheng (OOO Jun 25 - Jul 1) wrote:
> How does the new Blink API interact with the browser-side hit testing for
> OOPIFs?

Robert, I remember you said the code on the browser side is ready to call this
code. Can you point me and Daniel to that code? I can also add the rest of the
plumbing and add the tests to that place as well.

[riajiang, 2017-06-27 16:37:27]

riajiang@chromium.org changed reviewers:
+ riajiang@chromium.org

[riajiang, 2017-06-27 16:37:28]

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
File content/common/input/input_handler.mojom (right):

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:62: HitTestFrameAt(gfx.mojom.PointF
point) => (int32 frame_id);
On 2017/06/26 18:29:52, rjkroege wrote:
> What is your expectation of the coordinate system of point? I've been
presuming
> that the CTS of point is the CTS of the target frame?
> 
> API seems reasonable to me.

Input |point| would be in display-coordinate-pixels (or I can convert it to
screen-coordinates before asking blink) and we need to update it to be in
target's CTS after we found the target |frame_id|?

[rjkroege, 2017-06-28 18:37:15]

On 2017/06/26 20:58:18, Navid Zolghadr wrote:
> On 2017/06/26 20:56:38, dcheng (OOO Jun 25 - Jul 1) wrote:
> > How does the new Blink API interact with the browser-side hit testing for
> > OOPIFs?
> 
> Robert, I remember you said the code on the browser side is ready to call this
> code. Can you point me and Daniel to that code? I can also add the rest of the
> plumbing and add the tests to that place as well.

riajiang@ you wrote this. Perhaps you can comment here?

[rjkroege, 2017-06-28 18:40:19]

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
File content/common/input/input_handler.mojom (right):

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:62: HitTestFrameAt(gfx.mojom.PointF
point) => (int32 frame_id);
On 2017/06/27 16:37:28, riajiang wrote:
> On 2017/06/26 18:29:52, rjkroege wrote:
> > What is your expectation of the coordinate system of point? I've been
> presuming
> > that the CTS of point is the CTS of the target frame?
> > 
> > API seems reasonable to me.
> 
> Input |point| would be in display-coordinate-pixels (or I can convert it to
> screen-coordinates before asking blink) and we need to update it to be in
> target's CTS after we found the target |frame_id|?

display coordinate pixels is  likely wrong. I claim x,y w.r.t. the target frame

[riajiang, 2017-06-28 19:05:03]

On 2017/06/28 18:37:15, rjkroege wrote:
> On 2017/06/26 20:58:18, Navid Zolghadr wrote:
> > On 2017/06/26 20:56:38, dcheng (OOO Jun 25 - Jul 1) wrote:
> > > How does the new Blink API interact with the browser-side hit testing for
> > > OOPIFs?
> > 
> > Robert, I remember you said the code on the browser side is ready to call
this
> > code. Can you point me and Daniel to that code? I can also add the rest of
the
> > plumbing and add the tests to that place as well.
> 
> riajiang@ you wrote this. Perhaps you can comment here?

https://cs.chromium.org/chromium/src/services/ui/ws/event_targeter.cc?dr&l=53
is going to first try to get the target FrameSinkId from hit-test client 
library and if not successful, do an async call to ask Blink for help.


On 2017/06/28 18:40:19, rjkroege wrote:
>
https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
> File content/common/input/input_handler.mojom (right):
> 
>
https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
> content/common/input/input_handler.mojom:62: HitTestFrameAt(gfx.mojom.PointF
> point) => (int32 frame_id);
> On 2017/06/27 16:37:28, riajiang wrote:
> > On 2017/06/26 18:29:52, rjkroege wrote:
> > > What is your expectation of the coordinate system of point? I've been
> > presuming
> > > that the CTS of point is the CTS of the target frame?
> > > 
> > > API seems reasonable to me.
> > 
> > Input |point| would be in display-coordinate-pixels (or I can convert it to
> > screen-coordinates before asking blink) and we need to update it to be in
> > target's CTS after we found the target |frame_id|?
> 
> display coordinate pixels is  likely wrong. I claim x,y w.r.t. the target
frame

I'm confused - we only know the target frame after? Which means x,y cannot be
w.r.t the target frame when HitTestFrameAt first receive it? Did I misunderstand
sth?

[Navid Zolghadr, 2017-06-28 19:25:06]

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
File content/common/input/input_handler.mojom (right):

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:62: HitTestFrameAt(gfx.mojom.PointF
point) => (int32 frame_id);
On 2017/06/28 18:40:19, rjkroege wrote:
> On 2017/06/27 16:37:28, riajiang wrote:
> > On 2017/06/26 18:29:52, rjkroege wrote:
> > > What is your expectation of the coordinate system of point? I've been
> > presuming
> > > that the CTS of point is the CTS of the target frame?
> > > 
> > > API seems reasonable to me.
> > 
> > Input |point| would be in display-coordinate-pixels (or I can convert it to
> > screen-coordinates before asking blink) and we need to update it to be in
> > target's CTS after we found the target |frame_id|?
> 
> display coordinate pixels is  likely wrong. I claim x,y w.r.t. the target
frame

Essentially you are asking frame X to tell you the target frame of a point. That
point should be translated to the coordinate space of Frame X. Meaning that if
Frame X was rotated in its parent then it has no idea that it happened and you
need to transform the event accordingly for that frame X understands it.

[bokan, 2017-06-29 16:05:44]

Description was changed from

==========
Add Mojo API for Blink hit testing

Add Mojo API in the same place as other input
mojo APIs. So to be able to have this API
enabled kMojoInputMessages as the hit testing
will also go through the same path.

BUG=732405
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add Mojo API for Blink hit testing

Add Mojo API in the same place as other input
mojo APIs. So to be able to have this API
enabled kMojoInputMessages as the hit testing
will also go through the same path.

BUG=732405
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Navid Zolghadr, 2017-07-17 18:54:46]

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
File content/common/input/input_handler.mojom (right):

https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
content/common/input/input_handler.mojom:61: // Returns the hit-tested remote
frame id for the given coordinates.
On 2017/06/26 18:29:52, rjkroege wrote:
> how are we suppose to use this API from ui/ws? Couldn't this be a component?

riajiang@ I believe you have written some code that expect to use this code
based on what Robert said before. Can you point me to that code so I can work on
exposing this API to that part of the code?

[riajiang, 2017-07-18 18:18:28]

On 2017/07/17 18:54:46, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
> File content/common/input/input_handler.mojom (right):
> 
>
https://codereview.chromium.org/2951053005/diff/1/content/common/input/input_...
> content/common/input/input_handler.mojom:61: // Returns the hit-tested remote
> frame id for the given coordinates.
> On 2017/06/26 18:29:52, rjkroege wrote:
> > how are we suppose to use this API from ui/ws? Couldn't this be a component?
> 
> riajiang@ I believe you have written some code that expect to use this code
> based on what Robert said before. Can you point me to that code so I can work
on
> exposing this API to that part of the code?

As talked offline, replied in a previous comment (#12).

[bokan, 2017-07-18 19:26:34]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2017-07-18 19:26:35]

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:125: virtual WebFrame*
HitTestFrameAt(const WebFloatPoint& point) {
Would we generalise this a little? Instead of hit testing for Frames, Scroll
Unification
https://docs.google.com/document/d/1op5USoxDnN6yxB8EiFHYcGHacrZZRVKVqu4mSXFd6...)
will want the actual ElementId. Could we instead call this HitTestAt and return
a WebHitTestResult, possibly adding a method on there to return the WebFrame we
hit? I could then reuse this for the unification work.

[Navid Zolghadr, 2017-07-18 19:34:14]

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:125: virtual WebFrame*
HitTestFrameAt(const WebFloatPoint& point) {
On 2017/07/18 19:26:35, bokan wrote:
> Would we generalise this a little? Instead of hit testing for Frames, Scroll
> Unification
>
https://docs.google.com/document/d/1op5USoxDnN6yxB8EiFHYcGHacrZZRVKVqu4mSXFd6...)
> will want the actual ElementId. Could we instead call this HitTestAt and
return
> a WebHitTestResult, possibly adding a method on there to return the WebFrame
we
> hit? I could then reuse this for the unification work.

I had something like that in mind. But I saw no method on WebHitTestResult to
return the frame thing and I didn't think it was worth digging more into. Now
that it seems it has a legit use case I'll take a closer look to see whether I
can manage with returning a WebHitTestResult.

[bokan, 2017-07-18 19:39:43]

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2951053005/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebWidget.h:125: virtual WebFrame*
HitTestFrameAt(const WebFloatPoint& point) {
On 2017/07/18 19:34:14, Navid Zolghadr wrote:
> On 2017/07/18 19:26:35, bokan wrote:
> > Would we generalise this a little? Instead of hit testing for Frames, Scroll
> > Unification
> >
>
https://docs.google.com/document/d/1op5USoxDnN6yxB8EiFHYcGHacrZZRVKVqu4mSXFd6...)
> > will want the actual ElementId. Could we instead call this HitTestAt and
> return
> > a WebHitTestResult, possibly adding a method on there to return the WebFrame
> we
> > hit? I could then reuse this for the unification work.
> 
> I had something like that in mind. But I saw no method on WebHitTestResult to
> return the frame thing and I didn't think it was worth digging more into. Now
> that it seems it has a legit use case I'll take a closer look to see whether I
> can manage with returning a WebHitTestResult.

Great, thanks! From the WebNode you can get a WebDocument which can give you the
WebLocalFrame. Alternatively you can just add a method right on
WebHitTestResult.

[Navid Zolghadr, 2017-08-09 16:08:21]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org

[Navid Zolghadr, 2017-08-09 16:08:23]

ptal

https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
File components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc (right):

https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc:78: this,
nullptr, frame_sink_id_, is_root,
There are some places like this that were not being reached as the result of
CreateForWidget. So I wouldn't have the hit test interface instance. I wasn't
sure what to do with these and when each of those cases happen. Any idea?

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
File services/viz/public/interfaces/hit_test/input_hit_test.mojom (right):

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
services/viz/public/interfaces/hit_test/input_hit_test.mojom:1: module
viz.mojom;
Is this place alright for the mojom file?

[Navid Zolghadr, 2017-08-14 15:14:54]

On 2017/08/09 16:08:23, Navid Zolghadr wrote:
> ptal
> 
>
https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
> File components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc
(right):
> 
>
https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
> components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc:78: this,
> nullptr, frame_sink_id_, is_root,
> There are some places like this that were not being reached as the result of
> CreateForWidget. So I wouldn't have the hit test interface instance. I wasn't
> sure what to do with these and when each of those cases happen. Any idea?
> 
>
https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
> File services/viz/public/interfaces/hit_test/input_hit_test.mojom (right):
> 
>
https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
> services/viz/public/interfaces/hit_test/input_hit_test.mojom:1: module
> viz.mojom;
> Is this place alright for the mojom file?

ping

[rjkroege, 2017-08-14 18:25:27]

On 2017/08/14 15:14:54, Navid Zolghadr wrote:
> On 2017/08/09 16:08:23, Navid Zolghadr wrote:
> > ptal
> > 
> >
>
https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
> > File components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2951053005/diff/20001/components/viz/service/...
> > components/viz/service/frame_sinks/direct_layer_tree_frame_sink.cc:78: this,
> > nullptr, frame_sink_id_, is_root,
> > There are some places like this that were not being reached as the result of
> > CreateForWidget. So I wouldn't have the hit test interface instance. I
wasn't
> > sure what to do with these and when each of those cases happen. Any idea?
> > 
> >
>
https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
> > File services/viz/public/interfaces/hit_test/input_hit_test.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
> > services/viz/public/interfaces/hit_test/input_hit_test.mojom:1: module
> > viz.mojom;
> > Is this place alright for the mojom file?
> 
> ping

[rjkroege, 2017-08-14 18:26:58]

You don't plumb this into services/ui in a way that I'd expect? Or is that a
future CL?

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
File services/viz/public/interfaces/hit_test/input_hit_test.mojom (right):

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
services/viz/public/interfaces/hit_test/input_hit_test.mojom:1: module
viz.mojom;
On 2017/08/09 16:08:22, Navid Zolghadr wrote:
> Is this place alright for the mojom file?

We discussed collectively. In the short term, having this here is (reluctantly)
acceptable but in the fullness of time when all Chrome implementations use the
services/ui window server (i.e. mus), the interface should get relocated to
services/ui/public/

The reasoning behind this is that this interface is part (or associated with)
the WindowTreeClient interface. In a  mus context, a client would specify this
interface via its WindowTree interface (exact plumbing to be worked out over
time.)

So: please add an appropriate TODO

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
services/viz/public/interfaces/hit_test/input_hit_test.mojom:6: interface
FrameInputHitTest {
I am opposed to this name. The "Test" suffix suggests test code. I called this
"immediate" mode in the initial design doc but it's actually a delegate pattern
so I'm going to suggest calling this something like TargetFrameForInputDelegate
(and file name should match.)

[Navid Zolghadr, 2017-08-14 19:20:03]

nzolghadr@chromium.org changed reviewers:
+ sadrul@chromium.org

[Navid Zolghadr, 2017-08-18 15:29:49]

ptal.

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
File services/viz/public/interfaces/hit_test/input_hit_test.mojom (right):

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
services/viz/public/interfaces/hit_test/input_hit_test.mojom:1: module
viz.mojom;
On 2017/08/14 18:26:58, rjkroege wrote:
> On 2017/08/09 16:08:22, Navid Zolghadr wrote:
> > Is this place alright for the mojom file?
> 
> We discussed collectively. In the short term, having this here is
(reluctantly)
> acceptable but in the fullness of time when all Chrome implementations use the
> services/ui window server (i.e. mus), the interface should get relocated to
> services/ui/public/
> 
> The reasoning behind this is that this interface is part (or associated with)
> the WindowTreeClient interface. In a  mus context, a client would specify this
> interface via its WindowTree interface (exact plumbing to be worked out over
> time.)
> 
> So: please add an appropriate TODO

Done.

https://codereview.chromium.org/2951053005/diff/20001/services/viz/public/int...
services/viz/public/interfaces/hit_test/input_hit_test.mojom:6: interface
FrameInputHitTest {
On 2017/08/14 18:26:58, rjkroege wrote:
> I am opposed to this name. The "Test" suffix suggests test code. I called this
> "immediate" mode in the initial design doc but it's actually a delegate
pattern
> so I'm going to suggest calling this something like
TargetFrameForInputDelegate
> (and file name should match.)

Done.

[Navid Zolghadr, 2017-08-25 19:26:35]

ping.

I can certainly push the whole change to Gerrit if it is easier for people and
if you don't care about previous comments on this review.