Bug 109156 - Factor Event retargeting code.
Summary: Factor Event retargeting code.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on: 109278 109561 109650
Blocks: 107800
  Show dependency treegraph
 
Reported: 2013-02-07 01:29 PST by Hayato Ito
Modified: 2013-02-14 21:57 PST (History)
9 users (show)

See Also:


Attachments
Factor event retargeting (43.98 KB, patch)
2013-02-07 02:38 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rabased. (38.50 KB, patch)
2013-02-14 00:45 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Fix a regression. (38.50 KB, patch)
2013-02-14 03:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2013-02-07 01:29:38 PST
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.
Comment 1 Hayato Ito 2013-02-07 02:38:24 PST
Created attachment 187029 [details]
Factor event retargeting
Comment 2 Dimitri Glazkov (Google) 2013-02-07 08:55:22 PST
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
Comment 3 Alexey Proskuryakov 2013-02-07 10:20:29 PST
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.
Comment 4 Hayato Ito 2013-02-07 18:45:08 PST
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.
Comment 5 Hayato Ito 2013-02-07 18:46:24 PST
(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.
Comment 6 Dimitri Glazkov (Google) 2013-02-07 20:14:54 PST
(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.
Comment 7 Darin Adler 2013-02-08 09:54:30 PST
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?
Comment 8 Hayato Ito 2013-02-11 22:01:40 PST
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.
Comment 9 Hayato Ito 2013-02-14 00:45:28 PST
Created attachment 188277 [details]
Rabased.
Comment 10 WebKit Review Bot 2013-02-14 01:04:22 PST
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
Comment 11 Hayato Ito 2013-02-14 03:24:52 PST
Created attachment 188298 [details]
Fix a regression.
Comment 12 Dimitri Glazkov (Google) 2013-02-14 09:21:44 PST
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.
Comment 13 Hayato Ito 2013-02-14 20:12:56 PST
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.
Comment 14 WebKit Review Bot 2013-02-14 20:40:21 PST
Comment on attachment 188298 [details]
Fix a regression.

Clearing flags on attachment: 188298

Committed r142957: <http://trac.webkit.org/changeset/142957>
Comment 15 WebKit Review Bot 2013-02-14 20:40:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryosuke Niwa 2013-02-14 21:57:26 PST
Windows build fix landed in http://trac.webkit.org/changeset/142959.