Bug 57562

Created attachment 87760 [details] EventManager sketch.

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.

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?

(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?

(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

(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! :)

Created attachment 87813 [details] Patch

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

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

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