Bug 57168

Summary: Introduce EventDispatcher, the new common way to fire events.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: New BugsAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 55515    
Attachments:
Description Flags
Patch
none
Patch eric: review+

Dimitri Glazkov (Google)
Reported 2011-03-26 14:41:51 PDT
Introduce EventDispatcher, the new common way to fire events.
Attachments
Patch (45.00 KB, patch)
2011-03-26 14:50 PDT, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
Patch (44.22 KB, patch)
2011-03-26 20:09 PDT, Dimitri Glazkov (Google) 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2011-03-26 14:50:09 PDT
Created attachment 87048 [details] Patch
Sam Weinig
Comment 2 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.
Dimitri Glazkov (Google)
Comment 3 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.
Dimitri Glazkov (Google)
Comment 4 2011-03-26 20:09:23 PDT
Created attachment 87053 [details] Patch
Dimitri Glazkov (Google)
Comment 5 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.
Dimitri Glazkov (Google)
Comment 6 2011-03-26 21:22:08 PDT
Also related: bug 56410 about EventHandler cleanup.
Eric Seidel (no email)
Comment 7 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?
Eric Seidel (no email)
Comment 8 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.
Dimitri Glazkov (Google)
Comment 9 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.
Dimitri Glazkov (Google)
Comment 10 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.
Dimitri Glazkov (Google)
Comment 11 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.
Eric Seidel (no email)
Comment 12 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?
Dimitri Glazkov (Google)
Comment 13 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.
Dimitri Glazkov (Google)
Comment 14 2011-03-28 10:01:31 PDT
Committed r82127: <http://trac.webkit.org/changeset/82127>
WebKit Review Bot
Comment 15 2011-03-28 10:13:57 PDT
http://trac.webkit.org/changeset/82127 might have broken Qt Linux Release
Note You need to log in before you can comment on or make changes to this bug.