Introduce SimulatedClickEvent and move all simulated-click dispatch code there.

• Component: New Bugs
• Status: RESOLVED INVALID

[Dimitri Glazkov (Google)]

Introduce SimulatedClickEvent and move all simulated-click dispatch code there.

[Dimitri Glazkov (Google)]

Created attachment 87415 [details]
Patch

[Dimitri Glazkov (Google)]

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

[Darin Adler]

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?

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

(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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[Dimitri Glazkov (Google)]

(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.