Issue 823873004: Add basic PointerEvent support in NewEventHandler

Issue 823873004: Add basic PointerEvent support in NewEventHandler (Closed)

Created:
5 years, 11 months ago by abarth-chromium

Modified:
5 years, 11 months ago

Reviewers:
eseidel, esprehn, ojan

CC:
mojo-reviews_chromium.org, ojan

Base URL:
git@github.com:domokit/mojo.git@master

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Add basic PointerEvent support in NewEventHandler blink::EventHandler is really hairy and deals with a lot of complex cases that don't matter for Sky. This CL starts a NewEventHandler and adds support for PointerEvents there. The NewEventHandler will eventually replace EventHandler once we've actually migrated over from Mouse+Touch events to PointerEvents. R=esprehn@chromium.org, ojan@chromium.org, eseidel@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/a85a2cea82d816de115e15253742b0f88a9924eb

Patch Set 1 #

Patch Set 2 : Actually add NewEventHandler #

Total comments: 26

Patch Set 3 : Address reviewer comments #

Total comments: 4

Patch Set 4 : Address more reviewer comments #

Patch Set 5 : git cl try #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+220 lines, -16 lines)			Patch
M	sky/engine/core/core.gni	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
A	sky/engine/core/frame/NewEventHandler.h	View	1 2 3	1 chunk	+47 lines, -0 lines	0 comments	Download
A	sky/engine/core/frame/NewEventHandler.cpp	View	1 2 3 4	1 chunk	+152 lines, -0 lines	0 comments	Download
M	sky/engine/core/rendering/RenderView.cpp	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	sky/engine/public/platform/WebInputEvent.h	View		1 chunk	+16 lines, -16 lines	0 comments	Download

Messages

Total messages: 14 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

esprehn

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/NewEventHandler.cpp File sky/engine/core/frame/NewEventHandler.cpp (right): https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/NewEventHandler.cpp#newcode48 sky/engine/core/frame/NewEventHandler.cpp:48: Node* NewEventHandler::targetForHitTestResult(const HitTestResult& hitTestResult) This can really be far ...

5 years, 11 months ago (2015-01-20 18:14:33 UTC) #4

abarth-chromium

5 years, 11 months ago (2015-01-20 18:26:55 UTC) #5

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
File sky/engine/core/frame/NewEventHandler.cpp (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:48: Node*
NewEventHandler::targetForHitTestResult(const HitTestResult& hitTestResult)
On 2015/01/20 at 18:14:33, esprehn wrote:
> This can really be far more specific, we can fix it later though. innerNode()
is either Document, Element or Text. I'm not really sure it can be Document
either.

You're saying we can change the return type to ContainerNode?

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:61: if (!m_frame->contentRenderer() ||
(m_frame->view() &&
!m_frame->view()->visibleContentRect().contains(roundedIntPoint(point))))
On 2015/01/20 at 18:14:33, esprehn wrote:
> view() and RenderView (contentRenderer) should live and die together, checking
them separately seems weird.

Ok.  I cargo culted this from the old code.  Will change.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:63:
m_frame->contentRenderer()->hitTest(HitTestRequest(HitTestRequest::ReadOnly),
result);
On 2015/01/20 at 18:14:33, esprehn wrote:
> Why doesn't hitTest() should do the above contains() check for you, callers
shouldn't need to cull.

It looks like it doesn't do this because it's not always the case that your
children are contained within you.  That's a special case for the RenderView. 
We could move this check inside RenderView if you'd like.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:107: if (event.type ==
WebInputEvent::PointerMove)
On 2015/01/20 at 18:14:32, esprehn wrote:
> switch() then the compiler will shout if you miss cases.

Here |type| comes from the base class WebInputEvent and contains all the event
types.  I can change this to a switch, but we'd still have a |default| case to
handle KeyPress and such.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:135: eventSwallowed |=
!dispatchClickEvent(target.get(), event);
On 2015/01/20 at 18:14:33, esprehn wrote:
> I find |= really hard to catch when reading the code. Can we instead do:
> 
> if (!dispatchClickEvent(target.get(), event))
>   eventSwalloed = true;

Sure!

> Also having the booleans switch like this is super weird. handle*() returns
true if the event was swallowed, but the dispatch*() methods return true if the
event _wasn't_ swallowed? That always made the original EventHandler hard to
understand.

