Bug 57562 - Introduce EventDispatchMediator abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
Summary: Introduce EventDispatchMediator abstraction, which encapsulate all non-trivia...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 55515 57521 57639
  Show dependency treegraph
 
Reported: 2011-03-31 10:30 PDT by Dimitri Glazkov (Google)
Modified: 2011-04-04 16:23 PDT (History)
3 users (show)

See Also:


Attachments
EventManager sketch. (7.22 KB, patch)
2011-03-31 10:30 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2011-03-31 18:33 PDT, Dimitri Glazkov (Google)
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-03-31 10:30:01 PDT
Introduce EventManager abstraction, which encapsulate all non-trivial logic around firing a specific type of an event.
Comment 1 Dimitri Glazkov (Google) 2011-03-31 10:30:46 PDT
Created attachment 87760 [details]
EventManager sketch.
Comment 2 Darin Adler 2011-03-31 10:32:30 PDT
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.
Comment 3 Dimitri Glazkov (Google) 2011-03-31 10:32:47 PDT
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?
Comment 4 Dimitri Glazkov (Google) 2011-03-31 10:33:52 PDT
(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?
Comment 5 Dimitri Glazkov (Google) 2011-03-31 10:34:17 PDT
(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
Comment 6 Dimitri Glazkov (Google) 2011-03-31 10:46:50 PDT
(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! :)
Comment 7 Dimitri Glazkov (Google) 2011-03-31 18:33:21 PDT
Created attachment 87813 [details]
Patch
Comment 8 Adam Barth 2011-04-04 14:37:38 PDT
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
Comment 9 Adam Barth 2011-04-04 14:39:18 PDT
https://bugs.webkit.org/show_bug.cgi?id=57791 is the check-webkit-style bug.
Comment 10 Dimitri Glazkov (Google) 2011-04-04 16:23:15 PDT
Committed r82891: <http://trac.webkit.org/changeset/82891>