Created attachment 87415 [details] Patch

Created attachment 87418 [details] Rebased to ToT and sorted xcodeproj.

Comment on attachment 87415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87415&action=review > Source/WebCore/ChangeLog:8 > + The SimulatedClickEvent, instead of dispatching self, fires one or more SimulatedMouseEvents. I don’t understand why a simulated click, which is a sequence of events, now needs to be a kind of event. Can’t we simulate clicks by sending a sequence of events? Why do we need to trigger that with yet another event?

(In reply to comment #3) > (From update of attachment 87415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87415&action=review > > > Source/WebCore/ChangeLog:8 > > + The SimulatedClickEvent, instead of dispatching self, fires one or more SimulatedMouseEvents. > > I don’t understand why a simulated click, which is a sequence of events, now needs to be a kind of event. Can’t we simulate clicks by sending a sequence of events? Why do we need to trigger that with yet another event? The fact that it fires multiple events is a detail that isn't exposed anywhere from the call site of Node::dispatchSimulatedClick, just like dispatchMouseEvent may fire an extra dblclick event. Wrapping this into an Event seems consistent with how any other event would be dispatched: dispatchEvent(FooEvent::create(...)). Also, it wraps the logic neatly into its own class.

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 87415 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=87415&action=review > > > > > Source/WebCore/ChangeLog:8 > > > + The SimulatedClickEvent, instead of dispatching self, fires one or more SimulatedMouseEvents. > > > > I don’t understand why a simulated click, which is a sequence of events, now needs to be a kind of event. Can’t we simulate clicks by sending a sequence of events? Why do we need to trigger that with yet another event? > > The fact that it fires multiple events is a detail that isn't exposed anywhere from the call site of Node::dispatchSimulatedClick, just like dispatchMouseEvent may fire an extra dblclick event. > > Wrapping this into an Event seems consistent with how any other event would be dispatched: dispatchEvent(FooEvent::create(...)). > > Also, it wraps the logic neatly into its own class. I can also just have SimulatedMouseEvent (possibly put it the MouseEvent files), then leave EventDispatcher::dispatchSimulatedClick using them.

(In reply to comment #4) > The fact that it fires multiple events is a detail that isn't exposed anywhere from the call site of Node::dispatchSimulatedClick, just like dispatchMouseEvent may fire an extra dblclick event. Sure, we can encapsulate that into a function. > Wrapping this into an Event seems consistent with how any other event would be dispatched: dispatchEvent(FooEvent::create(...)). This seems to be the sole benefit, and I’m not sure it’s better. > Also, it wraps the logic neatly into its own class. You could do that without having the class be derived from Event.

(In reply to comment #5) > I can also just have SimulatedMouseEvent (possibly put it the MouseEvent files), then leave EventDispatcher::dispatchSimulatedClick using them. I’d prefer that.

(In reply to comment #7) > (In reply to comment #5) > > I can also just have SimulatedMouseEvent (possibly put it the MouseEvent files), then leave EventDispatcher::dispatchSimulatedClick using them. > > I’d prefer that. ok, patch coming up.