Yeah, it's super confusing.  Do you want me to switch that around?  It will take
a while to clean out all the inconsistencies...  Another choice is to wait until
we've switched over to NewEventHandler and then remove all the negations
together.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/public/platfo...
File sky/engine/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/public/platfo...
sky/engine/public/platform/WebInputEvent.h:253: float tilt = 0;
On 2015/01/20 at 18:14:33, esprehn wrote:
> Why do we not want double precision?

My understanding is that |float| is more consistent with the actual precision of
the underlying sensors.  I can change them all to double if you'd prefer, but I
don't think the extra precision actually reflects anything in reality.

abarth-chromium

PTAL https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/NewEventHandler.cpp File sky/engine/core/frame/NewEventHandler.cpp (right): https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/NewEventHandler.cpp#newcode61 sky/engine/core/frame/NewEventHandler.cpp:61: if (!m_frame->contentRenderer() || (m_frame->view() && !m_frame->view()->visibleContentRect().contains(roundedIntPoint(point)))) On 2015/01/20 ...

5 years, 11 months ago (2015-01-20 21:37:56 UTC) #6

esprehn

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/PointerEvent.h File sky/engine/core/events/PointerEvent.h (right): https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/PointerEvent.h#newcode80 sky/engine/core/events/PointerEvent.h:80: void setPrimary(bool primary) { m_primary = primary; } Does ...

