Move WheelEvent-dispatching logic into WheelEventDispatchMediator.
Created attachment 87901 [details] Patch
Created attachment 88274 [details] Ready to bake on EWSs.
Comment on attachment 88274 [details] Ready to bake on EWSs. View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) I guess you've not written the ChangelOg yet? > Source/WebCore/dom/Event.h:204 > + EventDispatchMediator(); What does this class mediate between?
(In reply to comment #3) > (From update of attachment 88274 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > I guess you've not written the ChangelOg yet? No, I just forgot to replace this line :) > > > Source/WebCore/dom/Event.h:204 > > + EventDispatchMediator(); > > What does this class mediate between? It mediates event dispatch. This class knows specifics of firing events, helping EventDispatcher do the right thing without having to know how.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 88274 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review > > > > > Source/WebCore/ChangeLog:8 > > > + No new tests. (OOPS!) > > > > I guess you've not written the ChangelOg yet? > > No, I just forgot to replace this line :) > > > > > > Source/WebCore/dom/Event.h:204 > > > + EventDispatchMediator(); > > > > What does this class mediate between? > > It mediates event dispatch. This class knows specifics of firing events, helping EventDispatcher do the right thing without having to know how. Believe it or not, the whole EventDispatchMediator was born out of your comment: you said the EventDispatcher knows too much :)
Comment on attachment 88274 [details] Ready to bake on EWSs. View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review > Source/WebCore/dom/Node.cpp:2665 > -void Node::dispatchWheelEvent(PlatformWheelEvent& e) > +bool Node::dispatchWheelEvent(PlatformWheelEvent& e) > { > - EventDispatcher::dispatchWheelEvent(this, e); > + return EventDispatcher::dispatchEvent(this, WheelEventDispatchMediator(e, document()->defaultView())); > } As long as you are touching this, I would prefer the name “event” to “e”. > Source/WebCore/dom/Node.h:536 > - void dispatchWheelEvent(PlatformWheelEvent&); > + bool dispatchWheelEvent(PlatformWheelEvent&); Is this really a good change? The DOM establishes a pattern where events themselves know if they have been handled. I think this is clearer than a boolean return value. It’s hard to name a function so you can understand what its boolean return value means. Worse, we have functions that tell whether an event is handled both with a return value and with state in the event too! Although this is not one of them. If you really ant to change this then I think it should take a const&. The reason this took a non-const before was so it could record that the event was handled. > Source/WebCore/dom/WheelEvent.cpp:104 > + : EventDispatchMediator() Do you need to say this explicitly? I am surprised if so.
Comment on attachment 88274 [details] Ready to bake on EWSs. View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review Darin, thank you for review. Will upload new patch shortly. >> Source/WebCore/dom/Node.cpp:2665 >> } > > As long as you are touching this, I would prefer the name “event” to “e”. Will change. >> Source/WebCore/dom/Node.h:536 >> + bool dispatchWheelEvent(PlatformWheelEvent&); > > Is this really a good change? > > The DOM establishes a pattern where events themselves know if they have been handled. I think this is clearer than a boolean return value. It’s hard to name a function so you can understand what its boolean return value means. > > Worse, we have functions that tell whether an event is handled both with a return value and with state in the event too! Although this is not one of them. > > If you really ant to change this then I think it should take a const&. The reason this took a non-const before was so it could record that the event was handled. This just brings it in line with other similar events. Both dispatchEvent and dispatchKeyEvent return false when they weren't handled. I definitely overlooked the non-const ref. Will change that. >> Source/WebCore/dom/WheelEvent.cpp:104 >> + : EventDispatchMediator() > > Do you need to say this explicitly? I am surprised if so. No. I just got too mechanical in writing the code. I'll remove.
Created attachment 88566 [details] Patch
<3
Committed r83298: <http://trac.webkit.org/changeset/83298>