Bug 57168 - Introduce EventDispatcher, the new common way to fire events.
Summary: Introduce EventDispatcher, the new common way to fire events.
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
  Show dependency treegraph
 
Reported: 2011-03-26 14:41 PDT by Dimitri Glazkov (Google)
Modified: 2011-03-28 10:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch (45.00 KB, patch)
2011-03-26 14:50 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (44.22 KB, patch)
2011-03-26 20:09 PDT, Dimitri Glazkov (Google)
eric: review+
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-26 14:41:51 PDT
Introduce EventDispatcher, the new common way to fire events.
Comment 1 Dimitri Glazkov (Google) 2011-03-26 14:50:09 PDT
Created attachment 87048 [details]
Patch
Comment 2 Sam Weinig 2011-03-26 20:04:31 PDT
Given how much of EventDispatcher.cpp comes from Node.cpp, it seems like it should probably use Node.cpp's license header.
Comment 3 Dimitri Glazkov (Google) 2011-03-26 20:06:05 PDT
(In reply to comment #2)
> Given how much of EventDispatcher.cpp comes from Node.cpp, it seems like it should probably use Node.cpp's license header.

Ah, good point.
Comment 4 Dimitri Glazkov (Google) 2011-03-26 20:09:23 PDT
Created attachment 87053 [details]
Patch
Comment 5 Dimitri Glazkov (Google) 2011-03-26 20:10:22 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Given how much of EventDispatcher.cpp comes from Node.cpp, it seems like it should probably use Node.cpp's license header.
> 
> Ah, good point.

License header updated. I also changed one for .h, because why not.
Comment 6 Dimitri Glazkov (Google) 2011-03-26 21:22:08 PDT
Also related: bug 56410 about EventHandler cleanup.
Comment 7 Eric Seidel (no email) 2011-03-27 01:03:17 PDT
Comment on attachment 87053 [details]
Patch

I really like this idea.  Is this this moving code?  Or do I need to carefullly review each line?
Comment 8 Eric Seidel (no email) 2011-03-27 01:04:17 PDT
Comment on attachment 87053 [details]
Patch

This is a depressing amount of header imports, btw.  Suggesting that this class is still knows more than it should.
Comment 9 Dimitri Glazkov (Google) 2011-03-27 07:14:50 PDT
(In reply to comment #7)
> (From update of attachment 87053 [details])
> I really like this idea.  Is this this moving code?  Or do I need to carefullly review each line?

This is mostly just moving code.
Comment 10 Dimitri Glazkov (Google) 2011-03-27 07:15:50 PDT
(In reply to comment #8)
> (From update of attachment 87053 [details])
> This is a depressing amount of header imports, btw.  Suggesting that this class is still knows more than it should.

Yeah. I've been experimenting with something like EventDispatcher<Event>, where Events has lifecycle hooks that are called at specific times, but it looks fuglier.
Comment 11 Dimitri Glazkov (Google) 2011-03-27 07:46:16 PDT
Comment on attachment 87053 [details]
Patch

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

> Source/WebCore/dom/EventDispatcher.cpp:150
> +    if (!m_node->inDocument() || m_ancestors.size())

This is the only non-obvious bit. Some events come in troves, with multiple events dispatched as a result of one "dispatchFoo". Because they are now scoped to the same EventDispatcher, the ancestor chain calculation happens only once (yay!). So we need this extra check for size.
Comment 12 Eric Seidel (no email) 2011-03-28 09:24:12 PDT
Comment on attachment 87053 [details]
Patch

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

>> Source/WebCore/dom/EventDispatcher.cpp:150
>> +    if (!m_node->inDocument() || m_ancestors.size())
> 
> This is the only non-obvious bit. Some events come in troves, with multiple events dispatched as a result of one "dispatchFoo". Because they are now scoped to the same EventDispatcher, the ancestor chain calculation happens only once (yay!). So we need this extra check for size.

Maybe this needs a comment?
Comment 13 Dimitri Glazkov (Google) 2011-03-28 09:36:50 PDT
(In reply to comment #12)
> (From update of attachment 87053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87053&action=review
> 
> >> Source/WebCore/dom/EventDispatcher.cpp:150
> >> +    if (!m_node->inDocument() || m_ancestors.size())
> > 
> > This is the only non-obvious bit. Some events come in troves, with multiple events dispatched as a result of one "dispatchFoo". Because they are now scoped to the same EventDispatcher, the ancestor chain calculation happens only once (yay!). So we need this extra check for size.
> 
> Maybe this needs a comment?

Even better, I'll turn it into a function explaining what's going on.
Comment 14 Dimitri Glazkov (Google) 2011-03-28 10:01:31 PDT
Committed r82127: <http://trac.webkit.org/changeset/82127>
Comment 15 WebKit Review Bot 2011-03-28 10:13:57 PDT
http://trac.webkit.org/changeset/82127 might have broken Qt Linux Release