5 years, 11 months ago (2015-01-21 02:47:52 UTC) #7

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
File sky/engine/core/events/PointerEvent.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
sky/engine/core/events/PointerEvent.h:80: void setPrimary(bool primary) {
m_primary = primary; }
Does this change dynamically? What happens if two events claim they're the
primary?

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
File sky/engine/core/frame/NewEventHandler.cpp (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:48: Node*
NewEventHandler::targetForHitTestResult(const HitTestResult& hitTestResult)
On 2015/01/20 at 18:26:54, abarth wrote:
> On 2015/01/20 at 18:14:33, esprehn wrote:
> > This can really be far more specific, we can fix it later though.
innerNode() is either Document, Element or Text. I'm not really sure it can be
Document either.
> 
> You're saying we can change the return type to ContainerNode?

Yes.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:63:
m_frame->contentRenderer()->hitTest(HitTestRequest(HitTestRequest::ReadOnly),
result);
On 2015/01/20 at 18:26:54, abarth wrote:
> On 2015/01/20 at 18:14:33, esprehn wrote:
> > Why doesn't hitTest() should do the above contains() check for you, callers
shouldn't need to cull.
> 
> It looks like it doesn't do this because it's not always the case that your
children are contained within you.  That's a special case for the RenderView. 
We could move this check inside RenderView if you'd like.

RenderView::hitTest is special, it's not a virtual thing. I'd put it in there.
All callers want this optimization. Actually if you look at hitTestInDocument
you'll see it already has copy pasta of this. :(

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:107: if (event.type ==
WebInputEvent::PointerMove)
On 2015/01/20 at 18:26:54, abarth wrote:
> On 2015/01/20 at 18:14:32, esprehn wrote:
> > switch() then the compiler will shout if you miss cases.
> 
> Here |type| comes from the base class WebInputEvent and contains all the event
types.  I can change this to a switch, but we'd still have a |default| case to
handle KeyPress and such.

There's no default cases in this function though, you have an ASSERT. So your
switch has an ASSERT_NOT_REACHED() for all the types this function can't handle.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:135: eventSwallowed |=
!dispatchClickEvent(target.get(), event);
On 2015/01/20 at 18:26:54, abarth wrote:
> On 2015/01/20 at 18:14:33, esprehn wrote:
> > I find |= really hard to catch when reading the code. Can we instead do:
> > 
> > if (!dispatchClickEvent(target.get(), event))
> >   eventSwalloed = true;
> 
> Sure!
> 
> > Also having the booleans switch like this is super weird. handle*() returns
true if the event was swallowed, but the dispatch*() methods return true if the
event _wasn't_ swallowed? That always made the original EventHandler hard to
understand.
> 
> Yeah, it's super confusing.  Do you want me to switch that around?  It will
take a while to clean out all the inconsistencies...  Another choice is to wait
until we've switched over to NewEventHandler and then remove all the negations
together.

Follow up cl.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
File sky/engine/core/frame/NewEventHandler.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.h:21: explicit
NewEventHandler(LocalFrame*);
ref &

https://codereview.chromium.org/823873004/diff/20001/sky/engine/public/platfo...
File sky/engine/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/public/platfo...
sky/engine/public/platform/WebInputEvent.h:253: float tilt = 0;
On 2015/01/20 at 18:26:54, abarth wrote:
> On 2015/01/20 at 18:14:33, esprehn wrote:
> > Why do we not want double precision?
> 
> My understanding is that |float| is more consistent with the actual precision
of the underlying sensors.  I can change them all to double if you'd prefer, but
I don't think the extra precision actually reflects anything in reality.

Float seems fine then.

esprehn

lgtm + nits and review comments from before. https://codereview.chromium.org/823873004/diff/40001/sky/engine/core/frame/NewEventHandler.h File sky/engine/core/frame/NewEventHandler.h (right): https://codereview.chromium.org/823873004/diff/40001/sky/engine/core/frame/NewEventHandler.h#newcode21 sky/engine/core/frame/NewEventHandler.h:21: explicit ...

5 years, 11 months ago (2015-01-21 02:49:06 UTC) #8

ojan

lgtm https://codereview.chromium.org/823873004/diff/40001/sky/engine/core/frame/NewEventHandler.cpp File sky/engine/core/frame/NewEventHandler.cpp (right): https://codereview.chromium.org/823873004/diff/40001/sky/engine/core/frame/NewEventHandler.cpp#newcode98 sky/engine/core/frame/NewEventHandler.cpp:98: m_frame->selection().setNonDirectionalSelectionIfNeeded(VisibleSelection(position), CharacterGranularity); Non direcitonal selections are a crazy-pants ...

5 years, 11 months ago (2015-01-21 03:39:28 UTC) #10

abarth-chromium

5 years, 11 months ago (2015-01-21 07:03:26 UTC) #11

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
File sky/engine/core/events/PointerEvent.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
sky/engine/core/events/PointerEvent.h:80: void setPrimary(bool primary) {
m_primary = primary; }
On 2015/01/21 at 02:47:51, esprehn wrote:
> Does this change dynamically?

Currently everything is marked as primary.  It's something that will be computed
by the EventHandler.  I can move it to the constructor if you like.

> What happens if two events claim they're the primary?

That can happen if they're being captured by different nodes.  For example, if
you have two touches on two separate scroll views, they both scroll
independently because each touch is primary for each view.  If you put a third
finger down in one of the views, it will become secondary in that view and can
be used for something else (e.g., pinching).

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
File sky/engine/core/frame/NewEventHandler.cpp (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:63:
m_frame->contentRenderer()->hitTest(HitTestRequest(HitTestRequest::ReadOnly),
result);
On 2015/01/21 at 02:47:51, esprehn wrote:
> RenderView::hitTest is special, it's not a virtual thing. I'd put it in there.
All callers want this optimization. Actually if you look at hitTestInDocument
you'll see it already has copy pasta of this. :(

Ah!  Will do.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/Ne...
sky/engine/core/frame/NewEventHandler.cpp:107: if (event.type ==
WebInputEvent::PointerMove)
On 2015/01/21 at 02:47:51, esprehn wrote:
> There's no default cases in this function though, you have an ASSERT. So your
switch has an ASSERT_NOT_REACHED() for all the types this function can't handle.

I can do that, but if we don't use |default| the switch will have a couple dozen
entries in order to handle four of them.

esprehn

On 2015/01/21 at 07:03:26, abarth wrote: > ... > > https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/frame/NewEventHandler.cpp#newcode107 > sky/engine/core/frame/NewEventHandler.cpp:107: if (event.type ...

5 years, 11 months ago (2015-01-22 01:26:31 UTC) #12

abarth-chromium

Committed patchset #5 (id:80001) manually as a85a2cea82d816de115e15253742b0f88a9924eb (presubmit successful).

5 years, 11 months ago (2015-01-22 20:54:33 UTC) #13

abarth-chromium

5 years, 11 months ago (2015-01-23 02:25:09 UTC) #14

Message was sent while issue was closed.

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
File sky/engine/core/events/PointerEvent.h (right):

https://codereview.chromium.org/823873004/diff/20001/sky/engine/core/events/P...
sky/engine/core/events/PointerEvent.h:80: void setPrimary(bool primary) {
m_primary = primary; }
I removed this for now.  When we have more of this system implemented, we can
add it back.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages