| Summary: | Introduce EventDispatcher, the new common way to fire events. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||
| Component: | New Bugs | Assignee: | 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
Dimitri Glazkov (Google)
2011-03-26 14:41:51 PDT
Created attachment 87048 [details]
Patch
Given how much of EventDispatcher.cpp comes from Node.cpp, it seems like it should probably use Node.cpp's license header. (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. Created attachment 87053 [details]
Patch
(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. Also related: bug 56410 about EventHandler cleanup. 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 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.
(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. (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 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 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? (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. Committed r82127: <http://trac.webkit.org/changeset/82127> http://trac.webkit.org/changeset/82127 might have broken Qt Linux Release |