Bug 57562 – Introduce EventDispatchMediator abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.

[Dimitri Glazkov (Google)]

Created attachment 87760 [details]
EventManager sketch.

[Darin Adler]

Comment on attachment 87760 [details]
EventManager sketch.

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

> Source/WebCore/ChangeLog:6
> +        Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
> +        https://bugs.webkit.org/show_bug.cgi?id=57562

I like the idea of factoring like this.

I do not like the name “manager”. Having objects that are “managers” is something of a class design anti-pattern.

We should think about other names and get a more specific sense of what this class is so we can name it well.

[Dimitri Glazkov (Google)]

Event and KeyboardEvent are converted to use the abstraction in this patch. It's not terribly exciting, but it will get more interesting with MouseEventManager, WheelEventManager, and SimulatedClickEventManager.

WDYT?

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> (From update of attachment 87760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87760&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
> > +        https://bugs.webkit.org/show_bug.cgi?id=57562
> 
> I like the idea of factoring like this.
> 
> I do not like the name “manager”. Having objects that are “managers” is something of a class design anti-pattern.
> 
> We should think about other names and get a more specific sense of what this class is so we can name it well.

Mediator?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 87760 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87760&action=review
> > 
> > > Source/WebCore/ChangeLog:6
> > > +        Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
> > > +        https://bugs.webkit.org/show_bug.cgi?id=57562
> > 
> > I like the idea of factoring like this.
> > 
> > I do not like the name “manager”. Having objects that are “managers” is something of a class design anti-pattern.
> > 
> > We should think about other names and get a more specific sense of what this class is so we can name it well.
> 
> Mediator?

http://en.wikipedia.org/wiki/Mediator_pattern

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 87760 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=87760&action=review
> > > 
> > > > Source/WebCore/ChangeLog:6
> > > > +        Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
> > > > +        https://bugs.webkit.org/show_bug.cgi?id=57562
> > > 
> > > I like the idea of factoring like this.
> > > 
> > > I do not like the name “manager”. Having objects that are “managers” is something of a class design anti-pattern.
> > > 
> > > We should think about other names and get a more specific sense of what this class is so we can name it well.
> > 
> > Mediator?
> 
> http://en.wikipedia.org/wiki/Mediator_pattern

It's an EventDispatchMediator, which is a mouthful. Maybe Sam can shine his moniker-smithing brilliance at us? BTW, I am sorry I forgot to tell Dominic to add the test to wk2/Skipped list! :)

[Dimitri Glazkov (Google)]

Created attachment 87813 [details]
Patch

[Adam Barth]

Comment on attachment 87813 [details]
Patch

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

> Source/WebCore/dom/Event.h:196
> +class EventDispatchMediator {

We usually have one class per file.  However, EventDispatchMediator looks pretty trivial, so maybe it makes sense to keep here...

> Source/WebCore/dom/Event.h:198
> +    EventDispatchMediator(PassRefPtr<Event>);

Please add the explicit keyword.  The style checker should really complain about that.

> Source/WebCore/dom/KeyboardEvent.h:119
> +    KeyboardEventDispatchMediator(PassRefPtr<KeyboardEvent>);

explicit

[Adam Barth]

https://bugs.webkit.org/show_bug.cgi?id=57791 is the check-webkit-style bug.

[Dimitri Glazkov (Google)]

Committed r82891: <http://trac.webkit.org/changeset/82891>