Bug 109156 – Factor Event retargeting code.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Hayato Ito]

To support Touch Event retargeting (bug 107800), we have to factor current code base which handles Event retargeting.

The current code base is tightly integrated with a MouseEvent. We have to factor it nicely so that we can support Touch Event retargeting.

[Hayato Ito]

Created attachment 187029 [details]
Factor event retargeting

[Dimitri Glazkov (Google)]

Comment on attachment 187029 [details]
Factor event retargeting

View in context: https://bugs.webkit.org/attachment.cgi?id=187029&action=review

I like the direction, but isn't this two patches in one?
1) introduces a (really nice abstraction of) MouseEventContext
2) splits retargeting off into two bits.
Also, EWS: Y SO PURPLE!?

> Source/WebCore/ChangeLog:22
> +        This patch make them separeted to make each intention clear.

separeted -> separated

[Alexey Proskuryakov]

Comment on attachment 187029 [details]
Factor event retargeting

View in context: https://bugs.webkit.org/attachment.cgi?id=187029&action=review

> Source/WebCore/dom/EventContext.cpp:63
> +MouseEventContext::MouseEventContext(PassRefPtr<Node> node, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target)

And so on.

> Source/WebCore/dom/EventContext.h:66
> +    MouseEventContext(PassRefPtr<Node>, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target);

These should not be PassRefPtrs, because ownership is not being passed.

> Source/WebCore/dom/EventContext.h:116
> +inline void MouseEventContext::setRelatedTarget(PassRefPtr<EventTarget> relatedTarget)

Ditto.

[Hayato Ito]

Thank you for the review.

(In reply to comment #2)
> (From update of attachment 187029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187029&action=review
> 
> I like the direction, but isn't this two patches in one?
> 1) introduces a (really nice abstraction of) MouseEventContext
> 2) splits retargeting off into two bits.

Okay. Let me separate this into some patches.

> Also, EWS: Y SO PURPLE!?
> 
> > Source/WebCore/ChangeLog:22
> > +        This patch make them separeted to make each intention clear.
> 
> separeted -> separated

Let me fix it.

[Hayato Ito]

(In reply to comment #3)
> (From update of attachment 187029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187029&action=review
> 
> > Source/WebCore/dom/EventContext.cpp:63
> > +MouseEventContext::MouseEventContext(PassRefPtr<Node> node, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target)
> 
> And so on.
> 
> > Source/WebCore/dom/EventContext.h:66
> > +    MouseEventContext(PassRefPtr<Node>, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target);
> 
> These should not be PassRefPtrs, because ownership is not being passed.

Good point. But I think EventContext (and MouseEventContext) needs to have an ownership of a node.

If EventContext does not have an ownership, I am afraid that EventContext will have a pointer to a node which is freed if the node is deleted by an event listener on other EventContext.

I'd like to hear an opinion from Dimitri, the original author of EventContext, I guess.


> 
> > Source/WebCore/dom/EventContext.h:116
> > +inline void MouseEventContext::setRelatedTarget(PassRefPtr<EventTarget> relatedTarget)
> 
> Ditto.

[Dimitri Glazkov (Google)]

(In reply to comment #5)

> I'd like to hear an opinion from Dimitri, the original author of EventContext, I guess.

I would just do what Alexey suggests.

[Darin Adler]

Comment on attachment 187029 [details]
Factor event retargeting

I know that smaller pieces are supposed to be easier to understand, but the event code is getting harder and harder for me to understand since we made the mediator changes. Basic event dispatch is now a maze of classes with names I don’t understand, whereas it used to be a one-page function that easily fit on my computer screen. What can we do about this?

[Hayato Ito]

Thank you for the comment, Darin!

(In reply to comment #7)
> (From update of attachment 187029 [details])
> I know that smaller pieces are supposed to be easier to understand, but the event code is getting harder and harder for me to understand since we made the mediator changes. Basic event dispatch is now a maze of classes with names I don’t understand, whereas it used to be a one-page function that easily fit on my computer screen. What can we do about this?

I understand your concerns. Basic event dispatch should not be complex than necessary!
The most of complexity comes from Event retargeting.
Spec: https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#events

I am afraid that the algorithm descriped in the spec is not easy to understand. I don't have a nice idea which makes the algorithm simpler.

So what we can do is to recover good old simple EventDispatcher.{h,cpp}. Factoring event disptch code might be helpful to separate concerns.

I don't like the current code of EventDispacher.cpp since it contains event retargeting code, which makes readers uncomfortable. One of the motivation of this patch is to make EventDispacher.{h,cpp} simple one , as described in http://dom.spec.whatwg.org/#dispatching-events.

Let me think further what we can do better.

[Hayato Ito]

Created attachment 188277 [details]
Rabased.

[WebKit Review Bot]

Comment on attachment 188277 [details]
Rabased.

Attachment 188277 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16514544

[Hayato Ito]

Created attachment 188298 [details]
Fix a regression.

[Dimitri Glazkov (Google)]

Comment on attachment 188298 [details]
Fix a regression.

View in context: https://bugs.webkit.org/attachment.cgi?id=188298&action=review

> Source/WebCore/dom/FocusEvent.cpp:96
> +    EventRetargeter::adjustForFocusEvent(dispatcher->node(), *event(), dispatcher->ensureEventPath(event()));

This is a bit awkward. The ensureEventPath calls EventRetargeter method, whose results are then passed back into another EventRetargeter method. Basically EventDispatcher is just a RAII wrapper for event path here.

[Hayato Ito]

Comment on attachment 188298 [details]
Fix a regression.

Thank you for the review.

View in context: https://bugs.webkit.org/attachment.cgi?id=188298&action=review

>> Source/WebCore/dom/FocusEvent.cpp:96
>> +    EventRetargeter::adjustForFocusEvent(dispatcher->node(), *event(), dispatcher->ensureEventPath(event()));
> 
> This is a bit awkward. The ensureEventPath calls EventRetargeter method, whose results are then passed back into another EventRetargeter method. Basically EventDispatcher is just a RAII wrapper for event path here.

Agreed. Before working on constructing event path in EventDispatcher's constructor, I'd like to check whether it's okay or not to do so.
Let me work on that in a following patch.

[WebKit Review Bot]

Comment on attachment 188298 [details]
Fix a regression.

Clearing flags on attachment: 188298

Committed r142957: <http://trac.webkit.org/changeset/142957>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Windows build fix landed in http://trac.webkit.org/changeset/142959.